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 11:20, Florian Weimer wrote:
>> * 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.
>
> The current code does calls _dl_close in this case, it does not if
> _dl_catch_error returns an err_str (in this case the object is not
> loaded).
I meant this part:
/* 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;
The not_loaded label is in the true branch of the surrounding if, and
the unloading on error only happens at the end of the else branch.
>> +/* 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)
>
> s/modile/module
Fixed.
>> + (void) _dl_catch_error (&objname, &err_str, &malloced,
>> + lookup_doit, &largs);
>
> Do we need this (void) cast?
No, I can remove it, it came from the original code. I removed it.
>
>> + 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);
>
> Maybe "ERROR: ld.so: audit interface does not contain a laversion symbol; ignored.\n" ?
No, the missing symbol case is covered earlier, where lookup_doit will
fail. This is really about a NULL symbol, a different error case.
Maybe this is so bizarre that we should drop this check? (You need an
SHN_ABS symbol or an IFUNC resolver that returns NULL to get this.)
>> + 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);
>
> I used the '\nfile=%s...' to follow the messages already printed for DL_DEBUG_FILES.
> Maybe 'file=%s requested to be ignored (returned version of 0).\n".
This doesn't say that it's ignored as an audit module.
>> + 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);
>
> My understanding is Carlos asked to this scenario not be reported as
> an error.
I think Carlos meant the lav == 0 case, where I agree that warning
should be suppressed by default. The lav > LAV_CURRENT suggests a
broken audit module because it did not properly check the required
version against LAV_CURRENT. (If it does not want ld.so to log an
error, the audit module can always return 0 instead.)
New patch below.
Thanks,
Florian
elf: Refactor audit module loading out of dl_main
This avoids a compiler warning about an uninitialized lav variable
(seen with GCC 8 and 9 on x86-64).
2019-01-24 Florian Weimer <fweimer@redhat.com>
* elf/rtld.c (unload_audit_module): New function.
(report_audit_module_load_error): Likewise.
(load_audit_module): Likewise. Extracted from dl_main. Call
_dl_close if the laversion symbol cannot be found. Use early
returns for error handling. Remove spurious comment about static
TLS initialization. Remove (void) casts of function results.
(load_audit_modules): New function. Extracted from dl_main.
(dl_main): Call load_audit_modules.
diff --git a/elf/rtld.c b/elf/rtld.c
index 9e0f752482..f88dba8910 100644
--- a/elf/rtld.c
+++ b/elf/rtld.c
@@ -863,6 +863,182 @@ 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_module (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;
+ _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;
+ _dl_catch_error (&objname, &err_str, &malloced, lookup_doit, &largs);
+ if (__glibc_likely (err_str != NULL))
+ {
+ unload_audit_module (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_module (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_module (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_module (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 +1571,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 +1582,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. */