{"thread":{"id":"59128","subject":"[PATCH] Makefile: suppress annotated leaks with certain ASan options","startedAt":"2023-01-20T18:46:28Z","lastAt":"2023-01-20T20:55:28Z","messageCount":5,"participants":["Taylor Blau","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"470799","messageId":"b1efe56ab5193d5505ccb9334f7d15e1795c27fb.1674240261.git.me@ttaylorr.com","threadId":"59128","inReplyTo":null,"subject":"[PATCH] Makefile: suppress annotated leaks with certain ASan options","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-01-20T18:46:16Z","receivedAt":"2023-01-20T18:46:28Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When building with `SANITIZE=leak`, we define `SUPPRESS_ANNOTATED_LEAKS`\nin order to make the `UNLEAK()` macro work (without the aforementioned\ndefine, `UNLEAK()` is a noop). This is from `UNLEAK()`'s introduction in\n0e5bba53af (add UNLEAK annotation for reducing leak false positives,\n2017-09-08), where `UNLEAK()` is a noop for performance reasons unless\nwe are using the leak sanitizer.\n\nHowever, it is possible to use the leak sanitizer without\n`SANITIZE=leak`. This happens when building with `SANITIZE=address` and\nenabling the leak sanitizer via the `ASAN_OPTIONS` variable (by\nincluding the string \"detect_leaks=1\").\n\nThis renders `UNLEAK()` useless when doing `SANITIZE=address` builds\nwhich also use the leak checker.\n\nUpdate our Makefile to pretend as if `SANITIZE=leak` was given when\n`SANITIZE=address` is given and the leak checker is enabled via\n`ASAN_OPTIONS`.\n\nPlaying around with all five options (two spelling \"enabled\", two\nspelling \"disabled\", and the empty set of options) yields the correct\nbehavior:\n\n    for opt in '' detect_leaks=1 detect_leaks=true detect_leaks=0 detect_leaks=false\n    do\n      echo \"==> ${opt:-(nothing)}\"\n      make -B builtin/add.o V=1 SANITIZE=address ASAN_OPTIONS=\"$opt\" 2>&1 |\n        grep -o -- '-DSUPPRESS_ANNOTATED_LEAKS'\n    done\n\ngives us:\n\n    ==> (nothing)\n    -DSUPPRESS_ANNOTATED_LEAKS\n    ==> detect_leaks=1\n    -DSUPPRESS_ANNOTATED_LEAKS\n    ==> detect_leaks=true\n    -DSUPPRESS_ANNOTATED_LEAKS\n    ==> detect_leaks=0\n    ==> detect_leaks=false\n\nMaking it possible to rely on `UNLEAK()` when implicitly using the leak\nchecker via SANITIZE=address builds.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\nI found this while playing around with GitHub's ASan-enabled CI builds\nfor our internal fork following a merge with v2.38.3.\n\nThe check-chainlint recipe in t/Makefile started using \"git diff\" via\nd00113ec34 (t/Makefile: apply chainlint.pl to existing self-tests,\n2022-09-01), which triggered a leak in some of GitHub's custom code. I\nwas surprised when marking the variable with UNLEAK() didn't do the\ntrick, and ended up down this rabbit hole ;-).\n\n Makefile | 9 +++++++--\n 1 file changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex db447d0738..b00bb8bd1e 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1445,13 +1445,18 @@ ifneq ($(filter undefined,$(SANITIZERS)),)\n BASIC_CFLAGS += -DSHA1DC_FORCE_ALIGNED_ACCESS\n endif\n ifneq ($(filter leak,$(SANITIZERS)),)\n-BASIC_CFLAGS += -DSUPPRESS_ANNOTATED_LEAKS\n-BASIC_CFLAGS += -O0\n SANITIZE_LEAK = YesCompiledWithIt\n endif\n ifneq ($(filter address,$(SANITIZERS)),)\n NO_REGEX = NeededForASAN\n SANITIZE_ADDRESS = YesCompiledWithIt\n+ifeq ($(filter $(patsubst detect_leaks=%,%,$(ASAN_OPTIONS)),0 false),)\n+SANITIZE_LEAK = YesViaASanOptions\n+endif\n+endif\n+ifneq ($(SANITIZE_LEAK),)\n+BASIC_CFLAGS += -DSUPPRESS_ANNOTATED_LEAKS\n+BASIC_CFLAGS += -O0\n endif\n endif\n\n--\n2.38.0.16.g393fd4c6db\n"},{"id":"470800","messageId":"xmqqsfg5vvrk.fsf@gitster.g","threadId":"59128","inReplyTo":"b1efe56ab5193d5505ccb9334f7d15e1795c27fb.1674240261.git.me@ttaylorr.com","subject":"Re: [PATCH] Makefile: suppress annotated leaks with certain ASan options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-20T19:41:35Z","receivedAt":"2023-01-20T19:41:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> However, it is possible to use the leak sanitizer without\n> `SANITIZE=leak`. This happens when building with `SANITIZE=address` and\n> enabling the leak sanitizer via the `ASAN_OPTIONS` variable (by\n> including the string \"detect_leaks=1\").\n\nYuck.  I cannot tell if this falls into \"don't do it then if it\nhurts\" or pretty common thing people do that is worth helping.\n\n> Making it possible to rely on `UNLEAK()` when implicitly using the leak\n> checker via SANITIZE=address builds.\n\nBut as long as you did all the work, sure, why not ;-).\n\n> I found this while playing around with GitHub's ASan-enabled CI builds\n> for our internal fork following a merge with v2.38.3.\n>\n> The check-chainlint recipe in t/Makefile started using \"git diff\" via\n> d00113ec34 (t/Makefile: apply chainlint.pl to existing self-tests,\n> 2022-09-01), which triggered a leak in some of GitHub's custom code. I\n> was surprised when marking the variable with UNLEAK() didn't do the\n> trick, and ended up down this rabbit hole ;-).\n\nThanks.  Will queue.\n"},{"id":"470802","messageId":"Y8r2Tn75g52YTIij@coredump.intra.peff.net","threadId":"59128","inReplyTo":"b1efe56ab5193d5505ccb9334f7d15e1795c27fb.1674240261.git.me@ttaylorr.com","subject":"Re: [PATCH] Makefile: suppress annotated leaks with certain ASan options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-20T20:15:10Z","receivedAt":"2023-01-20T20:15:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 20, 2023 at 01:46:16PM -0500, Taylor Blau wrote:\n\n> However, it is possible to use the leak sanitizer without\n> `SANITIZE=leak`. This happens when building with `SANITIZE=address` and\n> enabling the leak sanitizer via the `ASAN_OPTIONS` variable (by\n> including the string \"detect_leaks=1\").\n> \n> This renders `UNLEAK()` useless when doing `SANITIZE=address` builds\n> which also use the leak checker.\n\nYeah. I focused on LSan when adding the sanitize/unleak infrastructure\njust because it was faster than a full ASan run, which made iterating on\nfixes easier. I do think in the long run, once the test suite is leak\nfree, we may want to support leak-checking via ASan for the simple\nreason that it can be done for \"free\" during the existing ASan\nbuild/test, rather than requiring an extra LSan job.\n\nI do think there's some complexity here, though.\n\nOne problem UNLEAK() is that compile-time switch, but whether ASan does\nleak detection is a run-time choice. So you are stuck with either:\n\n  - you always turn on UNLEAK() for ASan builds, in which case test runs\n    using the default ASAN_OPTIONS we set do the extra work even though\n    they are not doing any leak detection. I doubt it's very measurable,\n    though (it's just shoving a few bytes onto a linked list),\n    especially compared to the overall slowness of ASan.\n\n  - you predicate the build-time choice on ASAN_OPTIONS. But this means\n    that:\n\n      make SANITIZE=address\n      cd t\n      ASAN_OPTIONS=detect_leaks=1 ./t0000-*.sh\n\n    will confusingly fail to use UNLEAK().\n\nYour patch does the second one, but I think the first may be the\nleast-bad option.\n\nThe other issue I'd worry about is optimizations. Generally you want to\nuse -O2 with ASan, because it speeds things up (even more than just\nregular -O2, I think, because it is optimizing the ASan instrumentation\ncode, too). I don't know offhand of any cases where it would find or not\nfind cases based on optimization level, though I could believe they\nexist.\n\nBut for leak-checking, we've already seen real cases where using LSan\nwith higher optimization levels can lead to false positives (because the\noptimizer drops a value that is still in scope but not used in a code\npath that leads to exit()).\n\nSo it may be that we really do want to keep leak-checking to \"-O0\n-fsanitize=leak\", and reserve \"-O2 -fsanitize=address\" for finding\naddress bugs.\n\n> I found this while playing around with GitHub's ASan-enabled CI builds\n> for our internal fork following a merge with v2.38.3.\n> \n> The check-chainlint recipe in t/Makefile started using \"git diff\" via\n> d00113ec34 (t/Makefile: apply chainlint.pl to existing self-tests,\n> 2022-09-01), which triggered a leak in some of GitHub's custom code. I\n> was surprised when marking the variable with UNLEAK() didn't do the\n> trick, and ended up down this rabbit hole ;-).\n\nYes, but I'd ask why your ASan builds were checking for leaks in the\nfirst place. There are presumably tons of leaks they'd detect, since the\ntest suite is far from leak-free.\n\n-Peff\n"},{"id":"470803","messageId":"xmqqmt6dvsqk.fsf@gitster.g","threadId":"59128","inReplyTo":"Y8r2Tn75g52YTIij@coredump.intra.peff.net","subject":"Re: [PATCH] Makefile: suppress annotated leaks with certain ASan options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-20T20:46:59Z","receivedAt":"2023-01-20T20:47:02Z","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> I do think there's some complexity here, though.\n>\n> One problem UNLEAK() is that compile-time switch, but whether ASan does\n> leak detection is a run-time choice. So you are stuck with either:\n>\n>   - you always turn on UNLEAK() for ASan builds, in which case test runs\n>     using the default ASAN_OPTIONS we set do the extra work even though\n>     they are not doing any leak detection. I doubt it's very measurable,\n>     though (it's just shoving a few bytes onto a linked list),\n>     especially compared to the overall slowness of ASan.\n>\n>   - you predicate the build-time choice on ASAN_OPTIONS. But this means\n>     that:\n>\n>       make SANITIZE=address\n>       cd t\n>       ASAN_OPTIONS=detect_leaks=1 ./t0000-*.sh\n>\n>     will confusingly fail to use UNLEAK().\n>\n> Your patch does the second one, but I think the first may be the\n> least-bad option.\n\nThanks, I totally missed the fact that ASAN_OPTIONS was a runtime\nthing.  If we were to pursue this topic of enabling UNLEAK() outside\nLSan, I agree the first would be necessary.\n\n> But for leak-checking, we've already seen real cases where using LSan\n> with higher optimization levels can lead to false positives (because the\n> optimizer drops a value that is still in scope but not used in a code\n> path that leads to exit()).\n> ...\n> So it may be that we really do want to keep leak-checking to \"-O0\n> -fsanitize=leak\", and reserve \"-O2 -fsanitize=address\" for finding\n> address bugs.\n\nYup, we have been burned a few times with this, IIRC.\n"},{"id":"470804","messageId":"Y8r/vIg3+UziR5X5@coredump.intra.peff.net","threadId":"59128","inReplyTo":"Y8r2Tn75g52YTIij@coredump.intra.peff.net","subject":"Re: [PATCH] Makefile: suppress annotated leaks with certain ASan options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-01-20T20:55:24Z","receivedAt":"2023-01-20T20:55:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 20, 2023 at 03:15:10PM -0500, Jeff King wrote:\n\n> One problem UNLEAK() is that compile-time switch, but whether ASan does\n> leak detection is a run-time choice. So you are stuck with either:\n\nEr, this should be \"one problem is that UNLEAK() is a compile-time\nswitch\".\n\n-Peff\n"}]}