[PATCH] Fix for PR breakpoints/16297: Fix catch syscall to work with syscall 0

[PATCH] Fix for PR breakpoints/16297: Fix catch syscall to work with syscall 0

[Gabriel Krisman Bertazi]

[-- Attachment #1: Type: text/plain, Size: 525 bytes --]

Hello,

This is a fix for bug 16297. The problem occurs when the user attempts
to catch any syscall 0 (such as syscall read on Linux/x86_64). GDB was
not able to catch the syscall and was missing the breakpoint.

Now, breakpoint_hit_catch_syscall returns immediately when it finds the
correct syscall number, avoiding a following check for the end of the
search vector, that returns a no hit if the syscall number was zero.

I already sent the form to FSF so I can sign the copyright
assigment.

-- 
Gabriel Krisman Bertazi


[-- Attachment #2: changelog.txt --]
[-- Type: text/plain, Size: 180 bytes --]

2013-12-15  Gabriel Krisman Bertazi  <gabriel@krisman.be>

	PR breakpoints/16297
	* breakpoint.c (breakpoint_hit_catch_syscall): Return immediately
	when expected syscall is hit.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: return_immediately_catch_syscall_hit.patch --]
[-- Type: text/x-diff, Size: 443 bytes --]

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 589aa19..e798d1a 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -8325,10 +8325,9 @@ breakpoint_hit_catch_syscall (const struct bp_location *bl,
            VEC_iterate (int, c->syscalls_to_be_caught, i, iter);
            i++)
 	if (syscall_number == iter)
-	  break;
+	  return 1;
       /* Not the same.  */
-      if (!iter)
-	return 0;
+      return 0;
     }
 
   return 1;

Re: [PATCH] Fix for PR breakpoints/16297: Fix catch syscall to work with syscall 0

[Sergio Durigan Junior]

On Sunday, December 15 2013, Gabriel Krisman Bertazi wrote:

> This is a fix for bug 16297. The problem occurs when the user attempts
> to catch any syscall 0 (such as syscall read on Linux/x86_64). GDB was
> not able to catch the syscall and was missing the breakpoint.
>
> Now, breakpoint_hit_catch_syscall returns immediately when it finds the
> correct syscall number, avoiding a following check for the end of the
> search vector, that returns a no hit if the syscall number was zero.

Thanks.

Just for the record, I pre-reviewed Krisman's patch and it's OK.  I am
also helping him in obtaining the copyright assignment.

I will send a patch to test this feature as soon as my other patch
(which touches catch-syscall.exp and improves it) gets approved.

-- 
Sergio

Re: [PATCH] Fix for PR breakpoints/16297: Fix catch syscall to work with syscall 0

[Pedro Alves]

On 12/15/2013 04:04 AM, Sergio Durigan Junior wrote:
> On Sunday, December 15 2013, Gabriel Krisman Bertazi wrote:
> 
>> This is a fix for bug 16297. The problem occurs when the user attempts
>> to catch any syscall 0 (such as syscall read on Linux/x86_64). GDB was
>> not able to catch the syscall and was missing the breakpoint.
>>
>> Now, breakpoint_hit_catch_syscall returns immediately when it finds the
>> correct syscall number, avoiding a following check for the end of the
>> search vector, that returns a no hit if the syscall number was zero.
> 
> Thanks.
> 
> Just for the record, I pre-reviewed Krisman's patch and it's OK.  I am
> also helping him in obtaining the copyright assignment.

Thanks you both.  FAOD, this is OK.

> I will send a patch to test this feature as soon as my other patch
> (which touches catch-syscall.exp and improves it) gets approved.

-- 
Pedro Alves

Re: [PATCH] Fix for PR breakpoints/16297: Fix catch syscall to work with syscall 0

[Pedro Alves]

Bah, got distracted by something else, and forgot to write the
bit that made me want to reply in the first place.  :-P

I meant to say that I think this is small enough to not be
legally significant and so it can go in before the copyright
assignment is in place.  Feel free to push it in.

Thanks,
-- 
Pedro Alves

Re: [PATCH] Fix for PR breakpoints/16297: Fix catch syscall to work with syscall 0

[Sergio Durigan Junior]

On Monday, December 16 2013, Pedro Alves wrote:

> Bah, got distracted by something else, and forgot to write the
> bit that made me want to reply in the first place.  :-P
>
> I meant to say that I think this is small enough to not be
> legally significant and so it can go in before the copyright
> assignment is in place.  Feel free to push it in.

Thanks :-).  If it is OK to you, I will wait until my other patch
touching catch-syscall.exp is approved, so that I can write a simple
extension to the testcase that will test this specific bug.  Then, I can
commit both patches (this one and the testcase one) together.

-- 
Sergio

Re: [PATCH] Fix for PR breakpoints/16297: Fix catch syscall to work with syscall 0

[Pedro Alves]

On 12/16/2013 06:03 PM, Sergio Durigan Junior wrote:

> If it is OK to you, I will wait until my other patch
> touching catch-syscall.exp is approved, so that I can write a simple
> extension to the testcase that will test this specific bug.  Then, I can
> commit both patches (this one and the testcase one) together.

Yes, sounds great.

Thanks,
-- 
Pedro Alves

Re: [PATCH] Fix for PR breakpoints/16297: Fix catch syscall to work with syscall 0

[Sergio Durigan Junior]

On Monday, December 16 2013, Pedro Alves wrote:

> On 12/16/2013 06:03 PM, Sergio Durigan Junior wrote:
>
>> If it is OK to you, I will wait until my other patch
>> touching catch-syscall.exp is approved, so that I can write a simple
>> extension to the testcase that will test this specific bug.  Then, I can
>> commit both patches (this one and the testcase one) together.
>
> Yes, sounds great.

OK, here's the testsuite patch.  I will commit it along with the code,
and give Krisman the authorship.

This testcase is a little difficult to write.  By doing a quick
inspection at the Linux source, one can see that, in many targets, the
syscall number 0 is restart_syscall, which is forbidden to be called
from userspace.  Therefore, on many targets, there's just no way to test
this safely.

My decision was to take the simpler route and just adds the "read"
syscall on the default test.  Its number on x86_64 is zero, which is
"good enough" since many people here do their tests on x86_64 anyway and
it is a popular architecture.

-- 
Sergio

2013-12-19  Sergio Durigan Junior  <sergiodj@redhat.com>

	* gdb.base/catch-syscall.c (read_syscall): New variable.
	(main): Call dummy read syscall.
	* gdb.base/catch-syscall.exp (all_syscalls): Include "read"
	syscall.
	(fill_all_syscalls_numbers): Get "read" syscall number.  Include
	it in all_syscalls_numbers.

diff --git a/gdb/testsuite/gdb.base/catch-syscall.c b/gdb/testsuite/gdb.base/catch-syscall.c
index 8f94191..6f28f5b 100644
--- a/gdb/testsuite/gdb.base/catch-syscall.c
+++ b/gdb/testsuite/gdb.base/catch-syscall.c
@@ -16,6 +16,8 @@
 
 static int close_syscall = SYS_close;
 static int chroot_syscall = SYS_chroot;
+/* The "read" syscall is zero on x86_64.  */
+static int read_syscall = SYS_read;
 static int exit_group_syscall = SYS_exit_group;
 
 int
@@ -27,6 +29,8 @@ main (void)
 
 	chroot (".");
 
+	read (0, NULL, 0);
+
 	/* The last syscall.  Do not change this.  */
 	_exit (0);
 }
diff --git a/gdb/testsuite/gdb.base/catch-syscall.exp b/gdb/testsuite/gdb.base/catch-syscall.exp
index fd7d2db..4cac454 100644
--- a/gdb/testsuite/gdb.base/catch-syscall.exp
+++ b/gdb/testsuite/gdb.base/catch-syscall.exp
@@ -47,7 +47,7 @@ if  { [prepare_for_testing ${testfile}.exp $testfile ${testfile}.c] } {
 
 # All (but the last) syscalls from the example code
 # They are ordered according to the file, so do not change this.
-set all_syscalls { "close" "chroot" }
+set all_syscalls { "close" "chroot" "read" }
 set all_syscalls_numbers { }
 
 # The last syscall (exit()) does not return, so
@@ -396,7 +396,9 @@ proc fill_all_syscalls_numbers {} {
 
     set close_syscall [get_integer_valueof "close_syscall" -1]
     set chroot_syscall [get_integer_valueof "chroot_syscall" -1]
-    set all_syscalls_numbers [list $close_syscall $chroot_syscall]
+    set read_syscall [get_integer_valueof "read_syscall" -1]
+    set all_syscalls_numbers [list $close_syscall $chroot_syscall \
+				  $read_syscall]
     set last_syscall_number [get_integer_valueof "exit_group_syscall" -1]
 }
 

Re: [PATCH] Fix for PR breakpoints/16297: Fix catch syscall to work with syscall 0

[Pedro Alves]

On 12/19/2013 03:50 AM, Sergio Durigan Junior wrote:
> @@ -27,6 +29,8 @@ main (void)
>  
>  	chroot (".");
>  
> +	read (0, NULL, 0);

I think the C implementation (libc or the compiler) is
free to skip actually calling the syscall, given bytes is 0.
Something like creating a pipe, and reading a byte off
of it might be safer.  But I won't object to leaving
this as is for now.

>  static int chroot_syscall = SYS_chroot;
> +/* The "read" syscall is zero on x86_64.  */
> +static int read_syscall = SYS_read;

Future readers who might not be familiar with this bug
probably won't realize that the emphasis should be on
zero, rather than the comment just happening to be
trying to be informative.  I'd suggest extending the comment:

+/* GDB had a bug where it couldn't catch syscall number 0.  In most
+   Linux architectures, syscall number 0 is restart_syscall, which
+   can't be called from userspace.  However, the "read" syscall
+   is zero on x86_64.  */
+static int read_syscall = SYS_read;


Otherwise looks fine to me.

Thanks!

-- 
Pedro Alves

Re: [PATCH] Fix for PR breakpoints/16297: Fix catch syscall to work with syscall 0

[Sergio Durigan Junior]

On Thursday, December 19 2013, Pedro Alves wrote:

> On 12/19/2013 03:50 AM, Sergio Durigan Junior wrote:
>> @@ -27,6 +29,8 @@ main (void)
>>  
>>  	chroot (".");
>>  
>> +	read (0, NULL, 0);
>
> I think the C implementation (libc or the compiler) is
> free to skip actually calling the syscall, given bytes is 0.
> Something like creating a pipe, and reading a byte off
> of it might be safer.  But I won't object to leaving
> this as is for now.

Aha, that's a safer solution indeed!  Thanks for the insight.

>>  static int chroot_syscall = SYS_chroot;
>> +/* The "read" syscall is zero on x86_64.  */
>> +static int read_syscall = SYS_read;
>
> Future readers who might not be familiar with this bug
> probably won't realize that the emphasis should be on
> zero, rather than the comment just happening to be
> trying to be informative.  I'd suggest extending the comment:
>
> +/* GDB had a bug where it couldn't catch syscall number 0.  In most
> +   Linux architectures, syscall number 0 is restart_syscall, which
> +   can't be called from userspace.  However, the "read" syscall
> +   is zero on x86_64.  */
> +static int read_syscall = SYS_read;

Done.

> Otherwise looks fine to me.

Thanks.  Checked-in:

<https://sourceware.org/ml/gdb-cvs/2013-12/msg00101.html>

The full patch is below.

-- 
Sergio

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 374b8c5..2bae7ae 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2013-12-19  Gabriel Krisman Bertazi  <gabriel@krisman.be>
+
+	PR breakpoints/16297
+	* breakpoint.c (breakpoint_hit_catch_syscall): Return immediately
+	when expected syscall is hit.
+
 2013-12-19  Tom Tromey  <tromey@redhat.com>
 
 	* ser-unix.c (hardwire_ops): New global.
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 589aa19..6a11ddf 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -8325,10 +8325,9 @@ breakpoint_hit_catch_syscall (const struct bp_location *bl,
            VEC_iterate (int, c->syscalls_to_be_caught, i, iter);
            i++)
 	if (syscall_number == iter)
-	  break;
-      /* Not the same.  */
-      if (!iter)
-	return 0;
+	  return 1;
+
+      return 0;
     }
 
   return 1;
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 3e1c756..97ad49b 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,15 @@
+2013-12-19  Sergio Durigan Junior  <sergiodj@redhat.com>
+
+	PR breakpoints/16297
+	* gdb.base/catch-syscall.c (read_syscall, pipe_syscall)
+	(write_syscall): New variables.
+	(main): Create a pipe, write 1 byte in it, and read 1 byte from
+	it.
+	* gdb.base/catch-syscall.exp (all_syscalls): Include "pipe,
+	"write" and "read" syscalls.
+	(fill_all_syscalls_numbers): Improve the way to obtain syscalls
+	numbers.
+
 2013-12-19  Keven Boell  <keven.boell@intel.com>
 
 	* gdb.fortran/module.exp: Completion matches fortran module
diff --git a/gdb/testsuite/gdb.base/catch-syscall.c b/gdb/testsuite/gdb.base/catch-syscall.c
index 8f94191..aa5727a 100644
--- a/gdb/testsuite/gdb.base/catch-syscall.c
+++ b/gdb/testsuite/gdb.base/catch-syscall.c
@@ -16,17 +16,33 @@
 
 static int close_syscall = SYS_close;
 static int chroot_syscall = SYS_chroot;
+/* GDB had a bug where it couldn't catch syscall number 0 (PR 16297).
+   In most GNU/Linux architectures, syscall number 0 is
+   restart_syscall, which can't be called from userspace.  However,
+   the "read" syscall is zero on x86_64.  */
+static int read_syscall = SYS_read;
+static int pipe_syscall = SYS_pipe;
+static int write_syscall = SYS_write;
 static int exit_group_syscall = SYS_exit_group;
 
 int
 main (void)
 {
+	int fd[2];
+	char buf1[2] = "a";
+	char buf2[2];
+
 	/* A close() with a wrong argument.  We are only
 	   interested in the syscall.  */
 	close (-1);
 
 	chroot (".");
 
+	pipe (fd);
+
+	write (fd[1], buf1, sizeof (buf1));
+	read (fd[0], buf2, sizeof (buf2));
+
 	/* The last syscall.  Do not change this.  */
 	_exit (0);
 }
diff --git a/gdb/testsuite/gdb.base/catch-syscall.exp b/gdb/testsuite/gdb.base/catch-syscall.exp
index fd7d2db..5925419 100644
--- a/gdb/testsuite/gdb.base/catch-syscall.exp
+++ b/gdb/testsuite/gdb.base/catch-syscall.exp
@@ -47,7 +47,7 @@ if  { [prepare_for_testing ${testfile}.exp $testfile ${testfile}.c] } {
 
 # All (but the last) syscalls from the example code
 # They are ordered according to the file, so do not change this.
-set all_syscalls { "close" "chroot" }
+set all_syscalls { "close" "chroot" "pipe" "write" "read" }
 set all_syscalls_numbers { }
 
 # The last syscall (exit()) does not return, so
@@ -392,11 +392,12 @@ proc do_syscall_tests_without_xml {} {
 # This procedure fills the vector "all_syscalls_numbers" with the proper
 # numbers for the used syscalls according to the architecture.
 proc fill_all_syscalls_numbers {} {
-    global all_syscalls_numbers last_syscall_number
+    global all_syscalls_numbers last_syscall_number all_syscalls
+
+    foreach syscall $all_syscalls {
+	lappend all_syscalls_numbers [get_integer_valueof "${syscall}_syscall" -1]
+    }
 
-    set close_syscall [get_integer_valueof "close_syscall" -1]
-    set chroot_syscall [get_integer_valueof "chroot_syscall" -1]
-    set all_syscalls_numbers [list $close_syscall $chroot_syscall]
     set last_syscall_number [get_integer_valueof "exit_group_syscall" -1]
 }