{"thread":{"id":"46344","subject":"[PATCH 0/5] building git with clang/gcc address sanitizer","startedAt":"2017-07-10T13:24:25Z","lastAt":"2017-07-16T11:03:01Z","messageCount":18,"participants":["Jeff King","Lars Schneider","Junio C Hamano","Ramsay Jones","René Scharfe"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"324056","messageId":"20170710132418.d6bvzxwvbejretb4@sigill.intra.peff.net","threadId":"46344","inReplyTo":null,"subject":"[PATCH 0/5] building git with clang/gcc address sanitizer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-10T13:24:18Z","receivedAt":"2017-07-10T13:24:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I mentioned recently that I sometimes run the test suite with ASan, so\nhere are a few tweaks to make this easier (most of which I've been\ncarrying in my personal config.mak for a few years).\n\nIn my experience ASan does at least as well as valgrind at finding\nmemory bugs, and runs way faster. With this series, running:\n\n  make SANITIZE=address test\n\ntakes about 4.5 minutes on my machine. A normal build+test is about 1.5\nminutes on the same machine. I haven't timed a full run with --valgrind\nrecently, but I know that it is much much slower. :)\n\nIf you want to see it in action, you can do:\n\n  make SANITIZE=address\n  ./git log -g HEAD HEAD >/dev/null\n\nwhich finds a bug I recently fixed (but the fix isn't in master yet).\n\nThere are other sanitizers, too. I _thought_ I had\n\n  make SANITIZE=undefined test\n\nrunning cleanly at one point, but it's definitely not clean now. Patch 5\nhelps a little by disabling unaligned loads in our get_be32() macros.\nOnce upon a time I had to set INTERNAL_QSORT, but it isn't necessary\nanymore since everything goes through sane_qsort(). Most of the\nremaining bugs are of a similar form, doing something like:\n\n  memcpy(NULL, NULL, 0);\n\nI don't know if it's worth having a sane_memcpy() there, or simply\ntweaking the code to avoid the call (almost all of them are a single\ncall from apply.c:2870).\n\nIt looks like we also have a case of shifting off the left side of a\nsigned int, which is undefined.\n\nYou can also try:\n\n  make SANITIZE=thread test\n\nbut it's not clean. I poked at some of the errors, and I don't think\nthere a problem in practice (though they may be worth cleaning up in the\nname of code hygiene).\n\nThere's also:\n\n  make SANITIZE=memory test\n\nThis is clang-only. It's supposed to find uses of uninitialized memory.\nBut it complains about writing out anything that zlib has touched. I\nseem to recall that valgrind had a similar problem. I'm not sure what\nzlib does to confuse these analyzers. For valgrind we were able to add a\nsuppression. We could probably do the same here, but I haven't looked\ninto it.\n\n  [1/5]: test-lib: set ASAN_OPTIONS variable before we run git\n  [2/5]: test-lib: turn on ASan abort_on_error by default\n  [3/5]: Makefile: add helper for compiling with -fsanitize\n  [4/5]: Makefile: turn off -fomit-frame-pointer with sanitizers\n  [5/5]: Makefile: disable unaligned loads with UBSan\n\n Makefile      |  8 ++++++++\n t/test-lib.sh | 11 ++++++++---\n 2 files changed, 16 insertions(+), 3 deletions(-)\n\n-Peff\n"},{"id":"324057","messageId":"20170710132435.ncebevyvv2uwtzxp@sigill.intra.peff.net","threadId":"46344","inReplyTo":"20170710132418.d6bvzxwvbejretb4@sigill.intra.peff.net","subject":"[PATCH 1/5] test-lib: set ASAN_OPTIONS variable before we run git","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-10T13:24:35Z","receivedAt":"2017-07-10T13:24:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We turn off ASan's leak detection by default in the test\nsuite because it's too noisy. But we don't do so until\npart-way through test-lib. This is before we've run any\ntests, but after we do our initial \"./git\" to see if the\nbinary has even been built.\n\nWhen built with clang, this seems to work fine. However,\nusing \"gcc -fsanitize=address\", the leak checker seems to\ncomplain more aggressively:\n\n  $ ./git\n  ...\n  ==5352==ERROR: LeakSanitizer: detected memory leaks\n  Direct leak of 2 byte(s) in 1 object(s) allocated from:\n      #0 0x7f120e7afcf8 in malloc (/usr/lib/x86_64-linux-gnu/libasan.so.3+0xc1cf8)\n      #1 0x559fc2a3ce41 in do_xmalloc /home/peff/compile/git/wrapper.c:60\n      #2 0x559fc2a3cf1a in do_xmallocz /home/peff/compile/git/wrapper.c:100\n      #3 0x559fc2a3d0ad in xmallocz /home/peff/compile/git/wrapper.c:108\n      #4 0x559fc2a3d0ad in xmemdupz /home/peff/compile/git/wrapper.c:124\n      #5 0x559fc2a3d0ad in xstrndup /home/peff/compile/git/wrapper.c:130\n      #6 0x559fc274535a in main /home/peff/compile/git/common-main.c:39\n      #7 0x7f120dabd2b0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x202b0)\n\nThis is a leak in the sense that we never free it, but it's\nin a global that is meant to last the whole program. So it's\nnot really interesting or in need of fixing. And at any\nrate, mentioning leaks outside of the test_expect blocks is\ncertainly unwelcome, as it pollutes stderr.\n\nLet's bump the setting of ASAN_OPTIONS higher in test-lib.sh\nto catch our initial \"can we even run git?\" test.  While\nwe're at it, we can add a comment to make it a bit less\ninscrutable.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/test-lib.sh | 11 ++++++++---\n 1 file changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 2306574dc..961194a50 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -36,6 +36,14 @@ then\n fi\n GIT_BUILD_DIR=\"$TEST_DIRECTORY\"/..\n \n+# If we were built with ASAN, it may complain about leaks\n+# of program-lifetime variables. Disable it by default to lower\n+# the noise level. This needs to happen at the start of the script,\n+# before we even do our \"did we build git yet\" check (since we don't\n+# want that one to complain to stderr).\n+: ${ASAN_OPTIONS=detect_leaks=0}\n+export ASAN_OPTIONS\n+\n ################################################################\n # It appears that people try to run tests without building...\n \"$GIT_BUILD_DIR/git\" >/dev/null\n@@ -148,9 +156,6 @@ else\n \t}\n fi\n \n-: ${ASAN_OPTIONS=detect_leaks=0}\n-export ASAN_OPTIONS\n-\n # Protect ourselves from common misconfiguration to export\n # CDPATH into the environment\n unset CDPATH\n-- \n2.13.2.1071.gcd8104b61\n\n"},{"id":"324058","messageId":"20170710132442.epsox4rawxlyvrw3@sigill.intra.peff.net","threadId":"46344","inReplyTo":"20170710132418.d6bvzxwvbejretb4@sigill.intra.peff.net","subject":"[PATCH 3/5] Makefile: add helper for compiling with -fsanitize","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-10T13:24:42Z","receivedAt":"2017-07-10T13:24:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"You can already build and test with ASan by doing:\n\n  make CFLAGS=-fsanitize=address test\n\nbut there are a few slight annoyances:\n\n  1. It's a little long to type.\n\n  2. It override your CFLAGS completely. You'd probably\n     still want -O2, for instance.\n\n  3. It's a good idea to also turn off \"recovery\", which\n     lets the program keep running after a problem is\n     detected (with the intention of finding as many bugs as\n     possible in a given run). Since Git's test suite should\n     generally run without triggering any problems, it's\n     better to abort immediately and fail the test when we\n     do find an issue.\n\nWith this patch, all of that happens automatically when you\nrun:\n\n  make SANITIZE=address test\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Makefile | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/Makefile b/Makefile\nindex 9c9c42f8f..59f6bdcd7 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1012,6 +1012,10 @@ ifdef DEVELOPER\n CFLAGS += $(DEVELOPER_CFLAGS)\n endif\n \n+ifdef SANITIZE\n+BASIC_CFLAGS += -fsanitize=$(SANITIZE) -fno-sanitize-recover=$(SANITIZE)\n+endif\n+\n ifndef sysconfdir\n ifeq ($(prefix),/usr)\n sysconfdir = /etc\n-- \n2.13.2.1071.gcd8104b61\n\n"},{"id":"324059","messageId":"20170710132438.euiamhjgqzkzkefg@sigill.intra.peff.net","threadId":"46344","inReplyTo":"20170710132418.d6bvzxwvbejretb4@sigill.intra.peff.net","subject":"[PATCH 2/5] test-lib: turn on ASan abort_on_error by default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-10T13:24:39Z","receivedAt":"2017-07-10T13:24:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"By default, ASan will exit with code 1 when it sees an\nerror. This means we'll notice a problem when we expected\ngit to succeed, but not in a test_must_fail block.\n\nLet's ask it to actually raise SIGABRT instead. That will\ngive us a signal death that test_must_fail will notice. As a\nbonus, it may also leave a coredump, which can be handy for\ndigging into a failure.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/test-lib.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 961194a50..1b6e53f78 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -41,7 +41,7 @@ GIT_BUILD_DIR=\"$TEST_DIRECTORY\"/..\n # the noise level. This needs to happen at the start of the script,\n # before we even do our \"did we build git yet\" check (since we don't\n # want that one to complain to stderr).\n-: ${ASAN_OPTIONS=detect_leaks=0}\n+: ${ASAN_OPTIONS=detect_leaks=0:abort_on_error=1}\n export ASAN_OPTIONS\n \n ################################################################\n-- \n2.13.2.1071.gcd8104b61\n\n"},{"id":"324060","messageId":"20170710132450.yxojk5cybejy7und@sigill.intra.peff.net","threadId":"46344","inReplyTo":"20170710132418.d6bvzxwvbejretb4@sigill.intra.peff.net","subject":"[PATCH 5/5] Makefile: disable unaligned loads with UBSan","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-10T13:24:50Z","receivedAt":"2017-07-10T13:24:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The undefined behavior sanitizer complains about unaligned\nloads, even if they're OK for a particular platform in\npractice. It's possible that they _are_ a problem, of\ncourse, but since it's a known tradeoff the UBSan errors are\njust noise.\n\nLet's quiet it automatically by building with\nNO_UNALIGNED_LOADS when SANITIZE=undefined is in use.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Makefile | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/Makefile b/Makefile\nindex cc03b3c95..3c341b2a6 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1015,6 +1015,9 @@ endif\n ifdef SANITIZE\n BASIC_CFLAGS += -fsanitize=$(SANITIZE) -fno-sanitize-recover=$(SANITIZE)\n BASIC_CFLAGS += -fno-omit-frame-pointer\n+ifeq ($(SANITIZE),undefined)\n+BASIC_CFLAGS += -DNO_UNALIGNED_LOADS\n+endif\n endif\n \n ifndef sysconfdir\n-- \n2.13.2.1071.gcd8104b61\n"},{"id":"324061","messageId":"20170710132447.atwjp2pj6xk6md4i@sigill.intra.peff.net","threadId":"46344","inReplyTo":"20170710132418.d6bvzxwvbejretb4@sigill.intra.peff.net","subject":"[PATCH 4/5] Makefile: turn off -fomit-frame-pointer with sanitizers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-10T13:24:47Z","receivedAt":"2017-07-10T13:25:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The ASan manual recommends disabling this optimization, as\nit can make the backtraces produced by the tool harder to\nfollow (and since this is a test-debug build, we don't care\nabout squeezing out every last drop of performance).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Makefile | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/Makefile b/Makefile\nindex 59f6bdcd7..cc03b3c95 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1014,6 +1014,7 @@ endif\n \n ifdef SANITIZE\n BASIC_CFLAGS += -fsanitize=$(SANITIZE) -fno-sanitize-recover=$(SANITIZE)\n+BASIC_CFLAGS += -fno-omit-frame-pointer\n endif\n \n ifndef sysconfdir\n-- \n2.13.2.1071.gcd8104b61\n\n"},{"id":"324063","messageId":"20170710133040.yom65mjol3nmf2it@sigill.intra.peff.net","threadId":"46344","inReplyTo":"20170710132418.d6bvzxwvbejretb4@sigill.intra.peff.net","subject":"Re: [PATCH 0/5] building git with clang/gcc address sanitizer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-10T13:30:40Z","receivedAt":"2017-07-10T13:30:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 10, 2017 at 09:24:18AM -0400, Jeff King wrote:\n\n> You can also try:\n> \n>   make SANITIZE=thread test\n> \n> but it's not clean. I poked at some of the errors, and I don't think\n> there a problem in practice (though they may be worth cleaning up in the\n> name of code hygiene).\n\nHere's an example that I fixed up long ago, but never quite had the\nstomach to seriously propose.\n\nIf it were the last one, it might be worth applying to get a clean run\nof \"make test\", but I think it's just one of many.\n\n-- >8 --\nDate: Mon, 8 Dec 2014 03:02:34 -0500\nSubject: [PATCH] transport-helper: initialize debug flag before starting\n threads\n\nThe transport_debug() function uses a static variable to\nlazily cache the boolean value of $TRANSPORT_DEBUG. If a\nprogram uses transport-helper's bidirectional_transfer_loop\n(as remote-ext and remote-fd do), then two threads may both\ntry to write the variable at the same time.\n\nWe can fix this by initializing the variable right before\nstarting the threads.\n\nNoticed by \"-fsanitize=thread\". This probably isn't a\nproblem in practice, as both threads will write the same\nvalue, and we are unlikely to see a torn write on an \"int\"\n(i.e., on sane platforms a write to an int is atomic).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n transport-helper.c | 18 ++++++++++++++----\n 1 file changed, 14 insertions(+), 4 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 33cff38cc..76f19ddb0 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -1113,17 +1113,23 @@ int transport_helper_init(struct transport *transport, const char *name)\n /* This should be enough to hold debugging message. */\n #define PBUFFERSIZE 8192\n \n+static int transport_debug_enabled(void)\n+{\n+\tstatic int debug_enabled = -1;\n+\n+\tif (debug_enabled < 0)\n+\t\tdebug_enabled = getenv(\"GIT_TRANSLOOP_DEBUG\") ? 1 : 0;\n+\treturn debug_enabled;\n+}\n+\n /* Print bidirectional transfer loop debug message. */\n __attribute__((format (printf, 1, 2)))\n static void transfer_debug(const char *fmt, ...)\n {\n \tva_list args;\n \tchar msgbuf[PBUFFERSIZE];\n-\tstatic int debug_enabled = -1;\n \n-\tif (debug_enabled < 0)\n-\t\tdebug_enabled = getenv(\"GIT_TRANSLOOP_DEBUG\") ? 1 : 0;\n-\tif (!debug_enabled)\n+\tif (!transport_debug_enabled())\n \t\treturn;\n \n \tva_start(args, fmt);\n@@ -1292,6 +1298,10 @@ static int tloop_spawnwait_tasks(struct bidirectional_transfer_state *s)\n \tpthread_t ptg_thread;\n \tint err;\n \tint ret = 0;\n+\n+\t/* initialize static global debug flag as a side effect */\n+\ttransport_debug_enabled();\n+\n \terr = pthread_create(&gtp_thread, NULL, udt_copy_task_routine,\n \t\t&s->gtp);\n \tif (err)\n-- \n2.13.2.1071.gcd8104b61\n\n"},{"id":"324072","messageId":"40D62A0D-5636-4EC2-ABCB-14175FC541F9@gmail.com","threadId":"46344","inReplyTo":"20170710132418.d6bvzxwvbejretb4@sigill.intra.peff.net","subject":"Re: [PATCH 0/5] building git with clang/gcc address sanitizer","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-07-10T14:40:42Z","receivedAt":"2017-07-10T14:40:50Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 10 Jul 2017, at 15:24, Jeff King <peff@peff.net> wrote:\n> \n> I mentioned recently that I sometimes run the test suite with ASan, so\n> here are a few tweaks to make this easier (most of which I've been\n> carrying in my personal config.mak for a few years).\n> \n> In my experience ASan does at least as well as valgrind at finding\n> memory bugs, and runs way faster. With this series, running:\n> \n>  make SANITIZE=address test\n> \n> takes about 4.5 minutes on my machine. A normal build+test is about 1.5\n> minutes on the same machine. I haven't timed a full run with --valgrind\n> recently, but I know that it is much much slower. :)\n> \n> If you want to see it in action, you can do:\n> \n>  make SANITIZE=address\n>  ./git log -g HEAD HEAD >/dev/null\n> \n> which finds a bug I recently fixed (but the fix isn't in master yet).\n\nDo you think it would make sense to run these sanitizers on TravisCI\nto ensure they keep clean? If yes, should we run only \"address\" or all\nof them (if they run clean)?\n\n- Lars\n\n\n> \n> There are other sanitizers, too. I _thought_ I had\n> \n>  make SANITIZE=undefined test\n> \n> running cleanly at one point, but it's definitely not clean now. Patch 5\n> helps a little by disabling unaligned loads in our get_be32() macros.\n> Once upon a time I had to set INTERNAL_QSORT, but it isn't necessary\n> anymore since everything goes through sane_qsort(). Most of the\n> remaining bugs are of a similar form, doing something like:\n> \n>  memcpy(NULL, NULL, 0);\n> \n> I don't know if it's worth having a sane_memcpy() there, or simply\n> tweaking the code to avoid the call (almost all of them are a single\n> call from apply.c:2870).\n> \n> It looks like we also have a case of shifting off the left side of a\n> signed int, which is undefined.\n> \n> You can also try:\n> \n>  make SANITIZE=thread test\n> \n> but it's not clean. I poked at some of the errors, and I don't think\n> there a problem in practice (though they may be worth cleaning up in the\n> name of code hygiene).\n> \n> There's also:\n> \n>  make SANITIZE=memory test\n> \n> This is clang-only. It's supposed to find uses of uninitialized memory.\n> But it complains about writing out anything that zlib has touched. I\n> seem to recall that valgrind had a similar problem. I'm not sure what\n> zlib does to confuse these analyzers. For valgrind we were able to add a\n> suppression. We could probably do the same here, but I haven't looked\n> into it.\n> \n>  [1/5]: test-lib: set ASAN_OPTIONS variable before we run git\n>  [2/5]: test-lib: turn on ASan abort_on_error by default\n>  [3/5]: Makefile: add helper for compiling with -fsanitize\n>  [4/5]: Makefile: turn off -fomit-frame-pointer with sanitizers\n>  [5/5]: Makefile: disable unaligned loads with UBSan\n> \n> Makefile      |  8 ++++++++\n> t/test-lib.sh | 11 ++++++++---\n> 2 files changed, 16 insertions(+), 3 deletions(-)\n> \n> -Peff\n\n"},{"id":"324076","messageId":"20170710155831.3zxijp7bvbquvlau@sigill.intra.peff.net","threadId":"46344","inReplyTo":"40D62A0D-5636-4EC2-ABCB-14175FC541F9@gmail.com","subject":"Re: [PATCH 0/5] building git with clang/gcc address sanitizer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-10T15:58:31Z","receivedAt":"2017-07-10T15:58:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 10, 2017 at 04:40:42PM +0200, Lars Schneider wrote:\n\n> > If you want to see it in action, you can do:\n> > \n> >  make SANITIZE=address\n> >  ./git log -g HEAD HEAD >/dev/null\n> > \n> > which finds a bug I recently fixed (but the fix isn't in master yet).\n> \n> Do you think it would make sense to run these sanitizers on TravisCI\n> to ensure they keep clean? If yes, should we run only \"address\" or all\n> of them (if they run clean)?\n\nMaybe. It's expensive and it's relatively rare that it catches anything.\nI used to run valgrind (which is even more expensive) once every release\nor so. This is much cheaper, but I've noticed that the Travis\nenvironment is a lot slower than my laptop. So it might take an hour to\nrun there, which I think would trigger some timeouts?\n\nI guess the best way is to try it and see. I probably wouldn't do an\nASan run for each environment, but just one Linux ASan run, due to the\nCPU expense. (TBH, I think the existing gcc versus clang on both\nplatforms is already slight overkill. But I guess if we have CPU to\nburn, more coverage is better than less).\n\nI think \"address\" is the only one that runs clean right now. With some\nwork I think we could get \"undefined\" to run clean. The others, I'm not\nso sure.\n\nSome of them can actually be combined in a single build, but I'd have to\ndig into the documentation to see which (I think \"thread\" and \"address\"\ndon't work well together, but \"undefined\" and \"address\" might). My\nSANITIZE trick doesn't handle multiple entries, but it could probably be\ntaught to.\n\n-Peff\n"},{"id":"324087","messageId":"xmqqinj06wil.fsf@gitster.mtv.corp.google.com","threadId":"46344","inReplyTo":"20170710132438.euiamhjgqzkzkefg@sigill.intra.peff.net","subject":"Re: [PATCH 2/5] test-lib: turn on ASan abort_on_error by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-10T17:18:26Z","receivedAt":"2017-07-10T17:18:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> By default, ASan will exit with code 1 when it sees an\n> error. This means we'll notice a problem when we expected\n> git to succeed, but not in a test_must_fail block.\n>\n> Let's ask it to actually raise SIGABRT instead. That will\n> give us a signal death that test_must_fail will notice. As a\n> bonus, it may also leave a coredump, which can be handy for\n> digging into a failure.\n\nWell thought-out.  Thanks.\n\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  t/test-lib.sh | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 961194a50..1b6e53f78 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -41,7 +41,7 @@ GIT_BUILD_DIR=\"$TEST_DIRECTORY\"/..\n>  # the noise level. This needs to happen at the start of the script,\n>  # before we even do our \"did we build git yet\" check (since we don't\n>  # want that one to complain to stderr).\n> -: ${ASAN_OPTIONS=detect_leaks=0}\n> +: ${ASAN_OPTIONS=detect_leaks=0:abort_on_error=1}\n>  export ASAN_OPTIONS\n>  \n>  ################################################################\n"},{"id":"324091","messageId":"xmqqefto6vqb.fsf@gitster.mtv.corp.google.com","threadId":"46344","inReplyTo":"20170710132442.epsox4rawxlyvrw3@sigill.intra.peff.net","subject":"Re: [PATCH 3/5] Makefile: add helper for compiling with -fsanitize","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-10T17:35:24Z","receivedAt":"2017-07-10T17:35:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> You can already build and test with ASan by doing:\n>\n>   make CFLAGS=-fsanitize=address test\n>\n> but there are a few slight annoyances:\n>\n>   1. It's a little long to type.\n>\n>   2. It override your CFLAGS completely. You'd probably\n>      still want -O2, for instance.\n>\n>   3. It's a good idea to also turn off \"recovery\", which\n>      lets the program keep running after a problem is\n>      detected (with the intention of finding as many bugs as\n>      possible in a given run). Since Git's test suite should\n>      generally run without triggering any problems, it's\n>      better to abort immediately and fail the test when we\n>      do find an issue.\n\nUnfortunately I do not think Comparing between versions in\nhttps://gcc.gnu.org/onlinedocs, it appears that -fsanitize-recover\nis not configurable for folks still with GCC 4.x series, and this\npatch is not very useful unless you disable the recovery for the\npurpose of running our tests as you said X-<.\n\n> With this patch, all of that happens automatically when you\n> run:\n>\n>   make SANITIZE=address test\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  Makefile | 4 ++++\n>  1 file changed, 4 insertions(+)\n>\n> diff --git a/Makefile b/Makefile\n> index 9c9c42f8f..59f6bdcd7 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -1012,6 +1012,10 @@ ifdef DEVELOPER\n>  CFLAGS += $(DEVELOPER_CFLAGS)\n>  endif\n>  \n> +ifdef SANITIZE\n> +BASIC_CFLAGS += -fsanitize=$(SANITIZE) -fno-sanitize-recover=$(SANITIZE)\n> +endif\n> +\n>  ifndef sysconfdir\n>  ifeq ($(prefix),/usr)\n>  sysconfdir = /etc\n"},{"id":"324092","messageId":"20170710174454.jbkednmzwts7yr7z@sigill.intra.peff.net","threadId":"46344","inReplyTo":"xmqqefto6vqb.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/5] Makefile: add helper for compiling with -fsanitize","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-10T17:44:54Z","receivedAt":"2017-07-10T17:45:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 10, 2017 at 10:35:24AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > You can already build and test with ASan by doing:\n> >\n> >   make CFLAGS=-fsanitize=address test\n> >\n> > but there are a few slight annoyances:\n> >\n> >   1. It's a little long to type.\n> >\n> >   2. It override your CFLAGS completely. You'd probably\n> >      still want -O2, for instance.\n> >\n> >   3. It's a good idea to also turn off \"recovery\", which\n> >      lets the program keep running after a problem is\n> >      detected (with the intention of finding as many bugs as\n> >      possible in a given run). Since Git's test suite should\n> >      generally run without triggering any problems, it's\n> >      better to abort immediately and fail the test when we\n> >      do find an issue.\n> \n> Unfortunately I do not think Comparing between versions in\n> https://gcc.gnu.org/onlinedocs, it appears that -fsanitize-recover\n> is not configurable for folks still with GCC 4.x series, and this\n> patch is not very useful unless you disable the recovery for the\n> purpose of running our tests as you said X-<.\n\nI didn't actually dig into the history of gcc support at all. Back in\nthe 4.x time-frame I tried using ASan and couldn't get it to work at\nall. I ended up just always building with clang (which from my\nmostly-ignorant view seems to to be the primary platform for ASan\ndevelopment).\n\nSince this is an optional build that doesn't need to be available\neverywhere, I'd actually be fine with saying \"just use clang\". But as\nfar as I can tell, gcc seems to work fine these days. I consider this\nmostly a best-effort tool.\n\nI'm also not sure of the behavior without -fno-sanitize-recover. I think\nASan may barf either way. The commit message for my config.mak from a\nyear or two ago claims that the problem was actually with UBSan. It\nwould be useful in the long run for that to work, too.\n\n-Peff\n"},{"id":"324094","messageId":"xmqq60f06u8l.fsf@gitster.mtv.corp.google.com","threadId":"46344","inReplyTo":"20170710174454.jbkednmzwts7yr7z@sigill.intra.peff.net","subject":"Re: [PATCH 3/5] Makefile: add helper for compiling with -fsanitize","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-10T18:07:38Z","receivedAt":"2017-07-10T18:07:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Mon, Jul 10, 2017 at 10:35:24AM -0700, Junio C Hamano wrote:\n>\n>> Jeff King <peff@peff.net> writes:\n>> \n>> > You can already build and test with ASan by doing:\n>> >\n>> >   make CFLAGS=-fsanitize=address test\n>> >\n>> > but there are a few slight annoyances:\n>> >\n>> >   1. It's a little long to type.\n>> >\n>> >   2. It override your CFLAGS completely. You'd probably\n>> >      still want -O2, for instance.\n>> >\n>> >   3. It's a good idea to also turn off \"recovery\", which\n>> >      lets the program keep running after a problem is\n>> >      detected (with the intention of finding as many bugs as\n>> >      possible in a given run). Since Git's test suite should\n>> >      generally run without triggering any problems, it's\n>> >      better to abort immediately and fail the test when we\n>> >      do find an issue.\n>> \n>> Unfortunately I do not think Comparing between versions in\n>> https://gcc.gnu.org/onlinedocs, it appears that -fsanitize-recover\n>> is not configurable for folks still with GCC 4.x series, and this\n>> patch is not very useful unless you disable the recovery for the\n>> purpose of running our tests as you said X-<.\n>\n> I didn't actually dig into the history of gcc support at all. Back in\n> the 4.x time-frame I tried using ASan and couldn't get it to work at\n> all. I ended up just always building with clang (which from my\n> mostly-ignorant view seems to to be the primary platform for ASan\n> development).\n>\n> Since this is an optional build that doesn't need to be available\n> everywhere, I'd actually be fine with saying \"just use clang\". But as\n> far as I can tell, gcc seems to work fine these days. I consider this\n> mostly a best-effort tool.\n>\n> I'm also not sure of the behavior without -fno-sanitize-recover. I think\n> ASan may barf either way. The commit message for my config.mak from a\n> year or two ago claims that the problem was actually with UBSan. It\n> would be useful in the long run for that to work, too.\n\nYes.  I'd agree with all of the above.  While copyediting my\nresponse, I somehow ended up removing one paragraph before that\n\"Unfortunately\" by accident X-<, but the paragraph said essentially\nthe same \"this is optional so it is a strict improvement, and I do\nagree recovery must be disabled to be useful in our context\".\n\nSorry for a possible confusion.\n"},{"id":"324103","messageId":"752db1ea-8416-3a98-babf-78b766432f0f@ramsayjones.plus.com","threadId":"46344","inReplyTo":"20170710174454.jbkednmzwts7yr7z@sigill.intra.peff.net","subject":"Re: [PATCH 3/5] Makefile: add helper for compiling with -fsanitize","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-07-10T20:02:24Z","receivedAt":"2017-07-10T20:02:32Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 10/07/17 18:44, Jeff King wrote:\n> On Mon, Jul 10, 2017 at 10:35:24AM -0700, Junio C Hamano wrote:\n> \n>> Jeff King <peff@peff.net> writes:\n>>\n>>> You can already build and test with ASan by doing:\n>>>\n>>>   make CFLAGS=-fsanitize=address test\n>>>\n>>> but there are a few slight annoyances:\n>>>\n>>>   1. It's a little long to type.\n>>>\n>>>   2. It override your CFLAGS completely. You'd probably\n>>>      still want -O2, for instance.\n>>>\n>>>   3. It's a good idea to also turn off \"recovery\", which\n>>>      lets the program keep running after a problem is\n>>>      detected (with the intention of finding as many bugs as\n>>>      possible in a given run). Since Git's test suite should\n>>>      generally run without triggering any problems, it's\n>>>      better to abort immediately and fail the test when we\n>>>      do find an issue.\n>>\n>> Unfortunately I do not think Comparing between versions in\n>> https://gcc.gnu.org/onlinedocs, it appears that -fsanitize-recover\n>> is not configurable for folks still with GCC 4.x series, and this\n>> patch is not very useful unless you disable the recovery for the\n>> purpose of running our tests as you said X-<.\n> \n> I didn't actually dig into the history of gcc support at all. Back in\n> the 4.x time-frame I tried using ASan and couldn't get it to work at\n> all. I ended up just always building with clang (which from my\n> mostly-ignorant view seems to to be the primary platform for ASan\n> development).\n> \n> Since this is an optional build that doesn't need to be available\n> everywhere, I'd actually be fine with saying \"just use clang\". But as\n> far as I can tell, gcc seems to work fine these days. I consider this\n> mostly a best-effort tool.\n> \n> I'm also not sure of the behavior without -fno-sanitize-recover. I think\n> ASan may barf either way. The commit message for my config.mak from a\n> year or two ago claims that the problem was actually with UBSan. It\n> would be useful in the long run for that to work, too.\n\nJust FYI, I had a quick look at this tonight. I applied your\npatches to master, the tried 'make SANITIZE=address test', which\nworked fine. I then tried 'make SANITIZE=undefined test' and I had\nto control+C it after nearly two hours on one test! ;-) (somewhere\nin the t4xxx - unfortunately I overwrote the output file without\nthinking).\n\n[BTW I am on Linux Mint 18.2 x86_64, gcc version 5.4.0]\n\nAfter a quick look at the ./t0000-basic.sh test, I managed to get\nthe test to complete (with 15 tests failing), with the following\npatch applied:\n\n-- >8 --\ndiff --git a/Makefile b/Makefile\nindex 3c341b2a6..8e6433738 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1016,7 +1016,7 @@ ifdef SANITIZE\n BASIC_CFLAGS += -fsanitize=$(SANITIZE) -fno-sanitize-recover=$(SANITIZE)\n BASIC_CFLAGS += -fno-omit-frame-pointer\n ifeq ($(SANITIZE),undefined)\n-BASIC_CFLAGS += -DNO_UNALIGNED_LOADS\n+BASIC_CFLAGS += -DNO_UNALIGNED_LOADS -DSHA1DC_FORCE_ALIGNED_ACCESS\n endif\n endif\n \ndiff --git a/sha1dc/sha1.c b/sha1dc/sha1.c\nindex 25eded139..3baddc636 100644\n--- a/sha1dc/sha1.c\n+++ b/sha1dc/sha1.c\n@@ -118,6 +118,10 @@\n #define SHA1DC_ALLOW_UNALIGNED_ACCESS\n #endif /*UNALIGNMENT DETECTION*/\n \n+#if defined(SHA1DC_ALLOW_UNALIGNED_ACCESS) && defined(SHA1DC_FORCE_ALIGNED_ACCESS)\n+#undef SHA1DC_ALLOW_UNALIGNED_ACCESS\n+#endif\n+\n \n #define rotate_right(x,n) (((x)>>(n))|((x)<<(32-(n))))\n #define rotate_left(x,n)  (((x)<<(n))|((x)>>(32-(n))))\n--------\n\nHmm, hopefully that is not whitespace damaged.\n\nATB,\nRamsay Jones\n\n"},{"id":"324149","messageId":"20170711044401.pf7gmovxe7yss2s7@sigill.intra.peff.net","threadId":"46344","inReplyTo":"752db1ea-8416-3a98-babf-78b766432f0f@ramsayjones.plus.com","subject":"Re: [PATCH 3/5] Makefile: add helper for compiling with -fsanitize","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-11T04:44:01Z","receivedAt":"2017-07-11T04:44:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 10, 2017 at 09:02:24PM +0100, Ramsay Jones wrote:\n\n> After a quick look at the ./t0000-basic.sh test, I managed to get\n> the test to complete (with 15 tests failing), with the following\n> patch applied:\n> \n> -- >8 --\n> diff --git a/Makefile b/Makefile\n> index 3c341b2a6..8e6433738 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -1016,7 +1016,7 @@ ifdef SANITIZE\n>  BASIC_CFLAGS += -fsanitize=$(SANITIZE) -fno-sanitize-recover=$(SANITIZE)\n>  BASIC_CFLAGS += -fno-omit-frame-pointer\n>  ifeq ($(SANITIZE),undefined)\n> -BASIC_CFLAGS += -DNO_UNALIGNED_LOADS\n> +BASIC_CFLAGS += -DNO_UNALIGNED_LOADS -DSHA1DC_FORCE_ALIGNED_ACCESS\n>  endif\n>  endif\n\nThanks, I forgot to mention SHA1DC. When I had originally tested with\n\"undefined\", it was before we had SHA1DC. I hacked around it earlier\ntoday by just using OPENSSL_SHA1. ;)\n\nI agree if we can ask it to avoid unaligned access that is even better.\n\n> diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c\n> index 25eded139..3baddc636 100644\n> --- a/sha1dc/sha1.c\n> +++ b/sha1dc/sha1.c\n> @@ -118,6 +118,10 @@\n>  #define SHA1DC_ALLOW_UNALIGNED_ACCESS\n>  #endif /*UNALIGNMENT DETECTION*/\n>  \n> +#if defined(SHA1DC_ALLOW_UNALIGNED_ACCESS) && defined(SHA1DC_FORCE_ALIGNED_ACCESS)\n> +#undef SHA1DC_ALLOW_UNALIGNED_ACCESS\n> +#endif\n\nI think our current strategy is to avoid touching sha1.c as much as\npossible. I think we'd prefer a patch to the upstream project to support\nFORCE_ALIGNED_ACCESS (unfortunately I do not see a way to tweak it using\nonly external defines.\n\n-Peff\n"},{"id":"324579","messageId":"de9c876c-f018-ad6b-3813-576ea73dd49a@web.de","threadId":"46344","inReplyTo":"20170710132450.yxojk5cybejy7und@sigill.intra.peff.net","subject":"Re: [PATCH 5/5] Makefile: disable unaligned loads with UBSan","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-07-15T17:18:56Z","receivedAt":"2017-07-15T17:19:12Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 10.07.2017 um 15:24 schrieb Jeff King:\n> The undefined behavior sanitizer complains about unaligned\n> loads, even if they're OK for a particular platform in\n> practice. It's possible that they _are_ a problem, of\n> course, but since it's a known tradeoff the UBSan errors are\n> just noise.\n> \n> Let's quiet it automatically by building with\n> NO_UNALIGNED_LOADS when SANITIZE=undefined is in use.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>   Makefile | 3 +++\n>   1 file changed, 3 insertions(+)\n> \n> diff --git a/Makefile b/Makefile\n> index cc03b3c95..3c341b2a6 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -1015,6 +1015,9 @@ endif\n>   ifdef SANITIZE\n>   BASIC_CFLAGS += -fsanitize=$(SANITIZE) -fno-sanitize-recover=$(SANITIZE)\n>   BASIC_CFLAGS += -fno-omit-frame-pointer\n> +ifeq ($(SANITIZE),undefined)\n> +BASIC_CFLAGS += -DNO_UNALIGNED_LOADS\n> +endif\n>   endif\n>   \n>   ifndef sysconfdir\n\nNice, but let's be even nicer!\n\n-- >8 --\nSubject: [PATCH] Makefile: allow combining UBSan with other sanitizers\n\nMultiple sanitizers can be specified as a comma-separated list.  Set\nthe flag NO_UNALIGNED_LOADS even if UndefinedBehaviorSanitizer is not\nthe only sanitizer to build with.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n Makefile | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/Makefile b/Makefile\nindex ba4359ef8d..9b98535a04 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1022,10 +1022,15 @@ ifdef DEVELOPER\n CFLAGS += $(DEVELOPER_CFLAGS)\n endif\n \n+comma := ,\n+empty :=\n+space := $(empty) $(empty)\n+\n ifdef SANITIZE\n+SANITIZERS := $(foreach flag,$(subst $(comma),$(space),$(SANITIZE)),$(flag))\n BASIC_CFLAGS += -fsanitize=$(SANITIZE) -fno-sanitize-recover=$(SANITIZE)\n BASIC_CFLAGS += -fno-omit-frame-pointer\n-ifeq ($(SANITIZE),undefined)\n+ifneq ($(filter undefined,$(SANITIZERS)),)\n BASIC_CFLAGS += -DNO_UNALIGNED_LOADS\n endif\n endif\n-- \n2.13.3\n"},{"id":"324604","messageId":"20170716101744.2g3x7v2dtasg45ll@sigill.intra.peff.net","threadId":"46344","inReplyTo":"de9c876c-f018-ad6b-3813-576ea73dd49a@web.de","subject":"Re: [PATCH 5/5] Makefile: disable unaligned loads with UBSan","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-07-16T10:17:44Z","receivedAt":"2017-07-16T10:17:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jul 15, 2017 at 07:18:56PM +0200, René Scharfe wrote:\n\n> -- >8 --\n> Subject: [PATCH] Makefile: allow combining UBSan with other sanitizers\n> \n> Multiple sanitizers can be specified as a comma-separated list.  Set\n> the flag NO_UNALIGNED_LOADS even if UndefinedBehaviorSanitizer is not\n> the only sanitizer to build with.\n\nNice, I didn't know you could do comma-separation here. ;)\n\nI always just built the various sanitizers separately since I was\ntesting whether each one worked. But if we can get UBSan to build\ncleanly, I agree that \"make SANITIZE=address,undefined test\" would be a\nnice thing to run periodically.\n\n> diff --git a/Makefile b/Makefile\n> index ba4359ef8d..9b98535a04 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -1022,10 +1022,15 @@ ifdef DEVELOPER\n>  CFLAGS += $(DEVELOPER_CFLAGS)\n>  endif\n>  \n> +comma := ,\n> +empty :=\n> +space := $(empty) $(empty)\n> +\n>  ifdef SANITIZE\n> +SANITIZERS := $(foreach flag,$(subst $(comma),$(space),$(SANITIZE)),$(flag))\n>  BASIC_CFLAGS += -fsanitize=$(SANITIZE) -fno-sanitize-recover=$(SANITIZE)\n>  BASIC_CFLAGS += -fno-omit-frame-pointer\n> -ifeq ($(SANITIZE),undefined)\n> +ifneq ($(filter undefined,$(SANITIZERS)),)\n\nThe implementation is convoluted in the way that \"make\" usually forces\nus into. ;)\n\nI would have written it as:\n\n  ifeq ($findstring $(SANITIZERS), undefined), undefined)\n\nbut I think your version:\n\n  1. Isn't fooled by superstrings like a potential undefined-foo\n     sanitizer.\n\n  2. Handles SANITIZE=undefined,undefined correctly.\n\nNeither are expected, but it doesn't hurt to be a bit more robust.\n\n-Peff\n"},{"id":"324611","messageId":"840f246f-e735-2a84-d9b9-95d8e4a9d99c@web.de","threadId":"46344","inReplyTo":"20170716101744.2g3x7v2dtasg45ll@sigill.intra.peff.net","subject":"Re: [PATCH 5/5] Makefile: disable unaligned loads with UBSan","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-07-16T11:02:46Z","receivedAt":"2017-07-16T11:03:01Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 16.07.2017 um 12:17 schrieb Jeff King:\n> On Sat, Jul 15, 2017 at 07:18:56PM +0200, René Scharfe wrote:\n> \n>> -- >8 --\n>> Subject: [PATCH] Makefile: allow combining UBSan with other sanitizers\n>>\n>> Multiple sanitizers can be specified as a comma-separated list.  Set\n>> the flag NO_UNALIGNED_LOADS even if UndefinedBehaviorSanitizer is not\n>> the only sanitizer to build with.\n> \n> Nice, I didn't know you could do comma-separation here. ;)\n> \n> I always just built the various sanitizers separately since I was\n> testing whether each one worked. But if we can get UBSan to build\n> cleanly, I agree that \"make SANITIZE=address,undefined test\" would be a\n> nice thing to run periodically.\n\nThere are *some* restrictions: You can only use one of address, memory\nand thread, as mentioned here:\n\nhttp://clang.llvm.org/docs/UsersManual.html#controlling-code-generation\n\nCombining ASan and UBSan works fine.\n\nRené\n"}]}