All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Andrzej Hunt via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: Carlo Arenas <carenas@gmail.com>,
	Andrzej Hunt <andrzej@ahunt.org>,
	Andrzej Hunt <ajrhunt@google.com>
Subject: [PATCH 2/2] log: UNLEAK original pending objects
Date: Sat, 18 Sep 2021 13:49:38 +0000	[thread overview]
Message-ID: <aad3fe7381ced5eeff9c8d57ce90911bc59e3923.1631972978.git.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.1092.git.git.1631972978.gitgitgadget@gmail.com>

From: Andrzej Hunt <ajrhunt@google.com>

cmd_show() uses objects to point to rev.pending's objects. Later, it might
detach rev.pending's objects: when handling an OBJ_COMMIT it will reset
rev.pending (without freeing its objects). Detaching (as opposed to freeing)
is necessary because cmd_show() continues iterating over the original objects
array.

We choose to UNLEAK because there's no real advantage to cleaning up
properly (cmd_show() exits immediately after looping over these
objects). A number of alternatives exist, but are all significantly more
complex for no gain:

Alternative 1:
  Convert objects into an object_array, and memcpy rev.pending into it
  (followed by detaching rev.pending immediately - making objects the
  owner of what used to be rev.pending). Then we could safely
  objects_array_clear() at the end of cmd_show(). And we can rely on
  a preexisting UNLEAK(rev) to avoid having to clean up rev.pending.
  This is a more complex and riskier approach vs a simple UNLEAK,
  and doesn't add any user-visible value.

Alternative 2:
  A variation on alternative 1. We make objects own the object_array as
  before. Once we're done, we free the new rev.pending array (which
  might be empty), and we memcpy objects back into rev.pending, relying
  on the existin UNLEAK(rev) to avoid having to free rev.pending.

ASAN output from t0000:

Direct leak of 41 byte(s) in 1 object(s) allocated from:
    #0 0x487504 in strdup ../projects/compiler-rt/lib/asan/asan_interceptors.cpp:437:3
    #1 0x9e4ef8 in xstrdup wrapper.c:29:14
    #2 0x86395d in add_object_array_with_path object.c:366:17
    #3 0x9264fc in add_pending_object_with_path revision.c:330:2
    #4 0x91c4e0 in handle_revision_arg_1 revision.c:2086:2
    #5 0x91bfcd in handle_revision_arg revision.c:2093:12
    #6 0x91ff5a in setup_revisions revision.c:2780:7
    #7 0x5a7678 in cmd_log_init_finish builtin/log.c:206:9
    #8 0x5a4f18 in cmd_log_init builtin/log.c:278:2
    #9 0x5a55d1 in cmd_show builtin/log.c:646:2
    #10 0x4cff30 in run_builtin git.c:461:11
    #11 0x4cdb00 in handle_builtin git.c:713:3
    #12 0x4cf527 in run_argv git.c:780:4
    #13 0x4cd426 in cmd_main git.c:911:19
    #14 0x6b2eb5 in main common-main.c:52:11
    #15 0x7f74fc9bd349 in __libc_start_main (/lib64/libc.so.6+0x24349)

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

Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
---
 builtin/log.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/log.c b/builtin/log.c
index 6faaddf17a6..769ee6a9258 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -702,7 +702,8 @@ int cmd_show(int argc, const char **argv, const char *prefix)
 			ret = error(_("unknown type: %d"), o->type);
 		}
 	}
-	free(objects);
+	UNLEAK(objects);
+
 	return ret;
 }
 
-- 
gitgitgadget

  parent reply	other threads:[~2021-09-18 13:49 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-18 13:49 [PATCH 0/2] Squash leaks in t0000 Andrzej Hunt via GitGitGadget
2021-09-18 13:49 ` [PATCH 1/2] log: UNLEAK rev to silence a large number of leaks Andrzej Hunt via GitGitGadget
2021-09-18 20:06   ` Carlo Marcelo Arenas Belón
2021-09-19 15:51     ` Andrzej Hunt
2021-09-19 16:13     ` Ævar Arnfjörð Bjarmason
2021-09-19 21:34       ` Carlo Marcelo Arenas Belón
2021-09-20  6:06         ` Eric Sunshine
2021-09-20 21:39           ` Carlo Marcelo Arenas Belón
2021-09-21  3:09             ` Jeff King
2021-09-18 13:49 ` Andrzej Hunt via GitGitGadget [this message]
2021-09-18 17:28 ` [PATCH 0/2] Squash leaks in t0000 Carlo Arenas
2021-09-19 15:38   ` Andrzej Hunt
2021-09-19 10:58 ` Ævar Arnfjörð Bjarmason
2021-09-20 17:55 ` Junio C Hamano
2021-09-21 23:01   ` Ævar Arnfjörð Bjarmason

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

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

  git send-email \
    --in-reply-to=aad3fe7381ced5eeff9c8d57ce90911bc59e3923.1631972978.git.gitgitgadget@gmail.com \
    --to=gitgitgadget@gmail.com \
    --cc=ajrhunt@google.com \
    --cc=andrzej@ahunt.org \
    --cc=carenas@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.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.