Skip PT_DYNAMIC segment if its p_filesz == 0 [BZ #22101]
Commit Message
ELF object generated with "objcopy --only-keep-debug" has
Type Offset VirtAddr PhysAddr FileSiz MemSiz Flg Align
DYNAMIC 0x0+e28 0x0+200e40 0x0+200e40 0x0+ 0x0+1a0 RW 0x8
with 0 file size. ld.so should skip such PT_DYNAMIC segments.
Tested on x86-64. OK for master?
H.J.
---
[BZ #22101]
* elf/Makefile (tests): Add tst-debug1.
($(objpfx)tst-debug1): New.
($(objpfx)tst-debug1.out): Likewise.
($(objpfx)tst-debug1mod1.so): Likewise.
* elf/dl-load.c (_dl_map_object_from_fd): Skip PT_DYNAMIC segment
if its p_filesz == 0.
* elf/tst-debug1.c: New file.
---
elf/Makefile | 9 ++++++++-
elf/dl-load.c | 7 +++++--
elf/tst-debug1.c | 34 ++++++++++++++++++++++++++++++++++
3 files changed, 47 insertions(+), 3 deletions(-)
create mode 100644 elf/tst-debug1.c
Comments
On Mon, Sep 25, 2017 at 8:33 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> ELF object generated with "objcopy --only-keep-debug" has
>
> Type Offset VirtAddr PhysAddr FileSiz MemSiz Flg Align
> DYNAMIC 0x0+e28 0x0+200e40 0x0+200e40 0x0+ 0x0+1a0 RW 0x8
>
> with 0 file size. ld.so should skip such PT_DYNAMIC segments.
Why should ld.so skip such PT_DYNAMIC segments? It seems like loading
empty segments should be harmless, and ...
> + void *h = dlopen ("tst-debug1mod1.so", RTLD_LAZY);
> + if (h != NULL)
> + {
> + puts ("shouldn't load tst-debug1mod1.so");
... the implications of this test are scary: I suspect there is code
out there that will break if dlopen starts returning NULL on
completely empty shared objects.
zw
On 9/25/17, Zack Weinberg <zackw@panix.com> wrote:
> On Mon, Sep 25, 2017 at 8:33 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> ELF object generated with "objcopy --only-keep-debug" has
>>
>> Type Offset VirtAddr PhysAddr FileSiz MemSiz Flg Align
>> DYNAMIC 0x0+e28 0x0+200e40 0x0+200e40 0x0+ 0x0+1a0 RW 0x8
>>
>> with 0 file size. ld.so should skip such PT_DYNAMIC segments.
>
> Why should ld.so skip such PT_DYNAMIC segments? It seems like loading
> empty segments should be harmless, and ...
Not all valid segments can have zero p_filesz. The valid PT_DYNAMIC segment
should have non-zero p_filesz.
>> + void *h = dlopen ("tst-debug1mod1.so", RTLD_LAZY);
>> + if (h != NULL)
>> + {
>> + puts ("shouldn't load tst-debug1mod1.so");
>
> ... the implications of this test are scary: I suspect there is code
> out there that will break if dlopen starts returning NULL on
My change doesn't simply return NULL. It just skips PT_DYNAMIC segments
with zero p_filesz. dlopen returns NULL because invalid PT_DYNAMIC segment.
> completely empty shared objects.
Even completely empty shared objects should have valid PT_DYNAMIC segment:
[hjl@gnu-efi-2 tmp]$ cat foo.s
[hjl@gnu-efi-2 tmp]$ gcc -c foo.s
[hjl@gnu-efi-2 tmp]$ ld -shared foo.o
[hjl@gnu-efi-2 tmp]$ readelf -lW a.out
Elf file type is DYN (Shared object file)
Entry point 0x1f1
There are 4 program headers, starting at offset 64
Program Headers:
Type Offset VirtAddr PhysAddr
FileSiz MemSiz Flg Align
LOAD 0x000000 0x0000000000000000 0x0000000000000000
0x0001f8 0x0001f8 R 0x200000
LOAD 0x000f40 0x0000000000200f40 0x0000000000200f40
0x0000c0 0x0000c0 RW 0x200000
DYNAMIC 0x000f40 0x0000000000200f40 0x0000000000200f40
0x0000c0 0x0000c0 RW 0x8
GNU_RELRO 0x000f40 0x0000000000200f40 0x0000000000200f40
0x0000c0 0x0000c0 R 0x1
Section to Segment mapping:
Segment Sections...
00 .hash .gnu.hash .dynsym .dynstr
01 .dynamic
02 .dynamic
03 .dynamic
[hjl@gnu-efi-2 tmp]$
[hjl@gnu-efi-2 tmp]$ readelf -d a.out
Dynamic section at offset 0xf40 contains 7 entries:
Tag Type Name/Value
0x0000000000000004 (HASH) 0x120
0x000000006ffffef5 (GNU_HASH) 0x148
0x0000000000000005 (STRTAB) 0x1d8
0x0000000000000006 (SYMTAB) 0x178
0x000000000000000a (STRSZ) 25 (bytes)
0x000000000000000b (SYMENT) 24 (bytes)
0x0000000000000000 (NULL) 0x0
[hjl@gnu-efi-2 tmp]$
On Mon, Sep 25, 2017 at 9:12 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On 9/25/17, Zack Weinberg <zackw@panix.com> wrote:
>> On Mon, Sep 25, 2017 at 8:33 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>> ELF object generated with "objcopy --only-keep-debug" has
>>>
>>> Type Offset VirtAddr PhysAddr FileSiz MemSiz Flg Align
>>> DYNAMIC 0x0+e28 0x0+200e40 0x0+200e40 0x0+ 0x0+1a0 RW 0x8
>>>
>>> with 0 file size. ld.so should skip such PT_DYNAMIC segments.
>>
>> Why should ld.so skip such PT_DYNAMIC segments? It seems like loading
>> empty segments should be harmless, and ...
>
> Not all valid segments can have zero p_filesz. The valid PT_DYNAMIC segment
> should have non-zero p_filesz.
Argh, I misremembered what a PT_DYNAMIC segment is. I've checked the
gABI now and I see that it specifies that an empty PT_DYNAMIC segment
is in fact invalid in a shared object (since certain entries in the
array are mandatory) ... but the change still makes me nervous. What
_exactly_ does our ld.so do with this now, in the absence of your
change? If it does anything other than crash, I fear there may be
programs out there relying on the behavior.
(And I'd like you to specifically check that your change doesn't break
GDB's ability to load debug-only objects, even though I'm pretty sure
it doesn't use dlopen to do it.)
zw
On 9/25/17, Zack Weinberg <zackw@panix.com> wrote:
> On Mon, Sep 25, 2017 at 9:12 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On 9/25/17, Zack Weinberg <zackw@panix.com> wrote:
>>> On Mon, Sep 25, 2017 at 8:33 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>> ELF object generated with "objcopy --only-keep-debug" has
>>>>
>>>> Type Offset VirtAddr PhysAddr FileSiz MemSiz Flg Align
>>>> DYNAMIC 0x0+e28 0x0+200e40 0x0+200e40 0x0+ 0x0+1a0 RW 0x8
>>>>
>>>> with 0 file size. ld.so should skip such PT_DYNAMIC segments.
>>>
>>> Why should ld.so skip such PT_DYNAMIC segments? It seems like loading
>>> empty segments should be harmless, and ...
>>
>> Not all valid segments can have zero p_filesz. The valid PT_DYNAMIC
>> segment
>> should have non-zero p_filesz.
>
> Argh, I misremembered what a PT_DYNAMIC segment is. I've checked the
> gABI now and I see that it specifies that an empty PT_DYNAMIC segment
> is in fact invalid in a shared object (since certain entries in the
> array are mandatory) ... but the change still makes me nervous. What
> _exactly_ does our ld.so do with this now, in the absence of your
> change? If it does anything other than crash, I fear there may be
ld.so simply crashes without my change:
Starting program:
/export/build/gnu/glibc/build-x86_64-linux/elf/tst-debug1 --direct
Program received signal SIGSEGV, Segmentation fault.
_dl_relocate_object (scope=0x604608, reloc_mode=reloc_mode@entry=1,
consider_profiling=consider_profiling@entry=0) at dl-reloc.c:231
231 const char *strtab = (const void *) D_PTR (l, l_info[DT_STRTAB]);
(gdb) bt
#0 _dl_relocate_object (scope=0x604608, reloc_mode=reloc_mode@entry=1,
consider_profiling=consider_profiling@entry=0) at dl-reloc.c:231
#1 0x00007ffff7dea573 in dl_open_worker (a=a@entry=0x7fffffffd910) at
dl-open.c:422
#2 0x00007ffff794c86c in __GI__dl_catch_exception (exception=0x7fffffffd8f0,
operate=0x7ffff7dea310 <dl_open_worker>, args=0x7fffffffd910)
at dl-error-skeleton.c:196
#3 0x00007ffff7de9f6a in _dl_open (file=0x401cd8 "tst-debug1mod1.so",
mode=-2147483647,
caller_dlopen=0x401323 <do_test+19>, nsid=<optimized out>, argc=2,
argv=<optimized out>, env=0x7fffffffdd30) at dl-open.c:645
#4 0x00007ffff7bd3f06 in dlopen_doit (a=a@entry=0x7fffffffdb40) at dlopen.c:66
#5 0x00007ffff794c86c in __GI__dl_catch_exception (
exception=exception@entry=0x7fffffffdae0, operate=0x7ffff7bd3eb0
<dlopen_doit>,
args=0x7fffffffdb40) at dl-error-skeleton.c:196
#6 0x00007ffff794c8df in __GI__dl_catch_error (objname=0x7ffff7dd60d0
<last_result+16>,
errstring=0x7ffff7dd60d8 <last_result+24>,
mallocedp=0x7ffff7dd60c8 <last_result+8>,
operate=<optimized out>, args=<optimized out>) at dl-error-skeleton.c:215
#7 0x00007ffff7bd4525 in _dlerror_run (
operate=operate@entry=0x7ffff7bd3eb0 <dlopen_doit>,
args=args@entry=0x7fffffffdb40)
at dlerror.c:162
#8 0x00007ffff7bd3f81 in __dlopen (file=file@entry=0x401cd8
"tst-debug1mod1.so",
mode=mode@entry=1) at dlopen.c:87
#9 0x0000000000401323 in do_test () at tst-debug1.c:25
#10 0x00000000004017a3 in support_test_main (argc=1, argv=0x7fffffffdd20,
config=config@entry=0x7fffffffdbe0) at support_test_main.c:321
#11 0x0000000000401225 in main (argc=<optimized out>, argv=<optimized out>)
at ../support/test-driver.c:164
(gdb)
> programs out there relying on the behavior.
>
> (And I'd like you to specifically check that your change doesn't break
> GDB's ability to load debug-only objects, even though I'm pretty sure
> it doesn't use dlopen to do it.)
>
GDB doesn't call dlopen to load debug-only objects.
On 09/25/2017 06:33 PM, H.J. Lu wrote:
> ELF object generated with "objcopy --only-keep-debug" has
>
> Type Offset VirtAddr PhysAddr FileSiz MemSiz Flg Align
> DYNAMIC 0x0+e28 0x0+200e40 0x0+200e40 0x0+ 0x0+1a0 RW 0x8
>
> with 0 file size. ld.so should skip such PT_DYNAMIC segments.
>
> Tested on x86-64. OK for master?
Are all such `objcopy --only-kee--debug` objects left with 0 file size?
After your patch what happens when you run ldd on such an object?
The idea in bug 22101 is to add minimal code early in the dynamic
loader to identify specially marked objects and ignore them. This
way we put an end to the guessing game of what constitutes a valid
ELF object.
Granted, the code you've added is quite small, so it looks like
an interesting short term solution. It needs a more verbose
comment for the PT_DYNAMIC case explaining why we check if
ph->p_filesz is zero and what the consequences of that are since
we *never* add such defensive checks in ld.so because they would
slow down the average case of a correctly formed binary (and
thus need a hefty comment).
On Mon, Sep 25, 2017 at 9:46 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On 9/25/17, Zack Weinberg <zackw@panix.com> wrote:
>> What
>> _exactly_ does our ld.so do with this now, in the absence of your
>> change?
>
> ld.so simply crashes without my change:
OK, in that case I am fine with the patch, _except_:
>> (And I'd like you to specifically check that your change doesn't break
>> GDB's ability to load debug-only objects, even though I'm pretty sure
>> it doesn't use dlopen to do it.)
>
> GDB doesn't call dlopen to load debug-only objects.
I want you to test it anyway.
zw
On 25/09/2017 18:12, H.J. Lu wrote:
> On 9/25/17, Zack Weinberg <zackw@panix.com> wrote:
>> On Mon, Sep 25, 2017 at 8:33 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>> ELF object generated with "objcopy --only-keep-debug" has
>>>
>>> Type Offset VirtAddr PhysAddr FileSiz MemSiz Flg Align
>>> DYNAMIC 0x0+e28 0x0+200e40 0x0+200e40 0x0+ 0x0+1a0 RW 0x8
>>>
>>> with 0 file size. ld.so should skip such PT_DYNAMIC segments.
>> Why should ld.so skip such PT_DYNAMIC segments? It seems like loading
>> empty segments should be harmless, and ...
> Not all valid segments can have zero p_filesz. The valid PT_DYNAMIC segment
> should have non-zero p_filesz.
Should we fix it on objcopy as well to avoid it create such invalid
segments?
On 09/26/2017 11:26 AM, Adhemerval Zanella wrote:
> On 25/09/2017 18:12, H.J. Lu wrote:
>> On 9/25/17, Zack Weinberg <zackw@panix.com> wrote:
>>> On Mon, Sep 25, 2017 at 8:33 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>> ELF object generated with "objcopy --only-keep-debug" has
>>>>
>>>> Type Offset VirtAddr PhysAddr FileSiz MemSiz Flg Align
>>>> DYNAMIC 0x0+e28 0x0+200e40 0x0+200e40 0x0+ 0x0+1a0 RW 0x8
>>>>
>>>> with 0 file size. ld.so should skip such PT_DYNAMIC segments.
>>> Why should ld.so skip such PT_DYNAMIC segments? It seems like loading
>>> empty segments should be harmless, and ...
>> Not all valid segments can have zero p_filesz. The valid PT_DYNAMIC segment
>> should have non-zero p_filesz.
>
> Should we fix it on objcopy as well to avoid it create such invalid
> segments?
No. You want a minimum sized debuginfo object, and that includes removing
things you don't need. The real solution here is as proposed in the bug
which is to mark debuginfo objects with a special ET_* marker and then
adjust the downstream tools to either tread lightly or reject such
pseudo-ELF files (they are effectively a *part* of an ELF file).
@@ -181,7 +181,8 @@ tests += restest1 preloadtest loadfail multiload origtest resolvfail \
tst-initorder tst-initorder2 tst-relsort1 tst-null-argv \
tst-tlsalign tst-tlsalign-extern tst-nodelete-opened \
tst-nodelete2 tst-audit11 tst-audit12 tst-dlsym-error tst-noload \
- tst-latepthread tst-tls-manydynamic tst-nodelete-dlclose
+ tst-latepthread tst-tls-manydynamic tst-nodelete-dlclose \
+ tst-debug1
# reldep9
tests-internal += loadtest unload unload2 circleload1 \
neededtest neededtest2 neededtest3 neededtest4 \
@@ -1417,3 +1418,9 @@ tst-env-setuid-ENV = MALLOC_CHECK_=2 MALLOC_MMAP_THRESHOLD_=4096 \
LD_HWCAP_MASK=0x1
tst-env-setuid-tunables-ENV = \
GLIBC_TUNABLES=glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096
+
+$(objpfx)tst-debug1: $(libdl)
+$(objpfx)tst-debug1.out: $(objpfx)tst-debug1mod1.so
+
+$(objpfx)tst-debug1mod1.so: $(objpfx)testobj1.so
+ $(OBJCOPY) --only-keep-debug $< $@
@@ -1052,8 +1052,11 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
segments are mapped in. We record the addresses it says
verbatim, and later correct for the run-time load address. */
case PT_DYNAMIC:
- l->l_ld = (void *) ph->p_vaddr;
- l->l_ldnum = ph->p_memsz / sizeof (ElfW(Dyn));
+ if (ph->p_filesz)
+ {
+ l->l_ld = (void *) ph->p_vaddr;
+ l->l_ldnum = ph->p_memsz / sizeof (ElfW(Dyn));
+ }
break;
case PT_PHDR:
new file mode 100644
@@ -0,0 +1,34 @@
+/* Unit test for dlopen on ELF object from "objcopy --only-keep-debug".
+ Copyright (C) 2017 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 <dlfcn.h>
+#include <stdio.h>
+
+static int
+do_test (void)
+{
+ void *h = dlopen ("tst-debug1mod1.so", RTLD_LAZY);
+ if (h != NULL)
+ {
+ puts ("shouldn't load tst-debug1mod1.so");
+ return 1;
+ }
+ return 0;
+}
+
+#include <support/test-driver.c>