{"thread":{"id":"58647","subject":"linux-leaks CI failure on master","startedAt":"2022-10-18T18:37:51Z","lastAt":"2022-10-21T06:04:34Z","messageCount":8,"participants":["Taylor Blau","Jeff King","Ævar Arnfjörð Bjarmason","Junio C Hamano","Rubén Justo"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"465216","messageId":"Y07yeEQu+C7AH7oN@nand.local","threadId":"58647","inReplyTo":null,"subject":"linux-leaks CI failure on master","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-18T18:37:44Z","receivedAt":"2022-10-18T18:37:51Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"After Junio pushed out the tags for v2.38.1 and friends, I noticed that\nour linux-leaks CI job is failing t1300.104 and t1300.109, claiming that\nthere is a leak here:\n\n    Direct leak of 7 byte(s) in 1 object(s) allocated from:\n    #0 0x7fc4a5b319c1 in __interceptor_malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:54\n    #1 0x7fc4a598638e in __GI___strdup /build/glibc-SzIz7B/glibc-2.31/string/strdup.c:42\n    #2 0x55ba23538f7c in xstrdup wrapper.c:39\n    #3 0x55ba233e258c in git_config_string config.c:1445\n    #4 0x55ba233e6b06 in git_config_include config.c:439\n    #5 0x55ba233e063f in get_value config.c:910\n    #6 0x55ba233e063f in git_parse_source config.c:1092\n    #7 0x55ba233e063f in do_config_from config.c:1937\n    #8 0x55ba233e3d2d in do_config_from_file config.c:1965\n    #9 0x55ba233e3d2d in git_config_from_file_with_options config.c:1987\n    #10 0x55ba233e4793 in git_config_from_file config.c:1996\n    #11 0x55ba233e4793 in do_git_config_sequence config.c:2143\n    #12 0x55ba233e4793 in config_with_options config.c:2198\n    #13 0x55ba233e4a50 in read_early_config config.c:2255\n    #14 0x55ba233acb36 in alias_lookup alias.c:35\n    #15 0x55ba232d0748 in handle_alias git.c:346\n    #16 0x55ba232d0748 in run_argv git.c:851\n    #17 0x55ba232d0748 in cmd_main git.c:921\n    #18 0x55ba232cee03 in main common-main.c:57\n    #19 0x7fc4a590b082 in __libc_start_main ../csu/libc-start.c:308\n    #20 0x55ba232cee8d in _start (git+0x1fe8d)\n\nI can't reproduce the failure locally with gcc (Debian 10.3.0-15)\n10.3.0, but Victoria (CC'd) can reproduce the failure with 9.4.0.\nInterestingly, the failure only appears when compiling with `-O2`, but\nnot `-O0` or `-O1`.\n\nThis is reminiscent to me of the discussion in:\n\n  https://lore.kernel.org/git/Yy4eo6500C0ijhk+@coredump.intra.peff.net/\n\nI'm not sure if I'm content to treat the 9.4.0 behavior as a compiler\nbug, but definitely running the linux-leaks build with `-O2` is\nsuspicious.\n\nI suppose we could temporarily mark t1300 as not passing with\nSANITIZE=leak turned on, but I tend to agree with Peff that that feels\nlike a hack working around compiler behavior, that will ultimately\nresult in us playing whack-a-mole.\n\nSo my preference would be to run the linux-leaks build with `-O0` in its\nCFLAGS, optionally with a newer compiler if one is available for Focal.\n\nThoughts?\n\nThanks,\nTaylor\n"},{"id":"465219","messageId":"Y08BPbWBj7SNluXq@coredump.intra.peff.net","threadId":"58647","inReplyTo":"Y07yeEQu+C7AH7oN@nand.local","subject":"Re: linux-leaks CI failure on master","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-18T19:40:45Z","receivedAt":"2022-10-18T19:40:50Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 18, 2022 at 02:37:44PM -0400, Taylor Blau wrote:\n\n> After Junio pushed out the tags for v2.38.1 and friends, I noticed that\n> our linux-leaks CI job is failing t1300.104 and t1300.109, claiming that\n> there is a leak here:\n> [...]\n>     #14 0x55ba233acb36 in alias_lookup alias.c:35\n>     #15 0x55ba232d0748 in handle_alias git.c:346\n\nThese are the interesting part of the trace. alias_lookup() returns an\nallocated string, and then split_cmdline() fails, so we call die() with:\n\n  fatal: bad alias.split-cmdline-fix string: unclosed quote\n\nSo yeah, I think it's probably the same issue as discussed previously:\nthe compiler presumably puts alias_string into a register, and then\nclobbers the register when calling die(), because it knows we're never\ncoming back.\n\n> I can't reproduce the failure locally with gcc (Debian 10.3.0-15)\n> 10.3.0, but Victoria (CC'd) can reproduce the failure with 9.4.0.\n> Interestingly, the failure only appears when compiling with `-O2`, but\n> not `-O0` or `-O1`.\n\nI can't reproduce on debian unstable using any of gcc 9-12 (but note\ngcc-9 here is 9.5.0). But then, I had trouble convincing gcc to find\n_actual_ leaks with lsan. Clang is much more reliable for me, but it\nturns up only the failure in t1300.135 that I reported earlier.\n\nBut given the trace above plus the findings on gcc 9.4.0, I feel pretty\nconfident saying this is another instance of the same problem.\n\n> I'm not sure if I'm content to treat the 9.4.0 behavior as a compiler\n> bug, but definitely running the linux-leaks build with `-O2` is\n> suspicious.\n\nIt's definitely not a compiler bug. What the optimizer is doing is\nperfectly reasonable; it's just that the leak checker interacts badly\nwith it.\n\n> I suppose we could temporarily mark t1300 as not passing with\n> SANITIZE=leak turned on, but I tend to agree with Peff that that feels\n> like a hack working around compiler behavior, that will ultimately\n> result in us playing whack-a-mole.\n> \n> So my preference would be to run the linux-leaks build with `-O0` in its\n> CFLAGS, optionally with a newer compiler if one is available for Focal.\n\nYes, I still think disabling optimizations is the best path forward. Not\njust to avoid whack-a-mole, but this is also something we'd eventually\nneed to confront when the code base really is leak-free.\n\nI don't think there's any need for a newer compiler. While the optimizer\nbehavior may change between versions, none of what we've seen is any\ncompiler being _wrong_, just different.\n\nAs a lesser change, I suspect that making NORETURN a noop in\nleak-checking builds would help in practice, because the compiler\nwouldn't realize that die() doesn't return. But it's not foolproof (the\nsame thing might trigger with a direct exit() call, or one that becomes\ndirect via inlining). Using -O0 is the more complete fix, and IMHO it's\nnot important to try to get optimal performance during the leak-checking\ntest run.\n\n-Peff\n"},{"id":"465220","messageId":"Y08JZVDgJpJvrBiz@coredump.intra.peff.net","threadId":"58647","inReplyTo":"Y08BPbWBj7SNluXq@coredump.intra.peff.net","subject":"[PATCH] Makefile: force -O0 when compiling with SANITIZE=leak","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-18T20:15:33Z","receivedAt":"2022-10-18T20:15:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 18, 2022 at 03:40:45PM -0400, Jeff King wrote:\n\n> > I suppose we could temporarily mark t1300 as not passing with\n> > SANITIZE=leak turned on, but I tend to agree with Peff that that feels\n> > like a hack working around compiler behavior, that will ultimately\n> > result in us playing whack-a-mole.\n> > \n> > So my preference would be to run the linux-leaks build with `-O0` in its\n> > CFLAGS, optionally with a newer compiler if one is available for Focal.\n> \n> Yes, I still think disabling optimizations is the best path forward. Not\n> just to avoid whack-a-mole, but this is also something we'd eventually\n> need to confront when the code base really is leak-free.\n\nSo here's a patch that does so. I think one of Ævar's reservations in\nthe other thread was not wanting to have to switch optimization levels\nmanually for various builds. I think the patch below should make things\nJust Work, whether in CI or running a custom build locally.\n\nI tested it with the clang t1300.135 failure I can reproduce, and it\ndoes indeed make the problem go away.\n\n-- >8 --\nSubject: Makefile: force -O0 when compiling with SANITIZE=leak\n\nCompiling with -O2 can interact badly with LSan's leak-checker, causing\nfalse positives. Imagine a simplified example like:\n\n  char *str = allocate_some_string();\n  if (some_func(str) < 0)\n          die(\"bad str\");\n  free(str);\n\nThe compiler may eliminate \"str\" as a stack variable, and just leave it\nin a register. The register is preserved through most of the function,\nincluding across the call to some_func(), since we'd eventually need to\nfree it. But because die() is marked with NORETURN, the compiler knows\nthat it doesn't need to save registers, and just clobbers it.\n\nWhen die() eventually exits, the leak-checker runs. It looks in\nregisters and on the stack for any reference to the memory allocated by\nstr (which would indicate that it's not leaked), but can't find one.  So\nit reports it as a leak.\n\nNeither system is wrong, really. The C standard (mostly section 5.1.2.3)\ndefines an abstract machine, and compilers are allowed to modify the\nprogram as long as the observable behavior of that abstract machine is\nunchanged. Looking at random memory values on the stack is undefined\nbehavior, and not something that the optimizer needs to support. But\nthere really isn't any other way for a leak checker to work; it\ninherently has to do undefined things like scouring memory for pointers.\nSo the two things are inherently at odds with each other. We can't fix\nit by changing the code, because from the perspective of the program\nrunning in an abstract machine, there is no leak.\n\nThis has caused real false positives in the past, like:\n\n  - https://lore.kernel.org/git/patch-v3-5.6-9a44204c4c9-20211022T175227Z-avarab@gmail.com/\n\n  - https://lore.kernel.org/git/Yy4eo6500C0ijhk+@coredump.intra.peff.net/\n\n  - https://lore.kernel.org/git/Y07yeEQu+C7AH7oN@nand.local/\n\nThis patch makes those go away by forcing -O0 when compiling with LSan.\nThere are a few ways we could do this:\n\n  - we could just teach the linux-leaks CI job to set -O0. That's the\n    smallest change, and means we wouldn't get spurious CI failures. But\n    it doesn't help people looking for leaks manually or in a specific\n    test (and because the problem depends on the vagaries of the\n    optimizer, investigating these can waste a lot of time in\n    head-scratching as the problem comes and goes)\n\n  - we default to -O2 in CFLAGS; we could pull this out to a separate\n    variable (\"-O$(O)\" or something) and modify \"O\" when LSan is in use.\n    This is the most flexible, in that you could still build with \"make\n    O=2 SANITIZE=leak\" if you really wanted to (say, for experimenting).\n    But it would also fail to kick in if the user defines their own\n    CFLAGS variable, which again leads to head-scratching.\n\n  - we can just stick -O0 into BASIC_CFLAGS when enabling LSan. Since\n    this comes after the user-provided CFLAGS, it will override any\n    previous -O setting found there. This is more foolproof, albeit less\n    flexible. If you want to experiment with an optimized leak-checking\n    build, you'll have to put \"-O2 -fsanitize=leak\" into CFLAGS\n    manually, rather than using our SANITIZE=leak Makefile magic.\n\nSince the final one is the least likely to break in normal use, this\npatch uses that approach.\n\nThe resulting build is a little slower, of course, but since LSan is\nalready about 2x slower than a regular build, another 10% slowdown isn't\nthat big a deal.\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 d93ad956e5..85f03c6aed 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1339,6 +1339,7 @@ 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-- \n2.38.1.387.g7766a44117\n\n"},{"id":"465225","messageId":"221018.86k04whkgf.gmgdl@evledraar.gmail.com","threadId":"58647","inReplyTo":"Y08JZVDgJpJvrBiz@coredump.intra.peff.net","subject":"Re: [PATCH] Makefile: force -O0 when compiling with SANITIZE=leak","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-18T21:00:54Z","receivedAt":"2022-10-18T21:48:40Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Oct 18 2022, Jeff King wrote:\n\nNote that we're discussing two different leak here,\ngit_config_push_env() one in\nhttps://lore.kernel.org/git/Yxl91jfycCo7O7Pp@coredump.intra.peff.net/;\nand now another one that originates in the exit() behavior in\ngit.c. They're both triggered from t1300, but they're otherwise\nunrelated.\n\n> On Tue, Oct 18, 2022 at 03:40:45PM -0400, Jeff King wrote:\n>\n>> > I suppose we could temporarily mark t1300 as not passing with\n>> > SANITIZE=leak turned on, but I tend to agree with Peff that that feels\n>> > like a hack working around compiler behavior, that will ultimately\n>> > result in us playing whack-a-mole.\n>> > \n>> > So my preference would be to run the linux-leaks build with `-O0` in its\n>> > CFLAGS, optionally with a newer compiler if one is available for Focal.\n>> \n>> Yes, I still think disabling optimizations is the best path forward. Not\n>> just to avoid whack-a-mole, but this is also something we'd eventually\n>> need to confront when the code base really is leak-free.\n>\n> So here's a patch that does so. I think one of Ævar's reservations in\n> the other thread was not wanting to have to switch optimization levels\n> manually for various builds. I think the patch below should make things\n> Just Work, whether in CI or running a custom build locally.\n\nNo, the opposite. I want to try out various combinations of compile\nflags & optimization levels and not have SANITIZE=leak second-guess it.\n\nNow if I want to compile with -O2 and SANITIZE=leak I'll need to\nmonkeypatch the Makefile.\n\nI'd prefer not to need to do that, because while non-O0 is noisy\nsometimes, I've also found that it points (at least indirectly) to some\nuseful edge-cases.\n\n> The compiler may eliminate \"str\" as a stack variable, and just leave it\n> in a register. The register is preserved through most of the function,\n> including across the call to some_func(), since we'd eventually need to\n> free it. But because die() is marked with NORETURN, the compiler knows\n> that it doesn't need to save registers, and just clobbers it.\n>\n> When die() eventually exits, the leak-checker runs. It looks in\n> registers and on the stack for any reference to the memory allocated by\n> str (which would indicate that it's not leaked), but can't find one.  So\n> it reports it as a leak.\n>\n> Neither system is wrong, really. The C standard (mostly section 5.1.2.3)\n> defines an abstract machine, and compilers are allowed to modify the\n> program as long as the observable behavior of that abstract machine is\n> unchanged. Looking at random memory values on the stack is undefined\n> behavior, and not something that the optimizer needs to support. But\n> there really isn't any other way for a leak checker to work; it\n> inherently has to do undefined things like scouring memory for pointers.\n> So the two things are inherently at odds with each other. We can't fix\n> it by changing the code, because from the perspective of the program\n> running in an abstract machine, there is no leak.\n\nIn the abstract program yes, there is a leak, and you can see that it\nleaks with e.g. valgrind regardless of optimization level:\n\n\t$ valgrind --leak-check=full --show-leak-kinds=all ./git --config-env=foo.flag= config --bool foo.flag\n        [...]\n\t==6540== 13 bytes in 1 blocks are still reachable in loss record 3 of 17\n\t==6540==    at 0x48437B4: malloc (vg_replace_malloc.c:381)\n\t==6540==    by 0x40278B: do_xmalloc (wrapper.c:51)\n\t==6540==    by 0x402884: do_xmallocz (wrapper.c:85)\n\t==6540==    by 0x4028C0: xmallocz (wrapper.c:93)\n\t==6540==    by 0x4028FD: xmemdupz (wrapper.c:109)\n\t==6540==    by 0x402962: xstrndup (wrapper.c:115)\n\t==6540==    by 0x342F53: strip_path_suffix (path.c:1300)\n\t==6540==    by 0x2B4C89: system_prefix (exec-cmd.c:50)\n\t==6540==    by 0x2B5057: system_path (exec-cmd.c:268)\n\t==6540==    by 0x2C5E17: git_setup_gettext (gettext.c:109)\n\t==6540==    by 0x21DC57: main (common-main.c:44)\n\nI think I elaborated on some of this in\nhttps://lore.kernel.org/git/220218.861r00ib86.gmgdl@evledraar.gmail.com/\n\nThe tl;dr is that I think you use \"leak\" in the sense that valgrind\ntalks about \"still reachable\" leaks, which is conceptually where\n-fsanitize=leak draws the line. I.e. it's not a malloc()/free() tracker,\nbut tries to see what's \"in scope\".\n\nThis is getting a bit academic, but I don't see how you can both say\nthat the \"compilers are allowed to modify the program as long as the\nobservable behavior of that abstract machine is unchanged\" *and* claim\nthat e.g. the git_config_push_env() case isn't a real leak.\n\nBecause surely the thing that makes it a \"leak\" by your definition (and\nwhat LSAN strives for) is that it's attributed to a variable that's \"in\nscope\", but the compiler is free to re-arrange all of that.\n\nAnyway, one reason I wanted to punt on that \"git --config-env\" issue is\nbecause we can entirely avoid the malloc()/free() there. See the \"-- >8\n--\" below, but if we just malloc() after we do our assertions we can\nun-confuse clang.\n\nAnd that seems like a good idea in general, and re whether the \"leak\" is\ngone, at that point valgrind will stop reporting it, so we're really not\nleaking at all, not just in the \"still reachable\" sense.\n\nThe reason I mention all of that is to try to define the problem here. I\nhaven't seen cases where the compilers get it wrong about there being a\nleak, it's just that they're mis-categorizing them as \"still reachable\"\nor not, re your \"abstract machine\" summary.\n\n> This has caused real false positives in the past, like:\n>\n>   - https://lore.kernel.org/git/patch-v3-5.6-9a44204c4c9-20211022T175227Z-avarab@gmail.com/\n>\n>   - https://lore.kernel.org/git/Yy4eo6500C0ijhk+@coredump.intra.peff.net/\n>\n>   - https://lore.kernel.org/git/Y07yeEQu+C7AH7oN@nand.local/\n\nNow, the other cases in t1300 are from git.c using exit() instead of\nretur-ing to our main().\n\nAmong numerous other leak fixes I have queued up I have a fix for that,\nwhich fixes the other t1300 cases that have been reported now:\nhttps://github.com/avar/git/tree/avar/git-c-do-not-exit\n\nSo as fuzzy as the \"abstract machine\" can be sometimes, I've found this\nuseful.\n\n-- >8 --\nSubject: config.c: do sanity checks before xmemdupz()\n\nAdjust the code added in 1ff21c05ba9 (config: store \"git -c\" variables\nusing more robust format, 2021-01-12) to avoid allocating a \"key\"\nuntil after we've done the sanity checks on it.\n\nThere was no reason to allocate it this early in the function, except\nbecause we were using the \"env_name\" pointer we were about to\nincrement, let's save a copy of it instead.\n\nAs a result of this early allocation the \"clang\" at higher\noptimization levels would report that we had a leak here. E.g. with\nDebian clang 14.0.6-2 with CFLAGS=\"-O3 -g\":\n\n\t$ ./git --config-env=foo.flag= config --bool foo.flag\n\tfatal: missing environment variable name for configuration 'foo.flag'\n\n\t=================================================================\n\t==18018==ERROR: LeakSanitizer: detected memory leaks\n\n\tDirect leak of 9 byte(s) in 1 object(s) allocated from:\n\t    #0 0x55e40aad7cd2 in __interceptor_malloc (/home/avar/g/git/git+0x78cd2) (BuildId: b89fa8f797ccb02ef1148beed300071a5f9b9ab1)\n\t    #1 0x55e40ad41071 in do_xmalloc /home/avar/g/git/wrapper.c:51:8\n\t    #2 0x55e40ad41071 in do_xmallocz /home/avar/g/git/wrapper.c:85:8\n\t    #3 0x55e40ad41071 in xmallocz /home/avar/g/git/wrapper.c:93:9\n\t    #4 0x55e40ad41071 in xmemdupz /home/avar/g/git/wrapper.c:109:16\n\t    #5 0x55e40abec960 in git_config_push_env /home/avar/g/git/config.c:521:8\n\t    #6 0x55e40aadb8b9 in handle_options /home/avar/g/git/git.c:268:4\n\t    #7 0x55e40aada68d in cmd_main /home/avar/g/git/git.c:896:2\n\t    #8 0x55e40abae219 in main /home/avar/g/git/common-main.c:57:11\n\t    #9 0x7fbb5287e209 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16\n\t    #10 0x7fbb5287e2bb in __libc_start_main csu/../csu/libc-start.c:389:3\n\t    #11 0x55e40aaac130 in _start (/home/avar/g/git/git+0x4d130) (BuildId: b89fa8f797ccb02ef1148beed300071a5f9b9ab1)\n\n\tSUMMARY: LeakSanitizer: 9 byte(s) leaked in 1 allocation(s).\n\tAborted\n\nThis has come up before, with an earlier proposed fix of mine[1] to\nsweep this under the rug.\n\nThe actual meaningful fix here is that we don't need to do this\nallocation at all. The only reason it's needed is because there's no\nvariant of \"sq_quote_buf()\" in quote.c that takes a \"len\"\nparameter (but I have one locally).\n\nIf we had that we could just pass a \"key\" and \"key_len\" to\ngit_config_push_split_parameter() instead and avoid the allocation\naltogether. But in lieu of that better fix (which other API users of\nquote.c would benefit from) let's defer the allocation, which happens\nto fix the leak reporting on\n\n1. https://lore.kernel.org/git/patch-1.1-fb2f0a660c0-20220908T153252Z-avarab@gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n config.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/config.c b/config.c\nindex cbb5a3bab74..e49fd87bbd3 100644\n--- a/config.c\n+++ b/config.c\n@@ -512,13 +512,14 @@ void git_config_push_parameter(const char *text)\n void git_config_push_env(const char *spec)\n {\n \tchar *key;\n+\tconst char *p;\n \tconst char *env_name;\n \tconst char *env_value;\n \n \tenv_name = strrchr(spec, '=');\n \tif (!env_name)\n \t\tdie(_(\"invalid config format: %s\"), spec);\n-\tkey = xmemdupz(spec, env_name - spec);\n+\tp = env_name;\n \tenv_name++;\n \tif (!*env_name)\n \t\tdie(_(\"missing environment variable name for configuration '%.*s'\"),\n@@ -529,6 +530,7 @@ void git_config_push_env(const char *spec)\n \t\tdie(_(\"missing environment variable '%s' for configuration '%.*s'\"),\n \t\t    env_name, (int)(env_name - spec - 1), spec);\n \n+\tkey = xmemdupz(spec, p - spec);\n \tgit_config_push_split_parameter(key, env_value);\n \tfree(key);\n }\n-- \n2.38.0.1093.gcd4a685f0b1\n\n"},{"id":"465240","messageId":"Y0+i1G5ybdhUGph2@coredump.intra.peff.net","threadId":"58647","inReplyTo":"221018.86k04whkgf.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] Makefile: force -O0 when compiling with SANITIZE=leak","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-19T07:10:12Z","receivedAt":"2022-10-19T07:17:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 18, 2022 at 11:00:54PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> Note that we're discussing two different leak here,\n> git_config_push_env() one in\n> https://lore.kernel.org/git/Yxl91jfycCo7O7Pp@coredump.intra.peff.net/;\n> and now another one that originates in the exit() behavior in\n> git.c. They're both triggered from t1300, but they're otherwise\n> unrelated.\n\nYes, there are two different cases being discussed. But they _are_\nrelated, in that they are caused by similar compiler optimizations.\n\n> > So here's a patch that does so. I think one of Ævar's reservations in\n> > the other thread was not wanting to have to switch optimization levels\n> > manually for various builds. I think the patch below should make things\n> > Just Work, whether in CI or running a custom build locally.\n> \n> No, the opposite. I want to try out various combinations of compile\n> flags & optimization levels and not have SANITIZE=leak second-guess it.\n>\n> Now if I want to compile with -O2 and SANITIZE=leak I'll need to\n> monkeypatch the Makefile.\n\nYou don't have to patch it. You can do:\n\n  make CFLAGS='-O2 -fsanitize=leak -DSUPPRESS_ANNOTATED_LEAKS'\n\nYes, that's a mouthful. If you really really want it, we can make the\n\"-O0\" behavior conditional. But I remain entirely unconvinced that what\nyou're trying to do is useful.\n\n> I'd prefer not to need to do that, because while non-O0 is noisy\n> sometimes, I've also found that it points (at least indirectly) to some\n> useful edge-cases.\n\nI haven't seen any evidence that leak-checking with -O2 produces\nanything of value. And I have seen several examples where it produces\ngarbage. I guess you're referring here to the elaboration you give below\nin your message. But I think you're just wrong about it being useful.\n\n> The tl;dr is that I think you use \"leak\" in the sense that valgrind\n> talks about \"still reachable\" leaks, which is conceptually where\n> -fsanitize=leak draws the line. I.e. it's not a malloc()/free() tracker,\n> but tries to see what's \"in scope\".\n\nNo, these \"still reachable\" cases are not at all interesting. If we end\nthe program by returning up the stack, then perhaps they could be (but\nIMHO they are not generally, because it's perfectly reasonable to leave\nglobals pointing to allocated memory at program exit). But if we exit in\nthe middle of a function, and variables are left on the stack, what in\nthe world is the use of a leak-checker complaining about that?\n\nIt is not hurting anything, because by definition we have exited the\nprogram and released the memory. And it is near-impossible to remove\nthese entirely. In the example we're discussing, there's a local\nvariable in git_config_push_env() that _could_ be freed before calling\ndie(). But not in the general case:\n\n  - what about heap allocations in callers? Imagine a program like this:\n\n      void foo(const char *str)\n      {\n        if (some_func(str))\n          die(\"bad str\");\n      }\n      void bar(void)\n      {\n        char *str = strdup(\"some string\");\n\tfoo(str);\n\tfree(str);\n      }\n\n    If foo() calls die(), then bar()'s str is a \"still reachable\" leak.\n    But we can't fix it in any reasonable way. foo() doesn't know about\n    freeing the string. And bar() doesn't know about calling die().\n\n  - likewise, we may actually need the heap-allocated string to call\n    die! Consider this:\n\n      void foo(void)\n      {\n        char *str = strdup(\"some string\");\n\tif (some_func(str))\n\t  die(\"bad str: %s\", str);\n\tfree(str);\n      }\n\n    You can't avoid a \"still reachable\" leak here, because die() would\n    need to use the string, then free it, then exit.\n\nSo if these \"still reachable\" leaks are not hurting anything, and if\nthey are impossible to get rid of, why do we want to care about them?\nDoing so just causes pain with no benefit.\n\n> This is getting a bit academic, but I don't see how you can both say\n> that the \"compilers are allowed to modify the program as long as the\n> observable behavior of that abstract machine is unchanged\" *and* claim\n> that e.g. the git_config_push_env() case isn't a real leak.\n\nThe words \"observable behavior\" are doing a lot of heavy lifting there.\nThat is the behavior which a conforming C program can observe. And in\nthis case, nothing is violated. We did not reach the free() call, so\nthere was no need to make sure we had the parameters available. The C\nstandard's abstract machine doesn't know about things like \"the stack is\nmemory that you can look at\". Doing that is undefined behavior, and a\nprogram which cares about that is not conforming.\n\nI agree this is mostly academic. My point in bringing it up was just to\nsay that no, this isn't a bug in the optimizer. Clobbering values for\nNORETURN is perfectly reasonable and valid according to the standard.\nAnd leak-checking, while basically undefined behavior from the\nperspective of the standard, is a useful tool. It's the interaction\nbetween the two that is ugly, and the most useful thing for us to do is\navoid using both at the same time.\n\n> Because surely the thing that makes it a \"leak\" by your definition (and\n> what LSAN strives for) is that it's attributed to a variable that's \"in\n> scope\", but the compiler is free to re-arrange all of that.\n\nIf you mean \"not a leak\", then yes: there's still an in-scope variable\nthat points to it, which is what makes it not a leak.\n\n> Anyway, one reason I wanted to punt on that \"git --config-env\" issue is\n> because we can entirely avoid the malloc()/free() there. See the \"-- >8\n> --\" below, but if we just malloc() after we do our assertions we can\n> un-confuse clang.\n> \n> And that seems like a good idea in general, and re whether the \"leak\" is\n> gone, at that point valgrind will stop reporting it, so we're really not\n> leaking at all, not just in the \"still reachable\" sense.\n\nI'm not convinced it's a good idea. The resulting code requires an extra\nvariable and is (IMHO) slightly harder to understand. And what have we\naccomplished? We silenced one false positive, but we know there are\nothers. And the linux-leaks CI job is failing on master _right now_, and\nthe patch below doesn't fix that.\n\nThat makes the job somewhat worthless. And worse, it makes all of CI\nless useful, because if it says \"failed\" on every build, people will\nstart to ignore it.\n\n> The reason I mention all of that is to try to define the problem here. I\n> haven't seen cases where the compilers get it wrong about there being a\n> leak, it's just that they're mis-categorizing them as \"still reachable\"\n> or not, re your \"abstract machine\" summary.\n\nThis paragraph makes it sound like we are mixing up a real leak and a\n\"still reachable\" leak. But it is mixing up \"there is no leak and\nnothing to fix\" with \"there is a still reachable leak\".\n\n> Now, the other cases in t1300 are from git.c using exit() instead of\n> retur-ing to our main().\n> \n> Among numerous other leak fixes I have queued up I have a fix for that,\n> which fixes the other t1300 cases that have been reported now:\n> https://github.com/avar/git/tree/avar/git-c-do-not-exit\n\nI said it in the last round of discussion, and I'll say it again: this\nis the wrong direction. These are not real leaks, and we should not be\ntwisting our code to try to avoid them. We should be fixing our\nleak-checking so that they don't come up.\n\n> The actual meaningful fix here is that we don't need to do this\n> allocation at all. The only reason it's needed is because there's no\n> variant of \"sq_quote_buf()\" in quote.c that takes a \"len\"\n> parameter (but I have one locally).\n> \n> If we had that we could just pass a \"key\" and \"key_len\" to\n> git_config_push_split_parameter() instead and avoid the allocation\n> altogether. But in lieu of that better fix (which other API users of\n> quote.c would benefit from) let's defer the allocation, which happens\n> to fix the leak reporting on\n\nSo as you probably guessed, I absolutely hate this patch and think we\nshould not take it.\n\nIf we had a version of sq_quote_buf() that took a \"len\" parameter, then\nsure, it would be reasonable to use. But it absolutely is not worth\ncaring about to silence a leak that is a false positive. There is\n_nothing_ wrong with the code as written.\n\n-Peff\n"},{"id":"465249","messageId":"xmqqfsfju8pa.fsf@gitster.g","threadId":"58647","inReplyTo":"Y08JZVDgJpJvrBiz@coredump.intra.peff.net","subject":"Re: [PATCH] Makefile: force -O0 when compiling with SANITIZE=leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-19T15:36:17Z","receivedAt":"2022-10-19T15:41:58Z","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> Subject: Makefile: force -O0 when compiling with SANITIZE=leak\n>\n> Compiling with -O2 can interact badly with LSan's leak-checker, causing\n> false positives. Imagine a simplified example like:\n>\n>   char *str = allocate_some_string();\n>   if (some_func(str) < 0)\n>           die(\"bad str\");\n>   free(str);\n>\n> The compiler may eliminate \"str\" as a stack variable, and just leave it\n> in a register. The register is preserved through most of the function,\n> including across the call to some_func(), since we'd eventually need to\n> free it. But because die() is marked with NORETURN, the compiler knows\n> that it doesn't need to save registers, and just clobbers it.\n\nYup, this is one weak point in the runtime checker in that it must\nsee the pointer held in the stack or register to ignore a still\nreachable cruft that does not matter upon program exit, which cannot\nwork well with certain optimizations.\n\nTheoretically there may be no guarantee that -O0 would disable all\noptimizations that are potentially problematic to what LSan expects\nto see, but I fully agree with you that this is the right direction.\n\nWill queue.  Thanks.\n"},{"id":"465326","messageId":"939ccb0c-005c-4f98-a6ca-3f8e5cda1641@gmail.com","threadId":"58647","inReplyTo":"Y0+i1G5ybdhUGph2@coredump.intra.peff.net","subject":"Re: [PATCH] Makefile: force -O0 when compiling with SANITIZE=leak","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2022-10-20T18:57:31Z","receivedAt":"2022-10-20T18:57:42Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 19/10/22 9:10, Jeff King wrote:\n \n> Yes, that's a mouthful. If you really really want it, we can make the\n> \"-O0\" behavior conditional. But I remain entirely unconvinced that what\n> you're trying to do is useful.\n> \n>> I'd prefer not to need to do that, because while non-O0 is noisy\n>> sometimes, I've also found that it points (at least indirectly) to some\n>> useful edge-cases.\n> \n> I haven't seen any evidence that leak-checking with -O2 produces\n> anything of value. And I have seen several examples where it produces\n> garbage. I guess you're referring here to the elaboration you give below\n> in your message. But I think you're just wrong about it being useful.\n> \n>> The tl;dr is that I think you use \"leak\" in the sense that valgrind\n>> talks about \"still reachable\" leaks, which is conceptually where\n>> -fsanitize=leak draws the line. I.e. it's not a malloc()/free() tracker,\n>> but tries to see what's \"in scope\".\n> \n> No, these \"still reachable\" cases are not at all interesting. If we end\n> the program by returning up the stack, then perhaps they could be (but\n> IMHO they are not generally, because it's perfectly reasonable to leave\n> globals pointing to allocated memory at program exit). But if we exit in\n> the middle of a function, and variables are left on the stack, what in\n> the world is the use of a leak-checker complaining about that?\n> \n> It is not hurting anything, because by definition we have exited the\n> program and released the memory. And it is near-impossible to remove\n> these entirely. In the example we're discussing, there's a local\n> variable in git_config_push_env() that _could_ be freed before calling\n> die(). But not in the general case:\n> \n>   - what about heap allocations in callers? Imagine a program like this:\n> \n>       void foo(const char *str)\n>       {\n>         if (some_func(str))\n>           die(\"bad str\");\n>       }\n>       void bar(void)\n>       {\n>         char *str = strdup(\"some string\");\n> \tfoo(str);\n> \tfree(str);\n>       }\n> \n>     If foo() calls die(), then bar()'s str is a \"still reachable\" leak.\n>     But we can't fix it in any reasonable way. foo() doesn't know about\n>     freeing the string. And bar() doesn't know about calling die().\n> \n>   - likewise, we may actually need the heap-allocated string to call\n>     die! Consider this:\n> \n>       void foo(void)\n>       {\n>         char *str = strdup(\"some string\");\n> \tif (some_func(str))\n> \t  die(\"bad str: %s\", str);\n> \tfree(str);\n>       }\n> \n>     You can't avoid a \"still reachable\" leak here, because die() would\n>     need to use the string, then free it, then exit.\n> \n> So if these \"still reachable\" leaks are not hurting anything, and if\n> they are impossible to get rid of, why do we want to care about them?\n> Doing so just causes pain with no benefit.\n> \n>> This is getting a bit academic, but I don't see how you can both say\n>> that the \"compilers are allowed to modify the program as long as the\n>> observable behavior of that abstract machine is unchanged\" *and* claim\n>> that e.g. the git_config_push_env() case isn't a real leak.\n> \n> The words \"observable behavior\" are doing a lot of heavy lifting there.\n> That is the behavior which a conforming C program can observe. And in\n> this case, nothing is violated. We did not reach the free() call, so\n> there was no need to make sure we had the parameters available. The C\n> standard's abstract machine doesn't know about things like \"the stack is\n> memory that you can look at\". Doing that is undefined behavior, and a\n> program which cares about that is not conforming.\n> \n> I agree this is mostly academic. My point in bringing it up was just to\n> say that no, this isn't a bug in the optimizer. Clobbering values for\n> NORETURN is perfectly reasonable and valid according to the standard.\n> And leak-checking, while basically undefined behavior from the\n> perspective of the standard, is a useful tool. It's the interaction\n> between the two that is ugly, and the most useful thing for us to do is\n> avoid using both at the same time.\n> \n>> Because surely the thing that makes it a \"leak\" by your definition (and\n>> what LSAN strives for) is that it's attributed to a variable that's \"in\n>> scope\", but the compiler is free to re-arrange all of that.\n> \n> If you mean \"not a leak\", then yes: there's still an in-scope variable\n> that points to it, which is what makes it not a leak.\n> \n>> Anyway, one reason I wanted to punt on that \"git --config-env\" issue is\n>> because we can entirely avoid the malloc()/free() there. See the \"-- >8\n>> --\" below, but if we just malloc() after we do our assertions we can\n>> un-confuse clang.\n>>\n>> And that seems like a good idea in general, and re whether the \"leak\" is\n>> gone, at that point valgrind will stop reporting it, so we're really not\n>> leaking at all, not just in the \"still reachable\" sense.\n> \n> I'm not convinced it's a good idea. The resulting code requires an extra\n> variable and is (IMHO) slightly harder to understand. And what have we\n> accomplished? We silenced one false positive, but we know there are\n> others. And the linux-leaks CI job is failing on master _right now_, and\n> the patch below doesn't fix that.\n> \n> That makes the job somewhat worthless. And worse, it makes all of CI\n> less useful, because if it says \"failed\" on every build, people will\n> start to ignore it.\n> \n>> The reason I mention all of that is to try to define the problem here. I\n>> haven't seen cases where the compilers get it wrong about there being a\n>> leak, it's just that they're mis-categorizing them as \"still reachable\"\n>> or not, re your \"abstract machine\" summary.\n> \n> This paragraph makes it sound like we are mixing up a real leak and a\n> \"still reachable\" leak. But it is mixing up \"there is no leak and\n> nothing to fix\" with \"there is a still reachable leak\".\n> \n>> Now, the other cases in t1300 are from git.c using exit() instead of\n>> retur-ing to our main().\n>>\n>> Among numerous other leak fixes I have queued up I have a fix for that,\n>> which fixes the other t1300 cases that have been reported now:\n>> https://github.com/avar/git/tree/avar/git-c-do-not-exit\n> \n> I said it in the last round of discussion, and I'll say it again: this\n> is the wrong direction. These are not real leaks, and we should not be\n> twisting our code to try to avoid them. We should be fixing our\n> leak-checking so that they don't come up.\n> \n>> The actual meaningful fix here is that we don't need to do this\n>> allocation at all. The only reason it's needed is because there's no\n>> variant of \"sq_quote_buf()\" in quote.c that takes a \"len\"\n>> parameter (but I have one locally).\n>>\n>> If we had that we could just pass a \"key\" and \"key_len\" to\n>> git_config_push_split_parameter() instead and avoid the allocation\n>> altogether. But in lieu of that better fix (which other API users of\n>> quote.c would benefit from) let's defer the allocation, which happens\n>> to fix the leak reporting on\n> \n> So as you probably guessed, I absolutely hate this patch and think we\n> should not take it.\n> \n> If we had a version of sq_quote_buf() that took a \"len\" parameter, then\n> sure, it would be reasonable to use. But it absolutely is not worth\n> caring about to silence a leak that is a false positive. There is\n> _nothing_ wrong with the code as written.\n> \n> -Peff\n> \n\n\"-O2\" is what goes public, testing it, directly or indirectly, is useful.\nAnother thing is the pay-off..\n\nWe're losing a few eyeballs on \"-O2\" and some test performance in the way, in\nexchange of avoiding some false positives.  And even with \"-O0\" the compiler is\nfree to keep playing the whack-a-mole with us.\n\nI can easily find motivations for people to switch to \"-O0\" and do its local\ntests for example, and so we add eyeballs... but not so easily the other way\naround, to switch to \"-O2\".\n\n\"False positives\" makes me think the leak checker does its best.  And the\ncompiler.  This \"-O0 leak\" is ignored by both, with \"-O2\":\n\n\tvoid func()\n\t{\n\t\tmalloc(1024);\n\t\texit(1);\n\t}\n\nUNLEAK(), \"-O0\" as a /second opinion/ /confirmation/, some attention to the\nfalse positives,... are things that doesn't sound so bad.  Maybe there is a\nbetter way.  Dunno.\n\nUn saludo.\n"},{"id":"465426","messageId":"Y1I2Z9K4iaAr+GiP@coredump.intra.peff.net","threadId":"58647","inReplyTo":"939ccb0c-005c-4f98-a6ca-3f8e5cda1641@gmail.com","subject":"Re: [PATCH] Makefile: force -O0 when compiling with SANITIZE=leak","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-21T06:04:23Z","receivedAt":"2022-10-21T06:04:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 20, 2022 at 08:57:31PM +0200, Rubén Justo wrote:\n\n> \"-O2\" is what goes public, testing it, directly or indirectly, is useful.\n> Another thing is the pay-off..\n\nSort of. The leak-check build itself is not what goes public, so we have\nalready diverged from a release build. But yes, if there were cases that\n-O2 caught that -O0 did not, that would be of interest.\n\nBut are there?\n\nI.e., we already know of false positives with -O2. Are there false\nnegatives with -O0?\n\nThe example you give below goes the other way:\n\n> \"False positives\" makes me think the leak checker does its best.  And the\n> compiler.  This \"-O0 leak\" is ignored by both, with \"-O2\":\n> \n> \tvoid func()\n> \t{\n> \t\tmalloc(1024);\n> \t\texit(1);\n> \t}\n\nIt is a false negative with -O2. Which again seems to argue for using\n-O0.\n\n> UNLEAK(), \"-O0\" as a /second opinion/ /confirmation/, some attention to the\n> false positives,... are things that doesn't sound so bad.  Maybe there is a\n> better way.  Dunno.\n\nI don't know why we'd want to prefer -O2 if we know it produces worse\nresults, but don't have any cases where it produce better ones (or even\na plausible explanation for why it might). Sure, we could squelch its\nfalse positives with UNLEAK(), but:\n\n  - that's per-site work, versus setting -O0 once\n\n  - UNLEAK() suppresses false positives, but it also would suppress true\n    positives (e.g., if the code later changed and somebody introduced a\n    leak). It seems better to avoid that if possible.\n\n-Peff\n"}]}