git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Andrzej Hunt via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: Han-Wen Nienhuys <hanwen@google.com>,
	Andrzej Hunt <andrzej@ahunt.org>,
	Andrzej Hunt <ajrhunt@google.com>
Subject: [PATCH] builtin/init-db: preemptively clear repo_fmt to avoid leaks
Date: Wed, 05 May 2021 18:40:22 +0000	[thread overview]
Message-ID: <pull.1018.git.git.1620240022594.gitgitgadget@gmail.com> (raw)

From: Andrzej Hunt <ajrhunt@google.com>

init_db() could populate various fields on repo_fmt, we add a
clear_repo_fmt() call to guarantee that we won't leak any of repo_fmt's
members.

At this time, we do not allocate any new data into repo_fmt, but that
changes in the following patch that is currently in seen:
  https://lore.kernel.org/git/2fd7cb8c0983501e2af2f195aec81d7c17fb80e1.1618832277.git.gitgitgadget@gmail.com/
Because it adds the following allocation in init_db():
  repo_fmt.ref_storage = xstrdup(ref_storage_format);

The patch linked to above is only exposing a preexisting problem - we
can add clear_repository_format() independently to preemptively avoid
this class of leak entirely.

Leaks in init_db() are of little significance as this function is called
immediately at the end of cmd_init_db(). However a leak in init_db()
will cause all known leak-free tests (t0000+1+5) to start failing when
run with LSAN - preemptively plugging such leaks is therefore helpful to
avoid regressing on those tests, and more significantly helps the ongoing
efforts to get all of t0000-t0099 to run leak-free.

LSAN output from t0000 on seen:

Direct leak of 6 byte(s) in 1 object(s) allocated from:
    #0 0x4868b4 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3
    #1 0xaa1218 in xstrdup wrapper.c:29:14
    #2 0x5a4b0a in init_db builtin/init-db.c:442:25
    #3 0x5a7a17 in cmd_init_db builtin/init-db.c:742:9
    #4 0x4ce8fe in run_builtin git.c:461:11
    #5 0x4ccbc8 in handle_builtin git.c:718:3
    #6 0x4cb0cc in run_argv git.c:785:4
    #7 0x4cb0cc in cmd_main git.c:916:19
    #8 0x6bf63d in main common-main.c:52:11
    #9 0x7f3222f5f349 in __libc_start_main (/lib64/libc.so.6+0x24349)

SUMMARY: AddressSanitizer: 6 byte(s) leaked in 1 allocation(s).

Signed-off-by: Andrzej Hunt <ajrhunt@google.com>
---
    builtin/init-db: preemptively plug repo_fmt leak
    
    This patch preemptively plugs a leak which is currently only occuring in
    seen in hn/reftable.
    
    This patch is orthogonal to that series (that series starts allocating
    new memory into an already existing struct - and my patch only adds a
    clear call for that struct - in other words my patch is safe to use
    independent of hn/reftable). But there's also no good reason to use this
    patch independently of that series since we're not leaking prior to that
    series. I'm not sure what the optimal logistics with my patch are, feel
    free to integrate it into that series and/or to fold my change into the
    relevant commit if that's easiest!

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1018%2Fahunt%2Finit_db_leak-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1018/ahunt/init_db_leak-v1
Pull-Request: https://github.com/git/git/pull/1018

 builtin/init-db.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index 31b718259a64..b25147ebaf59 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -512,6 +512,7 @@ int init_db(const char *git_dir, const char *real_git_dir,
 			       git_dir, len && git_dir[len-1] != '/' ? "/" : "");
 	}
 
+	clear_repository_format(&repo_fmt);
 	free(original_git_dir);
 	return 0;
 }

base-commit: c3901c8daf043cdc16ffb20d3a410f3a2d5494fd
-- 
gitgitgadget

             reply	other threads:[~2021-05-05 18:40 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-05 18:40 Andrzej Hunt via GitGitGadget [this message]
2021-05-05 19:28 ` [PATCH] builtin/init-db: preemptively clear repo_fmt to avoid leaks Johannes Schindelin
2021-05-06  5:40   ` Andrzej Hunt
2021-05-06 14:00     ` Johannes Schindelin

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=pull.1018.git.git.1620240022594.gitgitgadget@gmail.com \
    --to=gitgitgadget@gmail.com \
    --cc=ajrhunt@google.com \
    --cc=andrzej@ahunt.org \
    --cc=git@vger.kernel.org \
    --cc=hanwen@google.com \
    /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).