Re: [PATCH v3] line-range: Fix infinite loop bug with degenerate '$' regex

From: Lars Kellogg-Stedman <lars@oddbit.com>
Date: Dec 14, 2022, 14:53 UTC
Message-ID: <20221214145341.sonppjtshwqoxs6n@oddbit.com>
In-Reply-To: <xmqq5yeiwr6x.fsf@gitster.g>

On Sun, Dec 11, 2022 at 12:32:38PM +0900, Junio C Hamano wrote:

Show 9 quoted lines

> Lars Kellogg-Stedman <lars@oddbit.com> writes:
> 
> > When the -L argument to "git log" is passed the degenerate regular
> > expression "$" (as in "-L :$:line-range.c"), this results in an
> > infinite loop in find_funcname_matching_regexp().
> 
> Is "matching an empty line" the only way a regular expression can be
> a "degenerate" one?  If not, perhaps being a bit more explicit would
> help the readers, e.g.

"Matching an empty line" isn't the issue (and if I implied that it was, that's my bad). The issue only crops up when matching the zero-width regular expression at the *end* of a line.

I'll update the subject and description to be more clear...

> This clear explanation probably deserves to be in the commit log
> message proper.

...including adding the extended description to the commit message. Some projects object if commit messages get too wordy.

> Please write it on two lines, and highlight an empty body of the
> loop, like so

Will do.

Show 10 quoted lines

> > @@ -161,6 +160,7 @@ static const char *find_funcname_matching_regexp(xdemitconf_t *xecfg, const char
> >  			return bol;
> >  		start = eol;
> >  	}
> > +	return NULL;
> >  }
> 
> What is this change about?  Isn't the above an endless loop without
> break, from which the only way for the control to leave it is by
> returning?

It's not an endless loop without break; this change modified the loop condition:

> -       while (1) {
> +       while (*start) {

That would have prevented the infinite loop in the first place. I'm happy to drop this change if you prefer, but if this condition had been in place originally we wouldn't have had the infinite loop bug (we would still have had incorrect behavior, but it would have been perhaps easier for an end user to diagnose).

> Style: lose the SP after "cat >".

Will do.

-- 
Lars Kellogg-Stedman <lars@oddbit.com> | larsks @ {irc,twitter,github}
http://blog.oddbit.com/                | N1LKS