Re: [PATCH v9 17/17] fsck: report invalid object type-path combinations
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 30, 2021, 21:01 UTC
- Message-ID
- <xmqqsfxlaicg.fsf@gitster.g>
- In-Reply-To
- <patch-v9-17.17-8d926e41fc3-20210930T133300Z-avarab@gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 43 quoted lines
> diff --git a/builtin/fsck.c b/builtin/fsck.c
> index 623f8fc3194..980c26e3b25 100644
> --- a/builtin/fsck.c
> +++ b/builtin/fsck.c
> @@ -598,23 +598,30 @@ static int fsck_loose(const struct object_id *oid, const char *path, void *data)
> struct object *obj;
> enum object_type type;
> unsigned long size;
> - void *contents;
> + unsigned char *contents = NULL;
> int eaten;
> struct strbuf sb = STRBUF_INIT;
> struct object_info oi = OBJECT_INFO_INIT;
> - int err = 0;
> + struct object_id real_oid = *null_oid();
> + int ret;
>
> oi.type_name = &sb;
> oi.sizep = &size;
> oi.typep = &type;
>
> - if (read_loose_object(path, oid, &contents, &oi) < 0)
> - err = error(_("%s: object corrupt or missing: %s"),
> - oid_to_hex(oid), path);
> + ret = read_loose_object(path, oid, &real_oid, (void **)&contents, &oi);
> + if (ret < 0) {
> + if (contents && !oideq(&real_oid, oid))
> + error(_("%s: hash-path mismatch, found at: %s"),
> + oid_to_hex(&real_oid), path);
> + else
> + error(_("%s: object corrupt or missing: %s"),
> + oid_to_hex(oid), path);
> + }
> if (type < 0)
> - err = error(_("%s: object is of unknown type '%s': %s"),
> - oid_to_hex(oid), sb.buf, path);
> - if (err) {
> + ret = error(_("%s: object is of unknown type '%s': %s"),
> + oid_to_hex(&real_oid), sb.buf, path);
> + if (ret < 0) {
> errors_found |= ERROR_OBJECT;
> goto cleanup;
> }This is immediately touching up what 16/17 has introduced, which is making it a bit harder to follow than necessary, so let's take the whole postimage of 16+17.
Show 24 quoted lines
> static int fsck_loose(const struct object_id *oid, const char *path, void *data)
> {
> struct object *obj;
> enum object_type type;
> unsigned long size;
> unsigned char *contents = NULL;
> int eaten;
> struct strbuf sb = STRBUF_INIT;
> struct object_info oi = OBJECT_INFO_INIT;
> struct object_id real_oid = *null_oid();
> int ret;
>
> oi.type_name = &sb;
> oi.sizep = &size;
> oi.typep = &type;
>
> ret = read_loose_object(path, oid, &real_oid, (void **)&contents, &oi);
> if (ret < 0) {
> if (contents && !oideq(&real_oid, oid))
> error(_("%s: hash-path mismatch, found at: %s"),
> oid_to_hex(&real_oid), path);
> else
> error(_("%s: object corrupt or missing: %s"),
> oid_to_hex(oid), path);We can emit an error() message from either one of these. contents may or may not be NULL, ret is negative, and we continue. Do we know anything about the value of type at this point? IOW, will we get into the body of the next "if (type < 0)" statement to overwrite ret with -1?
Show 7 quoted lines
> }
> if (type < 0)
> ret = error(_("%s: object is of unknown type '%s': %s"),
> oid_to_hex(&real_oid), sb.buf, path);
> if (ret < 0) {
> errors_found |= ERROR_OBJECT;
> goto cleanup;In any case, we'd jump to clean-up if any of the above hold, so we'd avoid hittign the next BUG().
Show 25 quoted lines
> }
>
> if (!contents && type != OBJ_BLOB)
> BUG("read_loose_object streamed a non-blob");
>
> obj = parse_object_buffer(the_repository, oid, type, size,
> contents, &eaten);
>
> if (!obj) {
> errors_found |= ERROR_OBJECT;
> error(_("%s: object could not be parsed: %s"),
> oid_to_hex(oid), path);
> goto cleanup_eaten;
> }
>
> obj->flags &= ~(REACHABLE | SEEN);
> obj->flags |= HAS_OBJ;
> if (fsck_obj(obj, contents, size))
> errors_found |= ERROR_OBJECT;
>
> cleanup_eaten:
> if (!eaten)
> free(contents);
> cleanup:
> strbuf_release(&sb);In the "goto cleanup" error case above, we haven't done anything that would have caused the object contents eaten, and contents may either point at an allocated memory or NULL (in the "hash-path mismatch" case, we may have contents allocated but nobody has freed it yet, leaking it).
I am wondering if we initialized "eaten" to false, we can get rid of one of the two labels we added in this series, which would fix this leak as well, no?
> return 0; /* keep checking other objects, even if we saw an error */ > }