From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 61379 invoked by alias); 27 May 2015 04:27:41 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 61364 invoked by uid 89); 27 May 2015 04:27:40 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.8 required=5.0 tests=AWL,BAYES_00,FREEMAIL_ENVFROM_END_DIGIT,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-pd0-f176.google.com Received: from mail-pd0-f176.google.com (HELO mail-pd0-f176.google.com) (209.85.192.176) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Wed, 27 May 2015 04:27:39 +0000 Received: by pdbqa5 with SMTP id qa5so108218664pdb.0 for ; Tue, 26 May 2015 21:27:37 -0700 (PDT) X-Received: by 10.70.49.73 with SMTP id s9mr31100098pdn.149.1432700857419; Tue, 26 May 2015 21:27:37 -0700 (PDT) Received: from seba.sebabeach.org.gmail.com (173-13-178-53-sfba.hfc.comcastbusiness.net. [173.13.178.53]) by mx.google.com with ESMTPSA id f4sm14611661pdc.95.2015.05.26.21.27.36 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 26 May 2015 21:27:36 -0700 (PDT) From: Doug Evans To: Keith Seitz Cc: gdb-patches@sourceware.org Subject: Re: [PATCH v4 7/9] Explicit locations: add UI features for CLI References: <20150507180523.19629.77846.stgit@valrhona.uglyboxes.com> <20150507180602.19629.40685.stgit@valrhona.uglyboxes.com> <555B9FF2.4030203@redhat.com> Date: Wed, 27 May 2015 04:27:00 -0000 In-Reply-To: <555B9FF2.4030203@redhat.com> (Keith Seitz's message of "Tue, 19 May 2015 13:41:22 -0700") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2015-05/txt/msg00646.txt.bz2 Keith Seitz writes: >>> +/* See description in location.h. */ >>> + >>> +struct event_location * >>> +string_to_explicit_location (const char **argp, >>> + const struct language_defn *language, >>> + int dont_throw) >>> +{ >>> + struct cleanup *cleanup; >>> + struct event_location *location; >>> + >>> + /* It is assumed that input beginning with '-' and a non-digit >>> + character is an explicit location. */ >>> + if (argp == NULL >>> + || *argp == '\0' >>> + || *argp[0] != '-' >>> + || !isalpha ((*argp)[1])) >>> + return NULL; >>> + >>> + location = new_explicit_location (NULL); >>> + cleanup = make_cleanup_delete_event_location (location); >>> + >>> + /* Process option/argument pairs. dprintf_command >>> + requires that processing stop on ','. */ >>> + while ((*argp)[0] != '\0' && (*argp)[0] != ',') >>> + { >>> + int len; >>> + char *opt, *oarg; >>> + const char *start; >>> + struct cleanup *inner; >>> + >>> + /* If *ARGP starts with a keyword, stop processing >>> + options. */ >>> + if (linespec_lexer_lex_keyword (*argp) != NULL) >>> + break; >>> + >>> + /* Mark the start of the string in case we need to rewind. */ >>> + start = *argp; >>> + >>> + /* Get the option string. */ >>> + opt = explicit_location_lex_one (argp, language); >>> + inner = make_cleanup (xfree, opt); >>> + >>> + *argp = skip_spaces_const (*argp); >>> + >>> + /* Get the argument string. */ >>> + oarg = explicit_location_lex_one (argp, language); >>> + >>> + *argp = skip_spaces_const (*argp); >>> + >>> + /* Use the length of the option to allow abbreviations. */ >>> + len = strlen (opt); >>> + >>> + /* All options have a required argument. Checking for this required >>> + argument is deferred until later. */ >>> + if (strncmp (opt, "-source", len) == 0) >>> + EL_EXPLICIT (location)->source_filename = oarg; >>> + else if (strncmp (opt, "-function", len) == 0) >>> + EL_EXPLICIT (location)->function_name = oarg; >>> + else if (strncmp (opt, "-line", len) == 0) >>> + { >>> + if (oarg != NULL) >>> + { >>> + TRY >>> + { >>> + EL_EXPLICIT (location)->line_offset >>> + = linespec_parse_line_offset (oarg); >>> + } >>> + CATCH (e, RETURN_MASK_ERROR) >>> + { >>> + xfree (oarg); >> >> Could other exception types leak oarg here? > > Not that I can see. When any successful argument value is parsed, it is > added to the event_location, which has a cleanup on it which will free > any defined members when an exception occurs. > > The only two functions (in this loop) that could throw an exception are > explicit_location_lex_one and linespec_parse_line_offset. > > In the former case, the option name has a cleanup when parsing the > value. The value is either saved into the event_location or discarded if > we are going to throw an exception. > > linespec_parse_line_offset can throw an error (GENERIC_ERROR), but it is > already caught and memory for oarg is freed. Nothing can generate a > RETURN_QUIT as far as I can tell. Did you have a case specifically in mind? I'm just worried about the robustness (or lack thereof) to future changes. Plus the readability. You've done the research to conclude that the code is ok, (e.g., linespec_parse_line_offset doesn't call QUIT) but the casual reader will be left with the question: "Hmmm, can oarg leak?", and a conscientious reader would then have to put in the time to do that research too. It'd be preferable if the code could be read and understood to be correct without having to do that research.