diff mbox

Validate explicit locations with early termination

Message ID 20171207212046.13452-1-keiths@redhat.com
State New, archived
Headers

Commit Message

Keith Seitz Dec. 7, 2017, 9:20 p.m. UTC 
  breakpoints/22569 involves an internal error generated by the rather
innocent looking command:

(gdb) break -source test.cpp main
.../linespec.c:3302: internal-error: void decode_line_full(...):
Assertion `result.size () == 1 || canonical->pre_expanded' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n)

The input string is tokenized into "-source", "test.cpp", and "main"
(input parsing breaks on whitespace). create_breakpoint is then called with
the explicit location (containing only the source file name) and "main" as
the extra_string argument.

No SaLs are created for this underspecified explicit location, and the
"result.size () == 1" evaluates false (as does the pre_expanded condition).
This triggers the assertion.

Normally string_to_explicit_location validates the input string.  However,
the presence of the string "main" causes the parser to exit early:

   802        else
   803          {
   804            /* End of the explicit location specification.
   805               Stop parsing and return whatever explicit location was
   806               parsed.  */
   807            *argp = start;
   808            return location;
   809          }

This bypasses the validation that is done a few lines down in this function
which would have emitted the expected error.  This patch fixes that.

Additionally, this patch also fixes an inconsistency with error reporting
in this use case:

(gdb) b -source foo
Source filename requires function, label, or line offset.
(gdb) b -source foo main
No source file named foo.

These two commands should have elicited the same error message.

gdb/ChangeLog:

	PR breakpoints/22569
	* location.c (string_to_explicit_location): When terminating
	parsing early, break out of enclosing loop instead of returning.

gdb/testsuite/ChangeLog:

	PR breakpoints/22569
	* gdb.linespec/ls-errs.exp: Change expected result of "break
	-source this file has spaces.c -line 3".
	Check that an explicit source file followed by whitespace is
	identified as an invalid explicit location.
---
 gdb/location.c                         | 2 +-
 gdb/testsuite/gdb.linespec/ls-errs.exp | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

Comments

Simon Marchi Dec. 7, 2017, 9:33 p.m. UTC | #1 
On 2017-12-07 04:20 PM, Keith Seitz wrote:
> breakpoints/22569 involves an internal error generated by the rather
> innocent looking command:
> 
> (gdb) break -source test.cpp main
> .../linespec.c:3302: internal-error: void decode_line_full(...):
> Assertion `result.size () == 1 || canonical->pre_expanded' failed.
> A problem internal to GDB has been detected,
> further debugging may prove unreliable.
> Quit this debugging session? (y or n)
> 
> The input string is tokenized into "-source", "test.cpp", and "main"
> (input parsing breaks on whitespace). create_breakpoint is then called with
> the explicit location (containing only the source file name) and "main" as
> the extra_string argument.
> 
> No SaLs are created for this underspecified explicit location, and the
> "result.size () == 1" evaluates false (as does the pre_expanded condition).
> This triggers the assertion.
> 
> Normally string_to_explicit_location validates the input string.  However,
> the presence of the string "main" causes the parser to exit early:
> 
>    802        else
>    803          {
>    804            /* End of the explicit location specification.
>    805               Stop parsing and return whatever explicit location was
>    806               parsed.  */
>    807            *argp = start;
>    808            return location;
>    809          }
> 
> This bypasses the validation that is done a few lines down in this function
> which would have emitted the expected error.  This patch fixes that.
> 
> Additionally, this patch also fixes an inconsistency with error reporting
> in this use case:
> 
> (gdb) b -source foo
> Source filename requires function, label, or line offset.
> (gdb) b -source foo main
> No source file named foo.
> 
> These two commands should have elicited the same error message.
> 
> gdb/ChangeLog:
> 
> 	PR breakpoints/22569
> 	* location.c (string_to_explicit_location): When terminating
> 	parsing early, break out of enclosing loop instead of returning.
> 
> gdb/testsuite/ChangeLog:
> 
> 	PR breakpoints/22569
> 	* gdb.linespec/ls-errs.exp: Change expected result of "break
> 	-source this file has spaces.c -line 3".
> 	Check that an explicit source file followed by whitespace is
> 	identified as an invalid explicit location.

This is quick customer support!  This patch fixes the reported issue
and LGTM.

Thanks!

Simon
Keith Seitz Dec. 7, 2017, 11:40 p.m. UTC | #2 
On 12/07/2017 01:33 PM, Simon Marchi wrote:
hese two commands should have elicited the same error message.
>>
>> gdb/ChangeLog:
>>
>> 	PR breakpoints/22569
>> 	* location.c (string_to_explicit_location): When terminating
>> 	parsing early, break out of enclosing loop instead of returning.
>>
>> gdb/testsuite/ChangeLog:
>>
>> 	PR breakpoints/22569
>> 	* gdb.linespec/ls-errs.exp: Change expected result of "break
>> 	-source this file has spaces.c -line 3".
>> 	Check that an explicit source file followed by whitespace is
>> 	identified as an invalid explicit location.
> 
> This is quick customer support!  This patch fixes the reported issue
> and LGTM.

I'm just happy to see someone is using the explicit location feature, and as you can see from the patch, it was pretty trivial anyway.

Thank you for the quick review, this patch has been pushed, and I will close the bug.

Keith
diff mbox

Patch

diff --git a/gdb/location.c b/gdb/location.c
index 6752462bcf..69b7aa4c85 100644
--- a/gdb/location.c
+++ b/gdb/location.c
@@ -843,7 +843,7 @@  string_to_explicit_location (const char **argp,
 	     Stop parsing and return whatever explicit location was
 	     parsed.  */
 	  *argp = start;
-	  return location;
+	  break;
 	}
 
       *argp = skip_spaces (*argp);
diff --git a/gdb/testsuite/gdb.linespec/ls-errs.exp b/gdb/testsuite/gdb.linespec/ls-errs.exp
index ee8bb087f0..a0bcc528f8 100644
--- a/gdb/testsuite/gdb.linespec/ls-errs.exp
+++ b/gdb/testsuite/gdb.linespec/ls-errs.exp
@@ -175,7 +175,7 @@  proc do_test {lang} {
     # Test that option lexing stops at whitespace boundaries, except
     # when lexing function names, where we want to handle setting
     # breakpoints on e.g., "int template_function<int>()".
-    test_break "-source this file has spaces.c -line 3" invalid_file "this"
+    test_break "-source this file has spaces.c -line 3" source_incomplete
     test_break "-function ret_type tmpl_function" \
 	invalid_function "ret_type tmpl_function"
     test_break "-source $srcfile -function ret_type tmpl_function" \
@@ -267,6 +267,7 @@  proc do_test {lang} {
 
     # Explicit linespec-specific tests
     test_break "-source $srcfile" source_incomplete
+    test_break "-source $srcfile main" source_incomplete
 }
 
 foreach_with_prefix lang {"C" "C++"} {