@matheusbsbs / matheus-bagliano / commits / 16235e3b14

object-file: free(*contents) only in read_loose_object() caller

In the preceding commit a free() of uninitialized memory regression in 96e41f58fe1 (fsck: report invalid object type-path combinations, 2021-10-01) was fixed, but we'd still have an issue with leaking memory from fsck_loose(). Let's fix that issue too. That issue was introduced in my 31deb28f5e0 (fsck: don't hard die on invalid object types, 2021-10-01). It can be reproduced under SANITIZE=leak with the test I added in 093fffdfbec (fsck tests: add test for fsck-ing an unknown type, 2021-10-01): ./t1450-fsck.sh --run=84 -vixd In some sense it's not a problem, we lost the same amount of memory in terms of things malloc'd and not free'd. It just moved from the "still reachable" to "definitely lost" column in valgrind(1) nomenclature[1], since we'd have die()'d before. But now that we don't hard die() anymore in the library let's properly free() it. Doing so makes this code much easier to follow, since we'll now have one function owning the freeing of the "contents" variable, not two. For context on that memory management pattern the read_loose_object() function was added in f6371f92104 (sha1_file: add read_loose_object() function, 2017-01-13) and subsequently used in c68b489e564 (fsck: parse loose object paths directly, 2017-01-13). The pattern of it being the task of both sides to free() the memory has been there in this form since its inception. 1. https://valgrind.org/docs/manual/mc-manual.html#mc-manual.leaks Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Ævar Arnfjörð Bjarmason committed Nov 11, 2021 at 06:18 UTC 16235e3b1460ddd708995b4cc5028e49d47e3761

2 files changed +4 -6

builtin/fsck.c

+2 -1

@@ -604,7 +604,7 @@ static int fsck_loose(const struct object_id *oid, const char *path, void *data)

604 struct object *obj;

605 enum object_type type = OBJ_NONE;

606 unsigned long size;

607 - void *contents;

607 + void *contents = NULL;

608 int eaten;

609 struct object_info oi = OBJECT_INFO_INIT;

610 struct object_id real_oid = *null_oid();

@@ -629,6 +629,7 @@ static int fsck_loose(const struct object_id *oid, const char *path, void *data)

629 path);

630 if (err < 0) {

631 errors_found |= ERROR_OBJECT;

632 + free(contents);

633 return 0; /* keep checking other objects */

634 }

635

object-file.c

+2 -5

@@ -2533,8 +2533,6 @@ int read_loose_object(const char *path,

2533 char hdr[MAX_HEADER_LEN];

2534 unsigned long *size = oi->sizep;

2535

2536 - *contents = NULL;

2537 -

2536 map = map_loose_object_1(the_repository, path, NULL, &mapsize);

2537 if (!map) {

2538 error_errno(_("unable to mmap %s"), path);

@@ -2564,10 +2562,9 @@ int read_loose_object(const char *path,

2562 goto out;

2563 }

2564 if (check_object_signature(the_repository, expected_oid,

2567 - *contents, *size, oi->type_name->buf, real_oid)) {

2568 - free(*contents);

2565 + *contents, *size,

2566 + oi->type_name->buf, real_oid))

2567 goto out;

2570 - }

2568 }

2569

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