From patchwork Fri Jan 8 23:03:01 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Keith Seitz X-Patchwork-Id: 10307 Received: (qmail 30292 invoked by alias); 8 Jan 2016 23:03:06 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 30276 invoked by uid 89); 8 Jan 2016 23:03:05 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.6 required=5.0 tests=AWL, BAYES_00, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=sk:operato, sym, reaction, sk:lookup_ X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Fri, 08 Jan 2016 23:03:03 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (Postfix) with ESMTPS id ABCEE19F999; Fri, 8 Jan 2016 23:03:02 +0000 (UTC) Received: from valrhona.uglyboxes.com (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u08N31cg007433 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Fri, 8 Jan 2016 18:03:01 -0500 From: Keith Seitz Subject: Re: [PATCH] Fix problem handling colon in linespec, PR breakpoints/18303 References: <1452278651-8630-1-git-send-email-donb@codesourcery.com> To: donb@codesourcery.com Cc: "gdb-patches@sourceware.org ml" X-Enigmail-Draft-Status: N1110 Message-ID: <56904025.8060407@redhat.com> Date: Fri, 8 Jan 2016 15:03:01 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 MIME-Version: 1.0 In-Reply-To: <1452278651-8630-1-git-send-email-donb@codesourcery.com> X-IsSubscribed: yes 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 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 ':'. if (operator_possible 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); - 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; case 'o': /* Operator names can screw up the recursion. */