Re: [PATCHv2] pickaxe: fix segfault with '-S<...> --pickaxe-regex'

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 1885 bytes --]

Hi Gábor,

On Sat, 18 Mar 2017, SZEDER Gábor wrote:

> '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