{"thread":{"id":"44008","subject":"[PATCH 2/3] diff_populate_filespec: NUL-terminate buffers","startedAt":"2016-09-05T15:45:15Z","lastAt":"2016-09-25T14:02:33Z","messageCount":66,"participants":["Johannes Schindelin","Junio C Hamano","Jeff King","Ramsay Jones"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"301099","messageId":"f899957fa71537aa2686f17ce18aaf16f2fea2ac.1473090278.git.johannes.schindelin@gmx.de","threadId":"44008","inReplyTo":"cover.1473090278.git.johannes.schindelin@gmx.de","subject":"[PATCH 2/3] diff_populate_filespec: NUL-terminate buffers","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-05T15:45:06Z","receivedAt":"2016-09-05T15:45:15Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"While the xdiff machinery is quite capable of working with strings given\nas pointer and size, Git's add-on functionality simply assumes that we\nare operating on NUL-terminated strings, e.g. y running regexec() on the\nprovided pointer, with no way to pass the size, too.\n\nIn general, this assumption is wrong.\n\nIt is true that many code paths populate the mmfile_t structure silently\nappending a NUL, e.g. when running textconv on a temporary file and\nreading the results back into an strbuf.\n\nThe assumption is most definitely wrong, however, when mmap()ing a file.\n\nPractically, we seemed to be lucky that the bytes after mmap()ed memory\nwere 1) accessible and 2) somehow contained NUL bytes *somewhere*.\n\nIn a use case reported by Chris Sidi, it turned out that the mmap()ed\nfile had the precise size of a memory page, and on Windows the bytes\nafter memory-mapped pages are in general not valid.\n\nThis patch works around that issue, giving us time to discuss the best\ncourse how to fix this problem more generally.\n\nSigned-off-by: Johannes Schindelin \n---\n diff.c | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/diff.c b/diff.c\nindex 534c12e..32f7f46 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2826,6 +2826,15 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)\n \t\t\ts->data = strbuf_detach(&buf, &size);\n \t\t\ts->size = size;\n \t\t\ts->should_free = 1;\n+\t\t} else {\n+\t\t\t/* data must be NUL-terminated so e.g. for regexec() */\n+\t\t\tchar *data = xmalloc(s->size + 1);\n+\t\t\tmemcpy(data, s->data, s->size);\n+\t\t\tdata[s->size] = '\\0';\n+\t\t\tmunmap(s->data, s->size);\n+\t\t\ts->should_munmap = 0;\n+\t\t\ts->data = data;\n+\t\t\ts->should_free = 1;\n \t\t}\n \t}\n \telse {\n-- \n2.10.0.windows.1.2.g732a511\n\n\n"},{"id":"301100","messageId":"cover.1473090278.git.johannes.schindelin@gmx.de","threadId":"44008","inReplyTo":null,"subject":"[PATCH 0/3] Fix a segfault caused by regexec() being called on mmap()ed data","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-05T15:44:57Z","receivedAt":"2016-09-05T15:45:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This patch series addresses a problem where `git diff` is called using\n`-G` or `-S --pickaxe-regex` on new-born files that are configured\nwithout user diff drivers, and that hence get mmap()ed into memory.\n\nThe problem with that: mmap()ed memory is *not* NUL-terminated, yet the\npickaxe code calls regexec() on it just the same.\n\nThis problem has been reported by my colleague Chris Sidi.\n\nPlease note that this patch series is a hot fix I applied to Git for\nWindows (the bug does not trigger a segmentation fault for me on Linux,\nstrangely enough, but it is really a problem on Windows).\n\nSo at least I have a workaround in place. Ideally, though, we would\nNUL-terminate the buffers only when needed, or somehow call regexec() on\nptr/size parameters instead of passing a supposedly NUL-terminated\nstring to it?\n\n\nJohannes Schindelin (3):\n Demonstrate a problem: our pickaxe code assumes NUL-terminated buffers\n diff_populate_filespec: NUL-terminate buffers\n diff_grep: add assertions verifying that the buffers are\n NUL-terminated\n\n diff.c | 9 +++++++++\n diffcore-pickaxe.c | 2 ++\n t/t4059-diff-pickaxe.sh | 22 ++++++++++++++++++++++\n 3 files changed, 33 insertions(+)\n create mode 100755 t/t4059-diff-pickaxe.sh\n\nPublished-As: https://github.com/dscho/git/releases/tag/mmap-regexec-v1\nFetch-It-Via: git fetch https://github.com/dscho/git mmap-regexec-v1\n\n-- \n2.10.0.windows.1.2.g732a511\n\nbase-commit: 6ebdac1bab966b720d776aa43ca188fe378b1f4b\n"},{"id":"301101","messageId":"a264f55769cd6cfedb842710791d71b11d830091.1473090278.git.johannes.schindelin@gmx.de","threadId":"44008","inReplyTo":"cover.1473090278.git.johannes.schindelin@gmx.de","subject":"[PATCH 3/3] diff_grep: add assertions verifying that the buffers are NUL-terminated","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-05T15:45:09Z","receivedAt":"2016-09-05T15:45:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Before calling regexec() on the file contents, we better be certain that\nthe strings fulfill the contract of C strings assumed by said function.\n\nSigned-off-by: Johannes Schindelin \n---\n diffcore-pickaxe.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\nindex 55067ca..88820b6 100644\n--- a/diffcore-pickaxe.c\n+++ b/diffcore-pickaxe.c\n@@ -49,6 +49,8 @@ static int diff_grep(mmfile_t *one, mmfile_t *two,\n \txpparam_t xpp;\n \txdemitconf_t xecfg;\n \n+\tassert(!one || one->ptr[one->size] == '\\0');\n+\tassert(!two || two->ptr[two->size] == '\\0');\n \tif (!one)\n \t\treturn !regexec(regexp, two->ptr, 1, ®match, 0);\n \tif (!two)\n-- \n2.10.0.windows.1.2.g732a511\n"},{"id":"301102","messageId":"ca678535c64570add58cfff95709c3c67384139d.1473090278.git.johannes.schindelin@gmx.de","threadId":"44008","inReplyTo":"cover.1473090278.git.johannes.schindelin@gmx.de","subject":"[PATCH 1/3] Demonstrate a problem: our pickaxe code assumes NUL-terminated buffers","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-05T15:45:02Z","receivedAt":"2016-09-05T15:45:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"When our pickaxe code feeds file contents to regexec(), it implicitly\nassumes that the file contents are read into implicitly NUL-terminated\nbuffers (i.e. that we overallocate by 1, appending a single '\\0').\n\nThis is not so.\n\nIn particular when the file contents are simply mmap()ed, we can be\nvirtually certain that the buffer is preceding uninitialized bytes, or\ninvalid pages.\n\nNote that the test we add here is known to be flakey: we simply cannot\nknow whether the byte following the mmap()ed ones is a NUL or not.\n\nTypically, on Linux the test passes. On Windows, it fails virtually\nevery time due to an access violation (that's a segmentation fault for\nyou Unix-y people out there). And Windows would be correct: the\nregexec() call wants to operate on a regular, NUL-terminated string,\nthere is no NUL in the mmap()ed memory range, and it is undefined\nwhether the next byte is even legal to access.\n\nWhen run with --valgrind it demonstrates quite clearly the breakage, of\ncourse.\n\nSo we simply mark it with `test_expect_success` for now.\n\nThis test case represents a Minimal, Complete and Verifiable Example of\na breakage reported by Chris Sidi.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t4059-diff-pickaxe.sh | 22 ++++++++++++++++++++++\n 1 file changed, 22 insertions(+)\n create mode 100755 t/t4059-diff-pickaxe.sh\n\ndiff --git a/t/t4059-diff-pickaxe.sh b/t/t4059-diff-pickaxe.sh\nnew file mode 100755\nindex 0000000..f0bf50b\n--- /dev/null\n+++ b/t/t4059-diff-pickaxe.sh\n@@ -0,0 +1,22 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2016 Johannes Schindelin\n+#\n+\n+test_description='Pickaxe options'\n+\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\ttest_commit initial &&\n+\tprintf \"%04096d\" 0 >4096-zeroes.txt &&\n+\tgit add 4096-zeroes.txt &&\n+\ttest_tick &&\n+\tgit commit -m \"A 4k file\"\n+'\n+test_expect_success '-G matches' '\n+\tgit diff --name-only -G \"^0{4096}$\" HEAD^ >out &&\n+\ttest 4096-zeroes.txt = \"$(cat out)\"\n+'\n+\n+test_done\n-- \n2.10.0.windows.1.2.g732a511\n\n\n"},{"id":"301111","messageId":"xmqqwpiqp3ho.fsf@gitster.mtv.corp.google.com","threadId":"44008","inReplyTo":"cover.1473090278.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 0/3] Fix a segfault caused by regexec() being called on mmap()ed data","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-05T19:10:11Z","receivedAt":"2016-09-05T19:10:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> This patch series addresses a problem where `git diff` is called using\n> `-G` or `-S --pickaxe-regex` on new-born files that are configured\n> without user diff drivers, and that hence get mmap()ed into memory.\n\nGood spotting. This has been with us almost forever; I do not think\nthe original pickaxe had it, but I am sure it is broken after the\n\"--pickaxe-regex\" enhancement.\n\nI am somehow surprised that this is a problem on Windows, though.\nWouldn't we be at least running CRLF conversions, and causing diff\nor grep machinery to work on a NUL-terminated buffer? The convesion\ncode would have to look at mmap'ed memory but I do not think it\nassumes NUL-termination. Perhaps people on Windows do not usually\nuse straight-through and that is why this was discovered after many\nyears, or something? In any case, that is a digression.\n\n> Windows (the bug does not trigger a segmentation fault for me on Linux,\n> strangely enough, but it is really a problem on Windows).\n\nI think it is an issue on all platforms that lets us use mmap().\nWhen the size of a file is multiple of pagesize, the byte past the\nend of the file can very well fall on an unmapped address.\n\n> So at least I have a workaround in place. Ideally, though, we would\n> NUL-terminate the buffers only when needed, or somehow call regexec() on\n> ptr/size parameters instead of passing a supposedly NUL-terminated\n> string to it?\n\nI see two reasonable approaches.\n\n * We may want to revisit the performance numbers to see if using\n mmap() to read from the working tree files still buys us much.\n If not, we should stop borrowing from the working tree using\n mmap(); instead just slurp in and NUL-terminate it.\n\n * We could use variant of regexp engine as you proposed,\n which I think is a preferrable solution. Do people know of a\n widely accepted implementation that we can throw into compat/ as\n fallback that is compatible with GPLv2?\n\n"},{"id":"301163","messageId":"20160906065821.bxwitnk72rpn3xhl@sigill.intra.peff.net","threadId":"44008","inReplyTo":"cover.1473090278.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 0/3] Fix a segfault caused by regexec() being called on mmap()ed data","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-06T06:58:21Z","receivedAt":"2016-09-06T06:58:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 05, 2016 at 05:44:57PM +0200, Johannes Schindelin wrote:\n\n> The problem with that: mmap()ed memory is *not* NUL-terminated, yet the\n> pickaxe code calls regexec() on it just the same.\n> \n> This problem has been reported by my colleague Chris Sidi.\n> \n> Please note that this patch series is a hot fix I applied to Git for\n> Windows (the bug does not trigger a segmentation fault for me on Linux,\n> strangely enough, but it is really a problem on Windows).\n\nThis has come up before, and I think somebody mentioned that on Linux,\nyou are OK unless the buffer ends right at a page boundary (i.e., the\nbuffer size is a multiple of the page size). I don't know if that's true\nor not.\n\n> So at least I have a workaround in place. Ideally, though, we would\n> NUL-terminate the buffers only when needed, or somehow call regexec() on\n> ptr/size parameters instead of passing a supposedly NUL-terminated\n> string to it?\n\nThere's some discussion in:\n\n http://public-inbox.org/git/20121030121747.GA4231@sigill.intra.peff.net/#r\n\nand the thread below it. The quickest way to fix regexec() would be to\nhave everybody use the built-in GNU regex in compat/. People seemed\nsomewhat positive on that direction, but we never followed up.\n\n-Peff\n"},{"id":"301166","messageId":"20160906070604.i5rojh3kyc7x7kso@sigill.intra.peff.net","threadId":"44008","inReplyTo":"f899957fa71537aa2686f17ce18aaf16f2fea2ac.1473090278.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 2/3] diff_populate_filespec: NUL-terminate buffers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-06T07:06:04Z","receivedAt":"2016-09-06T07:06:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 05, 2016 at 05:45:06PM +0200, Johannes Schindelin wrote:\n\n> It is true that many code paths populate the mmfile_t structure silently\n> appending a NUL, e.g. when running textconv on a temporary file and\n> reading the results back into an strbuf.\n> \n> The assumption is most definitely wrong, however, when mmap()ing a file.\n> \n> Practically, we seemed to be lucky that the bytes after mmap()ed memory\n> were 1) accessible and 2) somehow contained NUL bytes *somewhere*.\n> \n> In a use case reported by Chris Sidi, it turned out that the mmap()ed\n> file had the precise size of a memory page, and on Windows the bytes\n> after memory-mapped pages are in general not valid.\n> \n> This patch works around that issue, giving us time to discuss the best\n> course how to fix this problem more generally.\n\nI don't know if we are in that much of a rush. This bug has been around\nfor many years (the thread I linked earlier is from 2012). Yes, it's bad\nand annoying, but we can probably spend a few days discussing the\nsolution.\n\n> diff --git a/diff.c b/diff.c\n> index 534c12e..32f7f46 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -2826,6 +2826,15 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)\n> \t\t\ts->data = strbuf_detach(&buf, &size);\n> \t\t\ts->size = size;\n> \t\t\ts->should_free = 1;\n> +\t\t} else {\n> +\t\t\t/* data must be NUL-terminated so e.g. for regexec() */\n> +\t\t\tchar *data = xmalloc(s->size + 1);\n> +\t\t\tmemcpy(data, s->data, s->size);\n> +\t\t\tdata[s->size] = '\\0';\n> +\t\t\tmunmap(s->data, s->size);\n> +\t\t\ts->should_munmap = 0;\n> +\t\t\ts->data = data;\n> +\t\t\ts->should_free = 1;\n> \t\t}\n\nWithout having done a complete audit recently, my gut and my\nrecollection from previous discussions is that regexec() really is the\nculprit here for the diff code[1]. If we are going to do a workaround\nlike this, I think we could limit it only to cases where know it\nmatters, like --pickaxe-regex.\n\nCan it be triggered with -G? I thought that operated on the diff content\nitself, which would always be in a heap buffer (which should be NUL\nterminated, but if it isn't, that would be a separate fix from this).\n\n-Peff\n\n[1] We do make the assumption elsewhere that git objects are\n NUL-terminated, but that is enforced by the object-reading code\n (with the exception of streamed blobs, but those are obviously dealt\n with separately anyway).\n"},{"id":"301167","messageId":"20160906070836.7brjtijxq7nukjkq@sigill.intra.peff.net","threadId":"44008","inReplyTo":"a264f55769cd6cfedb842710791d71b11d830091.1473090278.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 3/3] diff_grep: add assertions verifying that the buffers are NUL-terminated","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-06T07:08:36Z","receivedAt":"2016-09-06T07:09:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 05, 2016 at 05:45:09PM +0200, Johannes Schindelin wrote:\n\n> Before calling regexec() on the file contents, we better be certain that\n> the strings fulfill the contract of C strings assumed by said function.\n\nIf you have a buffer that is exactly \"size\" bytes and you are worried\nabout regexec reading off the end, then...\n\n> diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\n> index 55067ca..88820b6 100644\n> --- a/diffcore-pickaxe.c\n> +++ b/diffcore-pickaxe.c\n> @@ -49,6 +49,8 @@ static int diff_grep(mmfile_t *one, mmfile_t *two,\n> \txpparam_t xpp;\n> \txdemitconf_t xecfg;\n> \n> +\tassert(!one || one->ptr[one->size] == '\\0');\n> +\tassert(!two || two->ptr[two->size] == '\\0');\n> \tif (!one)\n> \t\treturn !regexec(regexp, two->ptr, 1, ®match, 0);\n\n...don't your asserts also read off the end?\n\nSo you might still segfault, though you do catch a case where we have N\nbytes of junk before the end of the page (and you have a 255/256 chance\nof catching it).\n\n-Peff\n"},{"id":"301170","messageId":"20160906071255.ggsoj2lh2f3kubhj@sigill.intra.peff.net","threadId":"44008","inReplyTo":"xmqqwpiqp3ho.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/3] Fix a segfault caused by regexec() being called on mmap()ed data","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-06T07:12:55Z","receivedAt":"2016-09-06T07:13:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 05, 2016 at 12:10:11PM -0700, Junio C Hamano wrote:\n\n> * We could use variant of regexp engine as you proposed,\n> which I think is a preferrable solution. Do people know of a\n> widely accepted implementation that we can throw into compat/ as\n> fallback that is compatible with GPLv2?\n\nMaybe the one already in compat/regex? ;P\n\nI think re_search() the correct replacement function but it's been a\nwhile since I've looked into it.\n\n-Peff\n"},{"id":"301187","messageId":"alpine.DEB.2.20.1609061509070.129229@virtualbox","threadId":"44008","inReplyTo":"xmqqwpiqp3ho.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/3] Fix a segfault caused by regexec() being called on mmap()ed data","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-06T13:21:36Z","receivedAt":"2016-09-06T13:21:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 5 Sep 2016, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > This patch series addresses a problem where `git diff` is called using\n> > `-G` or `-S --pickaxe-regex` on new-born files that are configured\n> > without user diff drivers, and that hence get mmap()ed into memory.\n> \n> Good spotting. This has been with us almost forever; I do not think\n> the original pickaxe had it, but I am sure it is broken after the\n> \"--pickaxe-regex\" enhancement.\n\nAgreed, regexec() is the call where it segfaults.\n\n> I am somehow surprised that this is a problem on Windows, though.\n> Wouldn't we be at least running CRLF conversions, and causing diff\n> or grep machinery to work on a NUL-terminated buffer?\n\nIt is true that the CR/LF conversion hides this problem. In fact, in the\ncase reported to me, it turned out that the segfault happened only\nrecently, when the repository was switched from LF line endings to CR/LF\nline endings.\n\nThat switch is unfortunately required: it saves *tons* of time because the\nregular CR/LF conversion just takes too much time. It was worse before the\nrepository defined the .gitattributes: the auto-detection contributed to\nthe time spent by Git.\n\nSo yes: the CR/LF conversion hid the bug, but no, we cannot re-introduce\nthe CR/LF conversion into said repository.\n\n> The convesion code would have to look at mmap'ed memory but I do not\n> think it assumes NUL-termination. Perhaps people on Windows do not\n> usually use straight-through and that is why this was discovered after\n> many years, or something? In any case, that is a digression.\n\nIndeed, it is.\n\n> > Windows (the bug does not trigger a segmentation fault for me on\n> > Linux, strangely enough, but it is really a problem on Windows).\n> \n> I think it is an issue on all platforms that lets us use mmap().\n> When the size of a file is multiple of pagesize, the byte past the\n> end of the file can very well fall on an unmapped address.\n\nCorrect.\n\n> > So at least I have a workaround in place. Ideally, though, we would\n> > NUL-terminate the buffers only when needed, or somehow call regexec()\n> > on ptr/size parameters instead of passing a supposedly NUL-terminated\n> > string to it?\n> \n> I see two reasonable approaches.\n> \n> * We may want to revisit the performance numbers to see if using\n> mmap() to read from the working tree files still buys us much.\n> If not, we should stop borrowing from the working tree using\n> mmap(); instead just slurp in and NUL-terminate it.\n\nI would like to warn against putting too much stock into such a test,\nunless it is performed on Linux, MacOSX, Windows and various BSDs. That\nwould make it hard, of course, to come up with a definitive result, but we\nsimply should not make the mistake of over-optimizing for one platform.\nWe used to, of course, and look how much performance it costs e.g. on\nWindows.\n\n> * We could use variant of regexp engine as you proposed,\n> which I think is a preferrable solution. Do people know of a\n> widely accepted implementation that we can throw into compat/ as\n> fallback that is compatible with GPLv2?\n\nAs suggested by Peff, there is a compat/regex/, and I will spout my\nthoughts in a reply to his mail.\n\nCiao,\nDscho\n"},{"id":"301190","messageId":"alpine.DEB.2.20.1609061521410.129229@virtualbox","threadId":"44008","inReplyTo":"20160906071255.ggsoj2lh2f3kubhj@sigill.intra.peff.net","subject":"Re: [PATCH 0/3] Fix a segfault caused by regexec() being called on mmap()ed data","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-06T14:06:32Z","receivedAt":"2016-09-06T14:07:22Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff & Junio,\n\nOn Tue, 6 Sep 2016, Jeff King wrote:\n\n> On Mon, Sep 05, 2016 at 12:10:11PM -0700, Junio C Hamano wrote:\n> \n> > * We could use variant of regexp engine as you proposed,\n> > which I think is a preferrable solution. Do people know of a\n> > widely accepted implementation that we can throw into compat/ as\n> > fallback that is compatible with GPLv2?\n> \n> Maybe the one already in compat/regex? ;P\n\nIndeed. That happens to be the implementation used by Git for Windows,\nanyway.\n\n> I think re_search() the correct replacement function but it's been a\n> while since I've looked into it.\n\nThe segfault I investigated happened in a call to strlen(). I see many\ncalls to strlen() in compat/regex/... The one that triggers the segfault\nis in regexec(), compat/regex/regexec.c:241.\n\nAs to re_search(): I have not been able to reason about its callees in a\nreasonable amount of time. I agree that they *should* not run over the\nbuffer, but I cannot easily verify it.\n\nThe bigger problem is that re_search() is defined in the __USE_GNU section\nof regex.h, and I do not think it is appropriate to universally #define\nsaid constant before #include'ing regex.h. So it would appear that major\nsurgery would be required if we wanted to use regular expressions on\nstrings that are not NUL-terminated.\n\nSo I agree that a better idea may be to simply ensure NUL-terminated\nbuffers when we require them, although that still might be tricky. More on\nthat in a reply to your comment to that end.\n\nCiao,\nDscho\n"},{"id":"301191","messageId":"alpine.DEB.2.20.1609061606411.129229@virtualbox","threadId":"44008","inReplyTo":"20160906065821.bxwitnk72rpn3xhl@sigill.intra.peff.net","subject":"Re: [PATCH 0/3] Fix a segfault caused by regexec() being called on mmap()ed data","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-06T14:13:18Z","receivedAt":"2016-09-06T14:13:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Tue, 6 Sep 2016, Jeff King wrote:\n\n> On Mon, Sep 05, 2016 at 05:44:57PM +0200, Johannes Schindelin wrote:\n> \n> > The problem with that: mmap()ed memory is *not* NUL-terminated, yet\n> > the pickaxe code calls regexec() on it just the same.\n> > \n> > This problem has been reported by my colleague Chris Sidi.\n> > \n> > Please note that this patch series is a hot fix I applied to Git for\n> > Windows (the bug does not trigger a segmentation fault for me on\n> > Linux, strangely enough, but it is really a problem on Windows).\n> \n> This has come up before, and I think somebody mentioned that on Linux,\n> you are OK unless the buffer ends right at a page boundary (i.e., the\n> buffer size is a multiple of the page size). I don't know if that's true\n> or not.\n\nIn my tests on Linux, even when the buffer ended right at the page\nboundary, the memory after that was still legal to access, and typically\nhad a NUL *somewhere*.\n\nThat's happenstance, of course, and could very well result in false\npositives (however unlikely that is).\n\n> > So at least I have a workaround in place. Ideally, though, we would\n> > NUL-terminate the buffers only when needed, or somehow call regexec() on\n> > ptr/size parameters instead of passing a supposedly NUL-terminated\n> > string to it?\n> \n> There's some discussion in:\n> \n> http://public-inbox.org/git/20121030121747.GA4231@sigill.intra.peff.net/#r\n> \n> and the thread below it. The quickest way to fix regexec() would be to\n> have everybody use the built-in GNU regex in compat/. People seemed\n> somewhat positive on that direction, but we never followed up.\n\nI had a brief look, and it is not pretty. You would have to pay me good\nmoney to dive in and try to implement a regexecn() based on what we have\nin compat/regex/. And then people would still complain, I guess, for not\nusing the native regex support, where available.\n\nCiao,\nDscho\n"},{"id":"301197","messageId":"alpine.DEB.2.20.1609061613270.129229@virtualbox","threadId":"44008","inReplyTo":"20160906070604.i5rojh3kyc7x7kso@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] diff_populate_filespec: NUL-terminate buffers","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-06T16:02:59Z","receivedAt":"2016-09-06T16:03:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Tue, 6 Sep 2016, Jeff King wrote:\n\n> On Mon, Sep 05, 2016 at 05:45:06PM +0200, Johannes Schindelin wrote:\n> \n> > It is true that many code paths populate the mmfile_t structure\n> > silently appending a NUL, e.g. when running textconv on a temporary\n> > file and reading the results back into an strbuf.\n> > \n> > The assumption is most definitely wrong, however, when mmap()ing a\n> > file.\n> > \n> > Practically, we seemed to be lucky that the bytes after mmap()ed\n> > memory were 1) accessible and 2) somehow contained NUL bytes\n> > *somewhere*.\n> > \n> > In a use case reported by Chris Sidi, it turned out that the mmap()ed\n> > file had the precise size of a memory page, and on Windows the bytes\n> > after memory-mapped pages are in general not valid.\n> > \n> > This patch works around that issue, giving us time to discuss the best\n> > course how to fix this problem more generally.\n> \n> I don't know if we are in that much of a rush.\n\nI am ;-)\n\n> This bug has been around for many years (the thread I linked earlier is\n> from 2012). Yes, it's bad and annoying, but we can probably spend a few\n> days discussing the solution.\n\nSure we can. But I got to have a solution due to a recent switch from\nstoring LF to storing CR/LF in the repository (that resulted in a\nnoticable performance improvement): combined with -G being an integral\npart of the workflow in the project that reported the issue, it is\nessential that this bug gets fixed. Before I go mostly offline.\n\n> > diff --git a/diff.c b/diff.c\n> > index 534c12e..32f7f46 100644\n> > --- a/diff.c\n> > +++ b/diff.c\n> > @@ -2826,6 +2826,15 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)\n> > \t\t\ts->data = strbuf_detach(&buf, &size);\n> > \t\t\ts->size = size;\n> > \t\t\ts->should_free = 1;\n> > +\t\t} else {\n> > +\t\t\t/* data must be NUL-terminated so e.g. for regexec() */\n> > +\t\t\tchar *data = xmalloc(s->size + 1);\n> > +\t\t\tmemcpy(data, s->data, s->size);\n> > +\t\t\tdata[s->size] = '\\0';\n> > +\t\t\tmunmap(s->data, s->size);\n> > +\t\t\ts->should_munmap = 0;\n> > +\t\t\ts->data = data;\n> > +\t\t\ts->should_free = 1;\n> > \t\t}\n> \n> Without having done a complete audit recently, my gut and my\n> recollection from previous discussions is that regexec() really is the\n> culprit here for the diff code[1]. If we are going to do a workaround\n> like this, I think we could limit it only to cases where know it\n> matters, like --pickaxe-regex.\n\nSure.\n\nWe could introduce a new NEEDS_NUL flag.\n\nIt will still be quite tricky, because we have to touch a function that is\nrather at the bottom of the food chain: diff_populate_filespec() is called\nfrom fill_textconv(), which in turn is called from pickaxe_match(), and\nonly pickaxe_match() knows whether we want to call regexec() or not (it\ndepends on its regexp parameter).\n\nAdding a flag to diff_populate_filespec() sounds really reasonable until\nyou see how many call sites fill_textconv() has.\n\nSee below for a better idea.\n\n> Can it be triggered with -G?\n\nIt can, and it is, as demonstrated by the test I introduced in 1/3.\n\n> I thought that operated on the diff content itself, which would always\n> be in a heap buffer (which should be NUL terminated, but if it isn't,\n> that would be a separate fix from this).\n\nThat is true.\n\nExcept when preimage or postimage does not exist. In which case we call\n\n\tregexec(regexp, two->ptr, 1, ®match, 0);\n\nor the same with one->ptr. Note the notable absence of two->size.\n\n> [1] We do make the assumption elsewhere that git objects are\n> NUL-terminated, but that is enforced by the object-reading code\n> (with the exception of streamed blobs, but those are obviously dealt\n> with separately anyway).\n\nI know. I am the reason you introduced that, because I added code to\nfsck.c that assumes that tag/commit messages are NUL-terminated.\n\nSo now for the better idea.\n\nWhile I was researching the code for this reply, I hit upon one thing that\nI never knew existed, introduced in f96e567 (grep: use REG_STARTEND for\nall matching if available, 2010-05-22). Apparently, NetBSD introduced an\nextension to regexec() where you can specify buffer boundaries using\nREG_STARTEND. Which is pretty much what we need.\n\nSo I have this as my current proof-of-concept (which passes the test\nsuite, but is white-space corrupted, because I really have no time to get\nnon-white-space-corrupted text into this here mailer):\n\n-- snipsnap --\ndiff --git a/diff.c b/diff.c\nindex 534c12e..2c5a360 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -951,7 +951,13 @@ static int find_word_boundaries(mmfile_t *buffer,\nregex_t *word_regex,\n {\n \tif (word_regex && *begin < buffer->size) {\n \t\tregmatch_t match[1];\n-\t\tif (!regexec(word_regex, buffer->ptr + *begin, 1, match,\n\t\t0)) {\n+\t\tint f = 0;\n+#ifdef REG_STARTEND\n+\t\tmatch[0].rm_so = 0;\n+\t\tmatch[0].rm_eo = *end - *begin;\n+\t\tf = REG_STARTEND;\n+#endif\n+\t\tif (!regexec(word_regex, buffer->ptr + *begin, 1, match,\nf)) {\n \t\t\tchar *p = memchr(buffer->ptr + *begin +\nmatch[0].rm_so,\n \t\t\t\t\t'\\n', match[0].rm_eo -\nmatch[0].rm_so);\n \t\t\t*end = p ? p - buffer->ptr : match[0].rm_eo +\n*begin;\n@@ -994,7 +1000,7 @@ static void diff_words_fill(struct diff_words_buffer\n*buffer, mmfile_t *out,\n \tbuffer->orig[0].begin = buffer->orig[0].end = buffer->text.ptr;\n \tbuffer->orig_nr = 1;\n \n-\tfor (i = 0; i < buffer->text.size; i++) {\n+\tfor (i = 0, j = buffer->text.size; i < buffer->text.size; i++) {\n \t\tif (find_word_boundaries(&buffer->text, word_regex, &i,\n&j))\n \t\t\treturn;\n \ndiff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\nindex 55067ca..2cd09e2 100644\n--- a/diffcore-pickaxe.c\n+++ b/diffcore-pickaxe.c\n@@ -23,7 +23,9 @@ static void diffgrep_consume(void *priv, char *line,\nunsigned long len)\n {\n \tstruct diffgrep_cb *data = priv;\n \tregmatch_t regmatch;\n+#ifndef REG_STARTEND\n \tint hold;\n+#endif\n \n \tif (line[0] != '+' && line[0] != '-')\n \t\treturn;\n@@ -33,11 +35,18 @@ static void diffgrep_consume(void *priv, char *line,\nunsigned long len)\n \t\t * caller early.\n \t\t */\n \t\treturn;\n+#ifdef REG_STARTEND\n+\tregmatch.rm_so = 0;\n+\tregmatch.rm_eo = len;\n+\tdata->hit = !regexec(data->regexp, line + 1, 1,\n+\t\t\t ®match, REG_STARTEND);\n+#else\n \t/* Yuck -- line ought to be \"const char *\"! */\n \thold = line[len];\n \tline[len] = '\\0';\n-\tdata->hit = !regexec(data->regexp, line + 1, 1, ®match, 0);\n+\tdata->hit = !regexec(data->regexp, line + 1, 1, ®match, f);\n \tline[len] = hold;\n+#endif\n }\n \n static int diff_grep(mmfile_t *one, mmfile_t *two,\n@@ -49,10 +58,24 @@ static int diff_grep(mmfile_t *one, mmfile_t *two,\n \txpparam_t xpp;\n \txdemitconf_t xecfg;\n \n-\tif (!one)\n-\t\treturn !regexec(regexp, two->ptr, 1, ®match, 0);\n-\tif (!two)\n-\t\treturn !regexec(regexp, one->ptr, 1, ®match, 0);\n+\tif (!one) {\n+\t\tint flags = 0;\n+#ifdef REG_STARTEND\n+\t\tregmatch.rm_so = 0;\n+\t\tregmatch.rm_eo = two->size;\n+\t\tflags = REG_STARTEND;\n+#endif\n+\t\treturn !regexec(regexp, two->ptr, 1, ®match, flags);\n+\t}\n+\tif (!two) {\n+\t\tint flags = 0;\n+#ifdef REG_STARTEND\n+\t\tregmatch.rm_so = 0;\n+\t\tregmatch.rm_eo = one->size;\n+\t\tflags = REG_STARTEND;\n+#endif\n+\t\treturn !regexec(regexp, one->ptr, 1, ®match, flags);\n+\t}\n \n \t/*\n \t * We have both sides; need to run textual diff and see if\n@@ -83,7 +106,13 @@ static unsigned int contains(mmfile_t *mf, regex_t\n*regexp, kwset_t kws)\n \t\tregmatch_t regmatch;\n \t\tint flags = 0;\n \n+#ifndef REG_STARTEND\n \t\tassert(data[sz] == '\\0');\n+#else\n+\t\tregmatch.rm_so = 0;\n+\t\tregmatch.rm_eo = sz;\n+\t\tflags |= REG_STARTEND;\n+#endif\n \t\twhile (*data && !regexec(regexp, data, 1, ®match,\nflags)) {\n \t\t\tflags |= REG_NOTBOL;\n \t\t\tdata += regmatch.rm_eo;\ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex f34ea76..c179d43 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -218,7 +218,7 @@ static long ff_regexp(const char *line, long len,\n \tstruct ff_regs *regs = priv;\n \tregmatch_t pmatch[2];\n \tint i;\n-\tint result = -1;\n+\tint result = -1, flags = 0;\n \n \t/* Exclude terminating newline (and cr) from matching */\n \tif (len > 0 && line[len-1] == '\\n') {\n@@ -228,11 +228,20 @@ static long ff_regexp(const char *line, long len,\n \t\t\tlen--;\n \t}\n \n+#ifndef REG_STARTEND\n \tline_buffer = xstrndup(line, len); /* make NUL terminated */\n+#else\n+\tline_buffer = (char *)line;\n+\tflags = REG_STARTEND;\n+#endif\n \n \tfor (i = 0; i < regs->nr; i++) {\n \t\tstruct ff_reg *reg = regs->array + i;\n-\t\tif (!regexec(®->re, line_buffer, 2, pmatch, 0)) {\n+#ifdef REG_STARTEND\n+\t\tpmatch->rm_so = 0;\n+\t\tpmatch->rm_eo = len;\n+#endif\n+\t\tif (!regexec(®->re, line_buffer, 2, pmatch, flags)) {\n \t\t\tif (reg->negate)\n \t\t\t\tgoto fail;\n \t\t\tbreak;\n@@ -249,7 +258,9 @@ static long ff_regexp(const char *line, long len,\n \t\tresult--;\n \tmemcpy(buffer, line, result);\n fail:\n+#ifndef REG_STARTEND\n \tfree(line_buffer);\n+#endif\n \treturn result;\n }\n"},{"id":"301200","messageId":"alpine.DEB.2.20.1609061803120.129229@virtualbox","threadId":"44008","inReplyTo":"20160906070836.7brjtijxq7nukjkq@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] diff_grep: add assertions verifying that the buffers are NUL-terminated","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-06T16:04:20Z","receivedAt":"2016-09-06T16:04:55Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Tue, 6 Sep 2016, Jeff King wrote:\n\n> On Mon, Sep 05, 2016 at 05:45:09PM +0200, Johannes Schindelin wrote:\n> \n> > Before calling regexec() on the file contents, we better be certain that\n> > the strings fulfill the contract of C strings assumed by said function.\n> \n> If you have a buffer that is exactly \"size\" bytes and you are worried\n> about regexec reading off the end, then...\n> \n> > diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\n> > index 55067ca..88820b6 100644\n> > --- a/diffcore-pickaxe.c\n> > +++ b/diffcore-pickaxe.c\n> > @@ -49,6 +49,8 @@ static int diff_grep(mmfile_t *one, mmfile_t *two,\n> > \txpparam_t xpp;\n> > \txdemitconf_t xecfg;\n> > \n> > +\tassert(!one || one->ptr[one->size] == '\\0');\n> > +\tassert(!two || two->ptr[two->size] == '\\0');\n> > \tif (!one)\n> > \t\treturn !regexec(regexp, two->ptr, 1, ®match, 0);\n> \n> ...don't your asserts also read off the end?\n\nYes, they would read off the end, *unless* a NUL was somehow appended to\nthe buffers.\n\n> So you might still segfault, though you do catch a case where we have N\n> bytes of junk before the end of the page (and you have a 255/256 chance\n> of catching it).\n\nRight. The assertion may fail, or a segfault happen. In both cases,\nassumptions are violated and we need to fix the code.\n\nCiao,\nDscho\n"},{"id":"301219","messageId":"20160906182942.s2mlge2vg65f5sy4@sigill.intra.peff.net","threadId":"44008","inReplyTo":"alpine.DEB.2.20.1609061521410.129229@virtualbox","subject":"Re: [PATCH 0/3] Fix a segfault caused by regexec() being called on mmap()ed data","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-06T18:29:43Z","receivedAt":"2016-09-06T18:30:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 06, 2016 at 04:06:32PM +0200, Johannes Schindelin wrote:\n\n> > I think re_search() the correct replacement function but it's been a\n> > while since I've looked into it.\n> \n> The segfault I investigated happened in a call to strlen(). I see many\n> calls to strlen() in compat/regex/... The one that triggers the segfault\n> is in regexec(), compat/regex/regexec.c:241.\n\nYes, that is the important one, I think. The others are for patterns,\nerror msgs, etc. Of course strlen() is not the only function that cares\nabout NUL delimiters (and there might even be a \"while (*p)\" somewhere\nin the code).\n\nI always assumed the _point_ of re_search taking a ptr/len pair was\nexactly to handle this case. The documentation[1] says:\n\n `string` is the string you want to match; it can contain newline and\n null characters. `size` is the length of that string.\n\nWhich seems pretty definitive to me (that's for re_match(), but\nre_search() is defined in the docs in terms of re_match()).\n\n[1] http://www.delorie.com/gnu/docs/regex/regex_47.html\n\n> As to re_search(): I have not been able to reason about its callees in a\n> reasonable amount of time. I agree that they *should* not run over the\n> buffer, but I cannot easily verify it.\n\nBetween the documentation above, and the fact that your new test passes\nwhen we switch to it (see below), I feel pretty good about it.\n\n> The bigger problem is that re_search() is defined in the __USE_GNU section\n> of regex.h, and I do not think it is appropriate to universally #define\n> said constant before #include'ing regex.h. So it would appear that major\n> surgery would be required if we wanted to use regular expressions on\n> strings that are not NUL-terminated.\n\nWe can contain this to the existing compat/regexec/regexec.c, and just\nprovide a wrapper that is similar to regexec but takes a ptr/len pair.\n\nLike:\n\ndiff --git a/compat/regex/regex.h b/compat/regex/regex.h\nindex 61c9683..b2dd0b7 100644\n--- a/compat/regex/regex.h\n+++ b/compat/regex/regex.h\n@@ -569,6 +569,11 @@ extern int regexec (const regex_t *__restrict __preg,\n \t\t regmatch_t __pmatch[__restrict_arr],\n \t\t int __eflags);\n \n+extern int regexecn (const regex_t *__restrict __preg,\n+\t\t const char *__restrict __cstring, size_t __length,\n+\t\t size_t __nmatch, regmatch_t __pmatch[__restrict_arr],\n+\t\t int __eflags);\n+\n extern size_t regerror (int __errcode, const regex_t *__restrict __preg,\n \t\t\tchar *__restrict __errbuf, size_t __errbuf_size);\n \ndiff --git a/compat/regex/regexec.c b/compat/regex/regexec.c\nindex eb5e1d4..8afe26b 100644\n--- a/compat/regex/regexec.c\n+++ b/compat/regex/regexec.c\n@@ -217,15 +217,16 @@ static reg_errcode_t extend_buffers (re_match_context_t *mctx)\n We return 0 if we find a match and REG_NOMATCH if not. */\n \n int\n-regexec (\n+regexecn (\n \tconst regex_t *__restrict preg,\n \tconst char *__restrict string,\n+\tsize_t length,\n \tsize_t nmatch,\n \tregmatch_t pmatch[],\n \tint eflags)\n {\n reg_errcode_t err;\n- int start, length;\n+ int start;\n \n if (eflags & ~(REG_NOTBOL | REG_NOTEOL | REG_STARTEND))\n return REG_BADPAT;\n@@ -238,7 +239,7 @@ regexec (\n else\n {\n start = 0;\n- length = strlen (string);\n+ /* length already passed in */\n }\n \n __libc_lock_lock (dfa->lock);\n@@ -252,6 +253,17 @@ regexec (\n return err != REG_NOERROR;\n }\n \n+int\n+regexec (\n+\tconst regex_t *__restrict preg,\n+\tconst char *__restrict string,\n+\tsize_t nmatch,\n+\tregmatch_t pmatch[],\n+\tint eflags)\n+{\n+ return regexecn(preg, string, strlen(string), nmatch, pmatch, eflags);\n+}\n+\n #ifdef _LIBC\n # include \n versioned_symbol (libc, __regexec, regexec, GLIBC_2_3_4);\ndiff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\nindex 55067ca..fdd08dd 100644\n--- a/diffcore-pickaxe.c\n+++ b/diffcore-pickaxe.c\n@@ -50,9 +50,9 @@ static int diff_grep(mmfile_t *one, mmfile_t *two,\n \txdemitconf_t xecfg;\n \n \tif (!one)\n-\t\treturn !regexec(regexp, two->ptr, 1, ®match, 0);\n+\t\treturn !regexecn(regexp, two->ptr, two->size, 1, ®match, 0);\n \tif (!two)\n-\t\treturn !regexec(regexp, one->ptr, 1, ®match, 0);\n+\t\treturn !regexecn(regexp, one->ptr, one->size, 1, ®match, 0);\n \n \t/*\n \t * We have both sides; need to run textual diff and see if\n"},{"id":"301221","messageId":"20160906184143.55a5zoa2mj6c2e5m@sigill.intra.peff.net","threadId":"44008","inReplyTo":"alpine.DEB.2.20.1609061613270.129229@virtualbox","subject":"Re: [PATCH 2/3] diff_populate_filespec: NUL-terminate buffers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-06T18:41:43Z","receivedAt":"2016-09-06T18:41:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 06, 2016 at 06:02:59PM +0200, Johannes Schindelin wrote:\n\n> It will still be quite tricky, because we have to touch a function that is\n> rather at the bottom of the food chain: diff_populate_filespec() is called\n> from fill_textconv(), which in turn is called from pickaxe_match(), and\n> only pickaxe_match() knows whether we want to call regexec() or not (it\n> depends on its regexp parameter).\n> \n> Adding a flag to diff_populate_filespec() sounds really reasonable until\n> you see how many call sites fill_textconv() has.\n\nI was thinking of something quite gross, like a global \"switch to using\nslower-but-safer NUL termination\" flag (but I agree with Junio's point\nelsewhere that we do not even know if it is \"slower\").\n\n> > I thought that operated on the diff content itself, which would always\n> > be in a heap buffer (which should be NUL terminated, but if it isn't,\n> > that would be a separate fix from this).\n> \n> That is true.\n> \n> Except when preimage or postimage does not exist. In which case we call\n> \n> \tregexec(regexp, two->ptr, 1, ®match, 0);\n> \n> or the same with one->ptr. Note the notable absence of two->size.\n\nThanks, I forgot about that case.\n\n> > [1] We do make the assumption elsewhere that git objects are\n> > NUL-terminated, but that is enforced by the object-reading code\n> > (with the exception of streamed blobs, but those are obviously dealt\n> > with separately anyway).\n> \n> I know. I am the reason you introduced that, because I added code to\n> fsck.c that assumes that tag/commit messages are NUL-terminated.\n\nSort of. I think it has been part of the design since e871b64\n(unpack_sha1_file: zero-pad the unpacked object., 2005-05-25), though I\ndo recall that we missed a code path that did its allocation differently\n(in index-pack, IIRC).\n\nAnyway, that is neither here nor there for the diff code, which as you\nnoticed may operate on things besides git objects.\n\n> So now for the better idea.\n> \n> While I was researching the code for this reply, I hit upon one thing that\n> I never knew existed, introduced in f96e567 (grep: use REG_STARTEND for\n> all matching if available, 2010-05-22). Apparently, NetBSD introduced an\n> extension to regexec() where you can specify buffer boundaries using\n> REG_STARTEND. Which is pretty much what we need.\n\nYes, and compat/regex support this, too. My question is whether it is\nportable. I see:\n\n> diff --git a/diff.c b/diff.c\n> index 534c12e..2c5a360 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -951,7 +951,13 @@ static int find_word_boundaries(mmfile_t *buffer,\n> regex_t *word_regex,\n> {\n> \tif (word_regex && *begin < buffer->size) {\n> \t\tregmatch_t match[1];\n> -\t\tif (!regexec(word_regex, buffer->ptr + *begin, 1, match,\n> \t\t0)) {\n> +\t\tint f = 0;\n> +#ifdef REG_STARTEND\n> +\t\tmatch[0].rm_so = 0;\n> +\t\tmatch[0].rm_eo = *end - *begin;\n> +\t\tf = REG_STARTEND;\n> +#endif\n> +\t\tif (!regexec(word_regex, buffer->ptr + *begin, 1, match,\n> f)) {\n\nWhat happens to those poor souls on systems without REG_STARTEND? Do\nthey get to keep segfaulting?\n\nI think the solution is to push them into setting NO_REGEX. So looking\nat this versus a \"regexecn\", it seems:\n\n - this lets people keep using their native regexec if it supports\n STARTEND\n\n - this is a bit more clunky to use at the callsites (though we could\n _create_ a portable regexecn wrapper that uses this technique on top\n of the native regex library)\n\nBut I much prefer this approach to copying the data just to add a NUL.\n\n-Peff\n"},{"id":"301222","messageId":"20160906184320.lzg5jizpw2kbzf72@sigill.intra.peff.net","threadId":"44008","inReplyTo":"ca678535c64570add58cfff95709c3c67384139d.1473090278.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 1/3] Demonstrate a problem: our pickaxe code assumes NUL-terminated buffers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-06T18:43:20Z","receivedAt":"2016-09-06T18:43:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 05, 2016 at 05:45:02PM +0200, Johannes Schindelin wrote:\n\n> Typically, on Linux the test passes. On Windows, it fails virtually\n> every time due to an access violation (that's a segmentation fault for\n> you Unix-y people out there). And Windows would be correct: the\n> regexec() call wants to operate on a regular, NUL-terminated string,\n> there is no NUL in the mmap()ed memory range, and it is undefined\n> whether the next byte is even legal to access.\n> \n> When run with --valgrind it demonstrates quite clearly the breakage, of\n> course.\n> \n> So we simply mark it with `test_expect_success` for now.\n\nI'd prefer if this were marked as expect_failure. It fails reliably for\nme on Linux, even without --valgrind. But even if that were not so,\nthere is no reason to hurt bisectability of somebody running with\n\"--valgrind\" (not when it costs so little to mark it correctly).\n\n-Peff\n"},{"id":"301308","messageId":"xmqqinu7muic.fsf@gitster.mtv.corp.google.com","threadId":"44008","inReplyTo":"20160906184143.55a5zoa2mj6c2e5m@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] diff_populate_filespec: NUL-terminate buffers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-07T18:31:39Z","receivedAt":"2016-09-07T18:31:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> What happens to those poor souls on systems without REG_STARTEND? Do\n> they get to keep segfaulting?\n>\n> I think the solution is to push them into setting NO_REGEX. So looking\n> at this versus a \"regexecn\", it seems:\n>\n> - this lets people keep using their native regexec if it supports\n> STARTEND\n>\n> - this is a bit more clunky to use at the callsites (though we could\n> _create_ a portable regexecn wrapper that uses this technique on top\n> of the native regex library)\n>\n> But I much prefer this approach to copying the data just to add a NUL.\n\nI first thought \"push them to NO_REGEX\" to mean \"they live with\ncrippled Git that does not do regexp\" and went \"Huh?\", but it merely\nmeans \"let's avoid platform regex library and use on from the\ncompat/ hierarchy\", which would solve the STARTEND portability issue\nfor everybody.\n\nWhich is very good.\n\nThe idea to create a thin regexecn() wrapper also sounds like a good\nidea, too. The changes to the callsites in the demonstration patch\ndoes look a bit clunky to me, too.\n\n"},{"id":"301374","messageId":"alpine.DEB.2.20.1609080921030.129229@virtualbox","threadId":"44008","inReplyTo":"20160906182942.s2mlge2vg65f5sy4@sigill.intra.peff.net","subject":"Re: [PATCH 0/3] Fix a segfault caused by regexec() being called on mmap()ed data","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-08T07:29:58Z","receivedAt":"2016-09-08T07:31:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nsorry for the late answer, I was really busy trying to come up with a new\nand improved version of the patch series, and while hunting a bug I\nintroduced got bogged down with other tasks.\n\nThe good news is that I made up my mind about releasing a Git for Windows\nv2.10.0(2): originally, I had planned to do that today, to have time for\nany hot fixes until Sunday, if necessary, before going semi-dark.\n\nFWIW I am now trying to track my plans for v2.10.0(2) (or v2.10.1, if\nupstream Git v2.10.1 is released before) on GitHub:\n\n\thttps://github.com/git-for-windows/git/milestone/3\n\nOn Tue, 6 Sep 2016, Jeff King wrote:\n\n> On Tue, Sep 06, 2016 at 04:06:32PM +0200, Johannes Schindelin wrote:\n> \n> > > I think re_search() the correct replacement function but it's been a\n> > > while since I've looked into it.\n> > \n> > The segfault I investigated happened in a call to strlen(). I see many\n> > calls to strlen() in compat/regex/... The one that triggers the segfault\n> > is in regexec(), compat/regex/regexec.c:241.\n> \n> Yes, that is the important one, I think. The others are for patterns,\n> error msgs, etc. Of course strlen() is not the only function that cares\n> about NUL delimiters (and there might even be a \"while (*p)\" somewhere\n> in the code).\n> \n> I always assumed the _point_ of re_search taking a ptr/len pair was\n> exactly to handle this case. The documentation[1] says:\n> \n> `string` is the string you want to match; it can contain newline and\n> null characters. `size` is the length of that string.\n> \n> Which seems pretty definitive to me (that's for re_match(), but\n> re_search() is defined in the docs in terms of re_match()).\n\nRight. The problem is: I *really* want to avoid using GNU-isms.\n\n> > The bigger problem is that re_search() is defined in the __USE_GNU section\n> > of regex.h, and I do not think it is appropriate to universally #define\n> > said constant before #include'ing regex.h. So it would appear that major\n> > surgery would be required if we wanted to use regular expressions on\n> > strings that are not NUL-terminated.\n> \n> We can contain this to the existing compat/regexec/regexec.c, and just\n> provide a wrapper that is similar to regexec but takes a ptr/len pair.\n\nBut we can do even better than that: we can provide a wrapper that uses\nREG_STARTEND where available (which is really the majority of platforms we\ncare about: Linux, MacOSX, Windows, and even the *BSDs). Where it is not\navailable, we simply malloc(), memcpy() and append a NUL.\n\nWhich is what my v2 does (will send it out in a moment).\n\nCiao,\nDscho\n"},{"id":"301375","messageId":"cover.1473319844.git.johannes.schindelin@gmx.de","threadId":"44008","inReplyTo":"cover.1473090278.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 0/3] Fix a segfault caused by regexec() being called on mmap()ed data","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-08T07:31:02Z","receivedAt":"2016-09-08T07:31:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This patch series addresses a problem where `git diff` is called using\n`-G` or `-S --pickaxe-regex` on new-born files that are configured\nwithout user diff drivers, and that hence get mmap()ed into memory.\n\nThe problem with that: mmap()ed memory is *not* NUL-terminated, yet the\npickaxe code calls regexec() on it just the same.\n\nThis problem has been reported by my colleague Chris Sidi.\n\nWe solve this by introducing a helper, regexec_buf(), that takes a\npointer and a length instead of a NUL-terminated string.\n\nThis helper then uses REG_STARTEND where available, and falls back to\nallocating and constructing a NUL-terminated string. Given the\nwide-spread support for REG_STARTEND (Linux has it, MacOSX has it, Git\nfor Windows has it because it uses compat/regex/ that has it), I think\nthis is a fair trade-off.\n\nChanges since v1:\n\n- completely revamped the approach: now we no longer force-append a NUL\n whenever mmap()ing buffers for use by the diff machinery, but we fix\n things at the regexec() level.\n\n\nJohannes Schindelin (3):\n Demonstrate a problem: our pickaxe code assumes NUL-terminated buffers\n Introduce a function to run regexec() on non-NUL-terminated buffers\n Use the newly-introduced regexec_buf() function\n\n diff.c | 3 ++-\n diffcore-pickaxe.c | 18 ++++++++----------\n git-compat-util.h | 21 +++++++++++++++++++++\n t/t4061-diff-pickaxe.sh | 22 ++++++++++++++++++++++\n xdiff-interface.c | 13 ++++---------\n 5 files changed, 57 insertions(+), 20 deletions(-)\n create mode 100755 t/t4061-diff-pickaxe.sh\n\nPublished-As: https://github.com/dscho/git/releases/tag/mmap-regexec-v2\nFetch-It-Via: git fetch https://github.com/dscho/git mmap-regexec-v2\n\nInterdiff vs v1:\n\n diff --git a/diff.c b/diff.c\n index 32f7f46..526775a 100644\n --- a/diff.c\n +++ b/diff.c\n @@ -951,7 +951,8 @@ static int find_word_boundaries(mmfile_t *buffer, regex_t *word_regex,\n {\n \tif (word_regex && *begin < buffer->size) {\n \t\tregmatch_t match[1];\n -\t\tif (!regexec(word_regex, buffer->ptr + *begin, 1, match, 0)) {\n +\t\tif (!regexec_buf(word_regex, buffer->ptr + *begin,\n +\t\t\t\t buffer->size - *begin, 1, match, 0)) {\n \t\t\tchar *p = memchr(buffer->ptr + *begin + match[0].rm_so,\n \t\t\t\t\t'\\n', match[0].rm_eo - match[0].rm_so);\n \t\t\t*end = p ? p - buffer->ptr : match[0].rm_eo + *begin;\n @@ -2826,15 +2827,6 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)\n \t\t\ts->data = strbuf_detach(&buf, &size);\n \t\t\ts->size = size;\n \t\t\ts->should_free = 1;\n -\t\t} else {\n -\t\t\t/* data must be NUL-terminated so e.g. for regexec() */\n -\t\t\tchar *data = xmalloc(s->size + 1);\n -\t\t\tmemcpy(data, s->data, s->size);\n -\t\t\tdata[s->size] = '\\0';\n -\t\t\tmunmap(s->data, s->size);\n -\t\t\ts->should_munmap = 0;\n -\t\t\ts->data = data;\n -\t\t\ts->should_free = 1;\n \t\t}\n \t}\n \telse {\n diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\n index 88820b6..9795ca1 100644\n --- a/diffcore-pickaxe.c\n +++ b/diffcore-pickaxe.c\n @@ -23,7 +23,6 @@ static void diffgrep_consume(void *priv, char *line, unsigned long len)\n {\n \tstruct diffgrep_cb *data = priv;\n \tregmatch_t regmatch;\n -\tint hold;\n \n \tif (line[0] != '+' && line[0] != '-')\n \t\treturn;\n @@ -33,11 +32,8 @@ static void diffgrep_consume(void *priv, char *line, unsigned long len)\n \t\t * caller early.\n \t\t */\n \t\treturn;\n -\t/* Yuck -- line ought to be \"const char *\"! */\n -\thold = line[len];\n -\tline[len] = '\\0';\n -\tdata->hit = !regexec(data->regexp, line + 1, 1, ®match, 0);\n -\tline[len] = hold;\n +\tdata->hit = !regexec_buf(data->regexp, line + 1, len - 1, 1,\n +\t\t\t\t ®match, 0);\n }\n \n static int diff_grep(mmfile_t *one, mmfile_t *two,\n @@ -49,12 +45,12 @@ static int diff_grep(mmfile_t *one, mmfile_t *two,\n \txpparam_t xpp;\n \txdemitconf_t xecfg;\n \n -\tassert(!one || one->ptr[one->size] == '\\0');\n -\tassert(!two || two->ptr[two->size] == '\\0');\n \tif (!one)\n -\t\treturn !regexec(regexp, two->ptr, 1, ®match, 0);\n +\t\treturn !regexec_buf(regexp, two->ptr, two->size,\n +\t\t\t\t 1, ®match, 0);\n \tif (!two)\n -\t\treturn !regexec(regexp, one->ptr, 1, ®match, 0);\n +\t\treturn !regexec_buf(regexp, one->ptr, one->size,\n +\t\t\t\t 1, ®match, 0);\n \n \t/*\n \t * We have both sides; need to run textual diff and see if\n @@ -85,8 +81,8 @@ static unsigned int contains(mmfile_t *mf, regex_t *regexp, kwset_t kws)\n \t\tregmatch_t regmatch;\n \t\tint flags = 0;\n \n -\t\tassert(data[sz] == '\\0');\n -\t\twhile (*data && !regexec(regexp, data, 1, ®match, flags)) {\n +\t\twhile (*data &&\n +\t\t !regexec_buf(regexp, data, sz, 1, ®match, flags)) {\n \t\t\tflags |= REG_NOTBOL;\n \t\t\tdata += regmatch.rm_eo;\n \t\t\tif (*data && regmatch.rm_so == regmatch.rm_eo)\n diff --git a/git-compat-util.h b/git-compat-util.h\n index db89ba7..19128b3 100644\n --- a/git-compat-util.h\n +++ b/git-compat-util.h\n @@ -965,6 +965,27 @@ void git_qsort(void *base, size_t nmemb, size_t size,\n #define qsort git_qsort\n #endif\n \n +static inline int regexec_buf(const regex_t *preg, const char *buf, size_t size,\n +\t\t\t size_t nmatch, regmatch_t pmatch[], int eflags)\n +{\n +#ifdef REG_STARTEND\n +\tassert(nmatch > 0 && pmatch);\n +\tpmatch[0].rm_so = 0;\n +\tpmatch[0].rm_eo = size;\n +\treturn regexec(preg, buf, nmatch, pmatch, eflags | REG_STARTEND);\n +#else\n +\tchar *buf2 = xmalloc(size + 1);\n +\tint ret;\n +\n +\tmemcpy(buf2, buf, size);\n +\tbuf2[size] = '\\0';\n +\tret = regexec(preg, buf2, nmatch, pmatch, eflags);\n +\tfree(buf2);\n +\n +\treturn ret;\n +#endif\n +}\n +\n #ifndef DIR_HAS_BSD_GROUP_SEMANTICS\n # define FORCE_DIR_SET_GID S_ISGID\n #else\n diff --git a/t/t4059-diff-pickaxe.sh b/t/t4061-diff-pickaxe.sh\n similarity index 91%\n rename from t/t4059-diff-pickaxe.sh\n rename to t/t4061-diff-pickaxe.sh\n index f0bf50b..5929f2e 100755\n --- a/t/t4059-diff-pickaxe.sh\n +++ b/t/t4061-diff-pickaxe.sh\n @@ -14,7 +14,7 @@ test_expect_success setup '\n \ttest_tick &&\n \tgit commit -m \"A 4k file\"\n '\n -test_expect_success '-G matches' '\n +test_expect_failure '-G matches' '\n \tgit diff --name-only -G \"^0{4096}$\" HEAD^ >out &&\n \ttest 4096-zeroes.txt = \"$(cat out)\"\n '\n diff --git a/xdiff-interface.c b/xdiff-interface.c\n index f34ea76..50702a2 100644\n --- a/xdiff-interface.c\n +++ b/xdiff-interface.c\n @@ -214,11 +214,10 @@ struct ff_regs {\n static long ff_regexp(const char *line, long len,\n \t\tchar *buffer, long buffer_size, void *priv)\n {\n -\tchar *line_buffer;\n \tstruct ff_regs *regs = priv;\n \tregmatch_t pmatch[2];\n \tint i;\n -\tint result = -1;\n +\tint result;\n \n \t/* Exclude terminating newline (and cr) from matching */\n \tif (len > 0 && line[len-1] == '\\n') {\n @@ -228,18 +227,16 @@ static long ff_regexp(const char *line, long len,\n \t\t\tlen--;\n \t}\n \n -\tline_buffer = xstrndup(line, len); /* make NUL terminated */\n -\n \tfor (i = 0; i < regs->nr; i++) {\n \t\tstruct ff_reg *reg = regs->array + i;\n -\t\tif (!regexec(®->re, line_buffer, 2, pmatch, 0)) {\n +\t\tif (!regexec_buf(®->re, line, len, 2, pmatch, 0)) {\n \t\t\tif (reg->negate)\n -\t\t\t\tgoto fail;\n +\t\t\t\treturn -1;\n \t\t\tbreak;\n \t\t}\n \t}\n \tif (regs->nr <= i)\n -\t\tgoto fail;\n +\t\treturn -1;\n \ti = pmatch[1].rm_so >= 0 ? 1 : 0;\n \tline += pmatch[i].rm_so;\n \tresult = pmatch[i].rm_eo - pmatch[i].rm_so;\n @@ -248,8 +245,6 @@ static long ff_regexp(const char *line, long len,\n \twhile (result > 0 && (isspace(line[result - 1])))\n \t\tresult--;\n \tmemcpy(buffer, line, result);\n - fail:\n -\tfree(line_buffer);\n \treturn result;\n }\n \n\n-- \n2.10.0.windows.1.10.g803177d\n\nbase-commit: 6ebdac1bab966b720d776aa43ca188fe378b1f4b\n"},{"id":"301376","messageId":"53cde4f47b078378feb202dfc44c690373cfcdd2.1473319844.git.johannes.schindelin@gmx.de","threadId":"44008","inReplyTo":"cover.1473319844.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 1/3] Demonstrate a problem: our pickaxe code assumes NUL-terminated buffers","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-08T07:31:15Z","receivedAt":"2016-09-08T07:31:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"When our pickaxe code feeds file contents to regexec(), it implicitly\nassumes that the file contents are read into implicitly NUL-terminated\nbuffers (i.e. that we overallocate by 1, appending a single '\\0').\n\nThis is not so.\n\nIn particular when the file contents are simply mmap()ed, we can be\nvirtually certain that the buffer is preceding uninitialized bytes, or\ninvalid pages.\n\nNote that the test we add here is known to be flakey: we simply cannot\nknow whether the byte following the mmap()ed ones is a NUL or not.\n\nTypically, on Linux the test passes. On Windows, it fails virtually\nevery time due to an access violation (that's a segmentation fault for\nyou Unix-y people out there). And Windows would be correct: the\nregexec() call wants to operate on a regular, NUL-terminated string,\nthere is no NUL in the mmap()ed memory range, and it is undefined\nwhether the next byte is even legal to access.\n\nWhen run with --valgrind it demonstrates quite clearly the breakage, of\ncourse.\n\nBeing marked with `test_expect_failure`, this test will sometimes be\ndeclare \"TODO fixed\", even if it only passes by mistake.\n\nThis test case represents a Minimal, Complete and Verifiable Example of\na breakage reported by Chris Sidi.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t4061-diff-pickaxe.sh | 22 ++++++++++++++++++++++\n 1 file changed, 22 insertions(+)\n create mode 100755 t/t4061-diff-pickaxe.sh\n\ndiff --git a/t/t4061-diff-pickaxe.sh b/t/t4061-diff-pickaxe.sh\nnew file mode 100755\nindex 0000000..5929f2e\n--- /dev/null\n+++ b/t/t4061-diff-pickaxe.sh\n@@ -0,0 +1,22 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2016 Johannes Schindelin\n+#\n+\n+test_description='Pickaxe options'\n+\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\ttest_commit initial &&\n+\tprintf \"%04096d\" 0 >4096-zeroes.txt &&\n+\tgit add 4096-zeroes.txt &&\n+\ttest_tick &&\n+\tgit commit -m \"A 4k file\"\n+'\n+test_expect_failure '-G matches' '\n+\tgit diff --name-only -G \"^0{4096}$\" HEAD^ >out &&\n+\ttest 4096-zeroes.txt = \"$(cat out)\"\n+'\n+\n+test_done\n-- \n2.10.0.windows.1.10.g803177d\n\n\n"},{"id":"301377","messageId":"94ee698b2736929d37640012a1b1735b134dd3d6.1473319844.git.johannes.schindelin@gmx.de","threadId":"44008","inReplyTo":"cover.1473319844.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 2/3] Introduce a function to run regexec() on non-NUL-terminated buffers","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-08T07:31:11Z","receivedAt":"2016-09-08T07:31:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"We just introduced a test that demonstrates that our sloppy use of\nregexec() on a mmap()ed area can result in incorrect results or even\nhard crashes.\n\nSo what we need to fix this is a function that calls regexec() on a\nlength-delimited, rather than a NUL-terminated, string.\n\nHappily, there is an extension to regexec() introduced by the NetBSD\nproject and present in all major regex implementation including\nLinux', MacOSX' and the one Git includes in compat/regex/: by using\nthe (non-POSIX) REG_STARTEND flag, it is possible to tell the\nregexec() function that it should only look at the offsets between\npmatch[0].rm_so and pmatch[0].rm_eo.\n\nThat is exactly what we need.\n\nSince support for REG_STARTEND is so widespread by now, let's just\nintroduce a helper function that uses it, and fall back to allocating\nand constructing a NUL-terminated when REG_STARTEND is not available.\n\nSigned-off-by: Johannes Schindelin \n---\n git-compat-util.h | 21 +++++++++++++++++++++\n 1 file changed, 21 insertions(+)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex db89ba7..19128b3 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -965,6 +965,27 @@ void git_qsort(void *base, size_t nmemb, size_t size,\n #define qsort git_qsort\n #endif\n \n+static inline int regexec_buf(const regex_t *preg, const char *buf, size_t size,\n+\t\t\t size_t nmatch, regmatch_t pmatch[], int eflags)\n+{\n+#ifdef REG_STARTEND\n+\tassert(nmatch > 0 && pmatch);\n+\tpmatch[0].rm_so = 0;\n+\tpmatch[0].rm_eo = size;\n+\treturn regexec(preg, buf, nmatch, pmatch, eflags | REG_STARTEND);\n+#else\n+\tchar *buf2 = xmalloc(size + 1);\n+\tint ret;\n+\n+\tmemcpy(buf2, buf, size);\n+\tbuf2[size] = '\\0';\n+\tret = regexec(preg, buf2, nmatch, pmatch, eflags);\n+\tfree(buf2);\n+\n+\treturn ret;\n+#endif\n+}\n+\n #ifndef DIR_HAS_BSD_GROUP_SEMANTICS\n # define FORCE_DIR_SET_GID S_ISGID\n #else\n-- \n2.10.0.windows.1.10.g803177d\n\n\n"},{"id":"301378","messageId":"324ecba64eb0436988aca846fb444eafda290d13.1473319844.git.johannes.schindelin@gmx.de","threadId":"44008","inReplyTo":"cover.1473319844.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 3/3] Use the newly-introduced regexec_buf() function","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-08T07:31:19Z","receivedAt":"2016-09-08T07:31:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"The new regexec_buf() function operates on buffers with an explicitly\nspecified length, rather than NUL-terminated strings.\n\nWe need to use this function whenever the buffer we want to pass to\nregexec() may have been mmap()ed (and is hence not NUL-terminated).\n\nNote: the original motivation for this patch was to fix a bug where\n`git diff -G ` would crash. This patch converts more callers,\nthough, some of which explicitly allocated and constructed\nNUL-terminated strings (or worse: modified read-only buffers to insert\nNULs).\n\nSome of the buffers actually may be NUL-terminated. As regexec_buf()\nuses REG_STARTEND where available, but has to fall back to allocating\nand constructing NUL-terminated strings where REG_STARTEND is not\navailable, this makes the code less efficient in the latter case.\n\nHowever, given the widespread support for REG_STARTEND, combined with\nthe improved ease of code maintenance, we strike the balance in favor\nof REG_STARTEND.\n\nSigned-off-by: Johannes Schindelin \n---\n diff.c | 3 ++-\n diffcore-pickaxe.c | 18 ++++++++----------\n xdiff-interface.c | 13 ++++---------\n 3 files changed, 14 insertions(+), 20 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 534c12e..526775a 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -951,7 +951,8 @@ static int find_word_boundaries(mmfile_t *buffer, regex_t *word_regex,\n {\n \tif (word_regex && *begin < buffer->size) {\n \t\tregmatch_t match[1];\n-\t\tif (!regexec(word_regex, buffer->ptr + *begin, 1, match, 0)) {\n+\t\tif (!regexec_buf(word_regex, buffer->ptr + *begin,\n+\t\t\t\t buffer->size - *begin, 1, match, 0)) {\n \t\t\tchar *p = memchr(buffer->ptr + *begin + match[0].rm_so,\n \t\t\t\t\t'\\n', match[0].rm_eo - match[0].rm_so);\n \t\t\t*end = p ? p - buffer->ptr : match[0].rm_eo + *begin;\ndiff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\nindex 55067ca..9795ca1 100644\n--- a/diffcore-pickaxe.c\n+++ b/diffcore-pickaxe.c\n@@ -23,7 +23,6 @@ static void diffgrep_consume(void *priv, char *line, unsigned long len)\n {\n \tstruct diffgrep_cb *data = priv;\n \tregmatch_t regmatch;\n-\tint hold;\n \n \tif (line[0] != '+' && line[0] != '-')\n \t\treturn;\n@@ -33,11 +32,8 @@ static void diffgrep_consume(void *priv, char *line, unsigned long len)\n \t\t * caller early.\n \t\t */\n \t\treturn;\n-\t/* Yuck -- line ought to be \"const char *\"! */\n-\thold = line[len];\n-\tline[len] = '\\0';\n-\tdata->hit = !regexec(data->regexp, line + 1, 1, ®match, 0);\n-\tline[len] = hold;\n+\tdata->hit = !regexec_buf(data->regexp, line + 1, len - 1, 1,\n+\t\t\t\t ®match, 0);\n }\n \n static int diff_grep(mmfile_t *one, mmfile_t *two,\n@@ -50,9 +46,11 @@ static int diff_grep(mmfile_t *one, mmfile_t *two,\n \txdemitconf_t xecfg;\n \n \tif (!one)\n-\t\treturn !regexec(regexp, two->ptr, 1, ®match, 0);\n+\t\treturn !regexec_buf(regexp, two->ptr, two->size,\n+\t\t\t\t 1, ®match, 0);\n \tif (!two)\n-\t\treturn !regexec(regexp, one->ptr, 1, ®match, 0);\n+\t\treturn !regexec_buf(regexp, one->ptr, one->size,\n+\t\t\t\t 1, ®match, 0);\n \n \t/*\n \t * We have both sides; need to run textual diff and see if\n@@ -83,8 +81,8 @@ static unsigned int contains(mmfile_t *mf, regex_t *regexp, kwset_t kws)\n \t\tregmatch_t regmatch;\n \t\tint flags = 0;\n \n-\t\tassert(data[sz] == '\\0');\n-\t\twhile (*data && !regexec(regexp, data, 1, ®match, flags)) {\n+\t\twhile (*data &&\n+\t\t !regexec_buf(regexp, data, sz, 1, ®match, flags)) {\n \t\t\tflags |= REG_NOTBOL;\n \t\t\tdata += regmatch.rm_eo;\n \t\t\tif (*data && regmatch.rm_so == regmatch.rm_eo)\ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex f34ea76..50702a2 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -214,11 +214,10 @@ struct ff_regs {\n static long ff_regexp(const char *line, long len,\n \t\tchar *buffer, long buffer_size, void *priv)\n {\n-\tchar *line_buffer;\n \tstruct ff_regs *regs = priv;\n \tregmatch_t pmatch[2];\n \tint i;\n-\tint result = -1;\n+\tint result;\n \n \t/* Exclude terminating newline (and cr) from matching */\n \tif (len > 0 && line[len-1] == '\\n') {\n@@ -228,18 +227,16 @@ static long ff_regexp(const char *line, long len,\n \t\t\tlen--;\n \t}\n \n-\tline_buffer = xstrndup(line, len); /* make NUL terminated */\n-\n \tfor (i = 0; i < regs->nr; i++) {\n \t\tstruct ff_reg *reg = regs->array + i;\n-\t\tif (!regexec(®->re, line_buffer, 2, pmatch, 0)) {\n+\t\tif (!regexec_buf(®->re, line, len, 2, pmatch, 0)) {\n \t\t\tif (reg->negate)\n-\t\t\t\tgoto fail;\n+\t\t\t\treturn -1;\n \t\t\tbreak;\n \t\t}\n \t}\n \tif (regs->nr <= i)\n-\t\tgoto fail;\n+\t\treturn -1;\n \ti = pmatch[1].rm_so >= 0 ? 1 : 0;\n \tline += pmatch[i].rm_so;\n \tresult = pmatch[i].rm_eo - pmatch[i].rm_so;\n@@ -248,8 +245,6 @@ static long ff_regexp(const char *line, long len,\n \twhile (result > 0 && (isspace(line[result - 1])))\n \t\tresult--;\n \tmemcpy(buffer, line, result);\n- fail:\n-\tfree(line_buffer);\n \treturn result;\n }\n \n-- \n2.10.0.windows.1.10.g803177d\n"},{"id":"301379","messageId":"alpine.DEB.2.20.1609080931250.129229@virtualbox","threadId":"44008","inReplyTo":"cover.1473319844.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v2 0/3] Fix a segfault caused by regexec() being called on mmap()ed data","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-08T07:33:29Z","receivedAt":"2016-09-08T07:33:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 8 Sep 2016, Johannes Schindelin wrote:\n\n> We solve this by introducing a helper, regexec_buf(), that takes a\n> pointer and a length instead of a NUL-terminated string.\n\nBTW I should have clarified why I decided on another name than regexecn()\n(I had considered this even before reading Peff's proposed patch): the \nin string functions suggest a limiting of NUL-terminated strings. In other\nwords, if n = 100 and the provided pointer points to a NUL-terminated\nstring of length 3, the *n function will treat it as a string of length 3.\n\nThat is not what regexec_buf() does: it ignores the NUL. Hence the\ndifferent name.\n\nCiao,\nDscho\n"},{"id":"301380","messageId":"alpine.DEB.2.20.1609080933470.129229@virtualbox","threadId":"44008","inReplyTo":"20160906184143.55a5zoa2mj6c2e5m@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] diff_populate_filespec: NUL-terminate buffers","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-08T07:49:38Z","receivedAt":"2016-09-08T07:49:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Tue, 6 Sep 2016, Jeff King wrote:\n\n> On Tue, Sep 06, 2016 at 06:02:59PM +0200, Johannes Schindelin wrote:\n> \n> > It will still be quite tricky, because we have to touch a function that is\n> > rather at the bottom of the food chain: diff_populate_filespec() is called\n> > from fill_textconv(), which in turn is called from pickaxe_match(), and\n> > only pickaxe_match() knows whether we want to call regexec() or not (it\n> > depends on its regexp parameter).\n> > \n> > Adding a flag to diff_populate_filespec() sounds really reasonable until\n> > you see how many call sites fill_textconv() has.\n> \n> I was thinking of something quite gross, like a global \"switch to using\n> slower-but-safer NUL termination\" flag (but I agree with Junio's point\n> elsewhere that we do not even know if it is \"slower\").\n\nUrgh.\n\n;-)\n\n> > So now for the better idea.\n> > \n> > While I was researching the code for this reply, I hit upon one thing\n> > that I never knew existed, introduced in f96e567 (grep: use\n> > REG_STARTEND for all matching if available, 2010-05-22). Apparently,\n> > NetBSD introduced an extension to regexec() where you can specify\n> > buffer boundaries using REG_STARTEND. Which is pretty much what we\n> > need.\n> \n> Yes, and compat/regex support this, too. My question is whether it is\n> portable.\n\nThat is only one question.\n\nAnother, important question is: is it efficient?\n\nI have no idea whether there exists any hardware-accelerated regex library\nout there, maybe even using CUDA (I know that there is some code out there\nusing SSE to perform LF -> CR/LF conversion, unfortunately it is\nintentionally incompatible with GPLv2).\n\nWe cannot simply switch everybody and her dog to compat/regex/ just\nbecause we want to avoid a segfault.\n\n> > diff --git a/diff.c b/diff.c\n> > index 534c12e..2c5a360 100644\n> > --- a/diff.c\n> > +++ b/diff.c\n> > @@ -951,7 +951,13 @@ static int find_word_boundaries(mmfile_t *buffer,\n> > regex_t *word_regex,\n> > {\n> > \tif (word_regex && *begin < buffer->size) {\n> > \t\tregmatch_t match[1];\n> > -\t\tif (!regexec(word_regex, buffer->ptr + *begin, 1, match,\n> > \t\t0)) {\n> > +\t\tint f = 0;\n> > +#ifdef REG_STARTEND\n> > +\t\tmatch[0].rm_so = 0;\n> > +\t\tmatch[0].rm_eo = *end - *begin;\n> > +\t\tf = REG_STARTEND;\n> > +#endif\n> > +\t\tif (!regexec(word_regex, buffer->ptr + *begin, 1, match,\n> > f)) {\n\nHeh. You introduced the same bug I did. Or maybe you just fetched my\nmmap-regexec branch and looked at an intermediate iteration?\n\nThe problem with this patch is that *end is uninitialized. I actually\ninitialized it in my patch, but it was still incorrect. I settled on using\nbuffer->size - *begin in the end.\n\n> What happens to those poor souls on systems without REG_STARTEND? Do\n> they get to keep segfaulting?\n\nOf course not. Those poor souls on systems without REG_STARTEND pay a\nlittle price for that: malloc(); memcpy(); *end = '\\0'; ... free();\n\nI think it is worth it: maintenance of the code is much easier that way\nthan forcing everybody and her dog and her dog's hamster to compat/regex/.\n\n> But I much prefer this approach to copying the data just to add a NUL.\n\nI think it is not worth the burden. The only regex implementation in\nsemi-widespread use that do not support REG_STARTEND seems to be musl.\n\nI'd rather not spend *so much* effort just to support an obscure platform.\nNot when the users of that obscure platform could spend that effort\nthemselves. And probably won't, because we only copy data to add a NUL on\nthose platforms when regexec() is called on an mmfile_t.\n\nBetter to keep it simple,\nDscho\n"},{"id":"301381","messageId":"alpine.DEB.2.20.1609080950090.129229@virtualbox","threadId":"44008","inReplyTo":"xmqqinu7muic.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/3] diff_populate_filespec: NUL-terminate buffers","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-08T07:52:39Z","receivedAt":"2016-09-08T07:52:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 7 Sep 2016, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > What happens to those poor souls on systems without REG_STARTEND? Do\n> > they get to keep segfaulting?\n> >\n> > I think the solution is to push them into setting NO_REGEX. So looking\n> > at this versus a \"regexecn\", it seems:\n> >\n> > - this lets people keep using their native regexec if it supports\n> > STARTEND\n> >\n> > - this is a bit more clunky to use at the callsites (though we could\n> > _create_ a portable regexecn wrapper that uses this technique on top\n> > of the native regex library)\n> >\n> > But I much prefer this approach to copying the data just to add a NUL.\n> \n> I first thought \"push them to NO_REGEX\" to mean \"they live with\n> crippled Git that does not do regexp\" and went \"Huh?\", but it merely\n> means \"let's avoid platform regex library and use on from the\n> compat/ hierarchy\", which would solve the STARTEND portability issue\n> for everybody.\n> \n> Which is very good.\n> \n> The idea to create a thin regexecn() wrapper also sounds like a good\n> idea, too. The changes to the callsites in the demonstration patch\n> does look a bit clunky to me, too.\n\nThe demonstration patch was only meant as a mere demonstration where this\nleads us. I DRY'd it up quite a bit (which was my plan all along, but it\nwas faster to make the changes in place, to avoid a full-sale\nrecompilation due to a central header change; you might not care because\nyou use Linux with its native POSIX, while I have to use MSYS2, making\neven my builds slower).\n\nAnd I really do not think that it would be a good idea to use\ncompat/regex/ for everybody, even if they already have a working regex.h\non their system.\n\nCiao,\nDscho\n"},{"id":"301382","messageId":"alpine.DEB.2.20.1609080953070.129229@virtualbox","threadId":"44008","inReplyTo":"20160906184320.lzg5jizpw2kbzf72@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] Demonstrate a problem: our pickaxe code assumes NUL-terminated buffers","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-08T07:53:52Z","receivedAt":"2016-09-08T07:54:06Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Tue, 6 Sep 2016, Jeff King wrote:\n\n> On Mon, Sep 05, 2016 at 05:45:02PM +0200, Johannes Schindelin wrote:\n> \n> > Typically, on Linux the test passes. On Windows, it fails virtually\n> > every time due to an access violation (that's a segmentation fault for\n> > you Unix-y people out there). And Windows would be correct: the\n> > regexec() call wants to operate on a regular, NUL-terminated string,\n> > there is no NUL in the mmap()ed memory range, and it is undefined\n> > whether the next byte is even legal to access.\n> > \n> > When run with --valgrind it demonstrates quite clearly the breakage, of\n> > course.\n> > \n> > So we simply mark it with `test_expect_success` for now.\n> \n> I'd prefer if this were marked as expect_failure. It fails reliably for\n> me on Linux, even without --valgrind. But even if that were not so,\n> there is no reason to hurt bisectability of somebody running with\n> \"--valgrind\" (not when it costs so little to mark it correctly).\n\nForgot to say in the cover letter: I did change this from\ntest_expect_success to test_expect_failure.\n\nBut of course, now I remember that I failed to change it back in 3/3. Bah.\n\nCiao,\nDscho\n"},{"id":"301383","messageId":"alpine.DEB.2.20.1609080954010.129229@virtualbox","threadId":"44008","inReplyTo":"324ecba64eb0436988aca846fb444eafda290d13.1473319844.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v2 3/3] Use the newly-introduced regexec_buf() function","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-08T07:54:51Z","receivedAt":"2016-09-08T07:55:22Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 8 Sep 2016, Johannes Schindelin wrote:\n\n> The new regexec_buf() function operates on buffers with an explicitly\n> specified length, rather than NUL-terminated strings.\n> \n> We need to use this function whenever the buffer we want to pass to\n> regexec() may have been mmap()ed (and is hence not NUL-terminated).\n> \n> Note: the original motivation for this patch was to fix a bug where\n> `git diff -G ` would crash. This patch converts more callers,\n> though, some of which explicitly allocated and constructed\n> NUL-terminated strings (or worse: modified read-only buffers to insert\n> NULs).\n> \n> Some of the buffers actually may be NUL-terminated. As regexec_buf()\n> uses REG_STARTEND where available, but has to fall back to allocating\n> and constructing NUL-terminated strings where REG_STARTEND is not\n> available, this makes the code less efficient in the latter case.\n> \n> However, given the widespread support for REG_STARTEND, combined with\n> the improved ease of code maintenance, we strike the balance in favor\n> of REG_STARTEND.\n> \n> Signed-off-by: Johannes Schindelin \n> ---\n> diff.c | 3 ++-\n> diffcore-pickaxe.c | 18 ++++++++----------\n> xdiff-interface.c | 13 ++++---------\n> 3 files changed, 14 insertions(+), 20 deletions(-)\n\nI just realized that this should switch the test_expect_failure from 1/3\nto a test_expect_success.\n\nWill send out v3 in a moment.\n\nCiao,\nDscho\n"},{"id":"301384","messageId":"cover.1473321437.git.johannes.schindelin@gmx.de","threadId":"44008","inReplyTo":"cover.1473319844.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 0/3] Fix a segfault caused by regexec() being called on mmap()ed data","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-08T07:57:27Z","receivedAt":"2016-09-08T07:57:52Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This patch series addresses a problem where `git diff` is called using\n`-G` or `-S --pickaxe-regex` on new-born files that are configured\nwithout user diff drivers, and that hence get mmap()ed into memory.\n\nThe problem with that: mmap()ed memory is *not* NUL-terminated, yet the\npickaxe code calls regexec() on it just the same.\n\nThis problem has been reported by my colleague Chris Sidi.\n\nWe solve this by introducing a helper, regexec_buf(), that takes a\npointer and a length instead of a NUL-terminated string.\n\nThis helper then uses REG_STARTEND where available, and falls back to\nallocating and constructing a NUL-terminated string. Given the\nwide-spread support for REG_STARTEND (Linux has it, MacOSX has it, Git\nfor Windows has it because it uses compat/regex/ that has it), I think\nthis is a fair trade-off.\n\nChanges since v2:\n\n- changed 3/3 to switch the test_expect_failure from 1/3 to a\n test_expect_success\n\n\nJohannes Schindelin (3):\n Demonstrate a problem: our pickaxe code assumes NUL-terminated buffers\n Introduce a function to run regexec() on non-NUL-terminated buffers\n Use the newly-introduced regexec_buf() function\n\n diff.c | 3 ++-\n diffcore-pickaxe.c | 18 ++++++++----------\n git-compat-util.h | 21 +++++++++++++++++++++\n t/t4061-diff-pickaxe.sh | 22 ++++++++++++++++++++++\n xdiff-interface.c | 13 ++++---------\n 5 files changed, 57 insertions(+), 20 deletions(-)\n create mode 100755 t/t4061-diff-pickaxe.sh\n\nPublished-As: https://github.com/dscho/git/releases/tag/mmap-regexec-v3\nFetch-It-Via: git fetch https://github.com/dscho/git mmap-regexec-v3\n\nInterdiff vs v2:\n\n diff --git a/t/t4061-diff-pickaxe.sh b/t/t4061-diff-pickaxe.sh\n index 5929f2e..f0bf50b 100755\n --- a/t/t4061-diff-pickaxe.sh\n +++ b/t/t4061-diff-pickaxe.sh\n @@ -14,7 +14,7 @@ test_expect_success setup '\n \ttest_tick &&\n \tgit commit -m \"A 4k file\"\n '\n -test_expect_failure '-G matches' '\n +test_expect_success '-G matches' '\n \tgit diff --name-only -G \"^0{4096}$\" HEAD^ >out &&\n \ttest 4096-zeroes.txt = \"$(cat out)\"\n '\n\n-- \n2.10.0.windows.1.10.g803177d\n\nbase-commit: 6ebdac1bab966b720d776aa43ca188fe378b1f4b\n"},{"id":"301385","messageId":"53cde4f47b078378feb202dfc44c690373cfcdd2.1473321437.git.johannes.schindelin@gmx.de","threadId":"44008","inReplyTo":"cover.1473321437.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 1/3] Demonstrate a problem: our pickaxe code assumes NUL-terminated buffers","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-08T07:57:46Z","receivedAt":"2016-09-08T07:58:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"When our pickaxe code feeds file contents to regexec(), it implicitly\nassumes that the file contents are read into implicitly NUL-terminated\nbuffers (i.e. that we overallocate by 1, appending a single '\\0').\n\nThis is not so.\n\nIn particular when the file contents are simply mmap()ed, we can be\nvirtually certain that the buffer is preceding uninitialized bytes, or\ninvalid pages.\n\nNote that the test we add here is known to be flakey: we simply cannot\nknow whether the byte following the mmap()ed ones is a NUL or not.\n\nTypically, on Linux the test passes. On Windows, it fails virtually\nevery time due to an access violation (that's a segmentation fault for\nyou Unix-y people out there). And Windows would be correct: the\nregexec() call wants to operate on a regular, NUL-terminated string,\nthere is no NUL in the mmap()ed memory range, and it is undefined\nwhether the next byte is even legal to access.\n\nWhen run with --valgrind it demonstrates quite clearly the breakage, of\ncourse.\n\nBeing marked with `test_expect_failure`, this test will sometimes be\ndeclare \"TODO fixed\", even if it only passes by mistake.\n\nThis test case represents a Minimal, Complete and Verifiable Example of\na breakage reported by Chris Sidi.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t4061-diff-pickaxe.sh | 22 ++++++++++++++++++++++\n 1 file changed, 22 insertions(+)\n create mode 100755 t/t4061-diff-pickaxe.sh\n\ndiff --git a/t/t4061-diff-pickaxe.sh b/t/t4061-diff-pickaxe.sh\nnew file mode 100755\nindex 0000000..5929f2e\n--- /dev/null\n+++ b/t/t4061-diff-pickaxe.sh\n@@ -0,0 +1,22 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2016 Johannes Schindelin\n+#\n+\n+test_description='Pickaxe options'\n+\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\ttest_commit initial &&\n+\tprintf \"%04096d\" 0 >4096-zeroes.txt &&\n+\tgit add 4096-zeroes.txt &&\n+\ttest_tick &&\n+\tgit commit -m \"A 4k file\"\n+'\n+test_expect_failure '-G matches' '\n+\tgit diff --name-only -G \"^0{4096}$\" HEAD^ >out &&\n+\ttest 4096-zeroes.txt = \"$(cat out)\"\n+'\n+\n+test_done\n-- \n2.10.0.windows.1.10.g803177d\n\n\n"},{"id":"301386","messageId":"94ee698b2736929d37640012a1b1735b134dd3d6.1473321437.git.johannes.schindelin@gmx.de","threadId":"44008","inReplyTo":"cover.1473321437.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 2/3] Introduce a function to run regexec() on non-NUL-terminated buffers","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-08T07:58:35Z","receivedAt":"2016-09-08T07:59:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"We just introduced a test that demonstrates that our sloppy use of\nregexec() on a mmap()ed area can result in incorrect results or even\nhard crashes.\n\nSo what we need to fix this is a function that calls regexec() on a\nlength-delimited, rather than a NUL-terminated, string.\n\nHappily, there is an extension to regexec() introduced by the NetBSD\nproject and present in all major regex implementation including\nLinux', MacOSX' and the one Git includes in compat/regex/: by using\nthe (non-POSIX) REG_STARTEND flag, it is possible to tell the\nregexec() function that it should only look at the offsets between\npmatch[0].rm_so and pmatch[0].rm_eo.\n\nThat is exactly what we need.\n\nSince support for REG_STARTEND is so widespread by now, let's just\nintroduce a helper function that uses it, and fall back to allocating\nand constructing a NUL-terminated when REG_STARTEND is not available.\n\nSigned-off-by: Johannes Schindelin \n---\n git-compat-util.h | 21 +++++++++++++++++++++\n 1 file changed, 21 insertions(+)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex db89ba7..19128b3 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -965,6 +965,27 @@ void git_qsort(void *base, size_t nmemb, size_t size,\n #define qsort git_qsort\n #endif\n \n+static inline int regexec_buf(const regex_t *preg, const char *buf, size_t size,\n+\t\t\t size_t nmatch, regmatch_t pmatch[], int eflags)\n+{\n+#ifdef REG_STARTEND\n+\tassert(nmatch > 0 && pmatch);\n+\tpmatch[0].rm_so = 0;\n+\tpmatch[0].rm_eo = size;\n+\treturn regexec(preg, buf, nmatch, pmatch, eflags | REG_STARTEND);\n+#else\n+\tchar *buf2 = xmalloc(size + 1);\n+\tint ret;\n+\n+\tmemcpy(buf2, buf, size);\n+\tbuf2[size] = '\\0';\n+\tret = regexec(preg, buf2, nmatch, pmatch, eflags);\n+\tfree(buf2);\n+\n+\treturn ret;\n+#endif\n+}\n+\n #ifndef DIR_HAS_BSD_GROUP_SEMANTICS\n # define FORCE_DIR_SET_GID S_ISGID\n #else\n-- \n2.10.0.windows.1.10.g803177d\n\n\n"},{"id":"301387","messageId":"d0537819a3676fda6928e7ad3282aa71643f0755.1473321437.git.johannes.schindelin@gmx.de","threadId":"44008","inReplyTo":"cover.1473321437.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 3/3] Use the newly-introduced regexec_buf() function","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-08T07:59:06Z","receivedAt":"2016-09-08T07:59:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"The new regexec_buf() function operates on buffers with an explicitly\nspecified length, rather than NUL-terminated strings.\n\nWe need to use this function whenever the buffer we want to pass to\nregexec() may have been mmap()ed (and is hence not NUL-terminated).\n\nNote: the original motivation for this patch was to fix a bug where\n`git diff -G ` would crash. This patch converts more callers,\nthough, some of which explicitly allocated and constructed\nNUL-terminated strings (or worse: modified read-only buffers to insert\nNULs).\n\nSome of the buffers actually may be NUL-terminated. As regexec_buf()\nuses REG_STARTEND where available, but has to fall back to allocating\nand constructing NUL-terminated strings where REG_STARTEND is not\navailable, this makes the code less efficient in the latter case.\n\nHowever, given the widespread support for REG_STARTEND, combined with\nthe improved ease of code maintenance, we strike the balance in favor\nof REG_STARTEND.\n\nSigned-off-by: Johannes Schindelin \n---\n diff.c | 3 ++-\n diffcore-pickaxe.c | 18 ++++++++----------\n t/t4061-diff-pickaxe.sh | 2 +-\n xdiff-interface.c | 13 ++++---------\n 4 files changed, 15 insertions(+), 21 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 534c12e..526775a 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -951,7 +951,8 @@ static int find_word_boundaries(mmfile_t *buffer, regex_t *word_regex,\n {\n \tif (word_regex && *begin < buffer->size) {\n \t\tregmatch_t match[1];\n-\t\tif (!regexec(word_regex, buffer->ptr + *begin, 1, match, 0)) {\n+\t\tif (!regexec_buf(word_regex, buffer->ptr + *begin,\n+\t\t\t\t buffer->size - *begin, 1, match, 0)) {\n \t\t\tchar *p = memchr(buffer->ptr + *begin + match[0].rm_so,\n \t\t\t\t\t'\\n', match[0].rm_eo - match[0].rm_so);\n \t\t\t*end = p ? p - buffer->ptr : match[0].rm_eo + *begin;\ndiff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\nindex 55067ca..9795ca1 100644\n--- a/diffcore-pickaxe.c\n+++ b/diffcore-pickaxe.c\n@@ -23,7 +23,6 @@ static void diffgrep_consume(void *priv, char *line, unsigned long len)\n {\n \tstruct diffgrep_cb *data = priv;\n \tregmatch_t regmatch;\n-\tint hold;\n \n \tif (line[0] != '+' && line[0] != '-')\n \t\treturn;\n@@ -33,11 +32,8 @@ static void diffgrep_consume(void *priv, char *line, unsigned long len)\n \t\t * caller early.\n \t\t */\n \t\treturn;\n-\t/* Yuck -- line ought to be \"const char *\"! */\n-\thold = line[len];\n-\tline[len] = '\\0';\n-\tdata->hit = !regexec(data->regexp, line + 1, 1, ®match, 0);\n-\tline[len] = hold;\n+\tdata->hit = !regexec_buf(data->regexp, line + 1, len - 1, 1,\n+\t\t\t\t ®match, 0);\n }\n \n static int diff_grep(mmfile_t *one, mmfile_t *two,\n@@ -50,9 +46,11 @@ static int diff_grep(mmfile_t *one, mmfile_t *two,\n \txdemitconf_t xecfg;\n \n \tif (!one)\n-\t\treturn !regexec(regexp, two->ptr, 1, ®match, 0);\n+\t\treturn !regexec_buf(regexp, two->ptr, two->size,\n+\t\t\t\t 1, ®match, 0);\n \tif (!two)\n-\t\treturn !regexec(regexp, one->ptr, 1, ®match, 0);\n+\t\treturn !regexec_buf(regexp, one->ptr, one->size,\n+\t\t\t\t 1, ®match, 0);\n \n \t/*\n \t * We have both sides; need to run textual diff and see if\n@@ -83,8 +81,8 @@ static unsigned int contains(mmfile_t *mf, regex_t *regexp, kwset_t kws)\n \t\tregmatch_t regmatch;\n \t\tint flags = 0;\n \n-\t\tassert(data[sz] == '\\0');\n-\t\twhile (*data && !regexec(regexp, data, 1, ®match, flags)) {\n+\t\twhile (*data &&\n+\t\t !regexec_buf(regexp, data, sz, 1, ®match, flags)) {\n \t\t\tflags |= REG_NOTBOL;\n \t\t\tdata += regmatch.rm_eo;\n \t\t\tif (*data && regmatch.rm_so == regmatch.rm_eo)\ndiff --git a/t/t4061-diff-pickaxe.sh b/t/t4061-diff-pickaxe.sh\nindex 5929f2e..f0bf50b 100755\n--- a/t/t4061-diff-pickaxe.sh\n+++ b/t/t4061-diff-pickaxe.sh\n@@ -14,7 +14,7 @@ test_expect_success setup '\n \ttest_tick &&\n \tgit commit -m \"A 4k file\"\n '\n-test_expect_failure '-G matches' '\n+test_expect_success '-G matches' '\n \tgit diff --name-only -G \"^0{4096}$\" HEAD^ >out &&\n \ttest 4096-zeroes.txt = \"$(cat out)\"\n '\ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex f34ea76..50702a2 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -214,11 +214,10 @@ struct ff_regs {\n static long ff_regexp(const char *line, long len,\n \t\tchar *buffer, long buffer_size, void *priv)\n {\n-\tchar *line_buffer;\n \tstruct ff_regs *regs = priv;\n \tregmatch_t pmatch[2];\n \tint i;\n-\tint result = -1;\n+\tint result;\n \n \t/* Exclude terminating newline (and cr) from matching */\n \tif (len > 0 && line[len-1] == '\\n') {\n@@ -228,18 +227,16 @@ static long ff_regexp(const char *line, long len,\n \t\t\tlen--;\n \t}\n \n-\tline_buffer = xstrndup(line, len); /* make NUL terminated */\n-\n \tfor (i = 0; i < regs->nr; i++) {\n \t\tstruct ff_reg *reg = regs->array + i;\n-\t\tif (!regexec(®->re, line_buffer, 2, pmatch, 0)) {\n+\t\tif (!regexec_buf(®->re, line, len, 2, pmatch, 0)) {\n \t\t\tif (reg->negate)\n-\t\t\t\tgoto fail;\n+\t\t\t\treturn -1;\n \t\t\tbreak;\n \t\t}\n \t}\n \tif (regs->nr <= i)\n-\t\tgoto fail;\n+\t\treturn -1;\n \ti = pmatch[1].rm_so >= 0 ? 1 : 0;\n \tline += pmatch[i].rm_so;\n \tresult = pmatch[i].rm_eo - pmatch[i].rm_so;\n@@ -248,8 +245,6 @@ static long ff_regexp(const char *line, long len,\n \twhile (result > 0 && (isspace(line[result - 1])))\n \t\tresult--;\n \tmemcpy(buffer, line, result);\n- fail:\n-\tfree(line_buffer);\n \treturn result;\n }\n \n-- \n2.10.0.windows.1.10.g803177d\n"},{"id":"301388","messageId":"20160908080035.czwn5y3re5bp5vkg@sigill.intra.peff.net","threadId":"44008","inReplyTo":"alpine.DEB.2.20.1609080921030.129229@virtualbox","subject":"Re: [PATCH 0/3] Fix a segfault caused by regexec() being called on mmap()ed data","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-08T08:00:35Z","receivedAt":"2016-09-08T08:00:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 08, 2016 at 09:29:58AM +0200, Johannes Schindelin wrote:\n\n> sorry for the late answer, I was really busy trying to come up with a new\n> and improved version of the patch series, and while hunting a bug I\n> introduced got bogged down with other tasks.\n\nNo problem. I am not in a hurry.\n\n> > I always assumed the _point_ of re_search taking a ptr/len pair was\n> > exactly to handle this case. The documentation[1] says:\n> > \n> > `string` is the string you want to match; it can contain newline and\n> > null characters. `size` is the length of that string.\n> > \n> > Which seems pretty definitive to me (that's for re_match(), but\n> > re_search() is defined in the docs in terms of re_match()).\n> \n> Right. The problem is: I *really* want to avoid using GNU-isms.\n\nI don't think GNU-isms are a problem if we wrap them to give a nice\ninterface, and if we rely on having compat/regex. But if you mean \"I do\nnot want to rely on using compat/regex everywhere\", then OK. I can see\narguments both for and against using a consistent regex library, but I\ndo not care that much either way myself.\n\n> > We can contain this to the existing compat/regexec/regexec.c, and just\n> > provide a wrapper that is similar to regexec but takes a ptr/len pair.\n> \n> But we can do even better than that: we can provide a wrapper that uses\n> REG_STARTEND where available (which is really the majority of platforms we\n> care about: Linux, MacOSX, Windows, and even the *BSDs). Where it is not\n> available, we simply malloc(), memcpy() and append a NUL.\n\nDoesn't that make things much _worse_ for people on systems without\nREG_STARTEND? If we imagine that most regexec calls would operate on a\nNUL-terminated buffer, then they are now paying the extra malloc and\ncopy for each call to regexec_buf(), even if the buffer was already\nNUL-terminated (because they have no idea whether it was or not).\n\nI think I'd rather just have:\n\n #ifndef REG_STARTEND\n #error \"Your regex library sucks. Compile with NO_REGEX=NeedsStartEnd\"\n #endif\n\n(or you could just use REG_STARTEND and let the compiler complain, but\nthen the user has to figure out the right knob to twiddle).\n\n\nOne other question about REG_STARTEND is: what does it do with NULs\ninside the buffer? Certainly glibc (and our compat/regex) treat it as a\nbuffer with a particular length and ignore embedded NULs, as we want.\nBut the NetBSD documentation says only:\n\n REG_STARTEND The string is considered to start at string +\n\t\t pmatch[0].rm_so and to have a terminating NUL\n\t\t located at string + pmatch[0].rm_eo (there need not\n\t\t actually be a NUL at that location), \n\nBesides avoiding a segfault, one of the benefits of regcomp_buf() is\nthat we will now find pickaxe-regex strings inside mixed binary/text\nfiles. But it's not clear to me that NetBSD's implementation does this.\n\nI guess we can assume it is fine (it is certainly no _worse_ than the\ncurrent behavior), and if people's platforms do not handle it, they can\nbuild with NO_REGEX.\n\n-Peff\n"},{"id":"301389","messageId":"20160908080447.adquu2e5d7bbeorn@sigill.intra.peff.net","threadId":"44008","inReplyTo":"94ee698b2736929d37640012a1b1735b134dd3d6.1473319844.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v2 2/3] Introduce a function to run regexec() on non-NUL-terminated buffers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-08T08:04:47Z","receivedAt":"2016-09-08T08:04:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 08, 2016 at 09:31:11AM +0200, Johannes Schindelin wrote:\n\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index db89ba7..19128b3 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -965,6 +965,27 @@ void git_qsort(void *base, size_t nmemb, size_t size,\n> #define qsort git_qsort\n> #endif\n> \n> +static inline int regexec_buf(const regex_t *preg, const char *buf, size_t size,\n> +\t\t\t size_t nmatch, regmatch_t pmatch[], int eflags)\n> +{\n> +#ifdef REG_STARTEND\n> +\tassert(nmatch > 0 && pmatch);\n> +\tpmatch[0].rm_so = 0;\n> +\tpmatch[0].rm_eo = size;\n> +\treturn regexec(preg, buf, nmatch, pmatch, eflags | REG_STARTEND);\n> +#else\n> +\tchar *buf2 = xmalloc(size + 1);\n> +\tint ret;\n> +\n> +\tmemcpy(buf2, buf, size);\n> +\tbuf2[size] = '\\0';\n\nI mentioned elsewhere that I'd prefer we just push people into using\ncompat/regex if they don't have REG_STARTEND. But if we _do_ keep this\nfallback, note that the above has a buffer overflow (think what happens\nwhen \"size\" is the maximum value for a size_t). You can avoid it by\nusing xmallocz().\n\n-Peff\n"},{"id":"301391","messageId":"20160908081024.yku2xlb3jj4rplkp@sigill.intra.peff.net","threadId":"44008","inReplyTo":"alpine.DEB.2.20.1609080954010.129229@virtualbox","subject":"Re: [PATCH v2 3/3] Use the newly-introduced regexec_buf() function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-08T08:10:24Z","receivedAt":"2016-09-08T08:10:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 08, 2016 at 09:54:51AM +0200, Johannes Schindelin wrote:\n\n> > diff.c | 3 ++-\n> > diffcore-pickaxe.c | 18 ++++++++----------\n> > xdiff-interface.c | 13 ++++---------\n> > 3 files changed, 14 insertions(+), 20 deletions(-)\n> \n> I just realized that this should switch the test_expect_failure from 1/3\n> to a test_expect_success.\n\nYep. I wonder if we also would want to test that we correctly find\nregexes inside binary files.\n\nE.g., given a mixed binary/text file like:\n\n printf 'binary\\0text' >file &&\n git add file &&\n git commit -m file\n\nthen \"git log -Stext\" will find that file, but \"--pickaxe-regex\" will\nnot (using stock git). Ditto for \"-Gtext\".\n\nYour patch should fix that.\n\n-Peff\n"},{"id":"301392","messageId":"20160908081305.yhzodcjjsobq4spo@sigill.intra.peff.net","threadId":"44008","inReplyTo":"alpine.DEB.2.20.1609080931250.129229@virtualbox","subject":"Re: [PATCH v2 0/3] Fix a segfault caused by regexec() being called on mmap()ed data","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-08T08:13:05Z","receivedAt":"2016-09-08T08:13:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 08, 2016 at 09:33:29AM +0200, Johannes Schindelin wrote:\n\n> On Thu, 8 Sep 2016, Johannes Schindelin wrote:\n> \n> > We solve this by introducing a helper, regexec_buf(), that takes a\n> > pointer and a length instead of a NUL-terminated string.\n> \n> BTW I should have clarified why I decided on another name than regexecn()\n> (I had considered this even before reading Peff's proposed patch): the \n> in string functions suggest a limiting of NUL-terminated strings. In other\n> words, if n = 100 and the provided pointer points to a NUL-terminated\n> string of length 3, the *n function will treat it as a string of length 3.\n> \n> That is not what regexec_buf() does: it ignores the NUL. Hence the\n> different name.\n\nI agree that is a better name (this was the exact thing I wondered about\nwith REG_STARTEND, but certainly what we _want_ is true \"_buf\"\nsemantics).\n\nI guess an argument that REG_STARTEND does what we want everywhere is\nthat the GNU implementation does what we want, and since it has been\naround for over a decade presumably _somebody_ would have complained if\nit did not match the NetBSD behavior.\n\n-Peff\n"},{"id":"301393","messageId":"20160908081446.zlcmz6ci4cw4vc2e@sigill.intra.peff.net","threadId":"44008","inReplyTo":"20160908081024.yku2xlb3jj4rplkp@sigill.intra.peff.net","subject":"Re: [PATCH v2 3/3] Use the newly-introduced regexec_buf() function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-08T08:14:46Z","receivedAt":"2016-09-08T08:14:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 08, 2016 at 04:10:24AM -0400, Jeff King wrote:\n\n> On Thu, Sep 08, 2016 at 09:54:51AM +0200, Johannes Schindelin wrote:\n> \n> > > diff.c | 3 ++-\n> > > diffcore-pickaxe.c | 18 ++++++++----------\n> > > xdiff-interface.c | 13 ++++---------\n> > > 3 files changed, 14 insertions(+), 20 deletions(-)\n> > \n> > I just realized that this should switch the test_expect_failure from 1/3\n> > to a test_expect_success.\n> \n> Yep. I wonder if we also would want to test that we correctly find\n> regexes inside binary files.\n> \n> E.g., given a mixed binary/text file like:\n> \n> printf 'binary\\0text' >file &&\n> git add file &&\n> git commit -m file\n> \n> then \"git log -Stext\" will find that file, but \"--pickaxe-regex\" will\n> not (using stock git). Ditto for \"-Gtext\".\n> \n> Your patch should fix that.\n\nOf course if I had actually _looked carefully_ at your patch, I would\nhave seen that your test doesn't just check that we don't segfault, but\nactually confirms that we find the entry.\n\nSorry for the noise.\n\n-Peff\n"},{"id":"301394","messageId":"20160908082246.saf7vlw2xgjo7jvg@sigill.intra.peff.net","threadId":"44008","inReplyTo":"alpine.DEB.2.20.1609080933470.129229@virtualbox","subject":"Re: [PATCH 2/3] diff_populate_filespec: NUL-terminate buffers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-08T08:22:46Z","receivedAt":"2016-09-08T08:22:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 08, 2016 at 09:49:38AM +0200, Johannes Schindelin wrote:\n\n> > > diff --git a/diff.c b/diff.c\n> > > index 534c12e..2c5a360 100644\n> > > --- a/diff.c\n> > > +++ b/diff.c\n> > > @@ -951,7 +951,13 @@ static int find_word_boundaries(mmfile_t *buffer,\n> > > regex_t *word_regex,\n> > > {\n> > > \tif (word_regex && *begin < buffer->size) {\n> > > \t\tregmatch_t match[1];\n> > > -\t\tif (!regexec(word_regex, buffer->ptr + *begin, 1, match,\n> > > \t\t0)) {\n> > > +\t\tint f = 0;\n> > > +#ifdef REG_STARTEND\n> > > +\t\tmatch[0].rm_so = 0;\n> > > +\t\tmatch[0].rm_eo = *end - *begin;\n> > > +\t\tf = REG_STARTEND;\n> > > +#endif\n> > > +\t\tif (!regexec(word_regex, buffer->ptr + *begin, 1, match,\n> > > f)) {\n> \n> Heh. You introduced the same bug I did. Or maybe you just fetched my\n> mmap-regexec branch and looked at an intermediate iteration?\n\nI do not think I introduced anything. The quoted text is what you\nsent. Which is perhaps why it has your bug. :)\n\n> > But I much prefer this approach to copying the data just to add a NUL.\n> \n> I think it is not worth the burden. The only regex implementation in\n> semi-widespread use that do not support REG_STARTEND seems to be musl.\n> \n> I'd rather not spend *so much* effort just to support an obscure platform.\n> Not when the users of that obscure platform could spend that effort\n> themselves. And probably won't, because we only copy data to add a NUL on\n> those platforms when regexec() is called on an mmfile_t.\n\nI'm confused about what you think I'm proposing. I was saying I _like_\nsomething like regexec_buf() instead of copying the data. Which seems\nlike the simpler thing to me (and presumably to you). Or do you mean\nusing compat/regex to build on re_search() consistently? I do not think\nthat is all that complex; the question is only whether people really\nwant to use their own regex libraries.\n\nBetween the two options for regexec_buf(), I think you have convinced me\nthat REG_STARTEND is better than just using compat/regex everywhere. I\ndo think the fallback for platforms like musl should be \"use\ncompat/regex\" and not doing an expensive copy (which in most cases is\nnot even necessary).\n\n-Peff\n"},{"id":"301396","messageId":"20160908083509.ygh4flpunyyp4hua@sigill.intra.peff.net","threadId":"44008","inReplyTo":"20160908081446.zlcmz6ci4cw4vc2e@sigill.intra.peff.net","subject":"Re: [PATCH v2 3/3] Use the newly-introduced regexec_buf() function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-08T08:35:09Z","receivedAt":"2016-09-08T08:35:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 08, 2016 at 04:14:46AM -0400, Jeff King wrote:\n\n> On Thu, Sep 08, 2016 at 04:10:24AM -0400, Jeff King wrote:\n> \n> > On Thu, Sep 08, 2016 at 09:54:51AM +0200, Johannes Schindelin wrote:\n> > \n> > > > diff.c | 3 ++-\n> > > > diffcore-pickaxe.c | 18 ++++++++----------\n> > > > xdiff-interface.c | 13 ++++---------\n> > > > 3 files changed, 14 insertions(+), 20 deletions(-)\n> > > \n> > > I just realized that this should switch the test_expect_failure from 1/3\n> > > to a test_expect_success.\n> > \n> > Yep. I wonder if we also would want to test that we correctly find\n> > regexes inside binary files.\n> > \n> > E.g., given a mixed binary/text file like:\n> > \n> > printf 'binary\\0text' >file &&\n> > git add file &&\n> > git commit -m file\n> > \n> > then \"git log -Stext\" will find that file, but \"--pickaxe-regex\" will\n> > not (using stock git). Ditto for \"-Gtext\".\n> > \n> > Your patch should fix that.\n> \n> Of course if I had actually _looked carefully_ at your patch, I would\n> have seen that your test doesn't just check that we don't segfault, but\n> actually confirms that we find the entry.\n> \n> Sorry for the noise.\n\nActually, I take it back again. Your test case doesn't have an embedded\nNUL in it (so we check that git finds it, but aside from the lack of\nsegfault, stock git would already find it).\n\nSorry for the double-noise.\n\n-Peff\n"},{"id":"301409","messageId":"xmqqa8fijpmg.fsf@gitster.mtv.corp.google.com","threadId":"44008","inReplyTo":"20160908082246.saf7vlw2xgjo7jvg@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] diff_populate_filespec: NUL-terminate buffers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-08T16:57:43Z","receivedAt":"2016-09-08T16:57:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Between the two options for regexec_buf(), I think you have convinced me\n> that REG_STARTEND is better than just using compat/regex everywhere. I\n> do think the fallback for platforms like musl should be \"use\n> compat/regex\" and not doing an expensive copy (which in most cases is\n> not even necessary).\n\nI agree with you that it would be the best approach to build\nregexec_buf() that unconditionally uses REG_STARTEND and tell people\nwithout REG_STARTEND to use compat/regex instead of their platform\nregex library.\n\nThe description in Makefile may want to be rephrased to clarify.\n\n-# Define NO_REGEX if you have no or inferior regex support in your C library.\n+# Define NO_REGEX if your C library lacks regex support with REG_STARTEND\n+# feature.\n\nThe word \"inferior\" is not giving any useful information there.\n\n"},{"id":"301410","messageId":"xmqq60q6jpc8.fsf@gitster.mtv.corp.google.com","threadId":"44008","inReplyTo":"94ee698b2736929d37640012a1b1735b134dd3d6.1473321437.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v3 2/3] Introduce a function to run regexec() on non-NUL-terminated buffers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-08T17:03:51Z","receivedAt":"2016-09-08T17:03:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> We just introduced a test that demonstrates that our sloppy use of\n> regexec() on a mmap()ed area can result in incorrect results or even\n> hard crashes.\n>\n> So what we need to fix this is a function that calls regexec() on a\n> length-delimited, rather than a NUL-terminated, string.\n>\n> Happily, there is an extension to regexec() introduced by the NetBSD\n> project and present in all major regex implementation including\n> Linux', MacOSX' and the one Git includes in compat/regex/: by using\n> the (non-POSIX) REG_STARTEND flag, it is possible to tell the\n> regexec() function that it should only look at the offsets between\n> pmatch[0].rm_so and pmatch[0].rm_eo.\n>\n> That is exactly what we need.\n\nYes, that is good.\n\n> Since support for REG_STARTEND is so widespread by now, let's just\n> introduce a helper function that uses it, and fall back to allocating\n> and constructing a NUL-terminated when REG_STARTEND is not available.\n\nI do not think this fallback is good; we do ship a compat/ fallback\nthat does support REG_STARTEND and you'd want to use that. Not\nhaving the copying fallback means you do not even have to worry\nabout the size+1 overflow and fix it with xmallocz() ;-)\n"},{"id":"301411","messageId":"xmqq1t0ujp2f.fsf@gitster.mtv.corp.google.com","threadId":"44008","inReplyTo":"d0537819a3676fda6928e7ad3282aa71643f0755.1473321437.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v3 3/3] Use the newly-introduced regexec_buf() function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-08T17:09:44Z","receivedAt":"2016-09-08T17:09:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> @@ -33,11 +32,8 @@ static void diffgrep_consume(void *priv, char *line, unsigned long len)\n> \t\t * caller early.\n> \t\t */\n> \t\treturn;\n> -\t/* Yuck -- line ought to be \"const char *\"! */\n> -\thold = line[len];\n> -\tline[len] = '\\0';\n> -\tdata->hit = !regexec(data->regexp, line + 1, 1, ®match, 0);\n> -\tline[len] = hold;\n> +\tdata->hit = !regexec_buf(data->regexp, line + 1, len - 1, 1,\n> +\t\t\t\t ®match, 0);\n\nThis is an unexpected happy surprise. It really feels good to see\nthat \"Yuck\" line go.\n\n> @@ -228,18 +227,16 @@ static long ff_regexp(const char *line, long len,\n> \t\t\tlen--;\n> \t}\n> \n> -\tline_buffer = xstrndup(line, len); /* make NUL terminated */\n> -\n> \tfor (i = 0; i < regs->nr; i++) {\n> \t\tstruct ff_reg *reg = regs->array + i;\n> -\t\tif (!regexec(®->re, line_buffer, 2, pmatch, 0)) {\n> +\t\tif (!regexec_buf(®->re, line, len, 2, pmatch, 0)) {\n\nSo is this hunk. Removing unnecessary copying is a very good thing.\n\nPlease give these three patches a common prefix, e.g.\n\n\tregex: -G feeds a non NUL-terminated string to\tregexec() and fails\n regex: add regexec_buf() that can work on a non NUL-terminated string\n\tregex: use regexec_buf()\n\nor something like that.\n\nAlso I agree with Peff that a test with an embedded NUL would be a\ngood thing.\n\nThis round is so close to perfect.\n"},{"id":"301427","messageId":"alpine.DEB.2.20.1609082022390.129229@virtualbox","threadId":"44008","inReplyTo":"xmqqa8fijpmg.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/3] diff_populate_filespec: NUL-terminate buffers","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-08T18:22:59Z","receivedAt":"2016-09-08T18:23:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff & Junio,\n\nOn Thu, 8 Sep 2016, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Between the two options for regexec_buf(), I think you have convinced me\n> > that REG_STARTEND is better than just using compat/regex everywhere. I\n> > do think the fallback for platforms like musl should be \"use\n> > compat/regex\" and not doing an expensive copy (which in most cases is\n> > not even necessary).\n> \n> I agree with you that it would be the best approach to build\n> regexec_buf() that unconditionally uses REG_STARTEND and tell people\n> without REG_STARTEND to use compat/regex instead of their platform\n> regex library.\n> \n> The description in Makefile may want to be rephrased to clarify.\n> \n> -# Define NO_REGEX if you have no or inferior regex support in your C library.\n> +# Define NO_REGEX if your C library lacks regex support with REG_STARTEND\n> +# feature.\n\nOkay, I will make that happen. But first I need to sleep.\n\nCiao,\nDscho\n"},{"id":"301430","messageId":"20160908184801.6w3dplmbcdcftxfg@sigill.intra.peff.net","threadId":"44008","inReplyTo":"xmqqa8fijpmg.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/3] diff_populate_filespec: NUL-terminate buffers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-08T18:48:01Z","receivedAt":"2016-09-08T18:48:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 08, 2016 at 09:57:43AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Between the two options for regexec_buf(), I think you have convinced me\n> > that REG_STARTEND is better than just using compat/regex everywhere. I\n> > do think the fallback for platforms like musl should be \"use\n> > compat/regex\" and not doing an expensive copy (which in most cases is\n> > not even necessary).\n> \n> I agree with you that it would be the best approach to build\n> regexec_buf() that unconditionally uses REG_STARTEND and tell people\n> without REG_STARTEND to use compat/regex instead of their platform\n> regex library.\n> \n> The description in Makefile may want to be rephrased to clarify.\n> \n> -# Define NO_REGEX if you have no or inferior regex support in your C library.\n> +# Define NO_REGEX if your C library lacks regex support with REG_STARTEND\n> +# feature.\n> \n> The word \"inferior\" is not giving any useful information there.\n\nYeah, very much agreed (and the \"#error\" when we lack REG_STARTEND I\nmentioned earlier may be a good hint).\n\n-Peff\n"},{"id":"301432","messageId":"ac0f3441-d28f-fc5c-0273-f614b2cd4e07@ramsayjones.plus.com","threadId":"44008","inReplyTo":"20160908083509.ygh4flpunyyp4hua@sigill.intra.peff.net","subject":"Re: [PATCH v2 3/3] Use the newly-introduced regexec_buf() function","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2016-09-08T19:06:35Z","receivedAt":"2016-09-08T19:06:56Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 08/09/16 09:35, Jeff King wrote:\n> On Thu, Sep 08, 2016 at 04:14:46AM -0400, Jeff King wrote:\n> \n>> On Thu, Sep 08, 2016 at 04:10:24AM -0400, Jeff King wrote:\n>>\n>>> On Thu, Sep 08, 2016 at 09:54:51AM +0200, Johannes Schindelin wrote:\n>>>\n>>>>> diff.c | 3 ++-\n>>>>> diffcore-pickaxe.c | 18 ++++++++----------\n>>>>> xdiff-interface.c | 13 ++++---------\n>>>>> 3 files changed, 14 insertions(+), 20 deletions(-)\n>>>>\n>>>> I just realized that this should switch the test_expect_failure from 1/3\n>>>> to a test_expect_success.\n>>>\n>>> Yep. I wonder if we also would want to test that we correctly find\n>>> regexes inside binary files.\n>>>\n>>> E.g., given a mixed binary/text file like:\n>>>\n>>> printf 'binary\\0text' >file &&\n>>> git add file &&\n>>> git commit -m file\n>>>\n>>> then \"git log -Stext\" will find that file, but \"--pickaxe-regex\" will\n>>> not (using stock git). Ditto for \"-Gtext\".\n>>>\n>>> Your patch should fix that.\n>>\n>> Of course if I had actually _looked carefully_ at your patch, I would\n>> have seen that your test doesn't just check that we don't segfault, but\n>> actually confirms that we find the entry.\n>>\n>> Sorry for the noise.\n> \n> Actually, I take it back again. Your test case doesn't have an embedded\n> NUL in it (so we check that git finds it, but aside from the lack of\n> segfault, stock git would already find it).\n\nThis reminds me ... despite the native cygwin regex library no longer\nhaving the 'regex bug' (ie t0070.5 now passes), I still have NO_REGEX\nset on cygwin. This is because, when building with the native library,\nwe have an \"unexpected pass\" for t7008.12, which looks like:\n\ntest_expect_failure 'git grep .fi a' '\n git grep .fi a\n'\n[where the file a is set up earlier by: echo 'binaryQfile' | q_to_nul >a]\n\ncommit f96e5673 (\"grep: use REG_STARTEND for all matching if available\",\n22-05-2010) introduced this test and expects \".. NUL characters themselves\nare not matched in any way\". With the native library on cygwin they are\nmatched, with the compat/regex they are not. Indeed, if you use the system\n'grep' command (rather than 'git grep'), then it will also not match ... :-D\n\nSlightly off topic, but ...\n\nATB,\nRamsay Jones\n\n\n"},{"id":"301436","messageId":"20160908195300.votzp3ysxewc2mip@sigill.intra.peff.net","threadId":"44008","inReplyTo":"ac0f3441-d28f-fc5c-0273-f614b2cd4e07@ramsayjones.plus.com","subject":"Re: [PATCH v2 3/3] Use the newly-introduced regexec_buf() function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-08T19:53:00Z","receivedAt":"2016-09-08T19:53:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 08, 2016 at 08:06:35PM +0100, Ramsay Jones wrote:\n\n> > Actually, I take it back again. Your test case doesn't have an embedded\n> > NUL in it (so we check that git finds it, but aside from the lack of\n> > segfault, stock git would already find it).\n> \n> This reminds me ... despite the native cygwin regex library no longer\n> having the 'regex bug' (ie t0070.5 now passes), I still have NO_REGEX\n> set on cygwin. This is because, when building with the native library,\n> we have an \"unexpected pass\" for t7008.12, which looks like:\n> \n> test_expect_failure 'git grep .fi a' '\n> git grep .fi a\n> '\n> [where the file a is set up earlier by: echo 'binaryQfile' | q_to_nul >a]\n> \n> commit f96e5673 (\"grep: use REG_STARTEND for all matching if available\",\n> 22-05-2010) introduced this test and expects \".. NUL characters themselves\n> are not matched in any way\". With the native library on cygwin they are\n> matched, with the compat/regex they are not. Indeed, if you use the system\n> 'grep' command (rather than 'git grep'), then it will also not match ... :-D\n> \n> Slightly off topic, but ...\n\nHmm. So it sounds like the \"regmatch\" in grep.c could go away in favor\nof Johannes's regexec_buf(), and cygwin ought to be using NO_REGEX.\n\n-Peff\n"},{"id":"301453","messageId":"xmqq8tv2f59y.fsf@gitster.mtv.corp.google.com","threadId":"44008","inReplyTo":"20160908195300.votzp3ysxewc2mip@sigill.intra.peff.net","subject":"Re: [PATCH v2 3/3] Use the newly-introduced regexec_buf() function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-08T21:30:49Z","receivedAt":"2016-09-08T21:30:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> commit f96e5673 (\"grep: use REG_STARTEND for all matching if available\",\n>> 22-05-2010) introduced this test and expects \".. NUL characters themselves\n>> are not matched in any way\". With the native library on cygwin they are\n>> matched, with the compat/regex they are not. Indeed, if you use the system\n>> 'grep' command (rather than 'git grep'), then it will also not match ... :-D\n>> \n>> Slightly off topic, but ...\n>\n> Hmm. So it sounds like the \"regmatch\" in grep.c could go away in favor\n> of Johannes's regexec_buf(), and cygwin ought to be using NO_REGEX.\n\nSounds like a plan.\n"},{"id":"301465","messageId":"alpine.DEB.2.20.1609091140420.129229@virtualbox","threadId":"44008","inReplyTo":"20160908080447.adquu2e5d7bbeorn@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/3] Introduce a function to run regexec() on non-NUL-terminated buffers","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-09T09:45:01Z","receivedAt":"2016-09-09T09:45:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Thu, 8 Sep 2016, Jeff King wrote:\n\n> On Thu, Sep 08, 2016 at 09:31:11AM +0200, Johannes Schindelin wrote:\n> \n> > diff --git a/git-compat-util.h b/git-compat-util.h\n> > index db89ba7..19128b3 100644\n> > --- a/git-compat-util.h\n> > +++ b/git-compat-util.h\n> > @@ -965,6 +965,27 @@ void git_qsort(void *base, size_t nmemb, size_t size,\n> > #define qsort git_qsort\n> > #endif\n> > \n> > +static inline int regexec_buf(const regex_t *preg, const char *buf, size_t size,\n> > +\t\t\t size_t nmatch, regmatch_t pmatch[], int eflags)\n> > +{\n> > +#ifdef REG_STARTEND\n> > +\tassert(nmatch > 0 && pmatch);\n> > +\tpmatch[0].rm_so = 0;\n> > +\tpmatch[0].rm_eo = size;\n> > +\treturn regexec(preg, buf, nmatch, pmatch, eflags | REG_STARTEND);\n> > +#else\n> > +\tchar *buf2 = xmalloc(size + 1);\n> > +\tint ret;\n> > +\n> > +\tmemcpy(buf2, buf, size);\n> > +\tbuf2[size] = '\\0';\n> \n> I mentioned elsewhere that I'd prefer we just push people into using\n> compat/regex if they don't have REG_STARTEND. But if we _do_ keep this\n> fallback, note that the above has a buffer overflow (think what happens\n> when \"size\" is the maximum value for a size_t). You can avoid it by\n> using xmallocz().\n\nThat buffer overflow does not exist: If size were the maximum value for\nsize_t, then buf->ptr would point at a buffer that occupies the entire\navailable memory, meaning that there is no space left for buf->ptr, let\nalone for buf.\n\nBut I get your point. It is better to be consistent and use the same logic\nfor *all* allocations.\n\nCiao,\nDscho\n"},{"id":"301466","messageId":"alpine.DEB.2.20.1609091151510.129229@virtualbox","threadId":"44008","inReplyTo":"xmqq1t0ujp2f.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 3/3] Use the newly-introduced regexec_buf() function","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-09T09:52:50Z","receivedAt":"2016-09-09T09:53:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 8 Sep 2016, Junio C Hamano wrote:\n\n> Please give these three patches a common prefix, e.g.\n> \n> \tregex: -G feeds a non NUL-terminated string to\tregexec() and fails\n> regex: add regexec_buf() that can work on a non NUL-terminated string\n> \tregex: use regexec_buf()\n> \n> or something like that.\n\nDone.\n\n> Also I agree with Peff that a test with an embedded NUL would be a\n> good thing.\n\nThis is something I will leave to somebody else, as it was not my\nintention to fix this and I *really* have more pressing things to do right\nnow... Sorry!\n\nCiao,\nDscho\n"},{"id":"301467","messageId":"20160909095735.zen2ynpsn7azqfss@sigill.intra.peff.net","threadId":"44008","inReplyTo":"alpine.DEB.2.20.1609091151510.129229@virtualbox","subject":"Re: [PATCH v3 3/3] Use the newly-introduced regexec_buf() function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-09T09:57:35Z","receivedAt":"2016-09-09T09:57:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 09, 2016 at 11:52:50AM +0200, Johannes Schindelin wrote:\n\n> > Also I agree with Peff that a test with an embedded NUL would be a\n> > good thing.\n> \n> This is something I will leave to somebody else, as it was not my\n> intention to fix this and I *really* have more pressing things to do right\n> now... Sorry!\n\nI think it is literally just squashing this into your final patch:\n\ndiff --git a/t/t4061-diff-pickaxe.sh b/t/t4061-diff-pickaxe.sh\nindex f0bf50b..37b8dde 100755\n--- a/t/t4061-diff-pickaxe.sh\n+++ b/t/t4061-diff-pickaxe.sh\n@@ -19,4 +19,13 @@ test_expect_success '-G matches' '\n \ttest 4096-zeroes.txt = \"$(cat out)\"\n '\n \n+test_expect_success '-G matches after embedded NUL' '\n+\tprintf \"one\\0two\" >file &&\n+\tgit add file &&\n+\tgit commit -m embedded &&\n+\techo embedded >expect &&\n+\tgit log -Gtwo --format=%s >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n\n-Peff\n"},{"id":"301469","messageId":"20160909095946.ejwyjriomtaezef2@sigill.intra.peff.net","threadId":"44008","inReplyTo":"alpine.DEB.2.20.1609091140420.129229@virtualbox","subject":"Re: [PATCH v2 2/3] Introduce a function to run regexec() on non-NUL-terminated buffers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-09T09:59:47Z","receivedAt":"2016-09-09T10:00:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 09, 2016 at 11:45:01AM +0200, Johannes Schindelin wrote:\n\n> > I mentioned elsewhere that I'd prefer we just push people into using\n> > compat/regex if they don't have REG_STARTEND. But if we _do_ keep this\n> > fallback, note that the above has a buffer overflow (think what happens\n> > when \"size\" is the maximum value for a size_t). You can avoid it by\n> > using xmallocz().\n> \n> That buffer overflow does not exist: If size were the maximum value for\n> size_t, then buf->ptr would point at a buffer that occupies the entire\n> available memory, meaning that there is no space left for buf->ptr, let\n> alone for buf.\n\nTrue. I fixed quite a lot of these last summer, but they are only really\ndangerous when we have not already allocated the buffer.\n\n> But I get your point. It is better to be consistent and use the same logic\n> for *all* allocations.\n\nYep. Also, it is easier to audit if you do not have to trace back and\nsee that even though we do overflow the argument to malloc, it can't\nhappen because of memory constraints (this one is fairly obvious, but\nquite a few that I fixed previously involved complicated reasoning about\nhow much RAM you could use).\n\n-Peff\n"},{"id":"301470","messageId":"alpine.DEB.2.20.1609091158550.129229@virtualbox","threadId":"44008","inReplyTo":"20160908080035.czwn5y3re5bp5vkg@sigill.intra.peff.net","subject":"Re: [PATCH 0/3] Fix a segfault caused by regexec() being called on mmap()ed data","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-09T10:09:18Z","receivedAt":"2016-09-09T10:09:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Thu, 8 Sep 2016, Jeff King wrote:\n\n> On Thu, Sep 08, 2016 at 09:29:58AM +0200, Johannes Schindelin wrote:\n> \n> > sorry for the late answer, I was really busy trying to come up with a new\n> > and improved version of the patch series, and while hunting a bug I\n> > introduced got bogged down with other tasks.\n> \n> No problem. I am not in a hurry.\n\nI kind of am. The second half of September, I won't be able to do much of\nanything Git-related, and this is a major bug that blocks some important\nwork.\n\nSo I kind of have to press on that front.\n\n> I think I'd rather just have:\n> \n> #ifndef REG_STARTEND\n> #error \"Your regex library sucks. Compile with NO_REGEX=NeedsStartEnd\"\n> #endif\n\nDone. Although I permitted myself to reword this a little ;-)\n\n> One other question about REG_STARTEND is: what does it do with NULs\n> inside the buffer? Certainly glibc (and our compat/regex) treat it as a\n> buffer with a particular length and ignore embedded NULs, as we want.\n> But the NetBSD documentation says only:\n> \n> REG_STARTEND The string is considered to start at string +\n> \t\t pmatch[0].rm_so and to have a terminating NUL\n> \t\t located at string + pmatch[0].rm_eo (there need not\n> \t\t actually be a NUL at that location), \n> \n> Besides avoiding a segfault, one of the benefits of regcomp_buf() is\n> that we will now find pickaxe-regex strings inside mixed binary/text\n> files. But it's not clear to me that NetBSD's implementation does this.\n> \n> I guess we can assume it is fine (it is certainly no _worse_ than the\n> current behavior), and if people's platforms do not handle it, they can\n> build with NO_REGEX.\n\nRené mentioned in f96e567 (grep: use REG_STARTEND for all matching if\navailable, 2010-05-22) something along the lines of REG_STARTEND being\nable to parse beyond NULs. My interpretation of NetBSD's documentation\nagrees with your interpretation, though, that the buffers are still\nthought of as being NUL-terminated, even if rm_eo makes the code *not*\nlook at that particular NUL.\n\nBe that as it may: it is completely outside the purpose of my patch series\nto take care of making it possible for Git's regex functions to match\nbuffers with embedded NULs. The only purpose of my patch series is to fix\nthe crash that was reported to me due to regexec() reading past a mmap()ed\nbuffer. I already let myself being talked into fixing more things than\nthat, and I have to leave it at that.\n\nCiao,\nDscho\n"},{"id":"301481","messageId":"alpine.DEB.2.20.1609091238020.129229@virtualbox","threadId":"44008","inReplyTo":"20160909095735.zen2ynpsn7azqfss@sigill.intra.peff.net","subject":"Re: [PATCH v3 3/3] Use the newly-introduced regexec_buf() function","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-09T10:41:01Z","receivedAt":"2016-09-09T10:41:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Fri, 9 Sep 2016, Jeff King wrote:\n\n> On Fri, Sep 09, 2016 at 11:52:50AM +0200, Johannes Schindelin wrote:\n> \n> > > Also I agree with Peff that a test with an embedded NUL would be a\n> > > good thing.\n> > \n> > This is something I will leave to somebody else, as it was not my\n> > intention to fix this and I *really* have more pressing things to do right\n> > now... Sorry!\n> \n> I think it is literally just squashing this into your final patch:\n> \n> diff --git a/t/t4061-diff-pickaxe.sh b/t/t4061-diff-pickaxe.sh\n> index f0bf50b..37b8dde 100755\n> --- a/t/t4061-diff-pickaxe.sh\n> +++ b/t/t4061-diff-pickaxe.sh\n> @@ -19,4 +19,13 @@ test_expect_success '-G matches' '\n> \ttest 4096-zeroes.txt = \"$(cat out)\"\n> '\n> \n> +test_expect_success '-G matches after embedded NUL' '\n> +\tprintf \"one\\0two\" >file &&\n> +\tgit add file &&\n> +\tgit commit -m embedded &&\n> +\techo embedded >expect &&\n> +\tgit log -Gtwo --format=%s >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_done\n\nThank you for providing me with the patch.\n\nHowever, the whole idea of supporting regular expressions on buffers with\nembedded NULs *is* different from the purpose of this patch series.\n\nAnd in my quick web search, I got the impression that the presence of\nREG_STARTEND really does not guarantee that regexec() won't stop at the\nfirst NUL when rm_eo points after it.\n\nSo yeah, the patch would be easy to squash in, but the entire \"rat's tail\"\nof making sure that this works everywhere, *in addition* to making sure\nthat the crash on mmap()ed buffers no longer occurs, would just delay this\npatch series.\n\nAnd unfortunately I do not have time for that right now.\n\nCiao,\nDscho\n"},{"id":"301533","messageId":"xmqqmvjhc6fm.fsf@gitster.mtv.corp.google.com","threadId":"44008","inReplyTo":"alpine.DEB.2.20.1609091158550.129229@virtualbox","subject":"Re: [PATCH 0/3] Fix a segfault caused by regexec() being called on mmap()ed data","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-09T17:46:21Z","receivedAt":"2016-09-09T17:46:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> Besides avoiding a segfault, one of the benefits of regcomp_buf() is\n>> that we will now find pickaxe-regex strings inside mixed binary/text\n>> files. But it's not clear to me that NetBSD's implementation does this.\n>> \n>> I guess we can assume it is fine (it is certainly no _worse_ than the\n>> current behavior), and if people's platforms do not handle it, they can\n>> build with NO_REGEX.\n>\n> René mentioned in f96e567 (grep: use REG_STARTEND for all matching if\n> available, 2010-05-22) something along the lines of REG_STARTEND being\n> able to parse beyond NULs. My interpretation of NetBSD's documentation\n> agrees with your interpretation, though, that the buffers are still\n> thought of as being NUL-terminated, even if rm_eo makes the code *not*\n> look at that particular NUL.\n>\n> Be that as it may: it is completely outside the purpose of my patch series\n> to take care of making it possible for Git's regex functions to match\n> buffers with embedded NULs.\n\nI think you two have agreed that regexec_buf() wrapper that always\nrelies on REG_STARTEND is a good first step whether we want to do an\nembedded NUL, that we just need that good first step for now, and\nthat that good first step would not block future progress, i.e. our\nwanting to handle embedded NUL.\n\nSo let's see how well the first step flies in practice. We tell\npeople to build with NO_REGEX if their platform's regexp does not do\n(any form of) REG_STARTEND. We _might_ later have to tell them to\nset NO_REGX even if they are on NetBSD and has REG_STARTEND that may\nnot handle embedded NUL the way we want, but that can safely be left\nto the future.\n\nThanks.\n"},{"id":"301534","messageId":"xmqqfup9c6b6.fsf@gitster.mtv.corp.google.com","threadId":"44008","inReplyTo":"alpine.DEB.2.20.1609091151510.129229@virtualbox","subject":"Re: [PATCH v3 3/3] Use the newly-introduced regexec_buf() function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-09T17:49:01Z","receivedAt":"2016-09-09T17:49:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> Also I agree with Peff that a test with an embedded NUL would be a\n>> good thing.\n>\n> This is something I will leave to somebody else, as it was not my\n> intention to fix this and I *really* have more pressing things to do right\n> now... Sorry!\n\nAs I said a few minutes ago, I think we can stop _before_ worrying\nabout an embedded NUL, which is something we haven't handled before\nanyway so it is a new feature that can be built later outside the\nscope of this series.\n"},{"id":"302313","messageId":"cover.1474482164.git.johannes.schindelin@gmx.de","threadId":"44008","inReplyTo":"cover.1473321437.git.johannes.schindelin@gmx.de","subject":"[PATCH v4 0/3] Fix a segfault caused by regexec() being called on mmap()ed data","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-21T18:23:11Z","receivedAt":"2016-09-21T18:23:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"[Cc:ing Benjamin Kramer & René Scharfe because they both worked on\nthe REG_STARTEND code in grep.c that I replace in this iteration of the\npatch series]\n\nThis patch series addresses a problem where `git diff` is called using\n`-G` or `-S --pickaxe-regex` on new-born files that are configured\nwithout user diff drivers, and that hence get mmap()ed into memory.\n\nThe problem with that: mmap()ed memory is *not* NUL-terminated, yet the\npickaxe code calls regexec() on it just the same.\n\nThis problem has been reported by my colleague Chris Sidi.\n\nWe solve this by introducing a helper, regexec_buf(), that takes a\npointer and a length instead of a NUL-terminated string.\n\nThis helper then uses REG_STARTEND where available, and falls back to\nallocating and constructing a NUL-terminated string. Given the\nwide-spread support for REG_STARTEND (Linux has it, MacOSX has it, Git\nfor Windows has it because it uses compat/regex/ that has it), I think\nthis is a fair trade-off.\n\nChanges since v3:\n\n- reworded the onelines as per Junio's suggestions.\n\n- removed fallback when REG_STARTEND is not supported, in favor of\n requiring NO_REGEX.\n\n- removed the regmatch() function from grep.c, in favor of using\n regexec_buf().\n\n\nJohannes Schindelin (3):\n regex: -G feeds a non NUL-terminated string to regexec() and\n fails\n regex: add regexec_buf() that can work on a non NUL-terminated string\n regex: use regexec_buf()\n\n Makefile | 3 ++-\n diff.c | 3 ++-\n diffcore-pickaxe.c | 18 ++++++++----------\n git-compat-util.h | 13 +++++++++++++\n grep.c | 14 ++------------\n t/t4061-diff-pickaxe.sh | 22 ++++++++++++++++++++++\n xdiff-interface.c | 13 ++++---------\n 7 files changed, 53 insertions(+), 33 deletions(-)\n create mode 100755 t/t4061-diff-pickaxe.sh\n\nPublished-As: https://github.com/dscho/git/releases/tag/mmap-regexec-v4\nFetch-It-Via: git fetch https://github.com/dscho/git mmap-regexec-v4\n\nInterdiff vs v3:\n\n diff --git a/Makefile b/Makefile\n index df4f86b..c6f7f66 100644\n --- a/Makefile\n +++ b/Makefile\n @@ -301,7 +301,8 @@ all::\n # crashes due to allocation and free working on different 'heaps'.\n # It's defined automatically if USE_NED_ALLOCATOR is set.\n #\n -# Define NO_REGEX if you have no or inferior regex support in your C library.\n +# Define NO_REGEX if your C library lacks regex support with REG_STARTEND\n +# feature.\n #\n # Define HAVE_DEV_TTY if your system can open /dev/tty to interact with the\n # user.\n diff --git a/git-compat-util.h b/git-compat-util.h\n index 627ec5f..8aab0c3 100644\n --- a/git-compat-util.h\n +++ b/git-compat-util.h\n @@ -977,25 +977,17 @@ void git_qsort(void *base, size_t nmemb, size_t size,\n #define qsort git_qsort\n #endif\n \n +#ifndef REG_STARTEND\n +#error \"Git requires REG_STARTEND support. Compile with NO_REGEX=NeedsStartEnd\"\n +#endif\n +\n static inline int regexec_buf(const regex_t *preg, const char *buf, size_t size,\n \t\t\t size_t nmatch, regmatch_t pmatch[], int eflags)\n {\n -#ifdef REG_STARTEND\n \tassert(nmatch > 0 && pmatch);\n \tpmatch[0].rm_so = 0;\n \tpmatch[0].rm_eo = size;\n \treturn regexec(preg, buf, nmatch, pmatch, eflags | REG_STARTEND);\n -#else\n -\tchar *buf2 = xmalloc(size + 1);\n -\tint ret;\n -\n -\tmemcpy(buf2, buf, size);\n -\tbuf2[size] = '\\0';\n -\tret = regexec(preg, buf2, nmatch, pmatch, eflags);\n -\tfree(buf2);\n -\n -\treturn ret;\n -#endif\n }\n \n #ifndef DIR_HAS_BSD_GROUP_SEMANTICS\n diff --git a/grep.c b/grep.c\n index d7d00b8..1194d35 100644\n --- a/grep.c\n +++ b/grep.c\n @@ -898,17 +898,6 @@ static int fixmatch(struct grep_pat *p, char *line, char *eol,\n \t}\n }\n \n -static int regmatch(const regex_t *preg, char *line, char *eol,\n -\t\t regmatch_t *match, int eflags)\n -{\n -#ifdef REG_STARTEND\n -\tmatch->rm_so = 0;\n -\tmatch->rm_eo = eol - line;\n -\teflags |= REG_STARTEND;\n -#endif\n -\treturn regexec(preg, line, 1, match, eflags);\n -}\n -\n static int patmatch(struct grep_pat *p, char *line, char *eol,\n \t\t regmatch_t *match, int eflags)\n {\n @@ -919,7 +908,8 @@ static int patmatch(struct grep_pat *p, char *line, char *eol,\n \telse if (p->pcre_regexp)\n \t\thit = !pcrematch(p, line, eol, match, eflags);\n \telse\n -\t\thit = !regmatch(&p->regexp, line, eol, match, eflags);\n +\t\thit = !regexec_buf(&p->regexp, line, eol - line, 1, match,\n +\t\t\t\t eflags);\n \n \treturn hit;\n }\n\n-- \n2.10.0.windows.1.10.g803177d\n\nbase-commit: f6727b0509ec3417a5183ba6e658143275a734f5"},{"id":"302314","messageId":"5d2418672a7761c9b514139ee9c53792d0cf638b.1474482164.git.johannes.schindelin@gmx.de","threadId":"44008","inReplyTo":"cover.1474482164.git.johannes.schindelin@gmx.de","subject":"[PATCH v4 1/3] regex: -G feeds a non NUL-terminated string to regexec() and fails","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-21T18:23:22Z","receivedAt":"2016-09-21T18:24:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"When our pickaxe code feeds file contents to regexec(), it implicitly\nassumes that the file contents are read into implicitly NUL-terminated\nbuffers (i.e. that we overallocate by 1, appending a single '\\0').\n\nThis is not so.\n\nIn particular when the file contents are simply mmap()ed, we can be\nvirtually certain that the buffer is preceding uninitialized bytes, or\ninvalid pages.\n\nNote that the test we add here is known to be flakey: we simply cannot\nknow whether the byte following the mmap()ed ones is a NUL or not.\n\nTypically, on Linux the test passes. On Windows, it fails virtually\nevery time due to an access violation (that's a segmentation fault for\nyou Unix-y people out there). And Windows would be correct: the\nregexec() call wants to operate on a regular, NUL-terminated string,\nthere is no NUL in the mmap()ed memory range, and it is undefined\nwhether the next byte is even legal to access.\n\nWhen run with --valgrind it demonstrates quite clearly the breakage, of\ncourse.\n\nBeing marked with `test_expect_failure`, this test will sometimes be\ndeclare \"TODO fixed\", even if it only passes by mistake.\n\nThis test case represents a Minimal, Complete and Verifiable Example of\na breakage reported by Chris Sidi.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t4061-diff-pickaxe.sh | 22 ++++++++++++++++++++++\n 1 file changed, 22 insertions(+)\n create mode 100755 t/t4061-diff-pickaxe.sh\n\ndiff --git a/t/t4061-diff-pickaxe.sh b/t/t4061-diff-pickaxe.sh\nnew file mode 100755\nindex 0000000..5929f2e\n--- /dev/null\n+++ b/t/t4061-diff-pickaxe.sh\n@@ -0,0 +1,22 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2016 Johannes Schindelin\n+#\n+\n+test_description='Pickaxe options'\n+\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\ttest_commit initial &&\n+\tprintf \"%04096d\" 0 >4096-zeroes.txt &&\n+\tgit add 4096-zeroes.txt &&\n+\ttest_tick &&\n+\tgit commit -m \"A 4k file\"\n+'\n+test_expect_failure '-G matches' '\n+\tgit diff --name-only -G \"^0{4096}$\" HEAD^ >out &&\n+\ttest 4096-zeroes.txt = \"$(cat out)\"\n+'\n+\n+test_done\n-- \n2.10.0.windows.1.10.g803177d\n\n\n"},{"id":"302315","messageId":"270cea11c4d8bfb332a6c014a11673b7f4666ee4.1474482164.git.johannes.schindelin@gmx.de","threadId":"44008","inReplyTo":"cover.1474482164.git.johannes.schindelin@gmx.de","subject":"[PATCH v4 2/3] regex: add regexec_buf() that can work on a non NUL-terminated string","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-21T18:24:04Z","receivedAt":"2016-09-21T18:24:24Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"We just introduced a test that demonstrates that our sloppy use of\nregexec() on a mmap()ed area can result in incorrect results or even\nhard crashes.\n\nSo what we need to fix this is a function that calls regexec() on a\nlength-delimited, rather than a NUL-terminated, string.\n\nHappily, there is an extension to regexec() introduced by the NetBSD\nproject and present in all major regex implementation including\nLinux', MacOSX' and the one Git includes in compat/regex/: by using\nthe (non-POSIX) REG_STARTEND flag, it is possible to tell the\nregexec() function that it should only look at the offsets between\npmatch[0].rm_so and pmatch[0].rm_eo.\n\nThat is exactly what we need.\n\nSince support for REG_STARTEND is so widespread by now, let's just\nintroduce a helper function that uses it, and fall back to allocating\nand constructing a NUL-terminated when REG_STARTEND is not available.\n\nSigned-off-by: Johannes Schindelin \n---\n Makefile | 3 ++-\n git-compat-util.h | 13 +++++++++++++\n 2 files changed, 15 insertions(+), 1 deletion(-)\n\ndiff --git a/Makefile b/Makefile\nindex df4f86b..c6f7f66 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -301,7 +301,8 @@ all::\n # crashes due to allocation and free working on different 'heaps'.\n # It's defined automatically if USE_NED_ALLOCATOR is set.\n #\n-# Define NO_REGEX if you have no or inferior regex support in your C library.\n+# Define NO_REGEX if your C library lacks regex support with REG_STARTEND\n+# feature.\n #\n # Define HAVE_DEV_TTY if your system can open /dev/tty to interact with the\n # user.\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 37cce07..8aab0c3 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -977,6 +977,19 @@ void git_qsort(void *base, size_t nmemb, size_t size,\n #define qsort git_qsort\n #endif\n \n+#ifndef REG_STARTEND\n+#error \"Git requires REG_STARTEND support. Compile with NO_REGEX=NeedsStartEnd\"\n+#endif\n+\n+static inline int regexec_buf(const regex_t *preg, const char *buf, size_t size,\n+\t\t\t size_t nmatch, regmatch_t pmatch[], int eflags)\n+{\n+\tassert(nmatch > 0 && pmatch);\n+\tpmatch[0].rm_so = 0;\n+\tpmatch[0].rm_eo = size;\n+\treturn regexec(preg, buf, nmatch, pmatch, eflags | REG_STARTEND);\n+}\n+\n #ifndef DIR_HAS_BSD_GROUP_SEMANTICS\n # define FORCE_DIR_SET_GID S_ISGID\n #else\n-- \n2.10.0.windows.1.10.g803177d\n\n\n"},{"id":"302316","messageId":"53f3609d99c865d59d7bfd8219a5334339e9e6bc.1474482164.git.johannes.schindelin@gmx.de","threadId":"44008","inReplyTo":"cover.1474482164.git.johannes.schindelin@gmx.de","subject":"[PATCH v4 3/3] regex: use regexec_buf()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-21T18:24:14Z","receivedAt":"2016-09-21T18:24:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"The new regexec_buf() function operates on buffers with an explicitly\nspecified length, rather than NUL-terminated strings.\n\nWe need to use this function whenever the buffer we want to pass to\nregexec() may have been mmap()ed (and is hence not NUL-terminated).\n\nNote: the original motivation for this patch was to fix a bug where\n`git diff -G ` would crash. This patch converts more callers,\nthough, some of which explicitly allocated and constructed\nNUL-terminated strings (or worse: modified read-only buffers to insert\nNULs).\n\nSome of the buffers actually may be NUL-terminated. As regexec_buf()\nuses REG_STARTEND where available, but has to fall back to allocating\nand constructing NUL-terminated strings where REG_STARTEND is not\navailable, this makes the code less efficient in the latter case.\n\nHowever, given the widespread support for REG_STARTEND, combined with\nthe improved ease of code maintenance, we strike the balance in favor\nof REG_STARTEND.\n\nSigned-off-by: Johannes Schindelin \n---\n diff.c | 3 ++-\n diffcore-pickaxe.c | 18 ++++++++----------\n grep.c | 14 ++------------\n t/t4061-diff-pickaxe.sh | 2 +-\n xdiff-interface.c | 13 ++++---------\n 5 files changed, 17 insertions(+), 33 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex c6da383..fb99235 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -952,7 +952,8 @@ static int find_word_boundaries(mmfile_t *buffer, regex_t *word_regex,\n {\n \tif (word_regex && *begin < buffer->size) {\n \t\tregmatch_t match[1];\n-\t\tif (!regexec(word_regex, buffer->ptr + *begin, 1, match, 0)) {\n+\t\tif (!regexec_buf(word_regex, buffer->ptr + *begin,\n+\t\t\t\t buffer->size - *begin, 1, match, 0)) {\n \t\t\tchar *p = memchr(buffer->ptr + *begin + match[0].rm_so,\n \t\t\t\t\t'\\n', match[0].rm_eo - match[0].rm_so);\n \t\t\t*end = p ? p - buffer->ptr : match[0].rm_eo + *begin;\ndiff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c\nindex 55067ca..9795ca1 100644\n--- a/diffcore-pickaxe.c\n+++ b/diffcore-pickaxe.c\n@@ -23,7 +23,6 @@ static void diffgrep_consume(void *priv, char *line, unsigned long len)\n {\n \tstruct diffgrep_cb *data = priv;\n \tregmatch_t regmatch;\n-\tint hold;\n \n \tif (line[0] != '+' && line[0] != '-')\n \t\treturn;\n@@ -33,11 +32,8 @@ static void diffgrep_consume(void *priv, char *line, unsigned long len)\n \t\t * caller early.\n \t\t */\n \t\treturn;\n-\t/* Yuck -- line ought to be \"const char *\"! */\n-\thold = line[len];\n-\tline[len] = '\\0';\n-\tdata->hit = !regexec(data->regexp, line + 1, 1, ®match, 0);\n-\tline[len] = hold;\n+\tdata->hit = !regexec_buf(data->regexp, line + 1, len - 1, 1,\n+\t\t\t\t ®match, 0);\n }\n \n static int diff_grep(mmfile_t *one, mmfile_t *two,\n@@ -50,9 +46,11 @@ static int diff_grep(mmfile_t *one, mmfile_t *two,\n \txdemitconf_t xecfg;\n \n \tif (!one)\n-\t\treturn !regexec(regexp, two->ptr, 1, ®match, 0);\n+\t\treturn !regexec_buf(regexp, two->ptr, two->size,\n+\t\t\t\t 1, ®match, 0);\n \tif (!two)\n-\t\treturn !regexec(regexp, one->ptr, 1, ®match, 0);\n+\t\treturn !regexec_buf(regexp, one->ptr, one->size,\n+\t\t\t\t 1, ®match, 0);\n \n \t/*\n \t * We have both sides; need to run textual diff and see if\n@@ -83,8 +81,8 @@ static unsigned int contains(mmfile_t *mf, regex_t *regexp, kwset_t kws)\n \t\tregmatch_t regmatch;\n \t\tint flags = 0;\n \n-\t\tassert(data[sz] == '\\0');\n-\t\twhile (*data && !regexec(regexp, data, 1, ®match, flags)) {\n+\t\twhile (*data &&\n+\t\t !regexec_buf(regexp, data, sz, 1, ®match, flags)) {\n \t\t\tflags |= REG_NOTBOL;\n \t\t\tdata += regmatch.rm_eo;\n \t\t\tif (*data && regmatch.rm_so == regmatch.rm_eo)\ndiff --git a/grep.c b/grep.c\nindex d7d00b8..1194d35 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -898,17 +898,6 @@ static int fixmatch(struct grep_pat *p, char *line, char *eol,\n \t}\n }\n \n-static int regmatch(const regex_t *preg, char *line, char *eol,\n-\t\t regmatch_t *match, int eflags)\n-{\n-#ifdef REG_STARTEND\n-\tmatch->rm_so = 0;\n-\tmatch->rm_eo = eol - line;\n-\teflags |= REG_STARTEND;\n-#endif\n-\treturn regexec(preg, line, 1, match, eflags);\n-}\n-\n static int patmatch(struct grep_pat *p, char *line, char *eol,\n \t\t regmatch_t *match, int eflags)\n {\n@@ -919,7 +908,8 @@ static int patmatch(struct grep_pat *p, char *line, char *eol,\n \telse if (p->pcre_regexp)\n \t\thit = !pcrematch(p, line, eol, match, eflags);\n \telse\n-\t\thit = !regmatch(&p->regexp, line, eol, match, eflags);\n+\t\thit = !regexec_buf(&p->regexp, line, eol - line, 1, match,\n+\t\t\t\t eflags);\n \n \treturn hit;\n }\ndiff --git a/t/t4061-diff-pickaxe.sh b/t/t4061-diff-pickaxe.sh\nindex 5929f2e..f0bf50b 100755\n--- a/t/t4061-diff-pickaxe.sh\n+++ b/t/t4061-diff-pickaxe.sh\n@@ -14,7 +14,7 @@ test_expect_success setup '\n \ttest_tick &&\n \tgit commit -m \"A 4k file\"\n '\n-test_expect_failure '-G matches' '\n+test_expect_success '-G matches' '\n \tgit diff --name-only -G \"^0{4096}$\" HEAD^ >out &&\n \ttest 4096-zeroes.txt = \"$(cat out)\"\n '\ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex 3bfc69c..060038c 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -214,11 +214,10 @@ struct ff_regs {\n static long ff_regexp(const char *line, long len,\n \t\tchar *buffer, long buffer_size, void *priv)\n {\n-\tchar *line_buffer;\n \tstruct ff_regs *regs = priv;\n \tregmatch_t pmatch[2];\n \tint i;\n-\tint result = -1;\n+\tint result;\n \n \t/* Exclude terminating newline (and cr) from matching */\n \tif (len > 0 && line[len-1] == '\\n') {\n@@ -228,18 +227,16 @@ static long ff_regexp(const char *line, long len,\n \t\t\tlen--;\n \t}\n \n-\tline_buffer = xstrndup(line, len); /* make NUL terminated */\n-\n \tfor (i = 0; i < regs->nr; i++) {\n \t\tstruct ff_reg *reg = regs->array + i;\n-\t\tif (!regexec(®->re, line_buffer, 2, pmatch, 0)) {\n+\t\tif (!regexec_buf(®->re, line, len, 2, pmatch, 0)) {\n \t\t\tif (reg->negate)\n-\t\t\t\tgoto fail;\n+\t\t\t\treturn -1;\n \t\t\tbreak;\n \t\t}\n \t}\n \tif (regs->nr <= i)\n-\t\tgoto fail;\n+\t\treturn -1;\n \ti = pmatch[1].rm_so >= 0 ? 1 : 0;\n \tline += pmatch[i].rm_so;\n \tresult = pmatch[i].rm_eo - pmatch[i].rm_so;\n@@ -248,8 +245,6 @@ static long ff_regexp(const char *line, long len,\n \twhile (result > 0 && (isspace(line[result - 1])))\n \t\tresult--;\n \tmemcpy(buffer, line, result);\n- fail:\n-\tfree(line_buffer);\n \treturn result;\n }\n \n-- \n2.10.0.windows.1.10.g803177d\n"},{"id":"302321","messageId":"xmqqtwd96p0b.fsf@gitster.mtv.corp.google.com","threadId":"44008","inReplyTo":"270cea11c4d8bfb332a6c014a11673b7f4666ee4.1474482164.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v4 2/3] regex: add regexec_buf() that can work on a non NUL-terminated string","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-21T19:17:56Z","receivedAt":"2016-09-21T19:18:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> ...\n> Happily, there is an extension to regexec() introduced by the NetBSD\n> project and present in all major regex implementation including\n> Linux', MacOSX' and the one Git includes in compat/regex/: by using\n> the (non-POSIX) REG_STARTEND flag, it is possible to tell the\n> regexec() function that it should only look at the offsets between\n> pmatch[0].rm_so and pmatch[0].rm_eo.\n>\n> That is exactly what we need.\n\nWonderful.\n\n> Since support for REG_STARTEND is so widespread by now, let's just\n> introduce a helper function that uses it, and fall back to allocating\n> and constructing a NUL-terminated when REG_STARTEND is not available.\n\nI'd somehow reword the last paragraph here, though ;-)\n\n Since support for REG_STARTEND is so widespread by now, let's just\n introduce a helper function that always uses it, and tell people\n on a platform whose regex library does not support it to use the\n one from our compat/regex/ directory.\n\nThe patch itself looks very sane. Thanks.\n"},{"id":"302322","messageId":"xmqqmvj16p06.fsf@gitster.mtv.corp.google.com","threadId":"44008","inReplyTo":"53f3609d99c865d59d7bfd8219a5334339e9e6bc.1474482164.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v4 3/3] regex: use regexec_buf()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-21T19:18:01Z","receivedAt":"2016-09-21T19:18:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> The new regexec_buf() function operates on buffers with an explicitly\n> specified length, rather than NUL-terminated strings.\n>\n> We need to use this function whenever the buffer we want to pass to\n> regexec() may have been mmap()ed (and is hence not NUL-terminated).\n>\n> Note: the original motivation for this patch was to fix a bug where\n> `git diff -G ` would crash. This patch converts more callers,\n> though, some of which explicitly allocated and constructed\n> NUL-terminated strings (or worse: modified read-only buffers to insert\n> NULs).\n>\n> Some of the buffers actually may be NUL-terminated. As regexec_buf()\n> uses REG_STARTEND where available, but has to fall back to allocating\n> and constructing NUL-terminated strings where REG_STARTEND is not\n> available, this makes the code less efficient in the latter case.\n>\n> However, given the widespread support for REG_STARTEND, combined with\n> the improved ease of code maintenance, we strike the balance in favor\n> of REG_STARTEND.\n\nThe last paragraph can go (2/3 was already justified separately),\nand the paragraph before that needs rewording, as you no longer do\nthe \"duplicate, run regexec, and free\" dance.\n\nWill comment on the patch text itself later.\n\nThanks for following it through. This topic actually fell under my\nradar until now.\n"},{"id":"302324","messageId":"xmqqintp6mm8.fsf@gitster.mtv.corp.google.com","threadId":"44008","inReplyTo":"xmqqmvj16p06.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4 3/3] regex: use regexec_buf()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-21T20:09:35Z","receivedAt":"2016-09-21T20:09:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Johannes Schindelin writes:\n>\n>> The new regexec_buf() function operates on buffers with an explicitly\n>> specified length, rather than NUL-terminated strings.\n>>\n>> We need to use this function whenever the buffer we want to pass to\n>> regexec() may have been mmap()ed (and is hence not NUL-terminated).\n>>\n>> Note: the original motivation for this patch was to fix a bug where\n>> `git diff -G ` would crash. This patch converts more callers,\n>> though, some of which explicitly allocated and constructed\n>> NUL-terminated strings (or worse: modified read-only buffers to insert\n>> NULs).\n\nAlso, I think there is nobody that modified read-only buffer.\ndiffgrep_consume() does say \"Yuck -- line ought to be const\",\nbut its \"line\" parameter is actually a non-const exactly for\nthis yuckiness (iow, it knew what it was doing).\n\nPerhaps like so?\n\n regex: use regexec_buf()\n \n The new regexec_buf() function operates on buffers with an explicitly\n specified length, rather than NUL-terminated strings.\n \n We need to use this function whenever the buffer we want to pass to\n regexec(3) may have been mmap(2)ed (and is hence not NUL-terminated).\n \n Note: the original motivation for this patch was to fix a bug where\n `git diff -G ` would crash. This patch converts more callers,\n though, some of which allocated to construct NUL-terminated strings,\n or worse, modified buffers to temporarily insert NULs while calling\n regexec(3). By converting them to use regexec_buf() they have become\n much cleaner.\n \n Signed-off-by: Johannes Schindelin \n Signed-off-by: Junio C Hamano \n\nThe patch text looked good to me.\n\nThanks.\n"},{"id":"302333","messageId":"20160921220310.5wk76qdsjyhvstk4@sigill.intra.peff.net","threadId":"44008","inReplyTo":"53f3609d99c865d59d7bfd8219a5334339e9e6bc.1474482164.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v4 3/3] regex: use regexec_buf()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-21T22:03:10Z","receivedAt":"2016-09-21T22:03:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 21, 2016 at 08:24:14PM +0200, Johannes Schindelin wrote:\n\n> The new regexec_buf() function operates on buffers with an explicitly\n> specified length, rather than NUL-terminated strings.\n> \n> We need to use this function whenever the buffer we want to pass to\n> regexec() may have been mmap()ed (and is hence not NUL-terminated).\n> \n> Note: the original motivation for this patch was to fix a bug where\n> `git diff -G ` would crash. This patch converts more callers,\n> though, some of which explicitly allocated and constructed\n> NUL-terminated strings (or worse: modified read-only buffers to insert\n> NULs).\n\nNice. I probably would have split these into their own patch, but I\nthink it is OK here.\n\n> @@ -228,18 +227,16 @@ static long ff_regexp(const char *line, long len,\n> \t\t\tlen--;\n> \t}\n> \n> -\tline_buffer = xstrndup(line, len); /* make NUL terminated */\n> -\n\nNice to see this one going away in particular, since it's called quite a\nlot. According to perf, \"git log -p\" on git.git drops about 1.5 million\nmalloc calls (about 9% of the total). And here are best-of-five results\nfor that same command:\n\n [before]\n real 0m14.676s\n user 0m13.988s\n sys 0m0.676s\n\n [after]\n real 0m14.394s\n user 0m13.624s\n sys 0m0.760s\n\nNot a _huge_ improvement, but more significant than the run-to-run\nnoise.\n\n-Peff\n"},{"id":"302334","messageId":"20160921220413.5iedndabx7v6wdl7@sigill.intra.peff.net","threadId":"44008","inReplyTo":"cover.1474482164.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v4 0/3] Fix a segfault caused by regexec() being called on mmap()ed data","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-21T22:04:13Z","receivedAt":"2016-09-21T22:04:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 21, 2016 at 08:23:11PM +0200, Johannes Schindelin wrote:\n\n> We solve this by introducing a helper, regexec_buf(), that takes a\n> pointer and a length instead of a NUL-terminated string.\n> \n> This helper then uses REG_STARTEND where available, and falls back to\n> allocating and constructing a NUL-terminated string. Given the\n> wide-spread support for REG_STARTEND (Linux has it, MacOSX has it, Git\n> for Windows has it because it uses compat/regex/ that has it), I think\n> this is a fair trade-off.\n\nI did a double-take on this, but then read:\n\n> Changes since v3:\n> [...]\n> - removed fallback when REG_STARTEND is not supported, in favor of\n> requiring NO_REGEX.\n\nSo I think we are all in agreement. :)\n\nWith the exception of a few commit message fixups that Junio already\npointed out, this looks good to me. Thanks.\n\n-Peff\n"},{"id":"302386","messageId":"alpine.DEB.2.20.1609222032330.129229@virtualbox","threadId":"44008","inReplyTo":"xmqqtwd96p0b.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4 2/3] regex: add regexec_buf() that can work on a non NUL-terminated string","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-22T18:38:00Z","receivedAt":"2016-09-22T18:38:59Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 21 Sep 2016, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > ...\n> > Happily, there is an extension to regexec() introduced by the NetBSD\n> > project and present in all major regex implementation including\n> > Linux', MacOSX' and the one Git includes in compat/regex/: by using\n> > the (non-POSIX) REG_STARTEND flag, it is possible to tell the\n> > regexec() function that it should only look at the offsets between\n> > pmatch[0].rm_so and pmatch[0].rm_eo.\n> >\n> > That is exactly what we need.\n> \n> Wonderful.\n> \n> > Since support for REG_STARTEND is so widespread by now, let's just\n> > introduce a helper function that uses it, and fall back to allocating\n> > and constructing a NUL-terminated when REG_STARTEND is not available.\n> \n> I'd somehow reword the last paragraph here, though ;-)\n\nOh drats. I thought I had prepared the fixed commit messages already :-(\n\nWhat you have in `pu` as of today looks good.\n\nCiao,\nDscho\n"},{"id":"302515","messageId":"alpine.DEB.2.20.1609251600120.129229@virtualbox","threadId":"44008","inReplyTo":"20160921220310.5wk76qdsjyhvstk4@sigill.intra.peff.net","subject":"Re: [PATCH v4 3/3] regex: use regexec_buf()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-25T14:01:54Z","receivedAt":"2016-09-25T14:02:33Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Wed, 21 Sep 2016, Jeff King wrote:\n\n> On Wed, Sep 21, 2016 at 08:24:14PM +0200, Johannes Schindelin wrote:\n> \n> > The new regexec_buf() function operates on buffers with an explicitly\n> > specified length, rather than NUL-terminated strings.\n> > \n> > We need to use this function whenever the buffer we want to pass to\n> > regexec() may have been mmap()ed (and is hence not NUL-terminated).\n> > \n> > Note: the original motivation for this patch was to fix a bug where\n> > `git diff -G ` would crash. This patch converts more callers,\n> > though, some of which explicitly allocated and constructed\n> > NUL-terminated strings (or worse: modified read-only buffers to insert\n> > NULs).\n> \n> Nice. I probably would have split these into their own patch, but I\n> think it is OK here.\n\nTrue. I guess I was a little lazy...\n\n> > @@ -228,18 +227,16 @@ static long ff_regexp(const char *line, long len,\n> > \t\t\tlen--;\n> > \t}\n> > \n> > -\tline_buffer = xstrndup(line, len); /* make NUL terminated */\n> > -\n> \n> Nice to see this one going away in particular, since it's called quite a\n> lot. According to perf, \"git log -p\" on git.git drops about 1.5 million\n> malloc calls (about 9% of the total). And here are best-of-five results\n> for that same command:\n> \n> [before]\n> real 0m14.676s\n> user 0m13.988s\n> sys 0m0.676s\n> \n> [after]\n> real 0m14.394s\n> user 0m13.624s\n> sys 0m0.760s\n> \n> Not a _huge_ improvement, but more significant than the run-to-run\n> noise.\n\nOh, that's nice! As you guessed, my aim was not to improve performance,\nbut it is a pretty side effect...\n\nThanks for testing!\nDscho\n"}]}