When we parse a commit via repo_parse_commit_internal(), if
save_commit_buffer is set we'll stuff the buffer of the object contents
into a cache, overwriting any previous value.
This can result in a leak of that previously cached value, though it's
rare in practice. If we have a value in the cache it would have come
from a previous parse, and during that parse we'd set the object.parsed
flag, causing any subsequent parse attempts to exit without doing any
work.
But it's possible to "unparse" a commit, which we do when registering a
commit graft. And since shallow fetches are implemented using grafts,
the leak is triggered in practice by t5539.
There are a number of possible ways to address this:
1. the unparsing function could clear the cached commit buffer, too. I
think this would work for the case I found, but I'm not sure if
there are other ways to end up in the same state (an unparsed
commit with an entry in the commit buffer cache).
2. when we parse, we could check the buffer cache and prefer it to
reading the contents from the object database. In theory the
contents of a particular sha1 are immutable, but the code in
question is violating the immutability with grafts. So this
approach makes me a bit nervous, although I think it would work in
practice (the grafts are applied to what we parse, but we still
retain the original contents).
3. We could realize the cache is already populated and discard its
contents before overwriting. It's possible some other code could be
holding on to a pointer to the old cache entry (and we'd introduce
a use-after-free), but I think the risk of that is relatively low.
4. The reverse of (3): when the cache is populated, don't bother
saving our new copy. This is perhaps a little weird, since we'll
have just populated the commit struct based on a different buffer.
But the two buffers should be the same, even in the presence of
grafts (as in (2) above).
I went with option 4. It addresses the leak directly and doesn't carry
any risk of breaking other assumptions. And it's the same technique used
by parse_object_buffer() for this situation, though I'm not sure when it
would even come up there. The extra safety has been there since
bd1e17e245 (Make "parse_object()" also fill in commit message buffer
data., 2005-05-25).
This lets us mark t5539 as leak-free.
Signed-off-by: Jeff King <peff@xxxxxxxx>
---
commit.c | 3 ++-
t/t5539-fetch-http-shallow.sh | 1 +
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/commit.c b/commit.c
index 3a54e4db0d..cc03a93036 100644
--- a/commit.c
+++ b/commit.c
@@ -595,7 +595,8 @@ int repo_parse_commit_internal(struct repository *r,
}
ret = parse_commit_buffer(r, item, buffer, size, 0);
- if (save_commit_buffer && !ret) {
+ if (save_commit_buffer && !ret &&
+ !get_cached_commit_buffer(r, item, NULL)) {
set_commit_buffer(r, item, buffer, size);
return 0;
}
diff --git a/t/t5539-fetch-http-shallow.sh b/t/t5539-fetch-http-shallow.sh
index 3ea75d34ca..82fe09d0a9 100755
--- a/t/t5539-fetch-http-shallow.sh
+++ b/t/t5539-fetch-http-shallow.sh
@@ -5,6 +5,7 @@ test_description='fetch/clone from a shallow clone over http'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
. "$TEST_DIRECTORY"/lib-httpd.sh
start_httpd
--
2.46.2.1011.gf1f9323e02
