This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] elf: Fix LD_AUDIT for modules with invalid version (BZ#24122)
* Adhemerval Zanella:
> On 24/01/2019 10:33, Florian Weimer wrote:
>> * Siddhesh Poyarekar:
>>
>>> On 24/01/19 2:14 AM, Carlos O'Donell wrote:
>>>>> Siddhesh,
>>>>>
>>>>> It is acceptable for 2.29?
>>>>
>>>> It's OK with me. Siddhesh gets to make the call.
>>>> This is only a bug fix.
>>>>
>>>> Reviewed-by: Carlos O'Donell <carlos@redhat.com>
>>>
>>> OK
>>
>> This broke the build with at least GCC 8 and the GCC 9 pre-release on
>> x86-64 and ppc64le:
>>
>> rtld.c: In function ‘dl_main’:
>> rtld.c:1556:8: error: ‘lav’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>> _dl_debug_printf (
>> ^~~~~~~~~~~~~~~~~~
>> " auditor disabled since expected version %d is greater than "
>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> "supported version %d.\n",
>> ~~~~~~~~~~~~~~~~~~~~~~~~~~
>> lav, LAV_CURRENT);
>> ~~~~~~~~~~~~~~~~~
>>
>> Looks like the control flow is too complicated. I think it's a false
>> positive.
>>
>> How should we fix this? Inhibit the warning (making the sources even
>> more convoluted than they are)? Revert the patch? Factor out the audit
>> module loading into a separate function and simplify the control flow?
>
> It does seem a false positive, since lav will always be set if laversion
> is not NULL and the code explicitly checks for it. Let me try if
> refactoring this can help gcc handle it correctly.
I'm testing this patch. It changes the error messages slightly.
It also calls _dl_close if we cannot find the laversion symbol,
something the previous code did not do.
If this is the direction we want to go in, I will write a ChangeLog
entry.
Thanks,
Florian
diff --git a/elf/rtld.c b/elf/rtld.c
index 9e0f752482..20cea19f6f 100644
--- a/elf/rtld.c
+++ b/elf/rtld.c
@@ -863,6 +863,184 @@ handle_ld_preload (const char *preloadlist, struct link_map *main_map)
return npreloads;
}
+/* Called the audit DSO cannot be used, if it does not have the
+ appropriate interfaces or it expects something more recent. */
+static void
+unload_audit_modile (struct link_map *map, int original_tls_idx)
+{
+#ifndef NDEBUG
+ Lmid_t ns = map->l_ns;
+#endif
+ _dl_close (map);
+
+ /* Make sure the namespace has been cleared entirely. */
+ assert (GL(dl_ns)[ns]._ns_loaded == NULL);
+ assert (GL(dl_ns)[ns]._ns_nloaded == 0);
+
+ GL(dl_tls_max_dtv_idx) = original_tls_idx;
+}
+
+/* Called to print an error message if loading of an audit module
+ failed. */
+static void
+report_audit_module_load_error (const char *name, const char *err_str,
+ bool malloced)
+{
+ _dl_error_printf ("\
+ERROR: ld.so: object '%s' cannot be loaded as audit interface: %s; ignored.\n",
+ name, err_str);
+ if (malloced)
+ free ((char *) err_str);
+}
+
+/* Load one audit module. */
+static void
+load_audit_module (const char *name, struct audit_ifaces **last_audit)
+{
+ int original_tls_idx = GL(dl_tls_max_dtv_idx);
+
+ struct dlmopen_args dlmargs;
+ dlmargs.fname = name;
+ dlmargs.map = NULL;
+
+ const char *objname;
+ const char *err_str = NULL;
+ bool malloced;
+ (void) _dl_catch_error (&objname, &err_str, &malloced, dlmopen_doit,
+ &dlmargs);
+ if (__glibc_unlikely (err_str != NULL))
+ {
+ report_audit_module_load_error (name, err_str, malloced);
+ return;
+ }
+
+ struct lookup_args largs;
+ largs.name = "la_version";
+ largs.map = dlmargs.map;
+ (void) _dl_catch_error (&objname, &err_str, &malloced,
+ lookup_doit, &largs);
+ if (__glibc_likely (err_str != NULL))
+ {
+ unload_audit_modile (dlmargs.map, original_tls_idx);
+ report_audit_module_load_error (name, err_str, malloced);
+ return;
+ }
+
+ unsigned int (*laversion) (unsigned int) = largs.result;
+ if (laversion == NULL)
+ {
+ _dl_error_printf ("\
+ERROR: ld.so: audit interface '%s' has NULL laversion symbol; ignored.\n",
+ name);
+ unload_audit_modile (dlmargs.map, original_tls_idx);
+ return;
+ }
+
+ unsigned lav = laversion (LAV_CURRENT);
+ if (lav == 0)
+ {
+ /* Only print an error message if debugging because this can
+ happen deliberately. */
+ if (GLRO(dl_debug_mask) & DL_DEBUG_FILES)
+ _dl_debug_printf ("\
+audit interface '%s' requested to be ignored (returned version of 0).\n",
+ name);
+ unload_audit_modile (dlmargs.map, original_tls_idx);
+ return;
+ }
+
+ if (lav > LAV_CURRENT)
+ {
+ _dl_debug_printf ("\
+ERROR: audit interface '%s' requires version %d (maximum supported version %d); ignored.\n",
+ name, lav, LAV_CURRENT);
+ unload_audit_modile (dlmargs.map, original_tls_idx);
+ return;
+ }
+
+ /* Allocate structure for the callback function pointers. This call
+ can never fail. */
+ enum { naudit_ifaces = 8 };
+ union
+ {
+ struct audit_ifaces ifaces;
+ void (*fptr[naudit_ifaces]) (void);
+ } *newp = malloc (sizeof (*newp));
+
+ /* Names of the auditing interfaces. All in one
+ long string. */
+ static const char audit_iface_names[] =
+ "la_activity\0"
+ "la_objsearch\0"
+ "la_objopen\0"
+ "la_preinit\0"
+#if __ELF_NATIVE_CLASS == 32
+ "la_symbind32\0"
+#elif __ELF_NATIVE_CLASS == 64
+ "la_symbind64\0"
+#else
+# error "__ELF_NATIVE_CLASS must be defined"
+#endif
+#define STRING(s) __STRING (s)
+ "la_" STRING (ARCH_LA_PLTENTER) "\0"
+ "la_" STRING (ARCH_LA_PLTEXIT) "\0"
+ "la_objclose\0";
+ unsigned int cnt = 0;
+ const char *cp = audit_iface_names;
+ do
+ {
+ largs.name = cp;
+ (void) _dl_catch_error (&objname, &err_str, &malloced,
+ lookup_doit, &largs);
+
+ /* Store the pointer. */
+ if (err_str == NULL && largs.result != NULL)
+ {
+ newp->fptr[cnt] = largs.result;
+
+ /* The dynamic linker link map is statically allocated,
+ initialize the data now. */
+ GL(dl_rtld_map).l_audit[cnt].cookie
+ = (intptr_t) &GL(dl_rtld_map);
+ }
+ else
+ newp->fptr[cnt] = NULL;
+ ++cnt;
+
+ cp = (char *) rawmemchr (cp, '\0') + 1;
+ }
+ while (*cp != '\0');
+ assert (cnt == naudit_ifaces);
+
+ /* Now append the new auditing interface to the list. */
+ newp->ifaces.next = NULL;
+ if (*last_audit == NULL)
+ *last_audit = GLRO(dl_audit) = &newp->ifaces;
+ else
+ *last_audit = (*last_audit)->next = &newp->ifaces;
+ ++GLRO(dl_naudit);
+
+ /* Mark the DSO as being used for auditing. */
+ dlmargs.map->l_auditing = 1;
+}
+
+/* Load all audit modules. */
+static void
+load_audit_modules (void)
+{
+ struct audit_ifaces *last_audit = NULL;
+ struct audit_list_iter al_iter;
+ audit_list_iter_init (&al_iter);
+
+ while (true)
+ {
+ const char *name = audit_list_iter_next (&al_iter);
+ if (name == NULL)
+ break;
+ load_audit_module (name, &last_audit);
+ }
+}
+
static void
dl_main (const ElfW(Phdr) *phdr,
ElfW(Word) phnum,
@@ -1395,10 +1573,6 @@ ERROR: '%s': cannot process note segment.\n", _dl_argv[0]);
if (__glibc_unlikely (audit_list != NULL)
|| __glibc_unlikely (audit_list_string != NULL))
{
- struct audit_ifaces *last_audit = NULL;
- struct audit_list_iter al_iter;
- audit_list_iter_init (&al_iter);
-
/* Since we start using the auditing DSOs right away we need to
initialize the data structures now. */
tcbp = init_tls ();
@@ -1410,158 +1584,7 @@ ERROR: '%s': cannot process note segment.\n", _dl_argv[0]);
security_init ();
need_security_init = false;
- while (true)
- {
- const char *name = audit_list_iter_next (&al_iter);
- if (name == NULL)
- break;
-
- int tls_idx = GL(dl_tls_max_dtv_idx);
-
- /* Now it is time to determine the layout of the static TLS
- block and allocate it for the initial thread. Note that we
- always allocate the static block, we never defer it even if
- no DF_STATIC_TLS bit is set. The reason is that we know
- glibc will use the static model. */
- struct dlmopen_args dlmargs;
- dlmargs.fname = name;
- dlmargs.map = NULL;
-
- const char *objname;
- const char *err_str = NULL;
- bool malloced;
- (void) _dl_catch_error (&objname, &err_str, &malloced, dlmopen_doit,
- &dlmargs);
- if (__glibc_unlikely (err_str != NULL))
- {
- not_loaded:
- _dl_error_printf ("\
-ERROR: ld.so: object '%s' cannot be loaded as audit interface: %s; ignored.\n",
- name, err_str);
- if (malloced)
- free ((char *) err_str);
- }
- else
- {
- struct lookup_args largs;
- largs.name = "la_version";
- largs.map = dlmargs.map;
-
- /* Check whether the interface version matches. */
- (void) _dl_catch_error (&objname, &err_str, &malloced,
- lookup_doit, &largs);
-
- unsigned int (*laversion) (unsigned int);
- unsigned int lav;
- if (err_str != NULL)
- goto not_loaded;
-
- if ((laversion = largs.result) != NULL
- && (lav = laversion (LAV_CURRENT)) > 0
- && lav <= LAV_CURRENT)
- {
- /* Allocate structure for the callback function pointers.
- This call can never fail. */
- union
- {
- struct audit_ifaces ifaces;
-#define naudit_ifaces 8
- void (*fptr[naudit_ifaces]) (void);
- } *newp = malloc (sizeof (*newp));
-
- /* Names of the auditing interfaces. All in one
- long string. */
- static const char audit_iface_names[] =
- "la_activity\0"
- "la_objsearch\0"
- "la_objopen\0"
- "la_preinit\0"
-#if __ELF_NATIVE_CLASS == 32
- "la_symbind32\0"
-#elif __ELF_NATIVE_CLASS == 64
- "la_symbind64\0"
-#else
-# error "__ELF_NATIVE_CLASS must be defined"
-#endif
-#define STRING(s) __STRING (s)
- "la_" STRING (ARCH_LA_PLTENTER) "\0"
- "la_" STRING (ARCH_LA_PLTEXIT) "\0"
- "la_objclose\0";
- unsigned int cnt = 0;
- const char *cp = audit_iface_names;
- do
- {
- largs.name = cp;
- (void) _dl_catch_error (&objname, &err_str, &malloced,
- lookup_doit, &largs);
-
- /* Store the pointer. */
- if (err_str == NULL && largs.result != NULL)
- {
- newp->fptr[cnt] = largs.result;
-
- /* The dynamic linker link map is statically
- allocated, initialize the data now. */
- GL(dl_rtld_map).l_audit[cnt].cookie
- = (intptr_t) &GL(dl_rtld_map);
- }
- else
- newp->fptr[cnt] = NULL;
- ++cnt;
-
- cp = (char *) rawmemchr (cp, '\0') + 1;
- }
- while (*cp != '\0');
- assert (cnt == naudit_ifaces);
-
- /* Now append the new auditing interface to the list. */
- newp->ifaces.next = NULL;
- if (last_audit == NULL)
- last_audit = GLRO(dl_audit) = &newp->ifaces;
- else
- last_audit = last_audit->next = &newp->ifaces;
- ++GLRO(dl_naudit);
-
- /* Mark the DSO as being used for auditing. */
- dlmargs.map->l_auditing = 1;
- }
- else
- {
- /* We cannot use the DSO, it does not have the
- appropriate interfaces or it expects something
- more recent. */
-#ifndef NDEBUG
- Lmid_t ns = dlmargs.map->l_ns;
-#endif
- _dl_close (dlmargs.map);
-
- /* Make sure the namespace has been cleared entirely. */
- assert (GL(dl_ns)[ns]._ns_loaded == NULL);
- assert (GL(dl_ns)[ns]._ns_nloaded == 0);
-
- GL(dl_tls_max_dtv_idx) = tls_idx;
- if (GLRO(dl_debug_mask) & DL_DEBUG_FILES)
- {
- _dl_debug_printf ("\
-\nfile=%s cannot be loaded as audit interface; ignored.\n", name);
- if (laversion == NULL)
- _dl_debug_printf (
-" la_version function not found.\n");
- else
- {
- if (lav == 0)
- _dl_debug_printf (
-" auditor requested to be ignored (returned version of 0).\n");
- else
- _dl_debug_printf (
-" auditor disabled since expected version %d is greater than "
-"supported version %d.\n",
- lav, LAV_CURRENT);
- }
- }
- }
- }
- }
+ load_audit_modules ();
/* If we have any auditing modules, announce that we already
have two objects loaded. */