From patchwork Tue Sep 26 07:55:49 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "H.J. Lu" X-Patchwork-Id: 23145 Received: (qmail 48569 invoked by alias); 26 Sep 2017 07:56:39 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 48259 invoked by uid 89); 26 Sep 2017 07:55:55 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.0 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, RCVD_IN_SORBS_SPAM, SPF_PASS autolearn=ham version=3.3.2 spammy=*never* X-HELO: mail-oi0-f45.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=9OHnXgxo216NWxzitc5jZiFx3rHXgWF+jilvm7xpf3E=; b=sgHbDs6OQqKAUjDgh1PG93L5My0bhmJPh4ICBwCuiCju2nLZrwH5Vu5tSKxDgmAM3+ ACgL81W1vN+sRHluC5mS1hlvlfIJggYTQnaAASnK3YDWTZcs67JBFvfBd58/uFs2I4jp wtiA3MmbJ68dQPbHd5IjSPvlo6ljtGH/vUlQGMW5sL0RD7to74VAIXqiXZ7/MNoIpf4/ tqO4iAI8Tkb0NAQHkmcsEjg88x0wOPfWD5nJqz5tE2FU+04hYACdNgqt8zHhAsPjkOCt nLarNAX28nNbtY5M0aYJLW5dYQ4C/tP2BghQEi40/xx7SwvfNPUyP9jHaDSrbZ+nuDOQ grWw== X-Gm-Message-State: AHPjjUhtO8Dk0WE2zACppa5tiTd6CBh5ofoUdY+X+IqhNTChMD+xQbZu OCQ4HyXm4potCOsfiEUIq68+ZR6s7A3vhhG6fiM= X-Google-Smtp-Source: AOwi7QDt9D3lK0rW4B3lgCOhcrSszXR0C2tY5wMU6Y7YEX9lkvazpuTjEXVhO9m2zj7tIWnRmGplHCSvj3lwTLNOeEI= X-Received: by 10.157.58.70 with SMTP id j64mr1543821otc.337.1506412551152; Tue, 26 Sep 2017 00:55:51 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: <20170926003314.GA18765@gmail.com> From: "H.J. Lu" Date: Tue, 26 Sep 2017 00:55:49 -0700 Message-ID: Subject: Re: [PATCH] Skip PT_DYNAMIC segment if its p_filesz == 0 [BZ #22101] To: "Carlos O'Donell" Cc: GNU C Library On 9/25/17, Carlos O'Donell wrote: > 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? Yes, that is correct. > After your patch what happens when you run ldd on such an object? Before: [hjl@gnu-efi-2 elf]$ /lib64/ld-2.25.so --list ./tst-debug1mod1.so statically linked After: [hjl@gnu-efi-2 elf]$ ./ld.so --list ./tst-debug1mod1.so ./tst-debug1mod1.so: error while loading shared libraries: ./tst-debug1mod1.so: object file has no dynamic section [hjl@gnu-efi-2 elf]$ > 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. I am not sure if this is necessary. Any such changes won't work with debug only objects generated by the current objcopy. > 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 This is not entirely true. There are plenty of sanity checks in ld.so. For example, a few lines below, there are case PT_LOAD: /* A load command tells us to map in part of the file. We record the load commands and process them all later. */ if (__glibc_unlikely ((ph->p_align & (GLRO(dl_pagesize) - 1)) != 0)) { errstring = N_("ELF load command alignment not page-aligned"); goto call_lose; } if (__glibc_unlikely (((ph->p_vaddr - ph->p_offset) & (ph->p_align - 1)) != 0)) { errstring = N_("ELF load command address/offset not properly aligned"); goto call_lose; } > slow down the average case of a correctly formed binary (and > thus need a hefty comment). > Here is the updated patch with comments: case PT_DYNAMIC: if (ph->p_filesz) { /* Debuginfo only file from "objcopy --only-keep-debug" contains PT_DYNAMIC segment with p_filesz == 0. Skip such segment to avoid crash later. */ l->l_ld = (void *) ph->p_vaddr; l->l_ldnum = ph->p_memsz / sizeof (ElfW(Dyn)); } break; Reviewed-by: Carlos O'Donell From 5b9b19d9ee5040cad920e8e31e4c2100fcf8d25f Mon Sep 17 00:00:00 2001 From: "H.J. Lu" Date: Mon, 25 Sep 2017 17:16:52 -0700 Subject: [PATCH] Skip PT_DYNAMIC segment with p_filesz == 0 [BZ #22101] 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. [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 with p_filesz == 0. * elf/tst-debug1.c: New file. --- elf/Makefile | 9 ++++++++- elf/dl-load.c | 10 ++++++++-- elf/tst-debug1.c | 34 ++++++++++++++++++++++++++++++++++ 3 files changed, 50 insertions(+), 3 deletions(-) create mode 100644 elf/tst-debug1.c diff --git a/elf/Makefile b/elf/Makefile index 7cf959aabd..e21f37e30b 100644 --- a/elf/Makefile +++ b/elf/Makefile @@ -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 $< $@ diff --git a/elf/dl-load.c b/elf/dl-load.c index a067760cc6..d43bb2dd5e 100644 --- a/elf/dl-load.c +++ b/elf/dl-load.c @@ -1052,8 +1052,14 @@ _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) + { + /* Debuginfo only file from "objcopy --only-keep-debug" + contains PT_DYNAMIC segment with p_filesz == 0. Skip + such segment to avoid crash later. */ + l->l_ld = (void *) ph->p_vaddr; + l->l_ldnum = ph->p_memsz / sizeof (ElfW(Dyn)); + } break; case PT_PHDR: diff --git a/elf/tst-debug1.c b/elf/tst-debug1.c new file mode 100644 index 0000000000..aa2f4886bf --- /dev/null +++ b/elf/tst-debug1.c @@ -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 + . */ + +#include +#include + +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 -- 2.13.5