[review] Avoid zero-length array at the end of struct link_map [BZ #25097]
Commit Message
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(-)
Comments
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
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.
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.
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 */
@@ -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
@@ -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 */