[review] Avoid zero-length array at the end of struct link_map [BZ #25097]

[review] Avoid zero-length array at the end of struct link_map [BZ #25097]

[Florian Weimer (Code Review)]

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/488
......................................................................

Avoid zero-length array at the end of struct link_map [BZ #25097]

l_audit ends up as an internal array with _rtld_global, and GCC 10
warns about this.

This commit does not change the layout of _rtld_global, so it is
suitable for backporting.  Future changes could allocate more of the
audit state dynamically and remove it from always-allocated data
structures, to optimize the common case of inactive auditing.

Change-Id: Ic911100730f9124d4ea977ead8e13cee64b84d45
---
M include/link.h
M sysdeps/generic/ldsodefs.h
2 files changed, 23 insertions(+), 12 deletions(-)



diff --git a/include/link.h b/include/link.h
index 1184201..be52b97 100644
--- a/include/link.h
+++ b/include/link.h
@@ -325,16 +325,18 @@
     size_t l_relro_size;
 
     unsigned long long int l_serial;
-
-    /* Audit information.  This array apparent must be the last in the
-       structure.  Never add something after it.  */
-    struct auditstate
-    {
-      uintptr_t cookie;
-      unsigned int bindflags;
-    } l_audit[0];
   };
 
+/* Information used by audit modules.  For most link maps, this data
+   immediate follows the link map in memory.  For the dynamic linker,
+   it is allocated separately.  See link_map_audit_state in
+   <ldsodefs.h>.  */
+struct auditstate
+{
+  uintptr_t cookie;
+  unsigned int bindflags;
+};
+
 
 #if __ELF_NATIVE_CLASS == 32
 # define symbind symbind32
diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
index 04b6d17..eb6cbea 100644
--- a/sysdeps/generic/ldsodefs.h
+++ b/sysdeps/generic/ldsodefs.h
@@ -379,11 +379,12 @@
   /* List of search directories.  */
   EXTERN struct r_search_path_elem *_dl_all_dirs;
 
-  /* Structure describing the dynamic linker itself.  We need to
-     reserve memory for the data the audit libraries need.  */
+  /* Structure describing the dynamic linker itself.  */
   EXTERN struct link_map _dl_rtld_map;
 #ifdef SHARED
-  struct auditstate audit_data[DL_NNS];
+  /* Used to store the audit information for the link map of the
+     dynamic loader.  */
+  struct auditstate _dl_rtld_auditstate[DL_NNS];
 #endif
 
 #if defined SHARED && defined _LIBC_REENTRANT \
@@ -1178,7 +1179,15 @@
 static inline struct auditstate *
 link_map_audit_state (struct link_map *l, size_t index)
 {
-  return &l->l_audit[index];
+  if (l == &GL (dl_rtld_map))
+    /* The auditstate array is stored separately.  */
+    return &GL (dl_rtld_auditstate) [index];
+  else
+    {
+      /* The auditstate array follows the link map in memory.  */
+      struct auditstate *base = (struct auditstate *) (l + 1);
+      return &base[index];
+    }
 }
 #endif /* SHARED */
 

-- 
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: Ic911100730f9124d4ea977ead8e13cee64b84d45
Gerrit-Change-Number: 488
Gerrit-PatchSet: 1
Gerrit-Owner: Florian Weimer <fweimer@redhat.com>
Gerrit-MessageType: newchange

Re: [review] Avoid zero-length array at the end of struct link_map [BZ #25097]

[Jeff Law]

On 11/3/19 10:11 AM, Florian Weimer (Code Review) wrote:
> Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/488
> ......................................................................
> 
> Avoid zero-length array at the end of struct link_map [BZ #25097]
> 
> l_audit ends up as an internal array with _rtld_global, and GCC 10
> warns about this.
> 
> This commit does not change the layout of _rtld_global, so it is
> suitable for backporting.  Future changes could allocate more of the
> audit state dynamically and remove it from always-allocated data
> structures, to optimize the common case of inactive auditing.
[ ... ]
Just wanted to say thanks for taking care of this over the weekend.
Every one of my -gnu targets started failing yesterday/today because of
these issues.



jeff

[review] Avoid zero-length array at the end of struct link_map [BZ #25097]

[Carlos O'Donell (Code Review)]

Carlos O'Donell has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/488
......................................................................


Patch Set 1: Code-Review+2

(4 comments)

Looks good to me.
Reviewed-by: Carlos O'Donell <carlos@redhat.com>

https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/488/1/include/link.h 
File include/link.h:

https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/488/1/include/link.h@338 
PS1, Line 338: 
333 |    <ldsodefs.h>.  */
334 | struct auditstate
335 | {
336 |   uintptr_t cookie;
337 |   unsigned int bindflags;
338 > };
339 | 
340 | 
341 | #if __ELF_NATIVE_CLASS == 32
342 | # define symbind symbind32
343 | #elif __ELF_NATIVE_CLASS == 64

OK. Move definition out of the link_map struct.


https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/488/1/sysdeps/generic/ldsodefs.h 
File sysdeps/generic/ldsodefs.h:

https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/488/1/sysdeps/generic/ldsodefs.h@387 
PS1, Line 387: 
382 |   /* Structure describing the dynamic linker itself.  */
383 |   EXTERN struct link_map _dl_rtld_map;
384 | #ifdef SHARED
385 |   /* Used to store the audit information for the link map of the
386 |      dynamic loader.  */
387 >   struct auditstate _dl_rtld_auditstate[DL_NNS];
388 | #endif
389 | 
390 | #if defined SHARED && defined _LIBC_REENTRANT \
391 |     && defined __rtld_lock_default_lock_recursive
392 |   EXTERN void (*_dl_rtld_lock_recursive) (void *);

OK. Renamed, but effectively the same.


https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/488/1/sysdeps/generic/ldsodefs.h@1184 
PS1, Line 1184: 
1179 | static inline struct auditstate *
1180 | link_map_audit_state (struct link_map *l, size_t index)
1181 | {
1182 |   if (l == &GL (dl_rtld_map))
1183 |     /* The auditstate array is stored separately.  */
1184 >     return &GL (dl_rtld_auditstate) [index];
1185 |   else
1186 |     {
1187 |       /* The auditstate array follows the link map in memory.  */
1188 |       struct auditstate *base = (struct auditstate *) (l + 1);
1189 |       return &base[index];

OK, for the normal layout of dl_rtld_map.


https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/488/1/sysdeps/generic/ldsodefs.h@1189 
PS1, Line 1189: 
1180 | link_map_audit_state (struct link_map *l, size_t index)
     | ...
1184 |     return &GL (dl_rtld_auditstate) [index];
1185 |   else
1186 |     {
1187 |       /* The auditstate array follows the link map in memory.  */
1188 |       struct auditstate *base = (struct auditstate *) (l + 1);
1189 >       return &base[index];
1190 |     }
1191 | }
1192 | #endif /* SHARED */
1193 | 
1194 | __END_DECLS

OK. We adjust the base pointer to point at the end of the link_map, then cast to an audit state structure. This is not an aliasing violation, the pointer is outside of the current structure. Once computed we take the array offset based on index and return that.



-- 
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: Ic911100730f9124d4ea977ead8e13cee64b84d45
Gerrit-Change-Number: 488
Gerrit-PatchSet: 1
Gerrit-Owner: Florian Weimer <fweimer@redhat.com>
Gerrit-Reviewer: Carlos O'Donell <carlos@redhat.com>
Gerrit-Comment-Date: Sun, 03 Nov 2019 21:19:14 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

Re: [review] Avoid zero-length array at the end of struct link_map [BZ #25097]

[Carlos O'Donell]

On 11/3/19 4:19 PM, Carlos O'Donell (Code Review) wrote:
> Carlos O'Donell has posted comments on this change.
> 
> Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/488
> ......................................................................
> 
> 
> Patch Set 1: Code-Review+2
> 
> (4 comments)
> 
> Looks good to me.
> Reviewed-by: Carlos O'Donell <carlos@redhat.com>
> 
> https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/488/1/include/link.h 
> File include/link.h:

Simon,

I couldn't be happier about how nice the review looks here:
https://www.sourceware.org/ml/libc-alpha/2019-11/msg00039.html

It captures all my points, and gets them to list so they aren't lost.
The leading and lagging context looks awesome.

Thank you very much for helping with this.

-- 
Cheers,
Carlos.

[review] Avoid zero-length array at the end of struct link_map [BZ #25097]

[Carlos O'Donell (Code Review)]

Carlos O'Donell has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/488
......................................................................


Patch Set 1:

(4 comments)

| --- include/link.h
| +++ include/link.h
| @@ -337,9 +329,19 @@ #endif
|  
| +/* Information used by audit modules.  For most link maps, this data
| +   immediate follows the link map in memory.  For the dynamic linker,
| +   it is allocated separately.  See link_map_audit_state in
| +   <ldsodefs.h>.  */
| +struct auditstate
| +{
| +  uintptr_t cookie;
| +  unsigned int bindflags;
| +};

PS1, Line 338:

Done

| +
|  
|  #if __ELF_NATIVE_CLASS == 32
|  # define symbind symbind32
|  #elif __ELF_NATIVE_CLASS == 64
|  # define symbind symbind64
|  #else
|  # error "__ELF_NATIVE_CLASS must be defined"
|  #endif
| --- sysdeps/generic/ldsodefs.h
| +++ sysdeps/generic/ldsodefs.h
| @@ -381,15 +381,16 @@ #endif
|  
| -  /* Structure describing the dynamic linker itself.  We need to
| -     reserve memory for the data the audit libraries need.  */
| +  /* Structure describing the dynamic linker itself.  */
|    EXTERN struct link_map _dl_rtld_map;
|  #ifdef SHARED
| -  struct auditstate audit_data[DL_NNS];
| +  /* Used to store the audit information for the link map of the
| +     dynamic loader.  */
| +  struct auditstate _dl_rtld_auditstate[DL_NNS];

PS1, Line 387:

Done

|  #endif
|  
|  #if defined SHARED && defined _LIBC_REENTRANT \
|      && defined __rtld_lock_default_lock_recursive
|    EXTERN void (*_dl_rtld_lock_recursive) (void *);
|    EXTERN void (*_dl_rtld_unlock_recursive) (void *);
|  #endif
|  
|    /* Get architecture specific definitions.  */

 ...

| @@ -1175,13 +1176,21 @@ rtld_active (void)
|    return GLRO(dl_init_all_dirs) != NULL;
|  }
|  
|  static inline struct auditstate *
|  link_map_audit_state (struct link_map *l, size_t index)
|  {
| -  return &l->l_audit[index];
| +  if (l == &GL (dl_rtld_map))
| +    /* The auditstate array is stored separately.  */
| +    return &GL (dl_rtld_auditstate) [index];

PS1, Line 1184:

Done

| +  else
| +    {
| +      /* The auditstate array follows the link map in memory.  */
| +      struct auditstate *base = (struct auditstate *) (l + 1);
| +      return &base[index];

PS1, Line 1189:

Done

| +    }
|  }
|  #endif /* SHARED */
|  
|  __END_DECLS
|  
|  #endif /* ldsodefs.h */

-- 
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: Ic911100730f9124d4ea977ead8e13cee64b84d45
Gerrit-Change-Number: 488
Gerrit-PatchSet: 1
Gerrit-Owner: Florian Weimer <fweimer@redhat.com>
Gerrit-Reviewer: Carlos O'Donell <carlos@redhat.com>
Gerrit-Comment-Date: Thu, 14 Nov 2019 14:59:34 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Carlos O'Donell <carlos@redhat.com>
Gerrit-MessageType: comment

[pushed] Avoid zero-length array at the end of struct link_map [BZ #25097]

[Sourceware to Gerrit sync (Code Review)]

Sourceware to Gerrit sync has submitted this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/488
......................................................................

Avoid zero-length array at the end of struct link_map [BZ #25097]

l_audit ends up as an internal array with _rtld_global, and GCC 10
warns about this.

This commit does not change the layout of _rtld_global, so it is
suitable for backporting.  Future changes could allocate more of the
audit state dynamically and remove it from always-allocated data
structures, to optimize the common case of inactive auditing.

Change-Id: Ic911100730f9124d4ea977ead8e13cee64b84d45
---
M include/link.h
M sysdeps/generic/ldsodefs.h
2 files changed, 23 insertions(+), 12 deletions(-)

Approvals:
  Carlos O'Donell: Looks good to me, approved


diff --git a/include/link.h b/include/link.h
index 1184201..be52b97 100644
--- a/include/link.h
+++ b/include/link.h
@@ -325,16 +325,18 @@
     size_t l_relro_size;
 
     unsigned long long int l_serial;
-
-    /* Audit information.  This array apparent must be the last in the
-       structure.  Never add something after it.  */
-    struct auditstate
-    {
-      uintptr_t cookie;
-      unsigned int bindflags;
-    } l_audit[0];
   };
 
+/* Information used by audit modules.  For most link maps, this data
+   immediate follows the link map in memory.  For the dynamic linker,
+   it is allocated separately.  See link_map_audit_state in
+   <ldsodefs.h>.  */
+struct auditstate
+{
+  uintptr_t cookie;
+  unsigned int bindflags;
+};
+
 
 #if __ELF_NATIVE_CLASS == 32
 # define symbind symbind32
diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
index 923bd4c..4d67c05 100644
--- a/sysdeps/generic/ldsodefs.h
+++ b/sysdeps/generic/ldsodefs.h
@@ -379,11 +379,12 @@
   /* List of search directories.  */
   EXTERN struct r_search_path_elem *_dl_all_dirs;
 
-  /* Structure describing the dynamic linker itself.  We need to
-     reserve memory for the data the audit libraries need.  */
+  /* Structure describing the dynamic linker itself.  */
   EXTERN struct link_map _dl_rtld_map;
 #ifdef SHARED
-  struct auditstate audit_data[DL_NNS];
+  /* Used to store the audit information for the link map of the
+     dynamic loader.  */
+  struct auditstate _dl_rtld_auditstate[DL_NNS];
 #endif
 
 #if defined SHARED && defined _LIBC_REENTRANT \
@@ -1178,7 +1179,15 @@
 static inline struct auditstate *
 link_map_audit_state (struct link_map *l, size_t index)
 {
-  return &l->l_audit[index];
+  if (l == &GL (dl_rtld_map))
+    /* The auditstate array is stored separately.  */
+    return &GL (dl_rtld_auditstate) [index];
+  else
+    {
+      /* The auditstate array follows the link map in memory.  */
+      struct auditstate *base = (struct auditstate *) (l + 1);
+      return &base[index];
+    }
 }
 #endif /* SHARED */
 

-- 
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: Ic911100730f9124d4ea977ead8e13cee64b84d45
Gerrit-Change-Number: 488
Gerrit-PatchSet: 2
Gerrit-Owner: Florian Weimer <fweimer@redhat.com>
Gerrit-Reviewer: Carlos O'Donell <carlos@redhat.com>
Gerrit-MessageType: merged