Re: [PATCH 2/3] diff_populate_filespec: NUL-terminate buffers

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Sep 8, 2016, 07:49 UTC
Message-ID: <alpine.DEB.2.20.1609080933470.129229@virtualbox>
In-Reply-To: <20160906184143.55a5zoa2mj6c2e5m@sigill.intra.peff.net>

Hi Peff,

On Tue, 6 Sep 2016, Jeff King wrote:

Show 14 quoted lines

> On Tue, Sep 06, 2016 at 06:02:59PM +0200, Johannes Schindelin wrote:
> 
> > It will still be quite tricky, because we have to touch a function that is
> > rather at the bottom of the food chain: diff_populate_filespec() is called
> > from fill_textconv(), which in turn is called from pickaxe_match(), and
> > only pickaxe_match() knows whether we want to call regexec() or not (it
> > depends on its regexp parameter).
> > 
> > Adding a flag to diff_populate_filespec() sounds really reasonable until
> > you see how many call sites fill_textconv() has.
> 
> I was thinking of something quite gross, like a global "switch to using
> slower-but-safer NUL termination" flag (but I agree with Junio's point
> elsewhere that we do not even know if it is "slower").

Urgh.

;-)

Show 11 quoted lines

> > So now for the better idea.
> > 
> > While I was researching the code for this reply, I hit upon one thing
> > that I never knew existed, introduced in f96e567 (grep: use
> > REG_STARTEND for all matching if available, 2010-05-22). Apparently,
> > NetBSD introduced an extension to regexec() where you can specify
> > buffer boundaries using REG_STARTEND. Which is pretty much what we
> > need.
> 
> Yes, and compat/regex support this, too. My question is whether it is
> portable.

That is only one question.

Another, important question is: is it efficient?

I have no idea whether there exists any hardware-accelerated regex library out there, maybe even using CUDA (I know that there is some code out there using SSE to perform LF -> CR/LF conversion, unfortunately it is intentionally incompatible with GPLv2).

We cannot simply switch everybody and her dog to compat/regex/ just because we want to avoid a segfault.

Show 19 quoted lines

> > diff --git a/diff.c b/diff.c
> > index 534c12e..2c5a360 100644
> > --- a/diff.c
> > +++ b/diff.c
> > @@ -951,7 +951,13 @@ static int find_word_boundaries(mmfile_t *buffer,
> > regex_t *word_regex,
> >  {
> >  	if (word_regex && *begin < buffer->size) {
> >  		regmatch_t match[1];
> > -		if (!regexec(word_regex, buffer->ptr + *begin, 1, match,
> > 		0)) {
> > +		int f = 0;
> > +#ifdef REG_STARTEND
> > +		match[0].rm_so = 0;
> > +		match[0].rm_eo = *end - *begin;
> > +		f = REG_STARTEND;
> > +#endif
> > +		if (!regexec(word_regex, buffer->ptr + *begin, 1, match,
> > f)) {

Heh. You introduced the same bug I did. Or maybe you just fetched my mmap-regexec branch and looked at an intermediate iteration?

The problem with this patch is that *end is uninitialized. I actually initialized it in my patch, but it was still incorrect. I settled on using buffer->size - *begin in the end.

> What happens to those poor souls on systems without REG_STARTEND? Do
> they get to keep segfaulting?

Of course not. Those poor souls on systems without REG_STARTEND pay a little price for that: malloc(); memcpy(); *end = '\0'; ... free();

I think it is worth it: maintenance of the code is much easier that way than forcing everybody and her dog and her dog's hamster to compat/regex/.

> But I much prefer this approach to copying the data just to add a NUL.

I think it is not worth the burden. The only regex implementation in semi-widespread use that do not support REG_STARTEND seems to be musl.

I'd rather not spend *so much* effort just to support an obscure platform. Not when the users of that obscure platform could spend that effort themselves. And probably won't, because we only copy data to add a NUL on those platforms when regexec() is called on an mmfile_t.

Better to keep it simple, Dscho