{"thread":{"id":"27244","subject":"[RFC/PATCH] userdiff.c: Avoid old glibc regex bug causing t4034-*.sh test failures","startedAt":"2011-05-03T17:49:21Z","lastAt":"2011-05-07T17:54:44Z","messageCount":3,"participants":["Ramsay Jones","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"166952","messageId":"4DC04021.1040606@ramsay1.demon.co.uk","threadId":"27244","inReplyTo":null,"subject":"[RFC/PATCH] userdiff.c: Avoid old glibc regex bug causing t4034-*.sh test failures","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2011-05-03T17:49:21Z","receivedAt":"2011-05-03T17:49:21Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nIn particular, this bug affects the word-diff regex for 'bibtex' and\n'html', leading to the test failures in t4034-diff-words.sh. The bug\nis described here:\n\n    http://sourceware.org/bugzilla/show_bug.cgi?id=3957\n\nand was fixed on 12-07-2007. In summary, when the REG_NEWLINE flag is\npassed to regcomp(), a non-matching list ([^...]) not containing a\nnewline should not match a newline. However, in some old versions of\nthe glibc regex library, the newline character was indeed matched.\n\nIn order to fix the problem, we add an explicit '\\n' to the list in\nthe non-matching list expression.\n\nSigned-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n---\n\nJunio,\n    I recently mentioned that a couple of tests in t4034-*.sh were\nfailing for me on Linux. I have now looked into it, and the problem\nturned out to be an old bug in the glibc regex routines. :-(\n\nThis is an RFC because:\n    - A simple fix would be for me to put NO_REGEX=1 in my config.mak,\n      since the compat/regex routines don't suffer this problem.\n    - I suspect this bug is old enough that it will not affect many users.\n    - I have not audited the other non-matching list expressions in\n      userdiff.c\n    - blame, grep and pickaxe all call regcomp() with the REG_NEWLINE\n      flag, but get the regex from the user (eg from command line).\n\nATB,\nRamsay Jones\n\n userdiff.c |    4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/userdiff.c b/userdiff.c\nindex 1ff4797..2f9ba37 100644\n--- a/userdiff.c\n+++ b/userdiff.c\n@@ -28,7 +28,7 @@ IPATTERN(\"fortran\",\n \t \"|[-+]?[0-9.]+([AaIiDdEeFfLlTtXx][Ss]?[-+]?[0-9.]*)?(_[a-zA-Z0-9][a-zA-Z0-9_]*)?\"\n \t \"|//|\\\\*\\\\*|::|[/<>=]=\"),\n PATTERNS(\"html\", \"^[ \\t]*(<[Hh][1-6][ \\t].*>.*)$\",\n-\t \"[^<>= \\t]+\"),\n+\t \"[^<>= \\t\\n]+\"),\n PATTERNS(\"java\",\n \t \"!^[ \\t]*(catch|do|for|if|instanceof|new|return|switch|throw|while)\\n\"\n \t \"^[ \\t]*(([A-Za-z_][A-Za-z_0-9]*[ \\t]+)+[A-Za-z_][A-Za-z_0-9]*[ \\t]*\\\\([^;]*)$\",\n@@ -94,7 +94,7 @@ PATTERNS(\"ruby\", \"^[ \\t]*((class|module|def)[ \\t].*)$\",\n \t \"|[-+0-9.e]+|0[xXbB]?[0-9a-fA-F]+|\\\\?(\\\\\\\\C-)?(\\\\\\\\M-)?.\"\n \t \"|//=?|[-+*/<>%&^|=!]=|<<=?|>>=?|===|\\\\.{1,3}|::|[!=]~\"),\n PATTERNS(\"bibtex\", \"(@[a-zA-Z]{1,}[ \\t]*\\\\{{0,1}[ \\t]*[^ \\t\\\"@',\\\\#}{~%]*).*$\",\n-\t \"[={}\\\"]|[^={}\\\" \\t]+\"),\n+\t \"[={}\\\"]|[^={}\\\" \\t\\n]+\"),\n PATTERNS(\"tex\", \"^(\\\\\\\\((sub)*section|chapter|part)\\\\*{0,1}\\\\{.*)$\",\n \t \"\\\\\\\\[a-zA-Z@]+|\\\\\\\\.|[a-zA-Z0-9\\x80-\\xff]+\"),\n PATTERNS(\"cpp\",\n-- \n1.7.5\n"},{"id":"166970","messageId":"20110503210716.GL1019@elie","threadId":"27244","inReplyTo":"4DC04021.1040606@ramsay1.demon.co.uk","subject":"Re: [RFC/PATCH] userdiff.c: Avoid old glibc regex bug causing t4034-*.sh test failures","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-05-03T21:07:16Z","receivedAt":"2011-05-03T21:07:16Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nRamsay Jones wrote:\n\n> This is an RFC because:\n>     - A simple fix would be for me to put NO_REGEX=1 in my config.mak,\n>       since the compat/regex routines don't suffer this problem.\n>     - I suspect this bug is old enough that it will not affect many users.\n>     - I have not audited the other non-matching list expressions in\n>       userdiff.c\n>     - blame, grep and pickaxe all call regcomp() with the REG_NEWLINE\n>       flag, but get the regex from the user (eg from command line).\n\nI think excluding \\n along with ' ' and \\t in similar places makes\nsense, but that meanwhile we should add tests to the testsuite for\nblame/grep/pickaxe to catch such implementations failing when\nNO_REGEX=1 is not set.\n\nDoes that make sense?  Thanks for tracking this down.\n\nJonathan\n"},{"id":"167327","messageId":"4DC58764.2070207@ramsay1.demon.co.uk","threadId":"27244","inReplyTo":"20110503210716.GL1019@elie","subject":"Re: [RFC/PATCH] userdiff.c: Avoid old glibc regex bug causing t4034-*.sh test failures","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2011-05-07T17:54:44Z","receivedAt":"2011-05-07T17:54:44Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Jonathan Nieder wrote:\n> Ramsay Jones wrote:\n>> This is an RFC because:\n>>     - A simple fix would be for me to put NO_REGEX=1 in my config.mak,\n>>       since the compat/regex routines don't suffer this problem.\n>>     - I suspect this bug is old enough that it will not affect many users.\n>>     - I have not audited the other non-matching list expressions in\n>>       userdiff.c\n>>     - blame, grep and pickaxe all call regcomp() with the REG_NEWLINE\n>>       flag, but get the regex from the user (eg from command line).\n> \n> I think excluding \\n along with ' ' and \\t in similar places makes\n> sense, ...\n\nI briefly considered replacing ' \\t' with [:space:], but that would\nalso add \\v, \\f and \\r in addition to \\n (and, maybe, some locale\nspecific chars too), so I decided not to go that route ...\n\n>        ... but that meanwhile we should add tests to the testsuite for\n> blame/grep/pickaxe to catch such implementations failing when\n> NO_REGEX=1 is not set.\n\nHmm, well I would certainly never complain about additional tests, but\nwho would these tests benefit? I suspect that the number of people\nbuilding git from source on an old (pre about 2008 say) glibc system\nwith this bug is somewhat small. (Of course I have no numbers to back\nup that gut feeling! :-))\n\nHaving said that, I have attached a patch (below), that adds a test\n(to t0070-fundamental.sh) using a stand-alone test-regex program, in\norder to simplify the detection of this bug.\n\nWhat do you think? (would this exchange in the list-archive suffice?)\n\n> Does that make sense?  Thanks for tracking this down.\n\nNo problem.\n\nATB,\nRamsay Jones\n\n-- >8 --\nSubject: [PATCH] test-regex: Add a test to check for a bug in the regex routines\n\n\nSigned-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n---\n .gitignore             |    1 +\n Makefile               |    1 +\n t/t0070-fundamental.sh |    5 +++++\n test-regex.c           |   35 +++++++++++++++++++++++++++++++++++\n 4 files changed, 42 insertions(+), 0 deletions(-)\n create mode 100644 test-regex.c\n\ndiff --git a/.gitignore b/.gitignore\nindex 711fce7..70af8b1 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -174,6 +174,7 @@\n /test-obj-pool\n /test-parse-options\n /test-path-utils\n+/test-regex\n /test-run-command\n /test-sha1\n /test-sigchain\ndiff --git a/Makefile b/Makefile\nindex 3a1fe20..482b9f9 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -428,6 +428,7 @@ TEST_PROGRAMS_NEED_X += test-match-trees\n TEST_PROGRAMS_NEED_X += test-obj-pool\n TEST_PROGRAMS_NEED_X += test-parse-options\n TEST_PROGRAMS_NEED_X += test-path-utils\n+TEST_PROGRAMS_NEED_X += test-regex\n TEST_PROGRAMS_NEED_X += test-run-command\n TEST_PROGRAMS_NEED_X += test-sha1\n TEST_PROGRAMS_NEED_X += test-sigchain\ndiff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh\nindex 9bee8bf..da2c504 100755\n--- a/t/t0070-fundamental.sh\n+++ b/t/t0070-fundamental.sh\n@@ -25,4 +25,9 @@ test_expect_success POSIXPERM 'mktemp to unwritable directory prints filename' '\n \tgrep \"cannotwrite/test\" err\n '\n \n+test_expect_success 'check for a bug in the regex routines' '\n+\t# if this test fails, re-build git with NO_REGEX=1\n+\ttest-regex\n+'\n+\n test_done\ndiff --git a/test-regex.c b/test-regex.c\nnew file mode 100644\nindex 0000000..9259985\n--- /dev/null\n+++ b/test-regex.c\n@@ -0,0 +1,35 @@\n+#include <stdlib.h>\n+#include <stdio.h>\n+#include <stdarg.h>\n+#include <sys/types.h>\n+#include <regex.h>\n+\n+static void die(const char *fmt, ...)\n+{\n+\tva_list p;\n+\n+\tva_start(p, fmt);\n+\tvfprintf(stderr, fmt, p);\n+\tva_end(p);\n+\tfputc('\\n', stderr);\n+\texit(128);\n+}\n+\n+int main(int argc, char **argv)\n+{\n+\tchar *pat = \"[^={} \\t]+\";\n+\tchar *str = \"={}\\nfred\";\n+\tregex_t r;\n+\tregmatch_t m[1];\n+\n+\tif (regcomp(&r, pat, REG_EXTENDED | REG_NEWLINE))\n+\t\tdie(\"failed regcomp() for pattern '%s'\", pat);\n+\tif (regexec(&r, str, 1, m, 0))\n+\t\tdie(\"no match of pattern '%s' to string '%s'\", pat, str);\n+\n+\t/* http://sourceware.org/bugzilla/show_bug.cgi?id=3957  */\n+\tif (m[0].rm_so == 3) /* matches '\\n' when it should not */\n+\t\texit(1);\n+\n+\texit(0);\n+}\n-- \n1.7.5\n"}]}