diff mbox

BZ#20292: Remove redundant statement in _dl_addr_inside_object (refactor and unit test).

Message ID f1b609a5-ad02-5478-883f-eee34771f73f@redhat.com
State Committed
Headers show

Commit Message

Carlos O'Donell July 4, 2016, 5:12 p.m. UTC
The following patch refactors two copies of _dl_addr_inside_object
into dl-addr-obj.c, and removes the redundant comparison detected by
static analysis. The remaining statement:
"reladdr - l->l_phdr[n].p_vaddr < l->l_phdr[n].p_memsz"
is always sufficient to detect if the unsigned address is inside the
load segment of the link map.

The copy in dl-addr-obj.c is then subjected to a dynamically linked
PIE test where the test driver is linked directly with the built *.os
version of the file and the operations exercised.

Note that the refactoring creates an additional object file
elf/rtld-dl-addr-obj.os needed by the dynamic linker, and an
elf/dl-addr-obj.os needed by libc. We test the latter for conformance
with the expected behaviours.

The goal behind the unit test is _not_ to find bugs, but to exercise
the expected behavior of the function. Integration tests are where we
should focus our bug finding. Instead the tests here serve to make it
precisely clear what the functions should or should not do. I plan to
follow the same tactic with the ELF sorting functions and dependency
sorting functions (not for this release cycle though).

I'll check this fix in before the end of the week if nobody objects.

2016-07-04  Carlos O'Donell  <carlos@redhat.com>

	[BZ #20292]
	* elf/Makefile (routines): Add dl-addr-obj.
	[ifeq (yesyes,$(have-fpie)$(build-shared))] (tests): Add
	tst-_dl_addr_inside_object.
	[ifeq (yesyes,$(have-fpie)$(build-shared))] (tests-pie): Likewise.
	[ifeq (yesyes,$(have-fpie)$(build-shared))]
	($(objpfx)tst-_dl_addr_inside_object): Add $(objpfx)dl-addr-obj.os.
	[ifeq (yesyes,$(have-fpie)$(build-shared))]
	(CFLAGS-tst-_dl_addr_inside_object.c): Add $(PIE-ccflag).
	* elf/dl-addr.c: Remove _dl_addr_inside_object function. 
	* elf/dl-open.c: Likewise.
	* elf/dl-addr-obj.c: New file.
	* elf/tst-_dl_addr_inside_object.c: New file.

Comments

Carlos O'Donell Sept. 30, 2016, 6:28 a.m. UTC | #1
On 07/04/2016 01:12 PM, Carlos O'Donell wrote:
> The following patch refactors two copies of _dl_addr_inside_object
> into dl-addr-obj.c, and removes the redundant comparison detected by
> static analysis. The remaining statement:
> "reladdr - l->l_phdr[n].p_vaddr < l->l_phdr[n].p_memsz"
> is always sufficient to detect if the unsigned address is inside the
> load segment of the link map.
> 
> The copy in dl-addr-obj.c is then subjected to a dynamically linked
> PIE test where the test driver is linked directly with the built *.os
> version of the file and the operations exercised.
> 
> Note that the refactoring creates an additional object file
> elf/rtld-dl-addr-obj.os needed by the dynamic linker, and an
> elf/dl-addr-obj.os needed by libc. We test the latter for conformance
> with the expected behaviours.
> 
> The goal behind the unit test is _not_ to find bugs, but to exercise
> the expected behavior of the function. Integration tests are where we
> should focus our bug finding. Instead the tests here serve to make it
> precisely clear what the functions should or should not do. I plan to
> follow the same tactic with the ELF sorting functions and dependency
> sorting functions (not for this release cycle though).
> 
> I'll check this fix in before the end of the week if nobody objects.
> 
> 2016-07-04  Carlos O'Donell  <carlos@redhat.com>
> 
> 	[BZ #20292]
> 	* elf/Makefile (routines): Add dl-addr-obj.
> 	[ifeq (yesyes,$(have-fpie)$(build-shared))] (tests): Add
> 	tst-_dl_addr_inside_object.
> 	[ifeq (yesyes,$(have-fpie)$(build-shared))] (tests-pie): Likewise.
> 	[ifeq (yesyes,$(have-fpie)$(build-shared))]
> 	($(objpfx)tst-_dl_addr_inside_object): Add $(objpfx)dl-addr-obj.os.
> 	[ifeq (yesyes,$(have-fpie)$(build-shared))]
> 	(CFLAGS-tst-_dl_addr_inside_object.c): Add $(PIE-ccflag).
> 	* elf/dl-addr.c: Remove _dl_addr_inside_object function. 
> 	* elf/dl-open.c: Likewise.
> 	* elf/dl-addr-obj.c: New file.
> 	* elf/tst-_dl_addr_inside_object.c: New file.

Checked in v2 with a few more comments and one extra unit test.
diff mbox

Patch

diff --git a/elf/Makefile b/elf/Makefile
index 210dde9..0b8dde9 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,13 @@  tests-special += $(objpfx)tst-prelink-cmp.out
 endif
 endif
 
+ifeq (yesyes,$(have-fpie)$(build-shared))
+tests += tst-_dl_addr_inside_object
+tests-pie += tst-_dl_addr_inside_object
+$(objpfx)tst-_dl_addr_inside_object: $(objpfx)dl-addr-obj.os
+CFLAGS-tst-_dl_addr_inside_object.c += $(PIE-ccflag)
+endif
+
 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..3982642
--- /dev/null
+++ b/elf/dl-addr-obj.c
@@ -0,0 +1,71 @@ 
+/* Determine if address is inside object load 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
+   <http://www.gnu.org/licenses/>.  */
+
+#include <link.h>
+#include <elf.h>
+
+/* Return non-zero if ADDR lies within one of L's loadable segments.
+   We have three cases we care about and we 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;
+
+  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..580c9b2
--- /dev/null
+++ b/elf/tst-_dl_addr_inside_object.c
@@ -0,0 +1,196 @@ 
+/* 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
+   <http://www.gnu.org/licenses/>.  */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <link.h>
+#include <elf.h>
+#include <libc-symbols.h>
+
+extern int internal_function _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"