@matheusbsbs / matheus-bagliano / commits / 84b5c1a099

unpack_loose_rest(): never clean up zstream

The unpack_loose_rest() function has funny ownership semantics: we pass in a z_stream opened by the caller, but then only _sometimes_ close it. This oddity has developed over time. When the function was originally split out in 5180cacc20 (Split up unpack_sha1_file() some more, 2005-06-02), it always called inflateEnd() to clean up the stream (though nowadays it is a git_zstream and we call git_inflate_end()). But in 7efbff7531 (unpack_sha1_file(): detect corrupt loose object files., 2007-03-05) we added error code paths which don't close the stream. This makes some sense, as we'd still look at parts of the stream struct to decide which error to show (though I am not sure in practice if inflateEnd() even touches those fields). This subtlety makes it hard to know when the caller has to clean up the stream and when it does not. That led to the leak fixed by aa9ef614dc (object-file: fix memory leak when reading corrupted headers, 2024-08-14). Let's instead always leave the stream intact, forcing the caller to clean it up. You might think that would create more work for the callers, but it actually ends up simplifying them, since they can put the call to git_inflate_end() in the common cleanup code path. Two things to note, though: - The check_stream_oid() function is used as a replacement for unpack_loose_rest() in read_loose_object() to read blobs. It inherited the same funny semantics, and we should fix it here, too (to keep the cleanup in read_loose_object() consistent). - In read_loose_object() we need a second "out" label, as we can jump to the existing label before opening the stream at all (and since the struct is opaque, there is no way to if it was initialized or not, so we must not call git_inflate_end() in that case). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Feb 25, 2025 at 01:33 UTC 84b5c1a099e6df35f4b54d651b425a894513e62b

1 file changed +8 -10

object-file.c

+8 -10

@@ -1348,7 +1348,6 @@ static void *unpack_loose_rest(git_zstream *stream,

1348 }

1349 }

1350 if (status == Z_STREAM_END && !stream->avail_in) {

1351 - git_inflate_end(stream);

1351 return buf;

1352 }

1353

@@ -1512,8 +1511,8 @@ static int loose_object_info(struct repository *r,

1511 die(_("loose object %s (stored in %s) is corrupt"),

1512 oid_to_hex(oid), path);

1513

1515 - git_inflate_end(&stream);

1514 cleanup:

1515 + git_inflate_end(&stream);

1516 munmap(map, mapsize);

1517 if (oi->sizep == &size_scratch)

1518 oi->sizep = NULL;

@@ -2735,7 +2734,6 @@ static int check_stream_oid(git_zstream *stream,

2734 the_hash_algo->update_fn(&c, buf, stream->next_out - buf);

2735 total_read += stream->next_out - buf;

2736 }

2738 - git_inflate_end(stream);

2737

2738 if (status != Z_STREAM_END) {

2739 error(_("corrupt loose object '%s'"), oid_to_hex(expected_oid));

@@ -2782,34 +2780,34 @@ int read_loose_object(const char *path,

2780 if (unpack_loose_header(&stream, map, mapsize, hdr, sizeof(hdr),

2781 NULL) != ULHR_OK) {

2782 error(_("unable to unpack header of %s"), path);

2785 - goto out;

2783 + goto out_inflate;

2784 }

2785

2786 if (parse_loose_header(hdr, oi) < 0) {

2787 error(_("unable to parse header of %s"), path);

2790 - git_inflate_end(&stream);

2791 - goto out;

2788 + goto out_inflate;

2789 }

2790

2791 if (*oi->typep == OBJ_BLOB && *size > big_file_threshold) {

2792 if (check_stream_oid(&stream, hdr, *size, path, expected_oid) < 0)

2796 - goto out;

2793 + goto out_inflate;

2794 } else {

2795 *contents = unpack_loose_rest(&stream, hdr, *size, expected_oid);

2796 if (!*contents) {

2797 error(_("unable to unpack contents of %s"), path);

2801 - git_inflate_end(&stream);

2802 - goto out;

2798 + goto out_inflate;

2799 }

2800 hash_object_file_literally(the_repository->hash_algo,

2801 *contents, *size,

2802 oi->type_name->buf, real_oid);

2803 if (!oideq(expected_oid, real_oid))

2808 - goto out;

2804 + goto out_inflate;

2805 }

2806

2807 ret = 0; /* everything checks out */

2808

2809 +out_inflate:

2810 + git_inflate_end(&stream);

2811 out:

2812 if (map)

2813 munmap(map, mapsize);