{"thread":{"id":"52649","subject":"[PATCH] Makefile: use compat regex with SANITIZE=address","startedAt":"2020-01-16T17:51:41Z","lastAt":"2020-01-18T03:38:07Z","messageCount":7,"participants":["Jeff King","Jonathan Nieder","Junio C Hamano","brian m. carlson"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"389902","messageId":"20200116175138.GA691238@coredump.intra.peff.net","threadId":"52649","inReplyTo":null,"subject":"[PATCH] Makefile: use compat regex with SANITIZE=address","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-01-16T17:51:38Z","receivedAt":"2020-01-16T17:51:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Recent versions of the gcc and clang Address Sanitizer produce test\nfailures related to regexec(). This triggers with gcc-10 and clang-8\n(but not gcc-9 nor clang-7). Running:\n\n make CC=gcc-10 SANITIZE=address test\n\nresults in failures in t4018, t3206, and t4062.\n\nThe cause seems to be that when built with ASan, we use a different\nversion of regexec() than normal. And this version doesn't understand\nthe REG_STARTEND flag. Here's my evidence supporting that.\n\nThe failure in t4062 is an ASan warning:\n\n expecting success of 4062.2 '-G matches':\n \tgit diff --name-only -G \"^(0{64}){64}$\" HEAD^ >out &&\n \ttest 4096-zeroes.txt = \"$(cat out)\"\n\n =================================================================\n ==672994==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7fa76f672000 at pc 0x7fa7726f75b6 bp 0x7ffe41bdda70 sp 0x7ffe41bdd220\n READ of size 4097 at 0x7fa76f672000 thread T0\n #0 0x7fa7726f75b5 (/lib/x86_64-linux-gnu/libasan.so.6+0x4f5b5)\n #1 0x562ae0c9c40e in regexec_buf /home/peff/compile/git/git-compat-util.h:1117\n #2 0x562ae0c9c40e in diff_grep /home/peff/compile/git/diffcore-pickaxe.c:52\n #3 0x562ae0c9cc28 in pickaxe_match /home/peff/compile/git/diffcore-pickaxe.c:166\n [...]\n\nIn this case we're looking in a buffer which was mmap'd via\nreuse_worktree_file(), and whose size is 4096 bytes. But libasan's\nregex tries to look at byte 4097 anyway! If we tweak Git like this:\n\n diff --git a/diff.c b/diff.c\n index 8e2914c031..cfae60c120 100644\n --- a/diff.c\n +++ b/diff.c\n @@ -3880,7 +3880,7 @@ static int reuse_worktree_file(struct index_state *istate,\n */\n if (ce_uptodate(ce) ||\n (!lstat(name, &st) && !ie_match_stat(istate, ce, &st, 0)))\n - return 1;\n + return 0;\n\n return 0;\n }\n\nto use a regular buffer (with a trailing NUL) instead of an mmap, then\nthe complaint goes away.\n\nThe other failures are actually diff output with an incorrect funcname\nheader. If I instrument xdiff to show the funcname matching like so:\n\n diff --git a/xdiff-interface.c b/xdiff-interface.c\n index 8509f9ea22..f6c3dc1986 100644\n --- a/xdiff-interface.c\n +++ b/xdiff-interface.c\n @@ -197,6 +197,7 @@ struct ff_regs {\n \tstruct ff_reg {\n \t\tregex_t re;\n \t\tint negate;\n +\t\tchar *printable;\n \t} *array;\n };\n\n @@ -218,7 +219,12 @@ static long ff_regexp(const char *line, long len,\n\n \tfor (i = 0; i < regs->nr; i++) {\n \t\tstruct ff_reg *reg = regs->array + i;\n -\t\tif (!regexec_buf(®->re, line, len, 2, pmatch, 0)) {\n +\t\tint ret = regexec_buf(®->re, line, len, 2, pmatch, 0);\n +\t\twarning(\"regexec %s:\\n regex: %s\\n buf: %.*s\",\n +\t\t\tret == 0 ? \"matched\" : \"did not match\",\n +\t\t\treg->printable,\n +\t\t\t(int)len, line);\n +\t\tif (!ret) {\n \t\t\tif (reg->negate)\n \t\t\t\treturn -1;\n \t\t\tbreak;\n @@ -264,6 +270,7 @@ void xdiff_set_find_func(xdemitconf_t *xecfg, const char *value, int cflags)\n \t\t\texpression = value;\n \t\tif (regcomp(®->re, expression, cflags))\n \t\t\tdie(\"Invalid regexp to look for hunk header: %s\", expression);\n +\t\treg->printable = xstrdup(expression);\n \t\tfree(buffer);\n \t\tvalue = ep + 1;\n \t}\n\nthen when compiling with ASan and gcc-10, running the diff from t4018.66\nproduces this:\n\n $ git diff -U1 cpp-skip-access-specifiers\n warning: regexec did not match:\n regex: ^[ ]*[A-Za-z_][A-Za-z_0-9]*:[[:space:]]*($|/[/*])\n buf: private:\n warning: regexec matched:\n regex: ^((::[[:space:]]*)?[A-Za-z_].*)$\n buf: private:\n diff --git a/cpp-skip-access-specifiers b/cpp-skip-access-specifiers\n index 4d4a9db..ebd6f42 100644\n --- a/cpp-skip-access-specifiers\n +++ b/cpp-skip-access-specifiers\n @@ -6,3 +6,3 @@ private:\n void DoSomething();\n int ChangeMe;\n };\n void DoSomething();\n - int ChangeMe;\n + int IWasChanged;\n };\n\nThat first regex should match (and is negated, so it should be telling\nus _not_ to match \"private:\"). But it wouldn't if regexec() is looking\nat the whole buffer, and not just the length-limited line we've fed to\nregexec_buf(). So this is consistent again with REG_STARTEND being\nignored.\n\nThe correct output (compiling without ASan, or gcc-9 with Asan) looks\nlike this:\n\n warning: regexec matched:\n regex: ^[ ]*[A-Za-z_][A-Za-z_0-9]*:[[:space:]]*($|/[/*])\n buf: private:\n [...more lines that we end up not using...]\n warning: regexec matched:\n regex: ^((::[[:space:]]*)?[A-Za-z_].*)$\n buf: class RIGHT : public Baseclass\n diff --git a/cpp-skip-access-specifiers b/cpp-skip-access-specifiers\n index 4d4a9db..ebd6f42 100644\n --- a/cpp-skip-access-specifiers\n +++ b/cpp-skip-access-specifiers\n @@ -6,3 +6,3 @@ class RIGHT : public Baseclass\n void DoSomething();\n - int ChangeMe;\n + int IWasChanged;\n };\n\nSo it really does seem like libasan's regex engine is ignoring\nREG_STARTEND. We should be able to work around it by compiling with\nNO_REGEX, which would use our local regexec(). But to make matters even\nmore interesting, this isn't enough by itself.\n\nBecause ASan has support from the compiler, it doesn't seem to intercept\nour call to regexec() at the dynamic library level. It actually\nrecognizes when we are compiling a call to regexec() and replaces it\nwith ASan-specific code at that point. And unlike most of our other\ncompat code, where we might have git_mmap() or similar, the actual\nsymbol name in the compiled compat/regex code is regexec(). So just\ncompiling with NO_REGEX isn't enough; we still end up in libasan!\n\nWe can work around that by having the preprocessor replace regexec with\ngit_regexec (both in the callers and in the actual implementation), and\nwe truly end up with a call to our custom regex code, even when\ncompiling with ASan. That's probably a good thing to do anyway, as it\nmeans anybody looking at the symbols later (e.g., in a debugger) would\nhave a better indication of which function is which. So we'll do the\nsame for the other common regex functions (even though just regexec() is\nenough to fix this ASan problem).\n\nSigned-off-by: Jeff King \n---\n Makefile | 3 +++\n compat/regex/regex.h | 5 +++++\n 2 files changed, 8 insertions(+)\n\ndiff --git a/Makefile b/Makefile\nindex 09f98b777c..9cd9826800 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1220,6 +1220,9 @@ endif\n ifneq ($(filter leak,$(SANITIZERS)),)\n BASIC_CFLAGS += -DSUPPRESS_ANNOTATED_LEAKS\n endif\n+ifneq ($(filter address,$(SANITIZERS)),)\n+NO_REGEX = NeededForASAN\n+endif\n endif\n \n ifndef sysconfdir\ndiff --git a/compat/regex/regex.h b/compat/regex/regex.h\nindex 08a2609663..2d3412860d 100644\n--- a/compat/regex/regex.h\n+++ b/compat/regex/regex.h\n@@ -41,6 +41,11 @@\n extern \"C\" {\n #endif\n \n+#define regcomp git_regcomp\n+#define regexec git_regexec\n+#define regerror git_regerror\n+#define regfree git_regfree\n+\n /* The following two types have to be signed and unsigned integer type\n wide enough to hold a value of a pointer. For most ANSI compilers\n ptrdiff_t and size_t should be likely OK. Still size of these two\n-- \n2.25.0.318.gee4019ba55\n"},{"id":"389908","messageId":"20200116183423.GA3011203@coredump.intra.peff.net","threadId":"52649","inReplyTo":"20200116175138.GA691238@coredump.intra.peff.net","subject":"[PATCH] t4018: drop \"debugging\" cat from hunk-header tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-01-16T18:34:23Z","receivedAt":"2020-01-16T18:34:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We run a series of hunk-header tests in a loop, and each one does this:\n\n test_when_finished 'cat actual' && # for debugging only\n\nThis is pretty pointless. When the test succeeds, we waste time running\na useless cat process. If you're debugging a failure with \"-i\", then we\nwon't run the when-finished part at all. So it helps only if you're\nrunning with something like \"--verbose-log\".\n\nSince we expect the tests to succeed most of the time, a better way to\ndo this would be a helper that checks the output and dumps \"actual\" only\nwhen it fails. But it's probably not even worth the effort, as anyone\ndebugging a failure could just run with \"-i\" and investigate the\n\"actual\" file themselves.\n\nSigned-off-by: Jeff King \n---\nJust noticed this when working with t4018 on an unrelated problem.\n\nI could be convinced otherwise on the final paragraph, but I think it\nwould only be worth it if we added a general test_grep() helper and used\nit in more places.\n\n t/t4018-diff-funcname.sh | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh\nindex c0f4839543..02255a08bf 100755\n--- a/t/t4018-diff-funcname.sh\n+++ b/t/t4018-diff-funcname.sh\n@@ -106,7 +106,6 @@ do\n \t\tresult=success\n \tfi\n \ttest_expect_$result \"hunk header: $i\" \"\n-\t\ttest_when_finished 'cat actual' &&\t# for debugging only\n \t\tgit diff -U1 $i >actual &&\n \t\tgrep '@@ .* @@.*RIGHT' actual\n \t\"\n-- \n2.25.0.318.gee4019ba55\n"},{"id":"389951","messageId":"20200116225655.GD71299@google.com","threadId":"52649","inReplyTo":"20200116183423.GA3011203@coredump.intra.peff.net","subject":"Re: [PATCH] t4018: drop \"debugging\" cat from hunk-header tests","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-01-16T22:56:55Z","receivedAt":"2020-01-16T22:56:59Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> We run a series of hunk-header tests in a loop, and each one does this:\n>\n> test_when_finished 'cat actual' && # for debugging only\n>\n> This is pretty pointless. When the test succeeds, we waste time running\n> a useless cat process. If you're debugging a failure with \"-i\", then we\n> won't run the when-finished part at all. So it helps only if you're\n> running with something like \"--verbose-log\".\n>\n> Since we expect the tests to succeed most of the time, a better way to\n> do this would be a helper that checks the output and dumps \"actual\" only\n> when it fails. But it's probably not even worth the effort, as anyone\n> debugging a failure could just run with \"-i\" and investigate the\n> \"actual\" file themselves.\n>\n> Signed-off-by: Jeff King \n> ---\n> Just noticed this when working with t4018 on an unrelated problem.\n>\n> I could be convinced otherwise on the final paragraph, but I think it\n> would only be worth it if we added a general test_grep() helper and used\n> it in more places.\n>\n> t/t4018-diff-funcname.sh | 1 -\n> 1 file changed, 1 deletion(-)\n\nReviewed-by: Jonathan Nieder \n\nThanks.\n\nSure, a test_grep would be nice for CI cases (especially for\nHeisenbugs), but in its absence I agree that this patch is the right\nthing to do.\n"},{"id":"390001","messageId":"xmqqr1zydlwt.fsf@gitster-ct.c.googlers.com","threadId":"52649","inReplyTo":"20200116175138.GA691238@coredump.intra.peff.net","subject":"Re: [PATCH] Makefile: use compat regex with SANITIZE=address","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-17T17:37:22Z","receivedAt":"2020-01-17T17:37:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Recent versions of the gcc and clang Address Sanitizer produce test\n> failures related to regexec(). This triggers with gcc-10 and clang-8\n> (but not gcc-9 nor clang-7). Running:\n>\n> make CC=gcc-10 SANITIZE=address test\n>\n> results in failures in t4018, t3206, and t4062.\n> ...\n> We can work around that by having the preprocessor replace regexec with\n> git_regexec (both in the callers and in the actual implementation), and\n> we truly end up with a call to our custom regex code, even when\n> compiling with ASan. That's probably a good thing to do anyway, as it\n> means anybody looking at the symbols later (e.g., in a debugger) would\n> have a better indication of which function is which. So we'll do the\n> same for the other common regex functions (even though just regexec() is\n> enough to fix this ASan problem).\n>\n> Signed-off-by: Jeff King \n> ---\n> Makefile | 3 +++\n> compat/regex/regex.h | 5 +++++\n> 2 files changed, 8 insertions(+)\n\nI guess we should treat this the same way as the recent vcproj \"fix\"\nby Dscho, i.e. fast-track to 'maint' to ensure that all public\nintegration branches has it soonish?\n\nThanks.\n"},{"id":"390002","messageId":"20200117174931.GA8958@coredump.intra.peff.net","threadId":"52649","inReplyTo":"xmqqr1zydlwt.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] Makefile: use compat regex with SANITIZE=address","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-01-17T17:49:31Z","receivedAt":"2020-01-17T17:49:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 17, 2020 at 09:37:22AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Recent versions of the gcc and clang Address Sanitizer produce test\n> > failures related to regexec(). This triggers with gcc-10 and clang-8\n> > (but not gcc-9 nor clang-7). Running:\n> >\n> > make CC=gcc-10 SANITIZE=address test\n> >\n> > results in failures in t4018, t3206, and t4062.\n> [...]\n> I guess we should treat this the same way as the recent vcproj \"fix\"\n> by Dscho, i.e. fast-track to 'maint' to ensure that all public\n> integration branches has it soonish?\n\nI don't think there's a huge rush. It only triggers ASan and recent\ncompilers, so AFAIK nobody has been hitting this in CI. I occasionally\ntest with whatever is the most recent compiler in Debian unstable to see\nif it turns up any interesting new warnings or errors (but gcc-9 is\nstill the default there).\n\n-Peff\n"},{"id":"390033","messageId":"20200117233931.GB6570@camp.crustytoothpaste.net","threadId":"52649","inReplyTo":"20200117174931.GA8958@coredump.intra.peff.net","subject":"Re: [PATCH] Makefile: use compat regex with SANITIZE=address","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-01-17T23:39:31Z","receivedAt":"2020-01-17T23:39:39Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-01-17 at 17:49:31, Jeff King wrote:\n> On Fri, Jan 17, 2020 at 09:37:22AM -0800, Junio C Hamano wrote:\n> \n> > Jeff King writes:\n> > \n> > > Recent versions of the gcc and clang Address Sanitizer produce test\n> > > failures related to regexec(). This triggers with gcc-10 and clang-8\n> > > (but not gcc-9 nor clang-7). Running:\n> > >\n> > > make CC=gcc-10 SANITIZE=address test\n> > >\n> > > results in failures in t4018, t3206, and t4062.\n> > [...]\n> > I guess we should treat this the same way as the recent vcproj \"fix\"\n> > by Dscho, i.e. fast-track to 'maint' to ensure that all public\n> > integration branches has it soonish?\n> \n> I don't think there's a huge rush. It only triggers ASan and recent\n> compilers, so AFAIK nobody has been hitting this in CI. I occasionally\n> test with whatever is the most recent compiler in Debian unstable to see\n> if it turns up any interesting new warnings or errors (but gcc-9 is\n> still the default there).\n\nI've reported this as a bug to Debian on the clang-8 and libasan6\npackages, along with a trivial test case. Hopefully this will get fixed\nsoon.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"390036","messageId":"20200118033455.GA33232@coredump.intra.peff.net","threadId":"52649","inReplyTo":"20200117233931.GB6570@camp.crustytoothpaste.net","subject":"Re: [PATCH] Makefile: use compat regex with SANITIZE=address","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-01-18T03:34:55Z","receivedAt":"2020-01-18T03:38:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 17, 2020 at 11:39:31PM +0000, brian m. carlson wrote:\n\n> > I don't think there's a huge rush. It only triggers ASan and recent\n> > compilers, so AFAIK nobody has been hitting this in CI. I occasionally\n> > test with whatever is the most recent compiler in Debian unstable to see\n> > if it turns up any interesting new warnings or errors (but gcc-9 is\n> > still the default there).\n> \n> I've reported this as a bug to Debian on the clang-8 and libasan6\n> packages, along with a trivial test case. Hopefully this will get fixed\n> soon.\n\nThanks. I'm not entirely convinced it's a bug (after all, we are\nsquatting on a well-known POSIX function name), but we'll see what they\nsay. :)\n\n-Peff\n"}]}