* Re: [PATCH] t6026-merge-attr: wait for process to release trash directory
From: Johannes Schindelin @ 2016-09-08 8:05 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Johannes Sixt, Git Mailing List
In-Reply-To: <20160907190004.dw3p6fxkdaubwuvu@sigill.intra.peff.net>
Hi Peff & Junio,
On Wed, 7 Sep 2016, Jeff King wrote:
> On Wed, Sep 07, 2016 at 11:39:57AM -0700, Junio C Hamano wrote:
>
> > > Can we do some signaling with fifos to tell the hook when it is safe to
> > > exit? Then we would just need to `wait` for its parent process.
> >
> > Is fifo safe on Windows, though?
>
> No clue. We seem to use mkfifo unconditionally in lib-daemon, but
> perhaps people do not run that test on Windows. Other invocations seem
> to be protected by the PIPE prerequisite. But...
AFAICT we do not use mkfifo on Windows. Let's see what t/test-lib.sh has
to say about the matter:
test_lazy_prereq PIPE '
# test whether the filesystem supports FIFOs
case $(uname -s) in
CYGWIN*|MINGW*)
false
;;
*)
rm -f testfifo && mkfifo testfifo
;;
esac
'
So there you go.
The reason it is disabled is that Cygwin/MSYS2 do have a concept of a
FIFO. But `git.exe` won't be able to access such a FIFO because it is
emulated by the POSIX emulation layer, which Git cannot access.
> > With v2 that explicitly kills, I guess we can make the sleep longer
> > without slowing down in the optimistic case?
>
> Yeah, I think the v2 one is non-racy (I thought at first we might race
> with the "echo", but it should be synchronous; the hook will not exit
> until we have written the pid file, and git will not exit until the hook
> is done running).
Please note that Hannes and I discussed this (as I originally suggested to
increase it to 10 seconds, and Hannes rightfully pointed out that we would
have to change the script name, too, as it says sleep-one-second.sh, and
that would have made the patch less readable) and we came to the same
conclusion: it's not necessary.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH v2 2/3] Introduce a function to run regexec() on non-NUL-terminated buffers
From: Jeff King @ 2016-09-08 8:04 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano
In-Reply-To: <94ee698b2736929d37640012a1b1735b134dd3d6.1473319844.git.johannes.schindelin@gmx.de>
On Thu, Sep 08, 2016 at 09:31:11AM +0200, Johannes Schindelin wrote:
> diff --git a/git-compat-util.h b/git-compat-util.h
> index db89ba7..19128b3 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -965,6 +965,27 @@ void git_qsort(void *base, size_t nmemb, size_t size,
> #define qsort git_qsort
> #endif
>
> +static inline int regexec_buf(const regex_t *preg, const char *buf, size_t size,
> + size_t nmatch, regmatch_t pmatch[], int eflags)
> +{
> +#ifdef REG_STARTEND
> + assert(nmatch > 0 && pmatch);
> + pmatch[0].rm_so = 0;
> + pmatch[0].rm_eo = size;
> + return regexec(preg, buf, nmatch, pmatch, eflags | REG_STARTEND);
> +#else
> + char *buf2 = xmalloc(size + 1);
> + int ret;
> +
> + memcpy(buf2, buf, size);
> + buf2[size] = '\0';
I mentioned elsewhere that I'd prefer we just push people into using
compat/regex if they don't have REG_STARTEND. But if we _do_ keep this
fallback, note that the above has a buffer overflow (think what happens
when "size" is the maximum value for a size_t). You can avoid it by
using xmallocz().
-Peff
^ permalink raw reply
* Re: [PATCH 0/3] Fix a segfault caused by regexec() being called on mmap()ed data
From: Jeff King @ 2016-09-08 8:00 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, git
In-Reply-To: <alpine.DEB.2.20.1609080921030.129229@virtualbox>
On Thu, Sep 08, 2016 at 09:29:58AM +0200, Johannes Schindelin wrote:
> sorry for the late answer, I was really busy trying to come up with a new
> and improved version of the patch series, and while hunting a bug I
> introduced got bogged down with other tasks.
No problem. I am not in a hurry.
> > I always assumed the _point_ of re_search taking a ptr/len pair was
> > exactly to handle this case. The documentation[1] says:
> >
> > `string` is the string you want to match; it can contain newline and
> > null characters. `size` is the length of that string.
> >
> > Which seems pretty definitive to me (that's for re_match(), but
> > re_search() is defined in the docs in terms of re_match()).
>
> Right. The problem is: I *really* want to avoid using GNU-isms.
I don't think GNU-isms are a problem if we wrap them to give a nice
interface, and if we rely on having compat/regex. But if you mean "I do
not want to rely on using compat/regex everywhere", then OK. I can see
arguments both for and against using a consistent regex library, but I
do not care that much either way myself.
> > We can contain this to the existing compat/regexec/regexec.c, and just
> > provide a wrapper that is similar to regexec but takes a ptr/len pair.
>
> But we can do even better than that: we can provide a wrapper that uses
> REG_STARTEND where available (which is really the majority of platforms we
> care about: Linux, MacOSX, Windows, and even the *BSDs). Where it is not
> available, we simply malloc(), memcpy() and append a NUL.
Doesn't that make things much _worse_ for people on systems without
REG_STARTEND? If we imagine that most regexec calls would operate on a
NUL-terminated buffer, then they are now paying the extra malloc and
copy for each call to regexec_buf(), even if the buffer was already
NUL-terminated (because they have no idea whether it was or not).
I think I'd rather just have:
#ifndef REG_STARTEND
#error "Your regex library sucks. Compile with NO_REGEX=NeedsStartEnd"
#endif
(or you could just use REG_STARTEND and let the compiler complain, but
then the user has to figure out the right knob to twiddle).
One other question about REG_STARTEND is: what does it do with NULs
inside the buffer? Certainly glibc (and our compat/regex) treat it as a
buffer with a particular length and ignore embedded NULs, as we want.
But the NetBSD documentation says only:
REG_STARTEND The string is considered to start at string +
pmatch[0].rm_so and to have a terminating NUL
located at string + pmatch[0].rm_eo (there need not
actually be a NUL at that location),
Besides avoiding a segfault, one of the benefits of regcomp_buf() is
that we will now find pickaxe-regex strings inside mixed binary/text
files. But it's not clear to me that NetBSD's implementation does this.
I guess we can assume it is fine (it is certainly no _worse_ than the
current behavior), and if people's platforms do not handle it, they can
build with NO_REGEX.
-Peff
^ permalink raw reply
* [PATCH v3 3/3] Use the newly-introduced regexec_buf() function
From: Johannes Schindelin @ 2016-09-08 7:59 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <cover.1473321437.git.johannes.schindelin@gmx.de>
The new regexec_buf() function operates on buffers with an explicitly
specified length, rather than NUL-terminated strings.
We need to use this function whenever the buffer we want to pass to
regexec() may have been mmap()ed (and is hence not NUL-terminated).
Note: the original motivation for this patch was to fix a bug where
`git diff -G <regex>` would crash. This patch converts more callers,
though, some of which explicitly allocated and constructed
NUL-terminated strings (or worse: modified read-only buffers to insert
NULs).
Some of the buffers actually may be NUL-terminated. As regexec_buf()
uses REG_STARTEND where available, but has to fall back to allocating
and constructing NUL-terminated strings where REG_STARTEND is not
available, this makes the code less efficient in the latter case.
However, given the widespread support for REG_STARTEND, combined with
the improved ease of code maintenance, we strike the balance in favor
of REG_STARTEND.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
diff.c | 3 ++-
diffcore-pickaxe.c | 18 ++++++++----------
t/t4061-diff-pickaxe.sh | 2 +-
xdiff-interface.c | 13 ++++---------
4 files changed, 15 insertions(+), 21 deletions(-)
diff --git a/diff.c b/diff.c
index 534c12e..526775a 100644
--- a/diff.c
+++ b/diff.c
@@ -951,7 +951,8 @@ static int find_word_boundaries(mmfile_t *buffer, regex_t *word_regex,
{
if (word_regex && *begin < buffer->size) {
regmatch_t match[1];
- if (!regexec(word_regex, buffer->ptr + *begin, 1, match, 0)) {
+ if (!regexec_buf(word_regex, buffer->ptr + *begin,
+ buffer->size - *begin, 1, match, 0)) {
char *p = memchr(buffer->ptr + *begin + match[0].rm_so,
'\n', match[0].rm_eo - match[0].rm_so);
*end = p ? p - buffer->ptr : match[0].rm_eo + *begin;
diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 55067ca..9795ca1 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -23,7 +23,6 @@ static void diffgrep_consume(void *priv, char *line, unsigned long len)
{
struct diffgrep_cb *data = priv;
regmatch_t regmatch;
- int hold;
if (line[0] != '+' && line[0] != '-')
return;
@@ -33,11 +32,8 @@ static void diffgrep_consume(void *priv, char *line, unsigned long len)
* caller early.
*/
return;
- /* Yuck -- line ought to be "const char *"! */
- hold = line[len];
- line[len] = '\0';
- data->hit = !regexec(data->regexp, line + 1, 1, ®match, 0);
- line[len] = hold;
+ data->hit = !regexec_buf(data->regexp, line + 1, len - 1, 1,
+ ®match, 0);
}
static int diff_grep(mmfile_t *one, mmfile_t *two,
@@ -50,9 +46,11 @@ static int diff_grep(mmfile_t *one, mmfile_t *two,
xdemitconf_t xecfg;
if (!one)
- return !regexec(regexp, two->ptr, 1, ®match, 0);
+ return !regexec_buf(regexp, two->ptr, two->size,
+ 1, ®match, 0);
if (!two)
- return !regexec(regexp, one->ptr, 1, ®match, 0);
+ return !regexec_buf(regexp, one->ptr, one->size,
+ 1, ®match, 0);
/*
* We have both sides; need to run textual diff and see if
@@ -83,8 +81,8 @@ static unsigned int contains(mmfile_t *mf, regex_t *regexp, kwset_t kws)
regmatch_t regmatch;
int flags = 0;
- assert(data[sz] == '\0');
- while (*data && !regexec(regexp, data, 1, ®match, flags)) {
+ while (*data &&
+ !regexec_buf(regexp, data, sz, 1, ®match, flags)) {
flags |= REG_NOTBOL;
data += regmatch.rm_eo;
if (*data && regmatch.rm_so == regmatch.rm_eo)
diff --git a/t/t4061-diff-pickaxe.sh b/t/t4061-diff-pickaxe.sh
index 5929f2e..f0bf50b 100755
--- a/t/t4061-diff-pickaxe.sh
+++ b/t/t4061-diff-pickaxe.sh
@@ -14,7 +14,7 @@ test_expect_success setup '
test_tick &&
git commit -m "A 4k file"
'
-test_expect_failure '-G matches' '
+test_expect_success '-G matches' '
git diff --name-only -G "^0{4096}$" HEAD^ >out &&
test 4096-zeroes.txt = "$(cat out)"
'
diff --git a/xdiff-interface.c b/xdiff-interface.c
index f34ea76..50702a2 100644
--- a/xdiff-interface.c
+++ b/xdiff-interface.c
@@ -214,11 +214,10 @@ struct ff_regs {
static long ff_regexp(const char *line, long len,
char *buffer, long buffer_size, void *priv)
{
- char *line_buffer;
struct ff_regs *regs = priv;
regmatch_t pmatch[2];
int i;
- int result = -1;
+ int result;
/* Exclude terminating newline (and cr) from matching */
if (len > 0 && line[len-1] == '\n') {
@@ -228,18 +227,16 @@ static long ff_regexp(const char *line, long len,
len--;
}
- line_buffer = xstrndup(line, len); /* make NUL terminated */
-
for (i = 0; i < regs->nr; i++) {
struct ff_reg *reg = regs->array + i;
- if (!regexec(®->re, line_buffer, 2, pmatch, 0)) {
+ if (!regexec_buf(®->re, line, len, 2, pmatch, 0)) {
if (reg->negate)
- goto fail;
+ return -1;
break;
}
}
if (regs->nr <= i)
- goto fail;
+ return -1;
i = pmatch[1].rm_so >= 0 ? 1 : 0;
line += pmatch[i].rm_so;
result = pmatch[i].rm_eo - pmatch[i].rm_so;
@@ -248,8 +245,6 @@ static long ff_regexp(const char *line, long len,
while (result > 0 && (isspace(line[result - 1])))
result--;
memcpy(buffer, line, result);
- fail:
- free(line_buffer);
return result;
}
--
2.10.0.windows.1.10.g803177d
^ permalink raw reply related
* [PATCH v3 2/3] Introduce a function to run regexec() on non-NUL-terminated buffers
From: Johannes Schindelin @ 2016-09-08 7:58 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <cover.1473321437.git.johannes.schindelin@gmx.de>
We just introduced a test that demonstrates that our sloppy use of
regexec() on a mmap()ed area can result in incorrect results or even
hard crashes.
So what we need to fix this is a function that calls regexec() on a
length-delimited, rather than a NUL-terminated, string.
Happily, there is an extension to regexec() introduced by the NetBSD
project and present in all major regex implementation including
Linux', MacOSX' and the one Git includes in compat/regex/: by using
the (non-POSIX) REG_STARTEND flag, it is possible to tell the
regexec() function that it should only look at the offsets between
pmatch[0].rm_so and pmatch[0].rm_eo.
That is exactly what we need.
Since support for REG_STARTEND is so widespread by now, let's just
introduce a helper function that uses it, and fall back to allocating
and constructing a NUL-terminated when REG_STARTEND is not available.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
git-compat-util.h | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/git-compat-util.h b/git-compat-util.h
index db89ba7..19128b3 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -965,6 +965,27 @@ void git_qsort(void *base, size_t nmemb, size_t size,
#define qsort git_qsort
#endif
+static inline int regexec_buf(const regex_t *preg, const char *buf, size_t size,
+ size_t nmatch, regmatch_t pmatch[], int eflags)
+{
+#ifdef REG_STARTEND
+ assert(nmatch > 0 && pmatch);
+ pmatch[0].rm_so = 0;
+ pmatch[0].rm_eo = size;
+ return regexec(preg, buf, nmatch, pmatch, eflags | REG_STARTEND);
+#else
+ char *buf2 = xmalloc(size + 1);
+ int ret;
+
+ memcpy(buf2, buf, size);
+ buf2[size] = '\0';
+ ret = regexec(preg, buf2, nmatch, pmatch, eflags);
+ free(buf2);
+
+ return ret;
+#endif
+}
+
#ifndef DIR_HAS_BSD_GROUP_SEMANTICS
# define FORCE_DIR_SET_GID S_ISGID
#else
--
2.10.0.windows.1.10.g803177d
^ permalink raw reply related
* [PATCH v3 1/3] Demonstrate a problem: our pickaxe code assumes NUL-terminated buffers
From: Johannes Schindelin @ 2016-09-08 7:57 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <cover.1473321437.git.johannes.schindelin@gmx.de>
When our pickaxe code feeds file contents to regexec(), it implicitly
assumes that the file contents are read into implicitly NUL-terminated
buffers (i.e. that we overallocate by 1, appending a single '\0').
This is not so.
In particular when the file contents are simply mmap()ed, we can be
virtually certain that the buffer is preceding uninitialized bytes, or
invalid pages.
Note that the test we add here is known to be flakey: we simply cannot
know whether the byte following the mmap()ed ones is a NUL or not.
Typically, on Linux the test passes. On Windows, it fails virtually
every time due to an access violation (that's a segmentation fault for
you Unix-y people out there). And Windows would be correct: the
regexec() call wants to operate on a regular, NUL-terminated string,
there is no NUL in the mmap()ed memory range, and it is undefined
whether the next byte is even legal to access.
When run with --valgrind it demonstrates quite clearly the breakage, of
course.
Being marked with `test_expect_failure`, this test will sometimes be
declare "TODO fixed", even if it only passes by mistake.
This test case represents a Minimal, Complete and Verifiable Example of
a breakage reported by Chris Sidi.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
t/t4061-diff-pickaxe.sh | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
create mode 100755 t/t4061-diff-pickaxe.sh
diff --git a/t/t4061-diff-pickaxe.sh b/t/t4061-diff-pickaxe.sh
new file mode 100755
index 0000000..5929f2e
--- /dev/null
+++ b/t/t4061-diff-pickaxe.sh
@@ -0,0 +1,22 @@
+#!/bin/sh
+#
+# Copyright (c) 2016 Johannes Schindelin
+#
+
+test_description='Pickaxe options'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+ test_commit initial &&
+ printf "%04096d" 0 >4096-zeroes.txt &&
+ git add 4096-zeroes.txt &&
+ test_tick &&
+ git commit -m "A 4k file"
+'
+test_expect_failure '-G matches' '
+ git diff --name-only -G "^0{4096}$" HEAD^ >out &&
+ test 4096-zeroes.txt = "$(cat out)"
+'
+
+test_done
--
2.10.0.windows.1.10.g803177d
^ permalink raw reply related
* [PATCH v3 0/3] Fix a segfault caused by regexec() being called on mmap()ed data
From: Johannes Schindelin @ 2016-09-08 7:57 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <cover.1473319844.git.johannes.schindelin@gmx.de>
This patch series addresses a problem where `git diff` is called using
`-G` or `-S --pickaxe-regex` on new-born files that are configured
without user diff drivers, and that hence get mmap()ed into memory.
The problem with that: mmap()ed memory is *not* NUL-terminated, yet the
pickaxe code calls regexec() on it just the same.
This problem has been reported by my colleague Chris Sidi.
We solve this by introducing a helper, regexec_buf(), that takes a
pointer and a length instead of a NUL-terminated string.
This helper then uses REG_STARTEND where available, and falls back to
allocating and constructing a NUL-terminated string. Given the
wide-spread support for REG_STARTEND (Linux has it, MacOSX has it, Git
for Windows has it because it uses compat/regex/ that has it), I think
this is a fair trade-off.
Changes since v2:
- changed 3/3 to switch the test_expect_failure from 1/3 to a
test_expect_success
Johannes Schindelin (3):
Demonstrate a problem: our pickaxe code assumes NUL-terminated buffers
Introduce a function to run regexec() on non-NUL-terminated buffers
Use the newly-introduced regexec_buf() function
diff.c | 3 ++-
diffcore-pickaxe.c | 18 ++++++++----------
git-compat-util.h | 21 +++++++++++++++++++++
t/t4061-diff-pickaxe.sh | 22 ++++++++++++++++++++++
xdiff-interface.c | 13 ++++---------
5 files changed, 57 insertions(+), 20 deletions(-)
create mode 100755 t/t4061-diff-pickaxe.sh
Published-As: https://github.com/dscho/git/releases/tag/mmap-regexec-v3
Fetch-It-Via: git fetch https://github.com/dscho/git mmap-regexec-v3
Interdiff vs v2:
diff --git a/t/t4061-diff-pickaxe.sh b/t/t4061-diff-pickaxe.sh
index 5929f2e..f0bf50b 100755
--- a/t/t4061-diff-pickaxe.sh
+++ b/t/t4061-diff-pickaxe.sh
@@ -14,7 +14,7 @@ test_expect_success setup '
test_tick &&
git commit -m "A 4k file"
'
-test_expect_failure '-G matches' '
+test_expect_success '-G matches' '
git diff --name-only -G "^0{4096}$" HEAD^ >out &&
test 4096-zeroes.txt = "$(cat out)"
'
--
2.10.0.windows.1.10.g803177d
base-commit: 6ebdac1bab966b720d776aa43ca188fe378b1f4b
^ permalink raw reply
* Re: [PATCH v2 3/3] Use the newly-introduced regexec_buf() function
From: Johannes Schindelin @ 2016-09-08 7:54 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <324ecba64eb0436988aca846fb444eafda290d13.1473319844.git.johannes.schindelin@gmx.de>
Hi,
On Thu, 8 Sep 2016, Johannes Schindelin wrote:
> The new regexec_buf() function operates on buffers with an explicitly
> specified length, rather than NUL-terminated strings.
>
> We need to use this function whenever the buffer we want to pass to
> regexec() may have been mmap()ed (and is hence not NUL-terminated).
>
> Note: the original motivation for this patch was to fix a bug where
> `git diff -G <regex>` would crash. This patch converts more callers,
> though, some of which explicitly allocated and constructed
> NUL-terminated strings (or worse: modified read-only buffers to insert
> NULs).
>
> Some of the buffers actually may be NUL-terminated. As regexec_buf()
> uses REG_STARTEND where available, but has to fall back to allocating
> and constructing NUL-terminated strings where REG_STARTEND is not
> available, this makes the code less efficient in the latter case.
>
> However, given the widespread support for REG_STARTEND, combined with
> the improved ease of code maintenance, we strike the balance in favor
> of REG_STARTEND.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> diff.c | 3 ++-
> diffcore-pickaxe.c | 18 ++++++++----------
> xdiff-interface.c | 13 ++++---------
> 3 files changed, 14 insertions(+), 20 deletions(-)
I just realized that this should switch the test_expect_failure from 1/3
to a test_expect_success.
Will send out v3 in a moment.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 1/3] Demonstrate a problem: our pickaxe code assumes NUL-terminated buffers
From: Johannes Schindelin @ 2016-09-08 7:53 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
In-Reply-To: <20160906184320.lzg5jizpw2kbzf72@sigill.intra.peff.net>
Hi Peff,
On Tue, 6 Sep 2016, Jeff King wrote:
> On Mon, Sep 05, 2016 at 05:45:02PM +0200, Johannes Schindelin wrote:
>
> > Typically, on Linux the test passes. On Windows, it fails virtually
> > every time due to an access violation (that's a segmentation fault for
> > you Unix-y people out there). And Windows would be correct: the
> > regexec() call wants to operate on a regular, NUL-terminated string,
> > there is no NUL in the mmap()ed memory range, and it is undefined
> > whether the next byte is even legal to access.
> >
> > When run with --valgrind it demonstrates quite clearly the breakage, of
> > course.
> >
> > So we simply mark it with `test_expect_success` for now.
>
> I'd prefer if this were marked as expect_failure. It fails reliably for
> me on Linux, even without --valgrind. But even if that were not so,
> there is no reason to hurt bisectability of somebody running with
> "--valgrind" (not when it costs so little to mark it correctly).
Forgot to say in the cover letter: I did change this from
test_expect_success to test_expect_failure.
But of course, now I remember that I failed to change it back in 3/3. Bah.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 2/3] diff_populate_filespec: NUL-terminate buffers
From: Johannes Schindelin @ 2016-09-08 7:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <xmqqinu7muic.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Wed, 7 Sep 2016, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > What happens to those poor souls on systems without REG_STARTEND? Do
> > they get to keep segfaulting?
> >
> > I think the solution is to push them into setting NO_REGEX. So looking
> > at this versus a "regexecn", it seems:
> >
> > - this lets people keep using their native regexec if it supports
> > STARTEND
> >
> > - this is a bit more clunky to use at the callsites (though we could
> > _create_ a portable regexecn wrapper that uses this technique on top
> > of the native regex library)
> >
> > But I much prefer this approach to copying the data just to add a NUL.
>
> I first thought "push them to NO_REGEX" to mean "they live with
> crippled Git that does not do regexp" and went "Huh?", but it merely
> means "let's avoid platform regex library and use on from the
> compat/ hierarchy", which would solve the STARTEND portability issue
> for everybody.
>
> Which is very good.
>
> The idea to create a thin regexecn() wrapper also sounds like a good
> idea, too. The changes to the callsites in the demonstration patch
> does look a bit clunky to me, too.
The demonstration patch was only meant as a mere demonstration where this
leads us. I DRY'd it up quite a bit (which was my plan all along, but it
was faster to make the changes in place, to avoid a full-sale
recompilation due to a central header change; you might not care because
you use Linux with its native POSIX, while I have to use MSYS2, making
even my builds slower).
And I really do not think that it would be a good idea to use
compat/regex/ for everybody, even if they already have a working regex.h
on their system.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 2/3] diff_populate_filespec: NUL-terminate buffers
From: Johannes Schindelin @ 2016-09-08 7:49 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
In-Reply-To: <20160906184143.55a5zoa2mj6c2e5m@sigill.intra.peff.net>
Hi Peff,
On Tue, 6 Sep 2016, Jeff King wrote:
> On Tue, Sep 06, 2016 at 06:02:59PM +0200, Johannes Schindelin wrote:
>
> > It will still be quite tricky, because we have to touch a function that is
> > rather at the bottom of the food chain: diff_populate_filespec() is called
> > from fill_textconv(), which in turn is called from pickaxe_match(), and
> > only pickaxe_match() knows whether we want to call regexec() or not (it
> > depends on its regexp parameter).
> >
> > Adding a flag to diff_populate_filespec() sounds really reasonable until
> > you see how many call sites fill_textconv() has.
>
> I was thinking of something quite gross, like a global "switch to using
> slower-but-safer NUL termination" flag (but I agree with Junio's point
> elsewhere that we do not even know if it is "slower").
Urgh.
;-)
> > So now for the better idea.
> >
> > While I was researching the code for this reply, I hit upon one thing
> > that I never knew existed, introduced in f96e567 (grep: use
> > REG_STARTEND for all matching if available, 2010-05-22). Apparently,
> > NetBSD introduced an extension to regexec() where you can specify
> > buffer boundaries using REG_STARTEND. Which is pretty much what we
> > need.
>
> Yes, and compat/regex support this, too. My question is whether it is
> portable.
That is only one question.
Another, important question is: is it efficient?
I have no idea whether there exists any hardware-accelerated regex library
out there, maybe even using CUDA (I know that there is some code out there
using SSE to perform LF -> CR/LF conversion, unfortunately it is
intentionally incompatible with GPLv2).
We cannot simply switch everybody and her dog to compat/regex/ just
because we want to avoid a segfault.
> > diff --git a/diff.c b/diff.c
> > index 534c12e..2c5a360 100644
> > --- a/diff.c
> > +++ b/diff.c
> > @@ -951,7 +951,13 @@ static int find_word_boundaries(mmfile_t *buffer,
> > regex_t *word_regex,
> > {
> > if (word_regex && *begin < buffer->size) {
> > regmatch_t match[1];
> > - if (!regexec(word_regex, buffer->ptr + *begin, 1, match,
> > 0)) {
> > + int f = 0;
> > +#ifdef REG_STARTEND
> > + match[0].rm_so = 0;
> > + match[0].rm_eo = *end - *begin;
> > + f = REG_STARTEND;
> > +#endif
> > + if (!regexec(word_regex, buffer->ptr + *begin, 1, match,
> > f)) {
Heh. You introduced the same bug I did. Or maybe you just fetched my
mmap-regexec branch and looked at an intermediate iteration?
The problem with this patch is that *end is uninitialized. I actually
initialized it in my patch, but it was still incorrect. I settled on using
buffer->size - *begin in the end.
> What happens to those poor souls on systems without REG_STARTEND? Do
> they get to keep segfaulting?
Of course not. Those poor souls on systems without REG_STARTEND pay a
little price for that: malloc(); memcpy(); *end = '\0'; ... free();
I think it is worth it: maintenance of the code is much easier that way
than forcing everybody and her dog and her dog's hamster to compat/regex/.
> But I much prefer this approach to copying the data just to add a NUL.
I think it is not worth the burden. The only regex implementation in
semi-widespread use that do not support REG_STARTEND seems to be musl.
I'd rather not spend *so much* effort just to support an obscure platform.
Not when the users of that obscure platform could spend that effort
themselves. And probably won't, because we only copy data to add a NUL on
those platforms when regexec() is called on an mmfile_t.
Better to keep it simple,
Dscho
^ permalink raw reply
* Re: [PATCH v2 0/3] Fix a segfault caused by regexec() being called on mmap()ed data
From: Johannes Schindelin @ 2016-09-08 7:33 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <cover.1473319844.git.johannes.schindelin@gmx.de>
Hi,
On Thu, 8 Sep 2016, Johannes Schindelin wrote:
> We solve this by introducing a helper, regexec_buf(), that takes a
> pointer and a length instead of a NUL-terminated string.
BTW I should have clarified why I decided on another name than regexecn()
(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
words, if n = 100 and the provided pointer points to a NUL-terminated
string of length 3, the *n function will treat it as a string of length 3.
That is not what regexec_buf() does: it ignores the NUL. Hence the
different name.
Ciao,
Dscho
^ permalink raw reply
* [PATCH v2 3/3] Use the newly-introduced regexec_buf() function
From: Johannes Schindelin @ 2016-09-08 7:31 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <cover.1473319844.git.johannes.schindelin@gmx.de>
The new regexec_buf() function operates on buffers with an explicitly
specified length, rather than NUL-terminated strings.
We need to use this function whenever the buffer we want to pass to
regexec() may have been mmap()ed (and is hence not NUL-terminated).
Note: the original motivation for this patch was to fix a bug where
`git diff -G <regex>` would crash. This patch converts more callers,
though, some of which explicitly allocated and constructed
NUL-terminated strings (or worse: modified read-only buffers to insert
NULs).
Some of the buffers actually may be NUL-terminated. As regexec_buf()
uses REG_STARTEND where available, but has to fall back to allocating
and constructing NUL-terminated strings where REG_STARTEND is not
available, this makes the code less efficient in the latter case.
However, given the widespread support for REG_STARTEND, combined with
the improved ease of code maintenance, we strike the balance in favor
of REG_STARTEND.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
diff.c | 3 ++-
diffcore-pickaxe.c | 18 ++++++++----------
xdiff-interface.c | 13 ++++---------
3 files changed, 14 insertions(+), 20 deletions(-)
diff --git a/diff.c b/diff.c
index 534c12e..526775a 100644
--- a/diff.c
+++ b/diff.c
@@ -951,7 +951,8 @@ static int find_word_boundaries(mmfile_t *buffer, regex_t *word_regex,
{
if (word_regex && *begin < buffer->size) {
regmatch_t match[1];
- if (!regexec(word_regex, buffer->ptr + *begin, 1, match, 0)) {
+ if (!regexec_buf(word_regex, buffer->ptr + *begin,
+ buffer->size - *begin, 1, match, 0)) {
char *p = memchr(buffer->ptr + *begin + match[0].rm_so,
'\n', match[0].rm_eo - match[0].rm_so);
*end = p ? p - buffer->ptr : match[0].rm_eo + *begin;
diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 55067ca..9795ca1 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -23,7 +23,6 @@ static void diffgrep_consume(void *priv, char *line, unsigned long len)
{
struct diffgrep_cb *data = priv;
regmatch_t regmatch;
- int hold;
if (line[0] != '+' && line[0] != '-')
return;
@@ -33,11 +32,8 @@ static void diffgrep_consume(void *priv, char *line, unsigned long len)
* caller early.
*/
return;
- /* Yuck -- line ought to be "const char *"! */
- hold = line[len];
- line[len] = '\0';
- data->hit = !regexec(data->regexp, line + 1, 1, ®match, 0);
- line[len] = hold;
+ data->hit = !regexec_buf(data->regexp, line + 1, len - 1, 1,
+ ®match, 0);
}
static int diff_grep(mmfile_t *one, mmfile_t *two,
@@ -50,9 +46,11 @@ static int diff_grep(mmfile_t *one, mmfile_t *two,
xdemitconf_t xecfg;
if (!one)
- return !regexec(regexp, two->ptr, 1, ®match, 0);
+ return !regexec_buf(regexp, two->ptr, two->size,
+ 1, ®match, 0);
if (!two)
- return !regexec(regexp, one->ptr, 1, ®match, 0);
+ return !regexec_buf(regexp, one->ptr, one->size,
+ 1, ®match, 0);
/*
* We have both sides; need to run textual diff and see if
@@ -83,8 +81,8 @@ static unsigned int contains(mmfile_t *mf, regex_t *regexp, kwset_t kws)
regmatch_t regmatch;
int flags = 0;
- assert(data[sz] == '\0');
- while (*data && !regexec(regexp, data, 1, ®match, flags)) {
+ while (*data &&
+ !regexec_buf(regexp, data, sz, 1, ®match, flags)) {
flags |= REG_NOTBOL;
data += regmatch.rm_eo;
if (*data && regmatch.rm_so == regmatch.rm_eo)
diff --git a/xdiff-interface.c b/xdiff-interface.c
index f34ea76..50702a2 100644
--- a/xdiff-interface.c
+++ b/xdiff-interface.c
@@ -214,11 +214,10 @@ struct ff_regs {
static long ff_regexp(const char *line, long len,
char *buffer, long buffer_size, void *priv)
{
- char *line_buffer;
struct ff_regs *regs = priv;
regmatch_t pmatch[2];
int i;
- int result = -1;
+ int result;
/* Exclude terminating newline (and cr) from matching */
if (len > 0 && line[len-1] == '\n') {
@@ -228,18 +227,16 @@ static long ff_regexp(const char *line, long len,
len--;
}
- line_buffer = xstrndup(line, len); /* make NUL terminated */
-
for (i = 0; i < regs->nr; i++) {
struct ff_reg *reg = regs->array + i;
- if (!regexec(®->re, line_buffer, 2, pmatch, 0)) {
+ if (!regexec_buf(®->re, line, len, 2, pmatch, 0)) {
if (reg->negate)
- goto fail;
+ return -1;
break;
}
}
if (regs->nr <= i)
- goto fail;
+ return -1;
i = pmatch[1].rm_so >= 0 ? 1 : 0;
line += pmatch[i].rm_so;
result = pmatch[i].rm_eo - pmatch[i].rm_so;
@@ -248,8 +245,6 @@ static long ff_regexp(const char *line, long len,
while (result > 0 && (isspace(line[result - 1])))
result--;
memcpy(buffer, line, result);
- fail:
- free(line_buffer);
return result;
}
--
2.10.0.windows.1.10.g803177d
^ permalink raw reply related
* [PATCH v2 2/3] Introduce a function to run regexec() on non-NUL-terminated buffers
From: Johannes Schindelin @ 2016-09-08 7:31 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <cover.1473319844.git.johannes.schindelin@gmx.de>
We just introduced a test that demonstrates that our sloppy use of
regexec() on a mmap()ed area can result in incorrect results or even
hard crashes.
So what we need to fix this is a function that calls regexec() on a
length-delimited, rather than a NUL-terminated, string.
Happily, there is an extension to regexec() introduced by the NetBSD
project and present in all major regex implementation including
Linux', MacOSX' and the one Git includes in compat/regex/: by using
the (non-POSIX) REG_STARTEND flag, it is possible to tell the
regexec() function that it should only look at the offsets between
pmatch[0].rm_so and pmatch[0].rm_eo.
That is exactly what we need.
Since support for REG_STARTEND is so widespread by now, let's just
introduce a helper function that uses it, and fall back to allocating
and constructing a NUL-terminated when REG_STARTEND is not available.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
git-compat-util.h | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/git-compat-util.h b/git-compat-util.h
index db89ba7..19128b3 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -965,6 +965,27 @@ void git_qsort(void *base, size_t nmemb, size_t size,
#define qsort git_qsort
#endif
+static inline int regexec_buf(const regex_t *preg, const char *buf, size_t size,
+ size_t nmatch, regmatch_t pmatch[], int eflags)
+{
+#ifdef REG_STARTEND
+ assert(nmatch > 0 && pmatch);
+ pmatch[0].rm_so = 0;
+ pmatch[0].rm_eo = size;
+ return regexec(preg, buf, nmatch, pmatch, eflags | REG_STARTEND);
+#else
+ char *buf2 = xmalloc(size + 1);
+ int ret;
+
+ memcpy(buf2, buf, size);
+ buf2[size] = '\0';
+ ret = regexec(preg, buf2, nmatch, pmatch, eflags);
+ free(buf2);
+
+ return ret;
+#endif
+}
+
#ifndef DIR_HAS_BSD_GROUP_SEMANTICS
# define FORCE_DIR_SET_GID S_ISGID
#else
--
2.10.0.windows.1.10.g803177d
^ permalink raw reply related
* [PATCH v2 1/3] Demonstrate a problem: our pickaxe code assumes NUL-terminated buffers
From: Johannes Schindelin @ 2016-09-08 7:31 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <cover.1473319844.git.johannes.schindelin@gmx.de>
When our pickaxe code feeds file contents to regexec(), it implicitly
assumes that the file contents are read into implicitly NUL-terminated
buffers (i.e. that we overallocate by 1, appending a single '\0').
This is not so.
In particular when the file contents are simply mmap()ed, we can be
virtually certain that the buffer is preceding uninitialized bytes, or
invalid pages.
Note that the test we add here is known to be flakey: we simply cannot
know whether the byte following the mmap()ed ones is a NUL or not.
Typically, on Linux the test passes. On Windows, it fails virtually
every time due to an access violation (that's a segmentation fault for
you Unix-y people out there). And Windows would be correct: the
regexec() call wants to operate on a regular, NUL-terminated string,
there is no NUL in the mmap()ed memory range, and it is undefined
whether the next byte is even legal to access.
When run with --valgrind it demonstrates quite clearly the breakage, of
course.
Being marked with `test_expect_failure`, this test will sometimes be
declare "TODO fixed", even if it only passes by mistake.
This test case represents a Minimal, Complete and Verifiable Example of
a breakage reported by Chris Sidi.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
t/t4061-diff-pickaxe.sh | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
create mode 100755 t/t4061-diff-pickaxe.sh
diff --git a/t/t4061-diff-pickaxe.sh b/t/t4061-diff-pickaxe.sh
new file mode 100755
index 0000000..5929f2e
--- /dev/null
+++ b/t/t4061-diff-pickaxe.sh
@@ -0,0 +1,22 @@
+#!/bin/sh
+#
+# Copyright (c) 2016 Johannes Schindelin
+#
+
+test_description='Pickaxe options'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+ test_commit initial &&
+ printf "%04096d" 0 >4096-zeroes.txt &&
+ git add 4096-zeroes.txt &&
+ test_tick &&
+ git commit -m "A 4k file"
+'
+test_expect_failure '-G matches' '
+ git diff --name-only -G "^0{4096}$" HEAD^ >out &&
+ test 4096-zeroes.txt = "$(cat out)"
+'
+
+test_done
--
2.10.0.windows.1.10.g803177d
^ permalink raw reply related
* [PATCH v2 0/3] Fix a segfault caused by regexec() being called on mmap()ed data
From: Johannes Schindelin @ 2016-09-08 7:31 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <cover.1473090278.git.johannes.schindelin@gmx.de>
This patch series addresses a problem where `git diff` is called using
`-G` or `-S --pickaxe-regex` on new-born files that are configured
without user diff drivers, and that hence get mmap()ed into memory.
The problem with that: mmap()ed memory is *not* NUL-terminated, yet the
pickaxe code calls regexec() on it just the same.
This problem has been reported by my colleague Chris Sidi.
We solve this by introducing a helper, regexec_buf(), that takes a
pointer and a length instead of a NUL-terminated string.
This helper then uses REG_STARTEND where available, and falls back to
allocating and constructing a NUL-terminated string. Given the
wide-spread support for REG_STARTEND (Linux has it, MacOSX has it, Git
for Windows has it because it uses compat/regex/ that has it), I think
this is a fair trade-off.
Changes since v1:
- completely revamped the approach: now we no longer force-append a NUL
whenever mmap()ing buffers for use by the diff machinery, but we fix
things at the regexec() level.
Johannes Schindelin (3):
Demonstrate a problem: our pickaxe code assumes NUL-terminated buffers
Introduce a function to run regexec() on non-NUL-terminated buffers
Use the newly-introduced regexec_buf() function
diff.c | 3 ++-
diffcore-pickaxe.c | 18 ++++++++----------
git-compat-util.h | 21 +++++++++++++++++++++
t/t4061-diff-pickaxe.sh | 22 ++++++++++++++++++++++
xdiff-interface.c | 13 ++++---------
5 files changed, 57 insertions(+), 20 deletions(-)
create mode 100755 t/t4061-diff-pickaxe.sh
Published-As: https://github.com/dscho/git/releases/tag/mmap-regexec-v2
Fetch-It-Via: git fetch https://github.com/dscho/git mmap-regexec-v2
Interdiff vs v1:
diff --git a/diff.c b/diff.c
index 32f7f46..526775a 100644
--- a/diff.c
+++ b/diff.c
@@ -951,7 +951,8 @@ static int find_word_boundaries(mmfile_t *buffer, regex_t *word_regex,
{
if (word_regex && *begin < buffer->size) {
regmatch_t match[1];
- if (!regexec(word_regex, buffer->ptr + *begin, 1, match, 0)) {
+ if (!regexec_buf(word_regex, buffer->ptr + *begin,
+ buffer->size - *begin, 1, match, 0)) {
char *p = memchr(buffer->ptr + *begin + match[0].rm_so,
'\n', match[0].rm_eo - match[0].rm_so);
*end = p ? p - buffer->ptr : match[0].rm_eo + *begin;
@@ -2826,15 +2827,6 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)
s->data = strbuf_detach(&buf, &size);
s->size = size;
s->should_free = 1;
- } else {
- /* data must be NUL-terminated so e.g. for regexec() */
- char *data = xmalloc(s->size + 1);
- memcpy(data, s->data, s->size);
- data[s->size] = '\0';
- munmap(s->data, s->size);
- s->should_munmap = 0;
- s->data = data;
- s->should_free = 1;
}
}
else {
diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 88820b6..9795ca1 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -23,7 +23,6 @@ static void diffgrep_consume(void *priv, char *line, unsigned long len)
{
struct diffgrep_cb *data = priv;
regmatch_t regmatch;
- int hold;
if (line[0] != '+' && line[0] != '-')
return;
@@ -33,11 +32,8 @@ static void diffgrep_consume(void *priv, char *line, unsigned long len)
* caller early.
*/
return;
- /* Yuck -- line ought to be "const char *"! */
- hold = line[len];
- line[len] = '\0';
- data->hit = !regexec(data->regexp, line + 1, 1, ®match, 0);
- line[len] = hold;
+ data->hit = !regexec_buf(data->regexp, line + 1, len - 1, 1,
+ ®match, 0);
}
static int diff_grep(mmfile_t *one, mmfile_t *two,
@@ -49,12 +45,12 @@ static int diff_grep(mmfile_t *one, mmfile_t *two,
xpparam_t xpp;
xdemitconf_t xecfg;
- assert(!one || one->ptr[one->size] == '\0');
- assert(!two || two->ptr[two->size] == '\0');
if (!one)
- return !regexec(regexp, two->ptr, 1, ®match, 0);
+ return !regexec_buf(regexp, two->ptr, two->size,
+ 1, ®match, 0);
if (!two)
- return !regexec(regexp, one->ptr, 1, ®match, 0);
+ return !regexec_buf(regexp, one->ptr, one->size,
+ 1, ®match, 0);
/*
* We have both sides; need to run textual diff and see if
@@ -85,8 +81,8 @@ static unsigned int contains(mmfile_t *mf, regex_t *regexp, kwset_t kws)
regmatch_t regmatch;
int flags = 0;
- assert(data[sz] == '\0');
- while (*data && !regexec(regexp, data, 1, ®match, flags)) {
+ while (*data &&
+ !regexec_buf(regexp, data, sz, 1, ®match, flags)) {
flags |= REG_NOTBOL;
data += regmatch.rm_eo;
if (*data && regmatch.rm_so == regmatch.rm_eo)
diff --git a/git-compat-util.h b/git-compat-util.h
index db89ba7..19128b3 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -965,6 +965,27 @@ void git_qsort(void *base, size_t nmemb, size_t size,
#define qsort git_qsort
#endif
+static inline int regexec_buf(const regex_t *preg, const char *buf, size_t size,
+ size_t nmatch, regmatch_t pmatch[], int eflags)
+{
+#ifdef REG_STARTEND
+ assert(nmatch > 0 && pmatch);
+ pmatch[0].rm_so = 0;
+ pmatch[0].rm_eo = size;
+ return regexec(preg, buf, nmatch, pmatch, eflags | REG_STARTEND);
+#else
+ char *buf2 = xmalloc(size + 1);
+ int ret;
+
+ memcpy(buf2, buf, size);
+ buf2[size] = '\0';
+ ret = regexec(preg, buf2, nmatch, pmatch, eflags);
+ free(buf2);
+
+ return ret;
+#endif
+}
+
#ifndef DIR_HAS_BSD_GROUP_SEMANTICS
# define FORCE_DIR_SET_GID S_ISGID
#else
diff --git a/t/t4059-diff-pickaxe.sh b/t/t4061-diff-pickaxe.sh
similarity index 91%
rename from t/t4059-diff-pickaxe.sh
rename to t/t4061-diff-pickaxe.sh
index f0bf50b..5929f2e 100755
--- a/t/t4059-diff-pickaxe.sh
+++ b/t/t4061-diff-pickaxe.sh
@@ -14,7 +14,7 @@ test_expect_success setup '
test_tick &&
git commit -m "A 4k file"
'
-test_expect_success '-G matches' '
+test_expect_failure '-G matches' '
git diff --name-only -G "^0{4096}$" HEAD^ >out &&
test 4096-zeroes.txt = "$(cat out)"
'
diff --git a/xdiff-interface.c b/xdiff-interface.c
index f34ea76..50702a2 100644
--- a/xdiff-interface.c
+++ b/xdiff-interface.c
@@ -214,11 +214,10 @@ struct ff_regs {
static long ff_regexp(const char *line, long len,
char *buffer, long buffer_size, void *priv)
{
- char *line_buffer;
struct ff_regs *regs = priv;
regmatch_t pmatch[2];
int i;
- int result = -1;
+ int result;
/* Exclude terminating newline (and cr) from matching */
if (len > 0 && line[len-1] == '\n') {
@@ -228,18 +227,16 @@ static long ff_regexp(const char *line, long len,
len--;
}
- line_buffer = xstrndup(line, len); /* make NUL terminated */
-
for (i = 0; i < regs->nr; i++) {
struct ff_reg *reg = regs->array + i;
- if (!regexec(®->re, line_buffer, 2, pmatch, 0)) {
+ if (!regexec_buf(®->re, line, len, 2, pmatch, 0)) {
if (reg->negate)
- goto fail;
+ return -1;
break;
}
}
if (regs->nr <= i)
- goto fail;
+ return -1;
i = pmatch[1].rm_so >= 0 ? 1 : 0;
line += pmatch[i].rm_so;
result = pmatch[i].rm_eo - pmatch[i].rm_so;
@@ -248,8 +245,6 @@ static long ff_regexp(const char *line, long len,
while (result > 0 && (isspace(line[result - 1])))
result--;
memcpy(buffer, line, result);
- fail:
- free(line_buffer);
return result;
}
--
2.10.0.windows.1.10.g803177d
base-commit: 6ebdac1bab966b720d776aa43ca188fe378b1f4b
^ permalink raw reply
* Re: [PATCH 0/3] Fix a segfault caused by regexec() being called on mmap()ed data
From: Johannes Schindelin @ 2016-09-08 7:29 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20160906182942.s2mlge2vg65f5sy4@sigill.intra.peff.net>
Hi Peff,
sorry for the late answer, I was really busy trying to come up with a new
and improved version of the patch series, and while hunting a bug I
introduced got bogged down with other tasks.
The good news is that I made up my mind about releasing a Git for Windows
v2.10.0(2): originally, I had planned to do that today, to have time for
any hot fixes until Sunday, if necessary, before going semi-dark.
FWIW I am now trying to track my plans for v2.10.0(2) (or v2.10.1, if
upstream Git v2.10.1 is released before) on GitHub:
https://github.com/git-for-windows/git/milestone/3
On Tue, 6 Sep 2016, Jeff King wrote:
> On Tue, Sep 06, 2016 at 04:06:32PM +0200, Johannes Schindelin wrote:
>
> > > I think re_search() the correct replacement function but it's been a
> > > while since I've looked into it.
> >
> > The segfault I investigated happened in a call to strlen(). I see many
> > calls to strlen() in compat/regex/... The one that triggers the segfault
> > is in regexec(), compat/regex/regexec.c:241.
>
> Yes, that is the important one, I think. The others are for patterns,
> error msgs, etc. Of course strlen() is not the only function that cares
> about NUL delimiters (and there might even be a "while (*p)" somewhere
> in the code).
>
> I always assumed the _point_ of re_search taking a ptr/len pair was
> exactly to handle this case. The documentation[1] says:
>
> `string` is the string you want to match; it can contain newline and
> null characters. `size` is the length of that string.
>
> Which seems pretty definitive to me (that's for re_match(), but
> re_search() is defined in the docs in terms of re_match()).
Right. The problem is: I *really* want to avoid using GNU-isms.
> > The bigger problem is that re_search() is defined in the __USE_GNU section
> > of regex.h, and I do not think it is appropriate to universally #define
> > said constant before #include'ing regex.h. So it would appear that major
> > surgery would be required if we wanted to use regular expressions on
> > strings that are not NUL-terminated.
>
> We can contain this to the existing compat/regexec/regexec.c, and just
> provide a wrapper that is similar to regexec but takes a ptr/len pair.
But we can do even better than that: we can provide a wrapper that uses
REG_STARTEND where available (which is really the majority of platforms we
care about: Linux, MacOSX, Windows, and even the *BSDs). Where it is not
available, we simply malloc(), memcpy() and append a NUL.
Which is what my v2 does (will send it out in a moment).
Ciao,
Dscho
^ permalink raw reply
* Re: [RFC/PATCH v2 0/3] patch-id for merges
From: Jeff King @ 2016-09-08 7:30 UTC (permalink / raw)
To: Josh Triplett
Cc: git, Michael Haggerty, Kevin Willford, Xiaolong Ye,
Johannes Schindelin
In-Reply-To: <20160907225104.f5wi2yo4d2f26tti@x>
On Wed, Sep 07, 2016 at 03:51:04PM -0700, Josh Triplett wrote:
> > This is still marked RFC, because there are really two approaches here,
> > and I'm not sure which one is better for "format-patch --base". I'd like
> > to get input from Xiaolong Ye (who worked on --base), and Josh Triplett
> > (who has proposed some patches in that area, and is presumably using
> > them).
>
> Thanks.
>
> I'd love to see a more resilient patch-id mechanism, to make it easier
> to match up patches between branches. I don't think it makes sense to
> talk about the patch-id of a merge commit (though it might make sense
> for a merge which makes additional changes not present in any of the
> parents). Even if someone wants to match up merge commits with merge
> commits, I don't think that should happen via patch-id; I think that
> should happen in terms of "what patches does this merge introduce",
> without constructing a merge-patch-id via a Merkle tree of commit
> patch-ids.
>
> So, I think this patch series makes sense (modulo the comments about the
> commit message in patch 3). We already don't respect merge commits when
> doing format-patch; this seems consistent with that. If we ever make it
> possible for format-patch to handle merge commits, then we should also
> allow it to have merge commits as prerequisites.
Thanks for the input. I knew that format-patch doesn't show merge
commits, but I didn't realize that merges were skipped entirely for the
base preparation (but I see it now; there is a "rev.max_parents = 1"
setting in prepare_bases).
So this really doesn't change the output there at all. And in fact, the
switch to:
if (commit_patch_id(commit, &diffopt, sha1, 0))
- die(_("cannot get patch id"))
+ continue;
should never hit that continue. It could be:
die("BUG: somehow a merge got fed to commit_patch_id?");
but the "continue" somehow seems like the right thing to me.
I'll wait another day or so for comments and then send the re-roll using
this approach.
-Peff
^ permalink raw reply
* Re: [PATCH 2/3] diff_flush_patch_id: stop returning error result
From: Jeff King @ 2016-09-08 7:20 UTC (permalink / raw)
To: Ramsay Jones
Cc: git, Michael Haggerty, Kevin Willford, Xiaolong Ye,
Johannes Schindelin, Josh Triplett
In-Reply-To: <5c4276ad-19ed-1481-41cf-3d506ef9c7c6@ramsayjones.plus.com>
On Thu, Sep 08, 2016 at 01:51:05AM +0100, Ramsay Jones wrote:
>
>
> On 07/09/16 23:04, Jeff King wrote:
> > All of our errors come from diff_get_patch_id(), which has
> > exactly three error conditions. The first is an internal
> > assertion, which should be a die("BUG") in the first place.
> >
> > The other two are caused by an inability to two diff blobs,
> ^^^^^^^^^^^^^^^^^
> Huh? ... to diff two blobs?
Sorry. English my getting worse be to seems.
Will fix in a re-roll.
-Peff
^ permalink raw reply
* Re: [PATCH 3/5] git-rebase--interactive: fix English grammar
From: Johannes Schindelin @ 2016-09-08 7:06 UTC (permalink / raw)
To: Alex Henrie; +Cc: Matthieu.Moy, gitster, git
In-Reply-To: <20160908043417.5946-1-alexhenrie24@gmail.com>
Hi Alex,
On Wed, 7 Sep 2016, Alex Henrie wrote:
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 7e558b0..6fd6d4e 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -1082,7 +1082,7 @@ If they are meant to go into a new commit, run:
>
> git commit \$gpg_sign_opt_quoted
>
> -In both case, once you're done, continue with:
> +In both cases, once you're done, continue with:
Good catch! I ported this into my `prepare-sequencer` patch series.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH] compat: move strdup(3) replacement to its own file
From: Johannes Schindelin @ 2016-09-08 6:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: René Scharfe, Git List
In-Reply-To: <xmqqfupbob9c.fsf@gitster.mtv.corp.google.com>
[-- Attachment #1: Type: text/plain, Size: 943 bytes --]
Hi,
On Wed, 7 Sep 2016, Junio C Hamano wrote:
> René Scharfe <l.s.r@web.de> writes:
>
> > Well, OK. I think the missing point is that the original nedmalloc
> > doesn't come with strdup() and doesn't need it. Only _users_ of
> > nedmalloc need it. Marius added it in nedmalloc.c, but strdup.c is a
> > better place for it.
>
> Thanks. I'll add these lines like so:
>
> Move our implementation of strdup(3) out of compat/nedmalloc/ and
> allow it to be used independently from USE_NED_ALLOCATOR. The
> original nedmalloc doesn't come with strdup() and doesn't need it.
> Only _users_ of nedmalloc need it, which was added when we imported
> it to our compat/ hierarchy.
>
> This reduces the difference of our copy of nedmalloc from the
> original, making it easier to update, and allows for easier testing
> and reusing of our version of strdup().
Excellent!
Thanks,
Dscho
^ permalink raw reply
* git-gui, was Re: [PATCH v2 6/6] git-gui: Update Japanese information
From: Johannes Schindelin @ 2016-09-08 6:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Satoshi Yasushima, Pat Thoyts, git, Jakub Narębski
In-Reply-To: <xmqqk2enobol.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Wed, 7 Sep 2016, Junio C Hamano wrote:
> Pat, we haven't heard from you for a long time.
Indeed. There are a couple of git-gui patches in Git for Windows that have
been contributed a long time ago and not been picked up.
Maybe it is time to just accept git-gui patches directly into Git, after
some other Tcl/Tk savvy people ACKed them?
Ciao,
Dscho
^ permalink raw reply
* Re: Bug? ran into a "fatal" using interactive rebase
From: Johannes Schindelin @ 2016-09-08 6:57 UTC (permalink / raw)
To: Ralf Thielow; +Cc: git, Junio C Hamano
In-Reply-To: <CAN0XMOL7LEcJAHhmSwq8QWH7mD7+jQXRkAdQA+3hZ2JQUJ99Gw@mail.gmail.com>
Hi Ralf,
On Wed, 7 Sep 2016, Ralf Thielow wrote:
> 2016-09-07 17:19 GMT+02:00 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> >
> > So, something like this should help (if you are interested in seeing this
> > patch included, please run with it, as I am running short on time):
What I meant is: could you craft a test, a proper commit message, and
submit it? I will be mostly offline during the second half of September
and have many other things to take care of right now.
Thanks,
Dscho
^ permalink raw reply
* Re: [PATCH v2 00/38] Virtualization of the refs API
From: Michael Haggerty @ 2016-09-08 6:42 UTC (permalink / raw)
To: Junio C Hamano
Cc: David Turner, Ramsay Jones, Eric Sunshine, Jeff King,
Nguyễn Thái Ngọc Duy, git
In-Reply-To: <xmqqmvjjldp2.fsf@gitster.mtv.corp.google.com>
On 09/07/2016 09:20 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>
>> This is v2 of the patch series to virtualize the references API
>> (though earlier patch series similar in spirit were submitted by
>> Ronnie Sahlberg and David Turner). Thanks to Junio, Eric, and Ramsay
>> for their comments about v1 [1].
>>
>> Nobody pointed out any fundamental problems with v1, but this version
>> includes the following improvements:
>
> Curiously, many of these improvements were already in 'pu'.
I pushed some of these changes to GitHub a while ago and mentioned that
fact on the mailing list [1]; I assume you fetched from there. But I was
waiting for the dust to settle from the earlier patch series and the
2.10 release before sending them to the mailing list again.
>> * In "refs: add methods for reflog":
>>
>> * Don't export `files_reflog_iterator_begin()` (suggested by
>> Ramsay).
>
> This I can see was missing in what has been in 'pu'.
And according to your "What's cooking", you were waiting on this change
before proceeding.
FYI: I haven't done significant work on any next steps, which IMO could
go in many possible directions:
* Splitting loose and packed refs storage into two separate ref_store
subclasses that are joined together using some kind of
overlay_ref_store. This is not necessarily a blocker for the following
ideas, but the overlay_ref_store would make it easier to construct other
hybrid ref-stores, as for example the ubiquitous "most references are
stored in main repo; per-worktree refs are stored in worktree".
* Implementing reference caching as a ref_store that "writes through"
changes to a backing reference store. This is also not a blocker for
other ideas, but it would allow reference caching easily to be turned
on/off for other reference stores (or even disabled for packed/loose
reference storage during command invocations that know they won't have
to iterate over references more than once).
* A variant of loose reference storage that encodes the reference names
somehow before turning them into filenames (1) to make refnames
independent of filesystem path-name encoding issues, and (2) to
eliminate D/F conflicts, thereby allowing reflogs to be retained after a
reference is deleted.
* A reference backend based on LMDB (or any other key-value store).
* A reference backend based on Shawn Pearce's RefTree proposal or
something like it [2].
It's uncertain when I'll next have a chunk of time to work on any of these.
Michael
[1] http://public-inbox.org/git/575990FB.5000908@alum.mit.edu/
[2]
http://public-inbox.org/git/CAJo=hJvnAPNAdDcAAwAvU9C4RVeQdoS3Ev9WTguHx4fD0V_nOg@mail.gmail.com/t
^ permalink raw reply
* Re: [PATCH 5/5] unpack-trees: do not capitalize "working"
From: Matthieu Moy @ 2016-09-08 6:36 UTC (permalink / raw)
To: Alex Henrie; +Cc: vascomalmeida, gitster, git
In-Reply-To: <20160908043453.6044-1-alexhenrie24@gmail.com>
Alex Henrie <alexhenrie24@gmail.com> writes:
> In English, only proper nouns are capitalized.
>
> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
> ---
> unpack-trees.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 11c37fb..c87a90a 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -123,9 +123,9 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
> msgs[ERROR_SPARSE_NOT_UPTODATE_FILE] =
> _("Cannot update sparse checkout: the following entries are not up-to-date:\n%s");
> msgs[ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN] =
> - _("The following Working tree files would be overwritten by sparse checkout update:\n%s");
> + _("The following working tree files would be overwritten by sparse checkout update:\n%s");
> msgs[ERROR_WOULD_LOSE_ORPHANED_REMOVED] =
> - _("The following Working tree files would be removed by sparse checkout update:\n%s");
> + _("The following working tree files would be removed by sparse checkout update:\n%s");
Probably a leftover from an old sentence starting with Working? In any
case, obviously correct too, thanks.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox