diff mbox series

[14/19] nptl: Change type of __default_pthread_attr

Message ID f81d5827c2c6362a768e02ef8a1880bb211c8de9.1589884403.git.fweimer@redhat.com
State Committed
Headers show
Series Signal mask for timer helper thread | expand

Commit Message

Florian Weimer May 19, 2020, 10:44 a.m. UTC
union pthread_attr_transparent has always the correct size, even if
pthread_attr_t has padding that is not present in struct pthread_attr.

This should not result in an observable behavioral change.  The
existing code appears to have been correct, but it was brittle because
it was not clear which functions were allowed to write to an entire
pthread_attr_t argument (e.g., by copying it).
---
 nptl/allocatestack.c              | 2 +-
 nptl/nptl-init.c                  | 4 ++--
 nptl/pthreadP.h                   | 2 +-
 nptl/pthread_attr_getstacksize.c  | 2 +-
 nptl/pthread_create.c             | 8 ++++----
 nptl/pthread_getattr_default_np.c | 3 +--
 nptl/pthread_setattr_default_np.c | 6 +++---
 nptl/vars.c                       | 2 +-
 8 files changed, 14 insertions(+), 15 deletions(-)

Comments

Carlos O'Donell June 2, 2020, 3:39 a.m. UTC | #1
On 5/19/20 6:44 AM, Florian Weimer via Libc-alpha wrote:
> union pthread_attr_transparent has always the correct size, even if
> pthread_attr_t has padding that is not present in struct pthread_attr.
> 
> This should not result in an observable behavioral change.  The
> existing code appears to have been correct, but it was brittle because
> it was not clear which functions were allowed to write to an entire
> pthread_attr_t argument (e.g., by copying it).

OK for master.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> ---
>  nptl/allocatestack.c              | 2 +-
>  nptl/nptl-init.c                  | 4 ++--
>  nptl/pthreadP.h                   | 2 +-
>  nptl/pthread_attr_getstacksize.c  | 2 +-
>  nptl/pthread_create.c             | 8 ++++----
>  nptl/pthread_getattr_default_np.c | 3 +--
>  nptl/pthread_setattr_default_np.c | 6 +++---
>  nptl/vars.c                       | 2 +-
>  8 files changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
> index c94980c21c..d16f3d71f8 100644
> --- a/nptl/allocatestack.c
> +++ b/nptl/allocatestack.c
> @@ -425,7 +425,7 @@ allocate_stack (const struct pthread_attr *attr, struct pthread **pdp,
>    else
>      {
>        lll_lock (__default_pthread_attr_lock, LLL_PRIVATE);
> -      size = __default_pthread_attr.stacksize;
> +      size = __default_pthread_attr.internal.stacksize;

OK.

>        lll_unlock (__default_pthread_attr_lock, LLL_PRIVATE);
>      }
>  
> diff --git a/nptl/nptl-init.c b/nptl/nptl-init.c
> index 96b1444a01..d4cf20e3d1 100644
> --- a/nptl/nptl-init.c
> +++ b/nptl/nptl-init.c
> @@ -318,8 +318,8 @@ __pthread_initialize_minimal_internal (void)
>    /* Round the resource limit up to page size.  */
>    limit.rlim_cur = ALIGN_UP (limit.rlim_cur, pagesz);
>    lll_lock (__default_pthread_attr_lock, LLL_PRIVATE);
> -  __default_pthread_attr.stacksize = limit.rlim_cur;
> -  __default_pthread_attr.guardsize = GLRO (dl_pagesize);
> +  __default_pthread_attr.internal.stacksize = limit.rlim_cur;
> +  __default_pthread_attr.internal.guardsize = GLRO (dl_pagesize);

OK.

>    lll_unlock (__default_pthread_attr_lock, LLL_PRIVATE);
>  
>  #ifdef SHARED
> diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
> index 9c6dd41b7c..acc8e88e4a 100644
> --- a/nptl/pthreadP.h
> +++ b/nptl/pthreadP.h
> @@ -199,7 +199,7 @@ enum
>  
>  
>  /* Default pthread attributes.  */
> -extern struct pthread_attr __default_pthread_attr attribute_hidden;
> +extern union pthread_attr_transparent __default_pthread_attr attribute_hidden;

OK.

>  extern int __default_pthread_attr_lock attribute_hidden;
>  
>  /* Size and alignment of static TLS block.  */
> diff --git a/nptl/pthread_attr_getstacksize.c b/nptl/pthread_attr_getstacksize.c
> index 346b375690..9830a635a6 100644
> --- a/nptl/pthread_attr_getstacksize.c
> +++ b/nptl/pthread_attr_getstacksize.c
> @@ -33,7 +33,7 @@ __pthread_attr_getstacksize (const pthread_attr_t *attr, size_t *stacksize)
>    if (size == 0)
>      {
>        lll_lock (__default_pthread_attr_lock, LLL_PRIVATE);
> -      size = __default_pthread_attr.stacksize;
> +      size = __default_pthread_attr.internal.stacksize;

OK.

>        lll_unlock (__default_pthread_attr_lock, LLL_PRIVATE);
>      }
>    *stacksize = size;
> diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
> index 347d510707..86fbeb5218 100644
> --- a/nptl/pthread_create.c
> +++ b/nptl/pthread_create.c
> @@ -612,16 +612,16 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
>    STACK_VARIABLES;
>  
>    const struct pthread_attr *iattr = (struct pthread_attr *) attr;
> -  struct pthread_attr default_attr;
> +  union pthread_attr_transparent default_attr;

OK.

>    bool destroy_default_attr = false;
>    bool c11 = (attr == ATTR_C11_THREAD);
>    if (iattr == NULL || c11)
>      {
> -      int ret = __pthread_getattr_default_np ((pthread_attr_t *) &default_attr);
> +      int ret = __pthread_getattr_default_np (&default_attr.external);

OK. Uses external type for public API.

>        if (ret != 0)
>  	return ret;
>        destroy_default_attr = true;
> -      iattr = &default_attr;
> +      iattr = &default_attr.internal;

OK.

>      }
>  
>    struct pthread *pd = NULL;
> @@ -852,7 +852,7 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
>  
>   out:
>    if (destroy_default_attr)
> -    __pthread_attr_destroy ((pthread_attr_t *) &default_attr);
> +    __pthread_attr_destroy (&default_attr.external);

OK.

>  
>    return retval;
>  }
> diff --git a/nptl/pthread_getattr_default_np.c b/nptl/pthread_getattr_default_np.c
> index 5c99f980e2..f3ce1c2885 100644
> --- a/nptl/pthread_getattr_default_np.c
> +++ b/nptl/pthread_getattr_default_np.c
> @@ -22,8 +22,7 @@ int
>  __pthread_getattr_default_np (pthread_attr_t *out)
>  {
>    lll_lock (__default_pthread_attr_lock, LLL_PRIVATE);
> -  int ret = __pthread_attr_copy (out,
> -                                 (pthread_attr_t *) &__default_pthread_attr);
> +  int ret = __pthread_attr_copy (out, &__default_pthread_attr.external);

OK.

>    lll_unlock (__default_pthread_attr_lock, LLL_PRIVATE);
>    return ret;
>  }
> diff --git a/nptl/pthread_setattr_default_np.c b/nptl/pthread_setattr_default_np.c
> index eb5d24d3bd..c4cfb4e8ef 100644
> --- a/nptl/pthread_setattr_default_np.c
> +++ b/nptl/pthread_setattr_default_np.c
> @@ -68,15 +68,15 @@ pthread_setattr_default_np (const pthread_attr_t *in)
>  
>    /* Preserve the previous stack size (see above).  */
>    if (temp.internal.stacksize == 0)
> -    temp.internal.stacksize = __default_pthread_attr.stacksize;
> +    temp.internal.stacksize = __default_pthread_attr.internal.stacksize;
>  
>    /* Destroy the old attribute structure because it will be
>       overwritten.  */
> -  __pthread_attr_destroy ((pthread_attr_t *) &__default_pthread_attr);
> +  __pthread_attr_destroy (&__default_pthread_attr.external);
>  
>    /* __default_pthread_attr takes ownership, so do not free
>       attrs.internal after this point.  */
> -  __default_pthread_attr = temp.internal;
> +  __default_pthread_attr = temp;

OK.

>  
>    lll_unlock (__default_pthread_attr_lock, LLL_PRIVATE);
>    return ret;
> diff --git a/nptl/vars.c b/nptl/vars.c
> index b88300d9b4..3696020145 100644
> --- a/nptl/vars.c
> +++ b/nptl/vars.c
> @@ -22,7 +22,7 @@
>  
>  /* Default thread attributes for the case when the user does not
>     provide any.  */
> -struct pthread_attr __default_pthread_attr attribute_hidden;
> +union pthread_attr_transparent __default_pthread_attr attribute_hidden;

OK.

>  
>  /* Mutex protecting __default_pthread_attr.  */
>  int __default_pthread_attr_lock = LLL_LOCK_INITIALIZER;
>
diff mbox series

Patch

diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
index c94980c21c..d16f3d71f8 100644
--- a/nptl/allocatestack.c
+++ b/nptl/allocatestack.c
@@ -425,7 +425,7 @@  allocate_stack (const struct pthread_attr *attr, struct pthread **pdp,
   else
     {
       lll_lock (__default_pthread_attr_lock, LLL_PRIVATE);
-      size = __default_pthread_attr.stacksize;
+      size = __default_pthread_attr.internal.stacksize;
       lll_unlock (__default_pthread_attr_lock, LLL_PRIVATE);
     }
 
diff --git a/nptl/nptl-init.c b/nptl/nptl-init.c
index 96b1444a01..d4cf20e3d1 100644
--- a/nptl/nptl-init.c
+++ b/nptl/nptl-init.c
@@ -318,8 +318,8 @@  __pthread_initialize_minimal_internal (void)
   /* Round the resource limit up to page size.  */
   limit.rlim_cur = ALIGN_UP (limit.rlim_cur, pagesz);
   lll_lock (__default_pthread_attr_lock, LLL_PRIVATE);
-  __default_pthread_attr.stacksize = limit.rlim_cur;
-  __default_pthread_attr.guardsize = GLRO (dl_pagesize);
+  __default_pthread_attr.internal.stacksize = limit.rlim_cur;
+  __default_pthread_attr.internal.guardsize = GLRO (dl_pagesize);
   lll_unlock (__default_pthread_attr_lock, LLL_PRIVATE);
 
 #ifdef SHARED
diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
index 9c6dd41b7c..acc8e88e4a 100644
--- a/nptl/pthreadP.h
+++ b/nptl/pthreadP.h
@@ -199,7 +199,7 @@  enum
 
 
 /* Default pthread attributes.  */
-extern struct pthread_attr __default_pthread_attr attribute_hidden;
+extern union pthread_attr_transparent __default_pthread_attr attribute_hidden;
 extern int __default_pthread_attr_lock attribute_hidden;
 
 /* Size and alignment of static TLS block.  */
diff --git a/nptl/pthread_attr_getstacksize.c b/nptl/pthread_attr_getstacksize.c
index 346b375690..9830a635a6 100644
--- a/nptl/pthread_attr_getstacksize.c
+++ b/nptl/pthread_attr_getstacksize.c
@@ -33,7 +33,7 @@  __pthread_attr_getstacksize (const pthread_attr_t *attr, size_t *stacksize)
   if (size == 0)
     {
       lll_lock (__default_pthread_attr_lock, LLL_PRIVATE);
-      size = __default_pthread_attr.stacksize;
+      size = __default_pthread_attr.internal.stacksize;
       lll_unlock (__default_pthread_attr_lock, LLL_PRIVATE);
     }
   *stacksize = size;
diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
index 347d510707..86fbeb5218 100644
--- a/nptl/pthread_create.c
+++ b/nptl/pthread_create.c
@@ -612,16 +612,16 @@  __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
   STACK_VARIABLES;
 
   const struct pthread_attr *iattr = (struct pthread_attr *) attr;
-  struct pthread_attr default_attr;
+  union pthread_attr_transparent default_attr;
   bool destroy_default_attr = false;
   bool c11 = (attr == ATTR_C11_THREAD);
   if (iattr == NULL || c11)
     {
-      int ret = __pthread_getattr_default_np ((pthread_attr_t *) &default_attr);
+      int ret = __pthread_getattr_default_np (&default_attr.external);
       if (ret != 0)
 	return ret;
       destroy_default_attr = true;
-      iattr = &default_attr;
+      iattr = &default_attr.internal;
     }
 
   struct pthread *pd = NULL;
@@ -852,7 +852,7 @@  __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
 
  out:
   if (destroy_default_attr)
-    __pthread_attr_destroy ((pthread_attr_t *) &default_attr);
+    __pthread_attr_destroy (&default_attr.external);
 
   return retval;
 }
diff --git a/nptl/pthread_getattr_default_np.c b/nptl/pthread_getattr_default_np.c
index 5c99f980e2..f3ce1c2885 100644
--- a/nptl/pthread_getattr_default_np.c
+++ b/nptl/pthread_getattr_default_np.c
@@ -22,8 +22,7 @@  int
 __pthread_getattr_default_np (pthread_attr_t *out)
 {
   lll_lock (__default_pthread_attr_lock, LLL_PRIVATE);
-  int ret = __pthread_attr_copy (out,
-                                 (pthread_attr_t *) &__default_pthread_attr);
+  int ret = __pthread_attr_copy (out, &__default_pthread_attr.external);
   lll_unlock (__default_pthread_attr_lock, LLL_PRIVATE);
   return ret;
 }
diff --git a/nptl/pthread_setattr_default_np.c b/nptl/pthread_setattr_default_np.c
index eb5d24d3bd..c4cfb4e8ef 100644
--- a/nptl/pthread_setattr_default_np.c
+++ b/nptl/pthread_setattr_default_np.c
@@ -68,15 +68,15 @@  pthread_setattr_default_np (const pthread_attr_t *in)
 
   /* Preserve the previous stack size (see above).  */
   if (temp.internal.stacksize == 0)
-    temp.internal.stacksize = __default_pthread_attr.stacksize;
+    temp.internal.stacksize = __default_pthread_attr.internal.stacksize;
 
   /* Destroy the old attribute structure because it will be
      overwritten.  */
-  __pthread_attr_destroy ((pthread_attr_t *) &__default_pthread_attr);
+  __pthread_attr_destroy (&__default_pthread_attr.external);
 
   /* __default_pthread_attr takes ownership, so do not free
      attrs.internal after this point.  */
-  __default_pthread_attr = temp.internal;
+  __default_pthread_attr = temp;
 
   lll_unlock (__default_pthread_attr_lock, LLL_PRIVATE);
   return ret;
diff --git a/nptl/vars.c b/nptl/vars.c
index b88300d9b4..3696020145 100644
--- a/nptl/vars.c
+++ b/nptl/vars.c
@@ -22,7 +22,7 @@ 
 
 /* Default thread attributes for the case when the user does not
    provide any.  */
-struct pthread_attr __default_pthread_attr attribute_hidden;
+union pthread_attr_transparent __default_pthread_attr attribute_hidden;
 
 /* Mutex protecting __default_pthread_attr.  */
 int __default_pthread_attr_lock = LLL_LOCK_INITIALIZER;