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)
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).
>
> If this is the direction we want to go in, I will write a ChangeLog
> entry.
This is similar to what I am working it, we can go with this version.
>
> 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)
s/modile/module
> +{
> +#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);
Do we need this (void) cast?
> + 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" ?
> + 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".
> + 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.
> + 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. */
>