[1/5] elf: Fix audit regression
Checks
Context |
Check |
Description |
dj/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
Commit Message
Commit 03e187a41d9 added a regression when an audit module does not have
libc as DT_NEEDED (although unusual it is possible).
Checked on x86_64-linux-gnu.
---
elf/Makefile | 12 +++++++++++-
elf/dl-open.c | 2 +-
elf/tst-audit17.c | 25 +++++++++++++++++++++++++
elf/tst-auditmod17.c | 24 ++++++++++++++++++++++++
4 files changed, 61 insertions(+), 2 deletions(-)
create mode 100644 elf/tst-audit17.c
create mode 100644 elf/tst-auditmod17.c
Comments
* Adhemerval Zanella via Libc-alpha:
> diff --git a/elf/Makefile b/elf/Makefile
> index b1e01d9516..5214196de6 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -361,6 +361,7 @@ modules-names += tst-gnu2-tls1mod
> $(objpfx)tst-gnu2-tls1: $(objpfx)tst-gnu2-tls1mod.so
> tst-gnu2-tls1mod.so-no-z-defs = yes
> CFLAGS-tst-gnu2-tls1mod.c += -mtls-dialect=gnu2
> +
Spurious whitespace change. Or rather, missing CFLAGS for
tst-auditmod17 to disable stack protector. I expect the test to fail
when building glibc with -fstack-protector-all.
> diff --git a/elf/dl-open.c b/elf/dl-open.c
> index a066f39bd0..66ec9d7ed5 100644
> --- a/elf/dl-open.c
> +++ b/elf/dl-open.c
> @@ -771,7 +771,7 @@ dl_open_worker (void *a)
> {
> struct link_map *libc_map = GL(dl_ns)[args->nsid].libc_map;
> #ifdef SHARED
> - bool initial = libc_map->l_ns == LM_ID_BASE;
> + bool initial = libc_map != NULL ? libc_map->l_ns == LM_ID_BASE : false;
> #else
> /* In the static case, there is only one namespace, but it
> contains a secondary libc (the primary libc is statically
Looks okay to me. The previous code was just broken.
Thanks,
Florian
On 07/07/2021 16:02, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
>
>> diff --git a/elf/Makefile b/elf/Makefile
>> index b1e01d9516..5214196de6 100644
>> --- a/elf/Makefile
>> +++ b/elf/Makefile
>
>> @@ -361,6 +361,7 @@ modules-names += tst-gnu2-tls1mod
>> $(objpfx)tst-gnu2-tls1: $(objpfx)tst-gnu2-tls1mod.so
>> tst-gnu2-tls1mod.so-no-z-defs = yes
>> CFLAGS-tst-gnu2-tls1mod.c += -mtls-dialect=gnu2
>> +
>
> Spurious whitespace change. Or rather, missing CFLAGS for
> tst-auditmod17 to disable stack protector. I expect the test to fail
> when building glibc with -fstack-protector-all.
I will check building with -fstack-protector-all and remove the
spurious whitespace.
>
>> diff --git a/elf/dl-open.c b/elf/dl-open.c
>> index a066f39bd0..66ec9d7ed5 100644
>> --- a/elf/dl-open.c
>> +++ b/elf/dl-open.c
>> @@ -771,7 +771,7 @@ dl_open_worker (void *a)
>> {
>> struct link_map *libc_map = GL(dl_ns)[args->nsid].libc_map;
>> #ifdef SHARED
>> - bool initial = libc_map->l_ns == LM_ID_BASE;
>> + bool initial = libc_map != NULL ? libc_map->l_ns == LM_ID_BASE : false;
>> #else
>> /* In the static case, there is only one namespace, but it
>> contains a secondary libc (the primary libc is statically
>
> Looks okay to me. The previous code was just broken.
>
> Thanks,
> Florian
>
On Jul 07 2021, Adhemerval Zanella via Libc-alpha wrote:
> @@ -771,7 +771,7 @@ dl_open_worker (void *a)
> {
> struct link_map *libc_map = GL(dl_ns)[args->nsid].libc_map;
> #ifdef SHARED
> - bool initial = libc_map->l_ns == LM_ID_BASE;
> + bool initial = libc_map != NULL ? libc_map->l_ns == LM_ID_BASE : false;
bool initial = libc_map != NULL && libc_map->l_ns == LM_ID_BASE;
Andreas.
On 07/07/2021 16:51, Andreas Schwab wrote:
> On Jul 07 2021, Adhemerval Zanella via Libc-alpha wrote:
>
>> @@ -771,7 +771,7 @@ dl_open_worker (void *a)
>> {
>> struct link_map *libc_map = GL(dl_ns)[args->nsid].libc_map;
>> #ifdef SHARED
>> - bool initial = libc_map->l_ns == LM_ID_BASE;
>> + bool initial = libc_map != NULL ? libc_map->l_ns == LM_ID_BASE : false;
>
> bool initial = libc_map != NULL && libc_map->l_ns == LM_ID_BASE;
>
> Andreas.
>
Ack.
* Andreas Schwab:
> On Jul 07 2021, Adhemerval Zanella via Libc-alpha wrote:
>
>> @@ -771,7 +771,7 @@ dl_open_worker (void *a)
>> {
>> struct link_map *libc_map = GL(dl_ns)[args->nsid].libc_map;
>> #ifdef SHARED
>> - bool initial = libc_map->l_ns == LM_ID_BASE;
>> + bool initial = libc_map != NULL ? libc_map->l_ns == LM_ID_BASE : false;
>
> bool initial = libc_map != NULL && libc_map->l_ns == LM_ID_BASE;
True … but:
This is only used by dlopen/dlmopen, right? And even if dlmopen is
called from an auditor (to load another libc), it is *never* the initial
libc in the base namespace.
The actual base namespace libc is handled in elf/rtld.c:dl_main:
/* Relocation is complete. Perform early libc initialization. This
is the initial libc, even if audit modules have been loaded with
other libcs. */
_dl_call_libc_early_init (GL(dl_ns)[LM_ID_BASE].libc_map, true);
And I think the dl_open_worker should mirror that and just do:
if (!args->libc_already_loaded)
/* This is never the initial libc because it has been loaded via
dlmopen. */
_dl_call_libc_early_init (libc_map, false);
Thanks,
Florian
* Florian Weimer:
> * Andreas Schwab:
>
>> On Jul 07 2021, Adhemerval Zanella via Libc-alpha wrote:
>>
>>> @@ -771,7 +771,7 @@ dl_open_worker (void *a)
>>> {
>>> struct link_map *libc_map = GL(dl_ns)[args->nsid].libc_map;
>>> #ifdef SHARED
>>> - bool initial = libc_map->l_ns == LM_ID_BASE;
>>> + bool initial = libc_map != NULL ? libc_map->l_ns == LM_ID_BASE : false;
>>
>> bool initial = libc_map != NULL && libc_map->l_ns == LM_ID_BASE;
>
> True … but:
>
> This is only used by dlopen/dlmopen, right? And even if dlmopen is
> called from an auditor (to load another libc), it is *never* the initial
> libc in the base namespace.
>
> The actual base namespace libc is handled in elf/rtld.c:dl_main:
>
> /* Relocation is complete. Perform early libc initialization. This
> is the initial libc, even if audit modules have been loaded with
> other libcs. */
> _dl_call_libc_early_init (GL(dl_ns)[LM_ID_BASE].libc_map, true);
>
> And I think the dl_open_worker should mirror that and just do:
>
> if (!args->libc_already_loaded)
> /* This is never the initial libc because it has been loaded via
> dlmopen. */
> _dl_call_libc_early_init (libc_map, false);
Eh, or rather:
if (!args->libc_already_loaded)
{
struct link_map *libc_map = GL(dl_ns)[args->nsid].libc_map;
/* This is never the initial libc because it has been loaded via
dlmopen. */
_dl_call_libc_early_init (libc_map, false);
}
(_dl_call_libc_early_init checks for a null link map.)
Thanks,
Florian
On 07/07/2021 17:07, Florian Weimer via Libc-alpha wrote:
> * Florian Weimer:
>
>> * Andreas Schwab:
>>
>>> On Jul 07 2021, Adhemerval Zanella via Libc-alpha wrote:
>>>
>>>> @@ -771,7 +771,7 @@ dl_open_worker (void *a)
>>>> {
>>>> struct link_map *libc_map = GL(dl_ns)[args->nsid].libc_map;
>>>> #ifdef SHARED
>>>> - bool initial = libc_map->l_ns == LM_ID_BASE;
>>>> + bool initial = libc_map != NULL ? libc_map->l_ns == LM_ID_BASE : false;
>>>
>>> bool initial = libc_map != NULL && libc_map->l_ns == LM_ID_BASE;
>>
>> True … but:
>>
>> This is only used by dlopen/dlmopen, right? And even if dlmopen is
>> called from an auditor (to load another libc), it is *never* the initial
>> libc in the base namespace.
>>
>> The actual base namespace libc is handled in elf/rtld.c:dl_main:
>>
>> /* Relocation is complete. Perform early libc initialization. This
>> is the initial libc, even if audit modules have been loaded with
>> other libcs. */
>> _dl_call_libc_early_init (GL(dl_ns)[LM_ID_BASE].libc_map, true);
>>
>> And I think the dl_open_worker should mirror that and just do:
>>
>> if (!args->libc_already_loaded)
>> /* This is never the initial libc because it has been loaded via
>> dlmopen. */
>> _dl_call_libc_early_init (libc_map, false);
>
> Eh, or rather:
>
> if (!args->libc_already_loaded)
> {
> struct link_map *libc_map = GL(dl_ns)[args->nsid].libc_map;
> /* This is never the initial libc because it has been loaded via
> dlmopen. */
> _dl_call_libc_early_init (libc_map, false);
> }
>
> (_dl_call_libc_early_init checks for a null link map.)
I think this make sense and it also simplifes the code a bit. I will
adjust the patch.
@@ -219,7 +219,7 @@ tests += restest1 preloadtest loadfail multiload origtest resolvfail \
tst-dlopen-self tst-auditmany tst-initfinilazyfail tst-dlopenfail \
tst-dlopenfail-2 \
tst-filterobj tst-filterobj-dlopen tst-auxobj tst-auxobj-dlopen \
- tst-audit14 tst-audit15 tst-audit16 \
+ tst-audit14 tst-audit15 tst-audit16 tst-audit17 \
tst-single_threaded tst-single_threaded-pthread \
tst-tls-ie tst-tls-ie-dlmopen argv0test \
tst-glibc-hwcaps tst-glibc-hwcaps-prepend tst-glibc-hwcaps-mask \
@@ -361,6 +361,7 @@ modules-names += tst-gnu2-tls1mod
$(objpfx)tst-gnu2-tls1: $(objpfx)tst-gnu2-tls1mod.so
tst-gnu2-tls1mod.so-no-z-defs = yes
CFLAGS-tst-gnu2-tls1mod.c += -mtls-dialect=gnu2
+
endif
ifeq (yes,$(have-protected-data))
modules-names += tst-protected1moda tst-protected1modb
@@ -1467,6 +1468,15 @@ $(objpfx)tst-auditlogmod-3.so: $(libsupport)
$(objpfx)tst-audit16.out: \
$(objpfx)tst-auditlogmod-1.so $(objpfx)tst-auditlogmod-2.so \
$(objpfx)tst-auditlogmod-3.so
+$(objpfx)tst-audit17.out: $(objpfx)tst-auditmod17.so
+# The test check if a audit library without libc.so on DT_NEEDED works as
+# intended, so it uses an explicit link rule.
+$(objpfx)tst-auditmod17.so: $(objpfx)tst-auditmod17.os
+ $(CC) -nostdlib -nostartfiles -shared -o $@.new \
+ $(filter-out $(map-file),$^)
+ $(call after-link,$@.new)
+ mv -f $@.new $@
+tst-audit17-ENV = LD_AUDIT=$(objpfx)tst-auditmod17.so
# tst-sonamemove links against an older implementation of the library.
LDFLAGS-tst-sonamemove-linkmod1.so = \
@@ -771,7 +771,7 @@ dl_open_worker (void *a)
{
struct link_map *libc_map = GL(dl_ns)[args->nsid].libc_map;
#ifdef SHARED
- bool initial = libc_map->l_ns == LM_ID_BASE;
+ bool initial = libc_map != NULL ? libc_map->l_ns == LM_ID_BASE : false;
#else
/* In the static case, there is only one namespace, but it
contains a secondary libc (the primary libc is statically
new file mode 100644
@@ -0,0 +1,25 @@
+/* Check DT_AUDIT with audit not linked against libc.
+ Copyright (C) 2021 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
+ <https://www.gnu.org/licenses/>. */
+
+static int
+do_test (void)
+{
+ return 0;
+}
+
+#include <support/test-driver.c>
new file mode 100644
@@ -0,0 +1,24 @@
+/* Check DT_AUDIT with audit not linked against libc.
+ Copyright (C) 2021 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
+ <https://www.gnu.org/licenses/>. */
+
+unsigned int
+la_version (unsigned int version)
+{
+ return version;
+}
+