Re: [PATCHv2] pickaxe: fix segfault with '-S<...> --pickaxe-regex'
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Mar 21, 2017, 11:46 UTC
- Message-ID
- <alpine.DEB.2.20.1703211244470.3767@virtualbox>
- In-Reply-To
- <20170318182408.4444-1-szeder.dev@gmail.com>
Hi Gábor,
On Sat, 18 Mar 2017, SZEDER Gábor wrote:
Show 29 quoted lines
> 'git {log,diff,...} -S<...> --pickaxe-regex' can segfault as a result of
> out-of-bounds memory reads.
>
> diffcore-pickaxe.c:contains() looks for all matches of the given regex
> in a buffer in a loop, advancing the buffer pointer to the end of the
> last match in each iteration. When we switched to REG_STARTEND in
> b7d36ffca (regex: use regexec_buf(), 2016-09-21), we started passing
> the size of that buffer to the regexp engine, too. Unfortunately,
> this buffer size is never updated on subsequent iterations, and as the
> buffer pointer advances on each iteration, this "bufptr+bufsize"
> points past the end of the buffer. This results in segmentation
> fault, if that memory can't be accessed. In case of 'git log' it can
> also result in erroneously listed commits, if the memory past the end
> of buffer is accessible and happens to contain data matching the
> regex.
>
> Reduce the buffer size on each iteration as the buffer pointer is
> advanced, thus maintaining the correct end of buffer location.
> Furthermore, make sure that the buffer pointer is not dereferenced in
> the control flow statements when we already reached the end of the
> buffer.
>
> The new test is flaky, I've never seen it fail on my Linux box even
> without the fix, but this is expected according to db5dfa3 (regex:
> -G<pattern> feeds a non NUL-terminated string to regexec() and fails,
> 2016-09-21). However, it did fail on Travis CI with the first (and
> incomplete) version of the fix, and based on that commit message I
> would expect the new test without the fix to fail most of the time on
> Windows.Thank you for catching and fixing this. On Windows, I indeed get a segmentation fault for the new test case without the patched code, and the patch indeed fixes this.
ACK, Dscho