From patchwork Fri Jun 24 23:59:12 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Carlos O'Donell X-Patchwork-Id: 13369 Received: (qmail 5785 invoked by alias); 24 Jun 2016 23:59:28 -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 5403 invoked by uid 89); 24 Jun 2016 23:59:28 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL, BAYES_00, KAM_LAZY_DOMAIN_SECURITY, RCVD_IN_DNSWL_LOW autolearn=no version=3.3.2 spammy=ls, corner, Lesser, quantities X-HELO: mail-qk0-f182.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:to:from:subject:organization:message-id:date :user-agent:mime-version:content-transfer-encoding; bh=xlUZ5tN8k5Tlg4V3/XL/7x3FhNVgNk5oIkChVq0XG5k=; b=MGBQEm3xNf8BdBuCzeFz4Za5893jAgJhovWqaRHSfzch/ewgaqId6vDcDozZXaMgux A7doCvgT6SMMpHAve//CgnJQkfsvqAMm7yrZYBx6ERTROJHTnddk+f57yFp4mg84YjlU 3yB4G/e1bHSqyJIRuPEtLuCFDwLBh4CJp1Ty38gEYsAApMkiEXptx9SYfyB6BcMakgY6 7e7lPX0g2CnTYvF5aFDdzluQC8xwzJyO1QHfeiGEmH34HgJnwbFDe9oUF3xIaetGppxB /PDVwk1dtlEw1B4NVBmCoHMfviwIR0WkBEMGwHAUjeKn77ZebuDj19HbXm1nHIC0YzH7 tnWA== X-Gm-Message-State: ALyK8tIZMn/l0w7qAK7dHhJLWZ6ozebq4iwkaYu1GVoYRiZsnQYk6bTsns0DGUtKCKTVb4jk X-Received: by 10.55.140.69 with SMTP id o66mr8465105qkd.143.1466812755293; Fri, 24 Jun 2016 16:59:15 -0700 (PDT) To: GNU C Library , Florian Weimer From: Carlos O'Donell Subject: Consensus on unit tests? Message-ID: <92609f90-7a53-a455-ca72-b9baae224a38@redhat.com> Date: Fri, 24 Jun 2016 19:59:12 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 MIME-Version: 1.0 There is a lot of work to be done inside the dynamic loader, particularly when it comes to accelerating the symbol lookup and searches. See bugs 17645 and 15310. It would seem immensely beneficial to me to be able to split up some of these functions and put them through unit testing to better define their behaviour and interfaces. I would like to be able to move in incrementally and split out some common functions, apply unit tests, and then improve the functional performance by using the same framework to put the function into a microbenchmark. Obviously this is not the end goal. We need far more complete tests that run the dynamic loader through the full sequence of operations that a user is going to do, but internally it would help build confidence in the changes to be able to unit test them. For example coverity detected a superfluous "always true" part of a conditional in _dl_addr_inside_object. The statement "&& reladdr - l->l_phdr[n].p_vaddr >= 0" is always true because both reladdr and p_vaddr are unsigned quantities whose difference is always greater than or equal to zero. I would like to remove the superfluous condition and add a unit test for all the cases that define the way the interface should behave. Since Florian asked for pretty diagrams, I have included them. If nobody objects to unit tests within the dynamic loader I would like to use unit tests, not to find bugs, but to effectively design and limit the implementation's behaviour. The unit tests could for example exercise that existing code continues to have some odd corner case behaviour that must not be removed. Should we allow unit tests for the dynamic loader? In general the addition of unit tests looks like this: - Split the function in question into a distinct C file. - Add the new C file to the list of objects used to build rtld. - Add a new unit test that links against the built object file that contains the function you want to test and provides all the things the function needs to operate (sometimes difficult). - The test shall setup and exercise the interface to define the implementation behaviour. The patch below is really a toy example, but it serves the purpose to show my idea. diff --git a/elf/Makefile b/elf/Makefile index 210dde9..cd8df19 100644 --- a/elf/Makefile +++ b/elf/Makefile @@ -23,7 +23,7 @@ include ../Makeconfig headers = elf.h bits/elfclass.h link.h bits/link.h routines = $(all-dl-routines) dl-support dl-iteratephdr \ - dl-addr enbl-secure dl-profstub \ + dl-addr dl-addr-obj enbl-secure dl-profstub \ dl-origin dl-libc dl-sym dl-tsd dl-sysdep # The core dynamic linking functions are in libc for the static and @@ -312,6 +312,9 @@ tests-special += $(objpfx)tst-prelink-cmp.out endif endif +tests += tst-_dl_addr_inside_object +$(objpfx)tst-_dl_addr_inside_object: $(objpfx)dl-addr-obj.os + include ../Rules ifeq (yes,$(build-shared)) diff --git a/elf/dl-addr-obj.c b/elf/dl-addr-obj.c new file mode 100644 index 0000000..2ac97a6 --- /dev/null +++ b/elf/dl-addr-obj.c @@ -0,0 +1,72 @@ +/* Determine if address is inside object segments. + Copyright (C) 1996-2016 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 + +/* Return non-zero if ADDR lies within one of L's segments. + We have three cases we care about and we do not need to avoid overflow. + + Case 1: addr is above a segment. + +==================+<- l_map_end + | |<- addr + |------------------|<- l_addr + p_vaddr + p_memsz + | | + | | + |------------------|<- l_addr + p_vaddr + |------------------|<- l_addr + | | + +==================+<- l_map_start + + Case 2: addr is within a segments. + +==================+<- l_map_end + | | + |------------------|<- l_addr + p_vaddr + p_memsz + | |<- addr + | | + |------------------|<- l_addr + p_vaddr + |------------------|<- l_addr + | | + +==================+<- l_map_start + + Case 3: addr is below a segments. + +==================+<- l_map_end + | | + |------------------|<- l_addr + p_vaddr + p_memsz + | | + | | + |------------------|<- l_addr + p_vaddr + |------------------|<- l_addr + | |<- addr + +==================+<- l_map_start + +*/ +int +internal_function +_dl_addr_inside_object (struct link_map *l, const ElfW(Addr) addr) +{ + int n = l->l_phnum; + const ElfW(Addr) reladdr = addr - l->l_addr; + + /*&& reladdr - l->l_phdr[n].p_vaddr >= 0*/ + while (--n >= 0) + if (l->l_phdr[n].p_type == PT_LOAD + && reladdr - l->l_phdr[n].p_vaddr < l->l_phdr[n].p_memsz) + return 1; + return 0; +} diff --git a/elf/dl-addr.c b/elf/dl-addr.c index 291ff55..5a4ab1e 100644 --- a/elf/dl-addr.c +++ b/elf/dl-addr.c @@ -143,19 +143,3 @@ _dl_addr (const void *address, Dl_info *info, return result; } libc_hidden_def (_dl_addr) - -/* Return non-zero if ADDR lies within one of L's segments. */ -int -internal_function -_dl_addr_inside_object (struct link_map *l, const ElfW(Addr) addr) -{ - int n = l->l_phnum; - const ElfW(Addr) reladdr = addr - l->l_addr; - - while (--n >= 0) - if (l->l_phdr[n].p_type == PT_LOAD - && reladdr - l->l_phdr[n].p_vaddr >= 0 - && reladdr - l->l_phdr[n].p_vaddr < l->l_phdr[n].p_memsz) - return 1; - return 0; -} diff --git a/elf/dl-open.c b/elf/dl-open.c index 6f178b3..70f3f1e 100644 --- a/elf/dl-open.c +++ b/elf/dl-open.c @@ -735,21 +735,3 @@ _dl_show_scope (struct link_map *l, int from) _dl_debug_printf (" no scope\n"); _dl_debug_printf ("\n"); } - -#if IS_IN (rtld) -/* Return non-zero if ADDR lies within one of L's segments. */ -int -internal_function -_dl_addr_inside_object (struct link_map *l, const ElfW(Addr) addr) -{ - int n = l->l_phnum; - const ElfW(Addr) reladdr = addr - l->l_addr; - - while (--n >= 0) - if (l->l_phdr[n].p_type == PT_LOAD - && reladdr - l->l_phdr[n].p_vaddr >= 0 - && reladdr - l->l_phdr[n].p_vaddr < l->l_phdr[n].p_memsz) - return 1; - return 0; -} -#endif diff --git a/elf/tst-_dl_addr_inside_object.c b/elf/tst-_dl_addr_inside_object.c new file mode 100644 index 0000000..b268578 --- /dev/null +++ b/elf/tst-_dl_addr_inside_object.c @@ -0,0 +1,194 @@ +/* Unit test for _dl_addr_inside_object. + Copyright (C) 2016 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 +#include +#include + +extern int _dl_addr_inside_object (struct link_map *l, const ElfW(Addr) addr); + +static int +do_test (void) +{ + int ret; + ElfW(Addr) addr; + struct link_map map; + ElfW(Phdr) header; + map.l_phdr = &header; + map.l_phnum = 1; + map.l_addr = 0x0; + /* Segment spans 0x2000 -> 0x4000. */ + header.p_vaddr = 0x2000; + header.p_memsz = 0x2000; + header.p_type = PT_LOAD; + /* Address is above the segment e.g. > 0x4000. */ + addr = 0x5000; + ret = _dl_addr_inside_object (&map, addr); + switch (ret) + { + case 0: + printf ("PASS: Above: Address is detected as outside the segment.\n"); + break; + case 1: + printf ("FAIL: Above: Address is detected as inside the segment.\n"); + exit (1); + break; + default: + printf ("FAIL: Above: Invalid return value.\n"); + exit (1); + } + /* Address is inside the segment e.g. 0x2000 < addr < 0x4000. */ + addr = 0x3000; + ret = _dl_addr_inside_object (&map, addr); + switch (ret) + { + case 0: + printf ("FAIL: Inside: Address is detected as outside the segment.\n"); + exit (1); + break; + case 1: + printf ("PASS: Inside: Address is detected as inside the segment.\n"); + break; + default: + printf ("FAIL: Inside: Invalid return value.\n"); + exit (1); + } + /* Address is below the segment e.g. < 0x2000. */ + addr = 0x1000; + ret = _dl_addr_inside_object (&map, addr); + switch (ret) + { + case 0: + printf ("PASS: Below: Address is detected as outside the segment.\n"); + break; + case 1: + printf ("FAIL: Below: Address is detected as inside the segment.\n"); + exit (1); + break; + default: + printf ("FAIL: Below: Invalid return value.\n"); + exit (1); + } + /* Address is in the segment and addr == p_vaddr. */ + addr = 0x2000; + ret = _dl_addr_inside_object (&map, addr); + switch (ret) + { + case 0: + printf ("FAIL: At p_vaddr: Address is detected as outside the segment.\n"); + exit (1); + break; + case 1: + printf ("PASS: At p_vaddr: Address is detected as inside the segment.\n"); + break; + default: + printf ("FAIL: At p_vaddr: Invalid return value.\n"); + exit (1); + } + /* Address is in the segment and addr == p_vaddr + p_memsz - 1. */ + addr = 0x2000 + 0x2000 - 0x1; + ret = _dl_addr_inside_object (&map, addr); + switch (ret) + { + case 0: + printf ("FAIL: At p_memsz-1: Address is detected as outside the segment.\n"); + exit (1); + break; + case 1: + printf ("PASS: At p_memsz-1: Address is detected as inside the segment.\n"); + break; + default: + printf ("FAIL: At p_memsz-1: Invalid return value.\n"); + exit (1); + } + /* Address is outside the segment and addr == p_vaddr + p_memsz. */ + addr = 0x2000 + 0x2000; + ret = _dl_addr_inside_object (&map, addr); + switch (ret) + { + case 0: + printf ("PASS: At p_memsz: Address is detected as outside the segment.\n"); + break; + case 1: + printf ("FAIL: At p_memsz: Address is detected as inside the segment.\n"); + exit (1); + break; + default: + printf ("FAIL: At p_memsz: Invalid return value.\n"); + exit (1); + } + /* Address is outside the segment and p_vaddr at maximum address. */ + addr = 0x0 - 0x2; + header.p_vaddr = 0x0 - 0x1; + header.p_memsz = 0x1; + ret = _dl_addr_inside_object (&map, addr); + switch (ret) + { + case 0: + printf ("PASS: At max: Address is detected as outside the segment.\n"); + break; + case 1: + printf ("FAIL: At max: Address is detected as inside the segment.\n"); + exit (1); + break; + default: + printf ("FAIL: At max: Invalid return value.\n"); + exit (1); + } + /* Address is outside the segment and p_vaddr at minimum address. */ + addr = 0x1; + header.p_vaddr = 0x0; + header.p_memsz = 0x1; + ret = _dl_addr_inside_object (&map, addr); + switch (ret) + { + case 0: + printf ("PASS: At min: Address is detected as outside the segment.\n"); + break; + case 1: + printf ("FAIL: At min: Address is detected as inside the segment.\n"); + exit (1); + break; + default: + printf ("FAIL: At min: Invalid return value.\n"); + exit (1); + } + /* Address is always inside the segment with p_memsz at max. */ + addr = 0x0; + header.p_vaddr = 0x0; + header.p_memsz = 0x0 - 0x1; + ret = _dl_addr_inside_object (&map, addr); + switch (ret) + { + case 0: + printf ("FAIL: At maxmem: Address is detected as outside the segment.\n"); + exit (1); + break; + case 1: + printf ("PASS: At maxmem: Address is detected as inside the segment.\n"); + break; + default: + printf ("FAIL: At maxmem: Invalid return value.\n"); + exit (1); + } + return 0; +} + +#define TEST_FUNCTION do_test () +#include "../test-skeleton.c"