Re: [PATCH] Fix problem handling colon in linespec, PR breakpoints/18303

[Doug Evans <dje@google.com>]

Keith Seitz writes:
  > Hi,
  >
  > Thank you for pointing this out and supplying a patch (and *tests*!).
  >
  > On 01/08/2016 10:44 AM, Don Breazeal wrote:
  >
  > > During GDB's parsing of the linespec, when the filename was not found  
in
  > > the program's symbols, GDB tried to determine if the filename string
  > > could be some other valid identifier.  Eventually it would get to a  
point
  > > where it concluded that the Windows logical volume, or whatever was to  
the
  > > left of the colon, was a C++ namespace.  When it found only one colon,  
it
  > > would assert.
  >
  > I have to admit, when I first read this, my reaction was that this is a
  > symbol lookup error. After investigating it a bit, I think it is. [More
  > rationale below.]
  >
  > > GDB's linespec grammar allows for several different linespecs that  
contain
  > > ':'.  The only one that has a number after the colon is  
filename:linenum.
  >
  > True, but for how long? I don't know. The parser actually does/could (or
  > for some brief time *did*) support offsets just about anywhere, e.g.,
  > foo.c:function:label:3. That support was removed and legacy (buggy)
  > behavior of ignoring the offset was maintained in the parser/sal
  > converter. There is no reason we couldn't support it, though.
  >
  > > There is another possible solution.  After no symbols are found for the
  > > file and we determine that it is a filename:linenum style linespec, we
  > > could just consume the filename token and parse the rest of the
  > > linespec.  That works as well, but there is no advantage to it.
  >
  > And, of course, I came up with an entirely different solution. :-)
  >
  > Essentially what is happening is that the linespec parser is calling
  > lookup_symbol on the user input (e.g., "spaces: and :colons.cc"). That
  > is causing several problems, all which assume the input is well-formed.
  > As you've discovered, that is a pretty poor assumption. Malformed (user)
  > input should not cause an assertion failure IMO.
  >
  > > I created two new tests for this.  One is just gdb.linespec/ls-errs.exp
  > > copied and converted to use C++ instead of C, and to add the Windows
  > > logical drive case.  The other is an MI test to provide a regression
  > > test for the specific case reported in PR 18303.
  >
  > EXCELLENT! Thank you so much for doing this. These tests were
  > outrageously useful while attempting to understand the problem.
  >
  > With that said, I offer *for discussion* this alternative solution,
  > which removes the assumption that input to lookup_symbol is/must be
  > well-formed.
  >
  > [There is one additional related/necessary fixlet snuck in here... The
  > C++ name parser always assumed that ':' was followed by another ':'. Bad
  > assumption. So it would return an incorrect result in the malformed  
case.]
  >
  > WDYT?
  >
  > Keith
  >
  > [apologies on mailer wrapping]
  >
  > Author: Keith Seitz <keiths@redhat.com>
  > Date:   Fri Jan 8 14:00:22 2016 -0800
  >
  > Tolerate malformed input for lookup_symbol-called functions.
  >
  > lookup_symbol is often called with user input. Consequently, any
  > function called from lookup_symbol{,_in_language} should attempt to deal
  > with malformed input gracefully. After all, malformed user input is
  > not a programming/API error.
  >
  > This patch does not attempt to find/correct all instances of this.
  > It only fixes locations in the code that trigger test suite failures.
  >
  >     gdb/ChangeLog
  >
  >     	* cp-namespace.c (cp_lookup_bare_symbol): Do not assert on
  >     	user input.
  >     	(cp_search_static_and_baseclasses): Return null_block_symbol for
  >     	malformed input.
  >     	Remove assertions.
  >     	* cp-support.c (cp_find_first_component_aux): Do not return
  >     	a prefix length for ':' unless the next character is also ':'.
  >
  > diff --git a/gdb/cp-namespace.c b/gdb/cp-namespace.c
  > index 72002d6..aa225fe 100644
  > --- a/gdb/cp-namespace.c
  > +++ b/gdb/cp-namespace.c
  > @@ -166,12 +166,6 @@ cp_lookup_bare_symbol (const struct language_defn
  > *langdef,
  >  {
  >    struct block_symbol sym;
  >
  > -  /* Note: We can't do a simple assert for ':' not being in NAME because
  > -     ':' may be in the args of a template spec.  This isn't intended to  
be
  > -     a complete test, just cheap and documentary.  */
  > -  if (strchr (name, '<') == NULL && strchr (name, '(') == NULL)
  > -    gdb_assert (strchr (name, ':') == NULL);
  > -

Heya.

The assert is intended to catch (some) violations of this
(from the function comment):

    NAME is guaranteed to not have any scope (no "::") in its name, though
    if for example NAME is a template spec then "::" may appear in the
    argument list.

This is an invariant on what name can (and cannot) be.
IOW, it is wrong to call this function with name == "foo::bar",
handling that is the caller's responsibility.
Thus I think having an assert here is ok and good
(as long as it tests for the correct thing!).

Whether it is ok to call this with name == "c:mumble" is the issue.
[or even "c:::mumble" or whatever else a user could type that
this function's caller isn't expected to handle]
On that I'm kinda ambivalent, but I like having the assert
watch for the stated invariant.

Thoughts?

  >    sym = lookup_symbol_in_static_block (name, block, domain);
  >    if (sym.symbol != NULL)
  >      return sym;
  > @@ -246,10 +240,9 @@ cp_search_static_and_baseclasses (const char *name,
  >    struct block_symbol klass_sym;
  >    struct type *klass_type;
  >
  > -  /* The test here uses <= instead of < because Fortran also uses this,
  > -     and the module.exp testcase will pass "modmany::" for NAME here.   
*/
  > -  gdb_assert (prefix_len + 2 <= strlen (name));
  > -  gdb_assert (name[prefix_len + 1] == ':');
  > +  /* Check for malformed input.  */
  > +  if (prefix_len + 2 > strlen (name) || name[prefix_len + 1] != ':')
  > +    return null_block_symbol;
  >
  >    /* Find the name of the class and the name of the method, variable,
  > etc.  */
  >
  > diff --git a/gdb/cp-support.c b/gdb/cp-support.c
  > index df127c4..a71c6ad 100644
  > --- a/gdb/cp-support.c
  > +++ b/gdb/cp-support.c
  > @@ -1037,8 +1037,13 @@ cp_find_first_component_aux (const char *name,
  > int permissive)
  >  	      return strlen (name);
  >  	    }
  >  	case '\0':
  > -	case ':':
  >  	  return index;
  > +	case ':':
  > +	  /* ':' marks a component iff the next character is also a ':'.
  > +	     Otherwise it is probably malformed input.  */
  > +	  if (name[index + 1] == ':')
  > +	    return index;
  > +	  break;

What if name[index+2] is also ':'? :-)

[btw, I think "a::::b" is illegal (note four colons),
but I don't know that for sure]

  >  	case 'o':
  >  	  /* Operator names can screw up the recursion.  */
  >  	  if (operator_possible
  >

-- 
/dje