This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] elf: Ignore LD_AUDIT interfaces if la_version returns 0 [BZ #24122]
On 04/02/2019 11:51, Florian Weimer wrote:
> * Adhemerval Zanella:
>
>>> The actual bug fix is the separate error message for the case when
>>> la_version is zero. The dynamic linker error message (which is NULL
>>> in this case) is no longer used. Based on the intended use of version
>>> zero (ignore this module due to explicit request), the message is only
>>> printed if debugging is enabled.
>>>
>>> 2019-02-02 Florian Weimer <fweimer@redhat.com>
>>>
>>> [BZ #24122]
>>> * 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. Check for a zero return value from
>>> la_version. Remove spurious comment about static TLS
>>> initialization. Remove (void) casts of function results.
>>> (load_audit_modules): New function. Extracted from dl_main.
>>> (early_audit_modules_notification): Likewise.
>>> (dl_main): Call load_audit_modules and
>>> early_audit_modules_notification.
>>
>> I think we should reinstate the testcase from original commit
>> 8e889c5da3c5981c5a46a93fec02de40131ac5a6 along with this change. It aligns
>> with our policy of fixing bug reports with a regression test.
>
> Absolutely. I already posted that as a separate patch. I want to
> retain your authorship, and since it is a completely separate change, a
> separate commit seems reasonable to me.
I don't mind, just add me on CL and put reviewed-by: on commit message.
>
>>> +/* 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;
>>
>> Maybe a simple initialization as 'struct dlmopen_args dlmargs = { name, NULL }'?
>
> It's what was there before. 8-/
>
>>> + 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;
>>> + assert (laversion != NULL);
>>
>> I think we need to add a comment why we assert on a NULL 'laversion'
>> symbol, maybe by adding this means a corrupted audit object.
>
> I can add:
>
> /* A null symbol indicates that something is very wrong with the loaded
> object because defined symbols are supposed to have a valid, non-null
> address. */
Ok.
>
>>> + unsigned int lav = laversion (LAV_CURRENT);
>>> + if (lav == 0)
>>
>> Maybe __glibc_unlikely here?
>
> We don't really know what's unlikely or not. Since audit modules are
> used only rarely, I don't want to worry about it.
Ok.
>
>
>>> + /* 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));
>>
>> If this call should never fail, should we also assert for the malloc ?
>
> I think we use _dl_fatal_printf elsewhere. Again, this is a
> pre-existing issue. 8-/
Ok, we can live with it.
>
>>
>>> +
>>> + /* 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);
>>
>> Do we need the cast here?
>
> I think we should switch to the new error handling framework
> (_dl_catch_exception) and not worry about it until then.
Alright, I think since you refactoring the code it would be good to
not propagate unnecessary code conventions like that.
>
>>> +
>>> + /* 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;
>>
>> Same as before.
>
> Yeah, an unrelated, pre-existing issue.
Ok.
>
>>> +/* If we have any auditing modules, announce that we already have two
>>> + objects loaded: the main program and the dynamic linker itself. */
>>> +static void
>>> +early_audit_modules_notification (struct link_map *main_map)
>>> +{
>>> + if (__glibc_likely (GLRO(dl_naudit) == 0))
>>> + return;
>>> +
>>> + struct link_map *ls[2] = { main_map, &GL(dl_rtld_map) };
>>> +
>>> + for (unsigned int outer = 0; outer < 2; ++outer)
>>
>> Maybe size_t here?
>
> Despite outer < 2?
>
> I can unroll the loop in the caller, like this:
>
> if (__glibc_likely (GLRO(dl_naudit) > 0)
> {
> early_audit_modules_notification (main_map);
> early_audit_modules_notification (&GL(dl_rtld_map));
> }
>
> That actually looks quite reasonable. What do you think?
Look better to me, thanks.
>
> Thanks,
> Florian
>