Re: Test failures in t4034

From: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
Date: Aug 21, 2012, 18:37 UTC
Message-ID: <5033D573.9030103@ramsay1.demon.co.uk>
In-Reply-To: <7vboi6nzym.fsf@alter.siamese.dyndns.org>

Junio C Hamano wrote:

Show 59 quoted lines

> Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:
> 
>> I had the same problem (or at least it *looks* like the same problem) on Linux
>> last year (May 2011), which turned out to be a bug in the regex routines in an
>> old version of glibc. 
>>
>> I don't know OS X at all, so this may not be relevent; does OS X use glibc?
>> (I didn't think so, but ...)
>>
>> I sent some patches to the list which may be helpful. I can't get to gmane to
>> look up a reference, but you need to search for:
>>
>>     [RFC/PATCH] userdiff.c: Avoid old glibc regex bug causing t4034-*.sh test failures
>>
>> sent on 3rd May 2011.
> 
> Thanks; that's $gmane/172676 for people who prefer easier to read
> threading interface.
> 
>> Also, in the same thread, a reply to Jonathan Nieder on 7th May contains a
>> test which checks whether your regex routines suffer this bug.
>>
>> These patches were not applied since I didn't think this would be a common
>> problem. I simply set NO_REGEX=1 in my config.mak, since the compat/ regex
>> routines don't suffer from this problem.
> 
> You also said:
> 
>   This is an RFC because:
>    - A simple fix would be for me to put NO_REGEX=1 in my config.mak,
>      since the compat/regex routines don't suffer this problem.
>    - I suspect this bug is old enough that it will not affect many users.
>    - I have not audited the other non-matching list expressions in
>      userdiff.c
>    - blame, grep and pickaxe all call regcomp() with the REG_NEWLINE
>      flag, but get the regex from the user (eg from command line).
> 
> I think:
> 
>  - the second "this is old enough" assumption was broken again by
>    Brian this week ;-)
> 
>  - the first "Use NO_REGEX if your regexp library is broken" is a
>    reasonable thing to do; is this something we may want to throw
>    into the platform specific section of the top-level Makefile?
> 
>  - among the fourth, "blame" and "grep" goes line by line, and even
>    though pickaxe is primarily meant to take multi-line pattern, I
>    do not think people give multi-line pattern when they use it in
>    the regexp mode.  So I do not think they pose a real issue even
>    though they get an arbitrary pattern from the user.
> 
>  - the third, combined with the fact that end user can define their
>    own pattern, is a killer.  We cannot really afford to let broken
>    regex library to break us.
> 
> I think a sensible way to go in the longer term, while we wait these
> old regexp libraries die out, is to help people to avoid building
> git without NO_REGEX on platforms where they need it.

Agreed. Did you take a look at the second patch I mentioned above? I've included a rebased version (onto v1.7.12) of that patch below.

NOTE: I have not even attempted to compile this version of the patch
and I can't remember how much testing I did last year, so this is
included *only* for discussion purposes ...

I think that, after some testing, this (or something like it) is the best that we can do. What do you think?

ATB, Ramsay Jones

-- >8 --
Subject: [PATCH] test-regex: Add a test to check for a bug in the regex routines

Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---
 .gitignore             |  1 +
 Makefile               |  1 +
 t/t0070-fundamental.sh |  5 +++++
 test-regex.c           | 35 +++++++++++++++++++++++++++++++++++
 4 files changed, 42 insertions(+)
 create mode 100644 test-regex.c

diff --git a/.gitignore b/.gitignore
index bb5c91e..68fe464 100644
--- a/.gitignore
+++ b/.gitignore
@@ -189,6 +189,7 @@
 /test-mktemp
 /test-parse-options
 /test-path-utils
+/test-regex
 /test-revision-walking
 /test-run-command
 /test-sha1
diff --git a/Makefile b/Makefile
index 6b0c961..3b760d3 100644
--- a/Makefile
+++ b/Makefile
@@ -496,6 +496,7 @@ TEST_PROGRAMS_NEED_X += test-mergesort
 TEST_PROGRAMS_NEED_X += test-mktemp
 TEST_PROGRAMS_NEED_X += test-parse-options
 TEST_PROGRAMS_NEED_X += test-path-utils
+TEST_PROGRAMS_NEED_X += test-regex
 TEST_PROGRAMS_NEED_X += test-revision-walking
 TEST_PROGRAMS_NEED_X += test-run-command
 TEST_PROGRAMS_NEED_X += test-scrap-cache-tree
diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
index 9bee8bf..da2c504 100755
--- a/t/t0070-fundamental.sh
+++ b/t/t0070-fundamental.sh
@@ -25,4 +25,9 @@ test_expect_success POSIXPERM 'mktemp to unwritable directory prints filename' '
 	grep "cannotwrite/test" err
 '
 
+test_expect_success 'check for a bug in the regex routines' '
+	# if this test fails, re-build git with NO_REGEX=1
+	test-regex
+'
+
 test_done
diff --git a/test-regex.c b/test-regex.c
new file mode 100644
index 0000000..9259985
--- /dev/null
+++ b/test-regex.c
@@ -0,0 +1,35 @@
+#include <stdlib.h>
+#include <stdio.h>
+#include <stdarg.h>
+#include <sys/types.h>
+#include <regex.h>
+
+static void die(const char *fmt, ...)
+{
+	va_list p;
+
+	va_start(p, fmt);
+	vfprintf(stderr, fmt, p);
+	va_end(p);
+	fputc('\n', stderr);
+	exit(128);
+}
+
+int main(int argc, char **argv)
+{
+	char *pat = "[^={} \t]+";
+	char *str = "={}\nfred";
+	regex_t r;
+	regmatch_t m[1];
+
+	if (regcomp(&r, pat, REG_EXTENDED | REG_NEWLINE))
+		die("failed regcomp() for pattern '%s'", pat);
+	if (regexec(&r, str, 1, m, 0))
+		die("no match of pattern '%s' to string '%s'", pat, str);
+
+	/* http://sourceware.org/bugzilla/show_bug.cgi?id=3957  */
+	if (m[0].rm_so == 3) /* matches '\n' when it should not */
+		exit(1);
+
+	exit(0);
+}

-- 
1.7.12