elf: Fix LD_AUDIT for modules with invalid version (BZ#24122)
Commit Message
On 23/01/2019 12:58, Carlos O'Donell wrote:
> On 1/23/19 9:07 AM, Adhemerval Zanella wrote:
>
> Thanks for fixing this!
>
> I'd like to see v2 please
>
> - Only output message when LD_DEBUG=all, since la_version returning 0
> or > LAV_CURRENT is not an error, but simply an API mandated situation
> where we ignore the module. I don't see that as an error.
>
> The Solaris docs say:
> "If the audit library return is zero, or a version that is greater
> than the rtld-audit interface the runtime linker supports, the audit
> library is discarded."
>
> Just discarded. It is up to the auditor to decide how it exposes to
> the user that it is running and operating as expected. We can provide
> feedback via LD_DEBUG=all.
Right, this rationale make sense. However I think this information is
relates to the 'files' option, instead of print when 'all' option are
selected.
>
> - Optional: use -Wl,z,lazy for the test, or remove the comment about
> the PLT call.
The idea is indeed to force PLT call, so the PLT audit stub is called
and tests explicit fails.
>
> - Suggested comment in tst-audit13mod1.c
Ack. Patch updated below.
---
[BZ #24122]
* elf/Makefile (tests): Add tst-audit13.
(modules-names): Add tst-audit13mod1.
(tst-audit13.out, LDFLAGS-tst-audit13mod1.so, tst-audit13-ENV): New
rule.
* elf/rtld.c (dl_main): Handle invalid audit module version.
* elf/tst-audit13.c: New file.
* elf/tst-audit13mod1.c: Likewise.
--
Comments
On 1/23/19 12:44 PM, Adhemerval Zanella wrote:
>
>
> On 23/01/2019 12:58, Carlos O'Donell wrote:
>> On 1/23/19 9:07 AM, Adhemerval Zanella wrote:
>>
>> Thanks for fixing this!
>>
>> I'd like to see v2 please
>>
>> - Only output message when LD_DEBUG=all, since la_version returning 0
>> or > LAV_CURRENT is not an error, but simply an API mandated situation
>> where we ignore the module. I don't see that as an error.
>>
>> The Solaris docs say:
>> "If the audit library return is zero, or a version that is greater
>> than the rtld-audit interface the runtime linker supports, the audit
>> library is discarded."
>>
>> Just discarded. It is up to the auditor to decide how it exposes to
>> the user that it is running and operating as expected. We can provide
>> feedback via LD_DEBUG=all.
>
> Right, this rationale make sense. However I think this information is
> relates to the 'files' option, instead of print when 'all' option are
> selected.
>
>>
>> - Optional: use -Wl,z,lazy for the test, or remove the comment about
>> the PLT call.
>
> The idea is indeed to force PLT call, so the PLT audit stub is called
> and tests explicit fails.
>
>>
>> - Suggested comment in tst-audit13mod1.c
>
> Ack. Patch updated below.
>
OK for master if you fix the error messages as noted below.
Reviewed-by: Carlos O'Donell <carlos@redhat.com>
> ---
>
> [BZ #24122]
> * elf/Makefile (tests): Add tst-audit13.
> (modules-names): Add tst-audit13mod1.
> (tst-audit13.out, LDFLAGS-tst-audit13mod1.so, tst-audit13-ENV): New
> rule.
> * elf/rtld.c (dl_main): Handle invalid audit module version.
> * elf/tst-audit13.c: New file.
> * elf/tst-audit13mod1.c: Likewise.
>
> --
>
> diff --git a/elf/Makefile b/elf/Makefile
> index 9cf5cd8dfd..c24d765730 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -187,7 +187,7 @@ tests += restest1 preloadtest loadfail multiload origtest resolvfail \
> tst-nodelete2 tst-audit11 tst-audit12 tst-dlsym-error tst-noload \
> tst-latepthread tst-tls-manydynamic tst-nodelete-dlclose \
> tst-debug1 tst-main1 tst-absolute-sym tst-absolute-zero tst-big-note \
> - tst-unwind-ctor tst-unwind-main
> + tst-unwind-ctor tst-unwind-main tst-audit13
OK.
> # reldep9
> tests-internal += loadtest unload unload2 circleload1 \
> neededtest neededtest2 neededtest3 neededtest4 \
> @@ -275,7 +275,8 @@ modules-names = testobj1 testobj2 testobj3 testobj4 testobj5 testobj6 \
> tst-latepthreadmod $(tst-tls-many-dynamic-modules) \
> tst-nodelete-dlclose-dso tst-nodelete-dlclose-plugin \
> tst-main1mod tst-libc_dlvsym-dso tst-absolute-sym-lib \
> - tst-absolute-zero-lib tst-big-note-lib tst-unwind-ctor-lib
> + tst-absolute-zero-lib tst-big-note-lib tst-unwind-ctor-lib \
> + tst-audit13mod1
OK.
> # Most modules build with _ISOMAC defined, but those filtered out
> # depend on internal headers.
> modules-names-tests = $(filter-out ifuncmod% tst-libc_dlvsym-dso tst-tlsmod%,\
> @@ -1382,6 +1383,10 @@ tst-audit12-ENV = LD_AUDIT=$(objpfx)tst-auditmod12.so
> $(objpfx)tst-audit12mod1.so: $(objpfx)tst-audit12mod2.so
> LDFLAGS-tst-audit12mod2.so = -Wl,--version-script=tst-audit12mod2.map
>
> +$(objpfx)tst-audit13.out: $(objpfx)tst-audit13mod1.so
> +LDFLAGS-tst-audit13mod1.so = -Wl,-z,lazy
> +tst-audit13-ENV = LD_AUDIT=$(objpfx)tst-audit13mod1.so
OK.
> +
> # Override -z defs, so that we can reference an undefined symbol.
> # Force lazy binding for the same reason.
> LDFLAGS-tst-latepthreadmod.so = \
> diff --git a/elf/rtld.c b/elf/rtld.c
> index 5d97f41b7b..5abb345b1e 100644
> --- a/elf/rtld.c
> +++ b/elf/rtld.c
> @@ -1453,10 +1453,12 @@ ERROR: ld.so: object '%s' cannot be loaded as audit interface: %s; ignored.\n",
>
> unsigned int (*laversion) (unsigned int);
> unsigned int lav;
> - if (err_str == NULL
> - && (laversion = largs.result) != NULL
> - && (lav = laversion (LAV_CURRENT)) > 0
> - && lav <= LAV_CURRENT)
> + if (err_str != NULL)
> + goto not_loaded;
OK, goto not_loaded if we had a real dlopen failure.
> +
> + if ((laversion = largs.result) != NULL
> + && (lav = laversion (LAV_CURRENT)) > 0
> + && lav <= LAV_CURRENT)
OK.
> {
> /* Allocate structure for the callback function pointers.
> This call can never fail. */
> @@ -1538,7 +1540,18 @@ ERROR: ld.so: object '%s' cannot be loaded as audit interface: %s; ignored.\n",
> assert (GL(dl_ns)[ns]._ns_nloaded == 0);
>
> GL(dl_tls_max_dtv_idx) = tls_idx;
> - goto not_loaded;
> + if (GLRO(dl_debug_mask) & DL_DEBUG_FILES)
OK, I think DL_DEBUG_FILES is an acceptable place to put the information.
Thanks for fixing this part.
> + {
> + _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
> + _dl_debug_printf (
> +" invalid version '%u' (expected minimum of '%u').\n",
> + lav, LAV_CURRENT);
It is not "invalid" and I think printing that will lead to confusion.
Likewise "expected minimum" ignores that 0 is low-enough but should also be ignored.
I think this needs a cleanup, and I should have been clearer:
For lav == 0 we should print "Auditor requested to be ignored (returned version of 0)."
For lav > LAV_CURRENT "Auditor disabled since expected version %d is greater than supported version %d."
All the information a developer needs is now in those messages.
We should be clear about why it's disabled.
> + }
> }
> }
> }
> diff --git a/elf/tst-audit13.c b/elf/tst-audit13.c
> new file mode 100644
> index 0000000000..6f587baf58
> --- /dev/null
> +++ b/elf/tst-audit13.c
> @@ -0,0 +1,28 @@
> +/* Check for invalid audit version (BZ#24122).
> + Copyright (C) 2019 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <http://www.gnu.org/licenses/>. */
> +
> +#include <stdio.h>
> +
> +static int
> +do_test (void)
> +{
> + puts ("plt call");
> + return 0;
> +}
OK.
> +
> +#include <support/test-driver.c>
> diff --git a/elf/tst-audit13mod1.c b/elf/tst-audit13mod1.c
> new file mode 100644
> index 0000000000..598dab5ec4
> --- /dev/null
> +++ b/elf/tst-audit13mod1.c
> @@ -0,0 +1,93 @@
> +/* Check for invalid audit version (BZ#24122).
> + Copyright (C) 2019 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <http://www.gnu.org/licenses/>. */
> +
> +#include <link.h>
> +#include <stdlib.h>
> +
> +unsigned int
> +la_version (unsigned int version)
> +{
> + /* The audit specification says that a version of 0 or a version
> + greater than any version supported by the dynamic loader shall
> + cause the module to be ignored. */
> + return 0;
> +}
OK.
> +
> +void
> +la_activity (uintptr_t *cookie, unsigned int flag)
> +{
> + exit (EXIT_FAILURE);
> +}
> +
> +char *
> +la_objsearch (const char *name, uintptr_t *cookie, unsigned int flag)
> +{
> + exit (EXIT_FAILURE);
> +}
> +
> +unsigned int
> +la_objopen (struct link_map *map, Lmid_t lmid, uintptr_t * cookie)
> +{
> + exit (EXIT_FAILURE);
> +}
> +
> +void
> +la_preinit (uintptr_t * cookie)
> +{
> + exit (EXIT_FAILURE);
> +}
> +
> +uintptr_t
> +#if __ELF_NATIVE_CLASS == 32
> +la_symbind32 (Elf32_Sym *sym, unsigned int ndx, uintptr_t *refcook,
> + uintptr_t *defcook, unsigned int *flags, const char *symname)
> +#else
> +la_symbind64 (Elf64_Sym *sym, unsigned int ndx, uintptr_t *refcook,
> + uintptr_t *defcook, unsigned int *flags, const char *symname)
> +#endif
> +{
> + exit (EXIT_FAILURE);
> +}
> +
> +unsigned int
> +la_objclose (uintptr_t * cookie)
> +{
> + exit (EXIT_FAILURE);
> +}
> +
> +#include <tst-audit.h>
> +#if (!defined (pltenter) || !defined (pltexit) || !defined (La_regs) \
> + || !defined (La_retval) || !defined (int_retval))
> +# error "architecture specific code needed in sysdeps/CPU/tst-audit.h"
> +#endif
> +
> +ElfW(Addr)
> +pltenter (ElfW(Sym) *sym, unsigned int ndx, uintptr_t *refcook,
> + uintptr_t *defcook, La_regs *regs, unsigned int *flags,
> + const char *symname, long int *framesizep)
> +{
> + exit (EXIT_FAILURE);
> +}
> +
> +unsigned int
> +pltexit (ElfW(Sym) *sym, unsigned int ndx, uintptr_t *refcook,
> + uintptr_t *defcook, const La_regs *inregs, La_retval *outregs,
> + const char *symname)
> +{
> + exit (EXIT_FAILURE);
> +}
>
On 23/01/2019 16:02, Carlos O'Donell wrote:
> On 1/23/19 12:44 PM, Adhemerval Zanella wrote:
>>
>>
>> On 23/01/2019 12:58, Carlos O'Donell wrote:
>>> On 1/23/19 9:07 AM, Adhemerval Zanella wrote:
>>>
>>> Thanks for fixing this!
>>>
>>> I'd like to see v2 please
>>>
>>> - Only output message when LD_DEBUG=all, since la_version returning 0
>>> or > LAV_CURRENT is not an error, but simply an API mandated situation
>>> where we ignore the module. I don't see that as an error.
>>>
>>> The Solaris docs say:
>>> "If the audit library return is zero, or a version that is greater
>>> than the rtld-audit interface the runtime linker supports, the audit
>>> library is discarded."
>>>
>>> Just discarded. It is up to the auditor to decide how it exposes to
>>> the user that it is running and operating as expected. We can provide
>>> feedback via LD_DEBUG=all.
>>
>> Right, this rationale make sense. However I think this information is
>> relates to the 'files' option, instead of print when 'all' option are
>> selected.
>>
>>>
>>> - Optional: use -Wl,z,lazy for the test, or remove the comment about
>>> the PLT call.
>>
>> The idea is indeed to force PLT call, so the PLT audit stub is called
>> and tests explicit fails.
>>
>>>
>>> - Suggested comment in tst-audit13mod1.c
>>
>> Ack. Patch updated below.
>>
>
> OK for master if you fix the error messages as noted below.
>
> Reviewed-by: Carlos O'Donell <carlos@redhat.com>
>
>> ---
>>
>> [BZ #24122]
>> * elf/Makefile (tests): Add tst-audit13.
>> (modules-names): Add tst-audit13mod1.
>> (tst-audit13.out, LDFLAGS-tst-audit13mod1.so, tst-audit13-ENV): New
>> rule.
>> * elf/rtld.c (dl_main): Handle invalid audit module version.
>> * elf/tst-audit13.c: New file.
>> * elf/tst-audit13mod1.c: Likewise.
>>
>> --
>>
>> diff --git a/elf/Makefile b/elf/Makefile
>> index 9cf5cd8dfd..c24d765730 100644
>> --- a/elf/Makefile
>> +++ b/elf/Makefile
>> @@ -187,7 +187,7 @@ tests += restest1 preloadtest loadfail multiload origtest resolvfail \
>> tst-nodelete2 tst-audit11 tst-audit12 tst-dlsym-error tst-noload \
>> tst-latepthread tst-tls-manydynamic tst-nodelete-dlclose \
>> tst-debug1 tst-main1 tst-absolute-sym tst-absolute-zero tst-big-note \
>> - tst-unwind-ctor tst-unwind-main
>> + tst-unwind-ctor tst-unwind-main tst-audit13
>
> OK.
>
>> # reldep9
>> tests-internal += loadtest unload unload2 circleload1 \
>> neededtest neededtest2 neededtest3 neededtest4 \
>> @@ -275,7 +275,8 @@ modules-names = testobj1 testobj2 testobj3 testobj4 testobj5 testobj6 \
>> tst-latepthreadmod $(tst-tls-many-dynamic-modules) \
>> tst-nodelete-dlclose-dso tst-nodelete-dlclose-plugin \
>> tst-main1mod tst-libc_dlvsym-dso tst-absolute-sym-lib \
>> - tst-absolute-zero-lib tst-big-note-lib tst-unwind-ctor-lib
>> + tst-absolute-zero-lib tst-big-note-lib tst-unwind-ctor-lib \
>> + tst-audit13mod1
>
> OK.
>
>> # Most modules build with _ISOMAC defined, but those filtered out
>> # depend on internal headers.
>> modules-names-tests = $(filter-out ifuncmod% tst-libc_dlvsym-dso tst-tlsmod%,\
>> @@ -1382,6 +1383,10 @@ tst-audit12-ENV = LD_AUDIT=$(objpfx)tst-auditmod12.so
>> $(objpfx)tst-audit12mod1.so: $(objpfx)tst-audit12mod2.so
>> LDFLAGS-tst-audit12mod2.so = -Wl,--version-script=tst-audit12mod2.map
>>
>> +$(objpfx)tst-audit13.out: $(objpfx)tst-audit13mod1.so
>> +LDFLAGS-tst-audit13mod1.so = -Wl,-z,lazy
>> +tst-audit13-ENV = LD_AUDIT=$(objpfx)tst-audit13mod1.so
>
> OK.
>
>> +
>> # Override -z defs, so that we can reference an undefined symbol.
>> # Force lazy binding for the same reason.
>> LDFLAGS-tst-latepthreadmod.so = \
>> diff --git a/elf/rtld.c b/elf/rtld.c
>> index 5d97f41b7b..5abb345b1e 100644
>> --- a/elf/rtld.c
>> +++ b/elf/rtld.c
>> @@ -1453,10 +1453,12 @@ ERROR: ld.so: object '%s' cannot be loaded as audit interface: %s; ignored.\n",
>>
>> unsigned int (*laversion) (unsigned int);
>> unsigned int lav;
>> - if (err_str == NULL
>> - && (laversion = largs.result) != NULL
>> - && (lav = laversion (LAV_CURRENT)) > 0
>> - && lav <= LAV_CURRENT)
>> + if (err_str != NULL)
>> + goto not_loaded;
>
> OK, goto not_loaded if we had a real dlopen failure.
>
>> +
>> + if ((laversion = largs.result) != NULL
>> + && (lav = laversion (LAV_CURRENT)) > 0
>> + && lav <= LAV_CURRENT)
>
> OK.
>
>> {
>> /* Allocate structure for the callback function pointers.
>> This call can never fail. */
>> @@ -1538,7 +1540,18 @@ ERROR: ld.so: object '%s' cannot be loaded as audit interface: %s; ignored.\n",
>> assert (GL(dl_ns)[ns]._ns_nloaded == 0);
>>
>> GL(dl_tls_max_dtv_idx) = tls_idx;
>> - goto not_loaded;
>> + if (GLRO(dl_debug_mask) & DL_DEBUG_FILES)
>
> OK, I think DL_DEBUG_FILES is an acceptable place to put the information.
> Thanks for fixing this part.
>
>> + {
>> + _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
>> + _dl_debug_printf (
>> +" invalid version '%u' (expected minimum of '%u').\n",
>> + lav, LAV_CURRENT);
>
> It is not "invalid" and I think printing that will lead to confusion.
>
> Likewise "expected minimum" ignores that 0 is low-enough but should also be ignored.
>
> I think this needs a cleanup, and I should have been clearer:
>
> For lav == 0 we should print "Auditor requested to be ignored (returned version of 0)."
>
> For lav > LAV_CURRENT "Auditor disabled since expected version %d is greater than supported version %d."
>
> All the information a developer needs is now in those messages.
>
> We should be clear about why it's disabled.
Right, I changed to:
---
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);
}
}
On 23/01/2019 17:00, Adhemerval Zanella wrote:
> On 23/01/2019 16:02, Carlos O'Donell wrote:
>> It is not "invalid" and I think printing that will lead to confusion.
>>
>> Likewise "expected minimum" ignores that 0 is low-enough but should also be ignored.
>>
>> I think this needs a cleanup, and I should have been clearer:
>>
>> For lav == 0 we should print "Auditor requested to be ignored (returned version of 0)."
>>
>> For lav > LAV_CURRENT "Auditor disabled since expected version %d is greater than supported version %d."
>>
>> All the information a developer needs is now in those messages.
>>
>> We should be clear about why it's disabled.
>
> Right, I changed to:
>
> ---
> 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);
> }
> }
>
Siddhesh,
It is acceptable for 2.29?
On 1/23/19 2:59 PM, Adhemerval Zanella wrote:
>
>
> On 23/01/2019 17:00, Adhemerval Zanella wrote:
>> On 23/01/2019 16:02, Carlos O'Donell wrote:
>>> It is not "invalid" and I think printing that will lead to confusion.
>>>
>>> Likewise "expected minimum" ignores that 0 is low-enough but should also be ignored.
>>>
>>> I think this needs a cleanup, and I should have been clearer:
>>>
>>> For lav == 0 we should print "Auditor requested to be ignored (returned version of 0)."
>>>
>>> For lav > LAV_CURRENT "Auditor disabled since expected version %d is greater than supported version %d."
>>>
>>> All the information a developer needs is now in those messages.
>>>
>>> We should be clear about why it's disabled.
>>
>> Right, I changed to:
Awesome, these are much clearer debug messages.
>> ---
>> 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);
>> }
>> }
>>
>
> 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>
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
Siddhesh
* 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?
Thanks,
Florian
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.
@@ -187,7 +187,7 @@ tests += restest1 preloadtest loadfail multiload origtest resolvfail \
tst-nodelete2 tst-audit11 tst-audit12 tst-dlsym-error tst-noload \
tst-latepthread tst-tls-manydynamic tst-nodelete-dlclose \
tst-debug1 tst-main1 tst-absolute-sym tst-absolute-zero tst-big-note \
- tst-unwind-ctor tst-unwind-main
+ tst-unwind-ctor tst-unwind-main tst-audit13
# reldep9
tests-internal += loadtest unload unload2 circleload1 \
neededtest neededtest2 neededtest3 neededtest4 \
@@ -275,7 +275,8 @@ modules-names = testobj1 testobj2 testobj3 testobj4 testobj5 testobj6 \
tst-latepthreadmod $(tst-tls-many-dynamic-modules) \
tst-nodelete-dlclose-dso tst-nodelete-dlclose-plugin \
tst-main1mod tst-libc_dlvsym-dso tst-absolute-sym-lib \
- tst-absolute-zero-lib tst-big-note-lib tst-unwind-ctor-lib
+ tst-absolute-zero-lib tst-big-note-lib tst-unwind-ctor-lib \
+ tst-audit13mod1
# Most modules build with _ISOMAC defined, but those filtered out
# depend on internal headers.
modules-names-tests = $(filter-out ifuncmod% tst-libc_dlvsym-dso tst-tlsmod%,\
@@ -1382,6 +1383,10 @@ tst-audit12-ENV = LD_AUDIT=$(objpfx)tst-auditmod12.so
$(objpfx)tst-audit12mod1.so: $(objpfx)tst-audit12mod2.so
LDFLAGS-tst-audit12mod2.so = -Wl,--version-script=tst-audit12mod2.map
+$(objpfx)tst-audit13.out: $(objpfx)tst-audit13mod1.so
+LDFLAGS-tst-audit13mod1.so = -Wl,-z,lazy
+tst-audit13-ENV = LD_AUDIT=$(objpfx)tst-audit13mod1.so
+
# Override -z defs, so that we can reference an undefined symbol.
# Force lazy binding for the same reason.
LDFLAGS-tst-latepthreadmod.so = \
@@ -1453,10 +1453,12 @@ ERROR: ld.so: object '%s' cannot be loaded as audit interface: %s; ignored.\n",
unsigned int (*laversion) (unsigned int);
unsigned int lav;
- if (err_str == NULL
- && (laversion = largs.result) != NULL
- && (lav = laversion (LAV_CURRENT)) > 0
- && lav <= LAV_CURRENT)
+ 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. */
@@ -1538,7 +1540,18 @@ ERROR: ld.so: object '%s' cannot be loaded as audit interface: %s; ignored.\n",
assert (GL(dl_ns)[ns]._ns_nloaded == 0);
GL(dl_tls_max_dtv_idx) = tls_idx;
- goto not_loaded;
+ 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
+ _dl_debug_printf (
+" invalid version '%u' (expected minimum of '%u').\n",
+ lav, LAV_CURRENT);
+ }
}
}
}
new file mode 100644
@@ -0,0 +1,28 @@
+/* Check for invalid audit version (BZ#24122).
+ Copyright (C) 2019 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <http://www.gnu.org/licenses/>. */
+
+#include <stdio.h>
+
+static int
+do_test (void)
+{
+ puts ("plt call");
+ return 0;
+}
+
+#include <support/test-driver.c>
new file mode 100644
@@ -0,0 +1,93 @@
+/* Check for invalid audit version (BZ#24122).
+ Copyright (C) 2019 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <http://www.gnu.org/licenses/>. */
+
+#include <link.h>
+#include <stdlib.h>
+
+unsigned int
+la_version (unsigned int version)
+{
+ /* The audit specification says that a version of 0 or a version
+ greater than any version supported by the dynamic loader shall
+ cause the module to be ignored. */
+ return 0;
+}
+
+void
+la_activity (uintptr_t *cookie, unsigned int flag)
+{
+ exit (EXIT_FAILURE);
+}
+
+char *
+la_objsearch (const char *name, uintptr_t *cookie, unsigned int flag)
+{
+ exit (EXIT_FAILURE);
+}
+
+unsigned int
+la_objopen (struct link_map *map, Lmid_t lmid, uintptr_t * cookie)
+{
+ exit (EXIT_FAILURE);
+}
+
+void
+la_preinit (uintptr_t * cookie)
+{
+ exit (EXIT_FAILURE);
+}
+
+uintptr_t
+#if __ELF_NATIVE_CLASS == 32
+la_symbind32 (Elf32_Sym *sym, unsigned int ndx, uintptr_t *refcook,
+ uintptr_t *defcook, unsigned int *flags, const char *symname)
+#else
+la_symbind64 (Elf64_Sym *sym, unsigned int ndx, uintptr_t *refcook,
+ uintptr_t *defcook, unsigned int *flags, const char *symname)
+#endif
+{
+ exit (EXIT_FAILURE);
+}
+
+unsigned int
+la_objclose (uintptr_t * cookie)
+{
+ exit (EXIT_FAILURE);
+}
+
+#include <tst-audit.h>
+#if (!defined (pltenter) || !defined (pltexit) || !defined (La_regs) \
+ || !defined (La_retval) || !defined (int_retval))
+# error "architecture specific code needed in sysdeps/CPU/tst-audit.h"
+#endif
+
+ElfW(Addr)
+pltenter (ElfW(Sym) *sym, unsigned int ndx, uintptr_t *refcook,
+ uintptr_t *defcook, La_regs *regs, unsigned int *flags,
+ const char *symname, long int *framesizep)
+{
+ exit (EXIT_FAILURE);
+}
+
+unsigned int
+pltexit (ElfW(Sym) *sym, unsigned int ndx, uintptr_t *refcook,
+ uintptr_t *defcook, const La_regs *inregs, La_retval *outregs,
+ const char *symname)
+{
+ exit (EXIT_FAILURE);
+}