Re: [PATCH 0/3] Fix a segfault caused by regexec() being called on mmap()ed data

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Sep 6, 2016, 14:13 UTC
Message-ID: <alpine.DEB.2.20.1609061606411.129229@virtualbox>
In-Reply-To: <20160906065821.bxwitnk72rpn3xhl@sigill.intra.peff.net>

Hi Peff,

On Tue, 6 Sep 2016, Jeff King wrote:

Show 15 quoted lines

> On Mon, Sep 05, 2016 at 05:44:57PM +0200, Johannes Schindelin wrote:
> 
> > The problem with that: mmap()ed memory is *not* NUL-terminated, yet
> > the pickaxe code calls regexec() on it just the same.
> > 
> > This problem has been reported by my colleague Chris Sidi.
> > 
> > Please note that this patch series is a hot fix I applied to Git for
> > Windows (the bug does not trigger a segmentation fault for me on
> > Linux, strangely enough, but it is really a problem on Windows).
> 
> This has come up before, and I think somebody mentioned that on Linux,
> you are OK unless the buffer ends right at a page boundary (i.e., the
> buffer size is a multiple of the page size). I don't know if that's true
> or not.

In my tests on Linux, even when the buffer ended right at the page boundary, the memory after that was still legal to access, and typically had a NUL *somewhere*.

That's happenstance, of course, and could very well result in false positives (however unlikely that is).

Show 12 quoted lines

> > So at least I have a workaround in place. Ideally, though, we would
> > NUL-terminate the buffers only when needed, or somehow call regexec() on
> > ptr/size parameters instead of passing a supposedly NUL-terminated
> > string to it?
> 
> There's some discussion in:
> 
>   http://public-inbox.org/git/20121030121747.GA4231@sigill.intra.peff.net/#r
> 
> and the thread below it. The quickest way to fix regexec() would be to
> have everybody use the built-in GNU regex in compat/. People seemed
> somewhat positive on that direction, but we never followed up.

I had a brief look, and it is not pretty. You would have to pay me good money to dive in and try to implement a regexecn() based on what we have in compat/regex/. And then people would still complain, I guess, for not using the native regex support, where available.

Ciao, Dscho