Patchwork [v2,2/2] Fix handling of null stap semaphores

login
register
mail settings
Submitter George Barrett
Date Jan. 2, 2020, 11:12 p.m.
Message ID <8jh6lq70pfevy2fnv252japnkr1a_e946wms0723runb/plb._aa@mail.bob131.so>
Download mbox | patch
Permalink /patch/37162/
State New
Headers show

Comments

George Barrett - Jan. 2, 2020, 11:12 p.m.
According to the SystemTap documentation on user-space probes[0], stap
probe points without semaphores are denoted by setting the semaphore
address in the probe's note to zero. At present the code does do a
comparison of the semaphore address against zero, but only after it's
been relocated; as such it will (almost?) always fail, commonly
resulting in GDB trying to overwrite the ELF magic located at the
image's base address.

This commit tests the address as specified in the SDT note rather than
the relocated value in order to correctly detect absent probe
semaphores.

gdb/Changelog:
2020-01-03  George Barrett  <bob@bob131.so>

	* stap-probe.c (stap_modify_semaphore): Don't check for null
	semaphores.
	(stap_probe::set_semaphore, stap_probe::clear_semaphore): Check
	for null semaphores.

gdb/testsuite/ChangeLog:
2020-01-03  George Barrett  <bob@bob131.so>

	* gdb.base/stap-probe.c (relocation_marker): Add dummy variable
	to help in finding the image relocation offset.
	* gdb.base/stap-probe.exp (stap_test): Accept arbitrary compile
	options in arguments.
	(stap_test_no_debuginfo): Likewise.
	(stap-probe-nosem-noopt-pie, stap-probe-nosem-noopt-nopie): Add
	test variants.
	(stap_test): Add null semaphore relocation test.
---
 gdb/stap-probe.c                      |  7 +++---
 gdb/testsuite/gdb.base/stap-probe.c   |  2 ++
 gdb/testsuite/gdb.base/stap-probe.exp | 36 +++++++++++++++++++++------
 3 files changed, 35 insertions(+), 10 deletions(-)
Simon Marchi - Jan. 10, 2020, 4:45 a.m.
Thanks, I put a couple of comments.  The patch LGTM with those addressed.

On 2020-01-02 6:12 p.m., George Barrett wrote:
> @@ -47,6 +53,18 @@ proc stap_test {exec_name {arg ""}} {
>  	fail "run to -pstap test:user"
>      }
>  
> +    if {[string first "-DUSE_SEMAPHORES" $args] == -1} {
> +	set relocation_base \
> +	   [expr [get_hexadecimal_valueof "&relocation_marker" "0"] - $semaphore_addr_var]
> +	if {$relocation_base == 0} {
> +	    xfail "skipping null semaphore test: no relocation performed"

Hmm, I probably wouldn't use "xfail" here, since it's not like something is
failing, it's just that this test doesn't apply to this configuration.  What
about just not mentioning at all when it does not apply?

  if {$relocation_base != 0} {
    gdb_test ...
  }

> +	} else {
> +	   gdb_test "p (*(char*) $relocation_base)@4" \
> +		" = \"\\\\177ELF\"" \
> +		"null semaphore relocation"

This test is a bit out of the ordinary, and I can imagine readers being
confused as to why we are testing this.  Could you please add a comment
(just a short sentence) above this to hint to why we are doing this test?
Something like (feel free to improve the formulation):

# Check that GDB doesn't wrongfully try setting null semaphore, and doing so
# overwriting the ELF magic number.

Simon
George Barrett - Jan. 10, 2020, 7:16 p.m.
On Thu, Jan 09, 2020 at 11:45:24PM -0500, Simon Marchi wrote:
> Hmm, I probably wouldn't use "xfail" here, since it's not like something is
> failing, it's just that this test doesn't apply to this configuration.  What
> about just not mentioning at all when it does not apply?

Sounds fine to me.

> This test is a bit out of the ordinary, and I can imagine readers being
> confused as to why we are testing this.  Could you please add a comment
> (just a short sentence) above this to hint to why we are doing this test?

Sure thing.

> Simon

Thanks

Patch

diff --git a/gdb/stap-probe.c b/gdb/stap-probe.c
index ba927790a5..e5e3cceacd 100644
--- a/gdb/stap-probe.c
+++ b/gdb/stap-probe.c
@@ -1425,9 +1425,6 @@  stap_modify_semaphore (CORE_ADDR address, int set, struct gdbarch *gdbarch)
   struct type *type = builtin_type (gdbarch)->builtin_unsigned_short;
   ULONGEST value;
 
-  if (address == 0)
-    return;
-
   /* Swallow errors.  */
   if (target_read_memory (address, bytes, TYPE_LENGTH (type)) != 0)
     {
@@ -1461,6 +1458,8 @@  stap_modify_semaphore (CORE_ADDR address, int set, struct gdbarch *gdbarch)
 void
 stap_probe::set_semaphore (struct objfile *objfile, struct gdbarch *gdbarch)
 {
+  if (m_sem_addr == 0)
+    return;
   stap_modify_semaphore (relocate_address (m_sem_addr, objfile), 1, gdbarch);
 }
 
@@ -1469,6 +1468,8 @@  stap_probe::set_semaphore (struct objfile *objfile, struct gdbarch *gdbarch)
 void
 stap_probe::clear_semaphore (struct objfile *objfile, struct gdbarch *gdbarch)
 {
+  if (m_sem_addr == 0)
+    return;
   stap_modify_semaphore (relocate_address (m_sem_addr, objfile), 0, gdbarch);
 }
 
diff --git a/gdb/testsuite/gdb.base/stap-probe.c b/gdb/testsuite/gdb.base/stap-probe.c
index d197e570d1..ae469880bf 100644
--- a/gdb/testsuite/gdb.base/stap-probe.c
+++ b/gdb/testsuite/gdb.base/stap-probe.c
@@ -31,6 +31,8 @@  __extension__ unsigned short test_pstr_semaphore __attribute__ ((unused)) __attr
 __extension__ unsigned short test_ps_semaphore __attribute__ ((unused)) __attribute__ ((section (".probes")));
 #else
 
+int relocation_marker __attribute__ ((unused));
+
 #define TEST 1
 #define TEST2 1
 
diff --git a/gdb/testsuite/gdb.base/stap-probe.exp b/gdb/testsuite/gdb.base/stap-probe.exp
index 30a4425c04..e925cd7bd4 100644
--- a/gdb/testsuite/gdb.base/stap-probe.exp
+++ b/gdb/testsuite/gdb.base/stap-probe.exp
@@ -18,14 +18,20 @@  standard_testfile
 # Run the tests.  We run the tests two different ways: once with a
 # plain probe, and once with a probe that has an associated semaphore.
 # This returns -1 on failure to compile or start, 0 otherwise.
-proc stap_test {exec_name {arg ""}} {
+proc stap_test {exec_name {args ""}} {
     global testfile hex srcfile
 
     if {[prepare_for_testing "failed to prepare" ${exec_name} $srcfile \
-	   [concat additional_flags=$arg debug]]} {
+	   [concat $args debug]]} {
 	return -1
     }
 
+    set semaphore_addr_var ""
+    if {[string first "-DUSE_SEMAPHORES" $args] == -1} {
+	gdb_test_no_output "set breakpoint always-inserted on"
+	set semaphore_addr_var [get_hexadecimal_valueof "&relocation_marker" "0"]
+    }
+
     if ![runto_main] {
 	return -1
     }
@@ -33,7 +39,7 @@  proc stap_test {exec_name {arg ""}} {
     gdb_test "print \$_probe_argc" "No probe at PC $hex" \
 	"check argument not at probe point"
 
-    if {[string first "-DUSE_SEMAPHORES" $arg] != -1} {
+    if {[string first "-DUSE_SEMAPHORES" $args] != -1} {
 	gdb_test "info probes stap" \
 	    "test *user *$hex *$hex .*"
     } else {
@@ -47,6 +53,18 @@  proc stap_test {exec_name {arg ""}} {
 	fail "run to -pstap test:user"
     }
 
+    if {[string first "-DUSE_SEMAPHORES" $args] == -1} {
+	set relocation_base \
+	   [expr [get_hexadecimal_valueof "&relocation_marker" "0"] - $semaphore_addr_var]
+	if {$relocation_base == 0} {
+	    xfail "skipping null semaphore test: no relocation performed"
+	} else {
+	   gdb_test "p (*(char*) $relocation_base)@4" \
+		" = \"\\\\177ELF\"" \
+		"null semaphore relocation"
+	}
+    }
+
     # Test probe arguments.
     gdb_test "print \$_probe_argc" " = 1" \
     "print \$_probe_argc for probe user"
@@ -97,11 +115,11 @@  proc stap_test {exec_name {arg ""}} {
     return 0
 }
 
-proc stap_test_no_debuginfo {exec_name {arg ""}} {
+proc stap_test_no_debuginfo {exec_name {args ""}} {
     global testfile hex
 
     if {[prepare_for_testing "failed to prepare" ${exec_name} ${testfile}.c \
-	   [concat additional_flags=$arg nodebug optimize=-O2]]} {
+	   [concat $args nodebug optimize=-O2]]} {
 	return -1
     }
 
@@ -168,10 +186,14 @@  with_test_prefix "without semaphore, not optimized" {
 	untested "stap probe test failed"
 	  return -1
     }
+
+    foreach_with_prefix pie { "nopie" "pie" } {
+	stap_test "stap-probe-nosem-noopt-$pie" $pie
+    }
 }
 
 with_test_prefix "with semaphore, not optimized" {
-    stap_test "stap-probe-sem-noopt" "-DUSE_SEMAPHORES"
+    stap_test "stap-probe-sem-noopt" additional_flags=-DUSE_SEMAPHORES
 }
 
 with_test_prefix "without semaphore, optimized" {
@@ -179,5 +201,5 @@  with_test_prefix "without semaphore, optimized" {
 }
 
 with_test_prefix "with semaphore, optimized" {
-    stap_test_no_debuginfo "stap-probe-sem-opt" "-DUSE_SEMAPHORES"
+    stap_test_no_debuginfo "stap-probe-sem-opt" additional_flags=-DUSE_SEMAPHORES
 }