git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: git@vger.kernel.org
Cc: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: LSan curiosity in t1300
Date: Thu, 8 Sep 2022 01:29:58 -0400	[thread overview]
Message-ID: <Yxl91jfycCo7O7Pp@coredump.intra.peff.net> (raw)

If I run:

  make SANITIZE=leak GIT_TEST_PASSING_SANITIZE_LEAK=true CFLAGS=-O0

locally, then all tests pass or are skipped. But if switch to -O2, then
t1300 starts failing!

The command that supposedly leaks is this:

  git --config-env=foo.flag= config --bool foo.flag

and the backtrace looks like:

  Direct leak of 9 byte(s) in 1 object(s) allocated from:
      #0 0x56195ae6dc62 in __interceptor_malloc (git+0x78c62) (BuildId: a5cfeb484fd14b6120fa26ff364fa2313fd23168)
      #1 0x56195b0ad6b6 in do_xmalloc wrapper.c
      #2 0x56195b0ad7bd in xmemdupz (git+0x2b87bd) (BuildId: a5cfeb484fd14b6120fa26ff364fa2313fd23168)
      #3 0x56195af6f8d0 in git_config_push_env (git+0x17a8d0) (BuildId: a5cfeb484fd14b6120fa26ff364fa2313fd23168)
      #4 0x56195ae71689 in handle_options git.c
      #5 0x56195ae7060f in cmd_main (git+0x7b60f) (BuildId: a5cfeb484fd14b6120fa26ff364fa2313fd23168)
      #6 0x56195af370c6 in main (git+0x1420c6) (BuildId: a5cfeb484fd14b6120fa26ff364fa2313fd23168)
      #7 0x7f45b9029209 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
      #8 0x7f45b90292bb in __libc_start_main csu/../csu/libc-start.c:389:3
      #9 0x56195ae420c0 in _start (git+0x4d0c0) (BuildId: a5cfeb484fd14b6120fa26ff364fa2313fd23168)

But here's the weird part. The function looks like this:

  void git_config_push_env(const char *spec)
  {
          char *key;
          const char *env_name;
          const char *env_value;
  
          env_name = strrchr(spec, '=');
          if (!env_name)
                  die(_("invalid config format: %s"), spec);
          key = xmemdupz(spec, env_name - spec);
          env_name++;
          if (!*env_name)
                  die(_("missing environment variable name for configuration '%.*s'"),
                      (int)(env_name - spec - 1), spec);
  
          env_value = getenv(env_name);
          if (!env_value)
                  die(_("missing environment variable '%s' for configuration '%.*s'"),
                      env_name, (int)(env_name - spec - 1), spec);
  
          git_config_push_split_parameter(key, env_value);
          free(key);
  }

And it is complaining that we've leaked "key". But as you can see, we
always free it. The problem is that for this particular invocation we
die("missing environment variable name"), so of course we don't make it
to the free(). Normally this is OK, though. The "key" variable is still
on the stack, so the leak-checker should realize that it's still
reachable.

But if you run this in a debugger, you'll find that under -O2 the "key"
variable has been optimized out! So LSan is producing the wrong result
due to the optimization. It doesn't know that "key" is conceptually
still reachable.

Maybe this is a known problem, but it was certainly surprising to me.
I'm not sure if we should do anything about it or not. It doesn't seem
to trigger in CI, even though I don't see us taking any steps there to
use -O0 or similar. So we can perhaps ignore it for now, and this
message can serve as a warning. But if we think LSan isn't reliable
under higher optimizations, we could perhaps teach the Makefile to
prefer -O0 when it sees SANITIZE=leak.

-Peff

             reply	other threads:[~2022-09-08  5:30 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-09-08  5:29 Jeff King [this message]
2022-09-08 15:42 ` [PATCH] tests: opt "git --config-env" test out of SANITIZE=leak Ævar Arnfjörð Bjarmason
2022-09-08 18:40   ` Jeff King
2022-09-23  8:28     ` Ævar Arnfjörð Bjarmason
2022-09-23 21:01       ` Jeff King
2022-09-26  8:56         ` Ævar Arnfjörð Bjarmason
2022-09-08 18:03 ` LSan curiosity in t1300 Junio C Hamano
2022-09-08 18:15   ` Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Yxl91jfycCo7O7Pp@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).