diff options
author | Ævar Arnfjörð Bjarmason <avarab@gmail.com> | 2021-10-01 12:16:51 +0300 |
---|---|---|
committer | Junio C Hamano <gitster@pobox.com> | 2021-10-02 01:06:00 +0300 |
commit | dccb32bf01411213297cde63cf689e476673a8ec (patch) | |
tree | 34799b342c1ae5c8570c26fc3032a9099d3b18e8 | |
parent | 5848fb11acd0b6aad6ba9e3e71bd91485e0d4c71 (diff) |
object-file.c: stop dying in parse_loose_header()
Make parse_loose_header() return error codes and data instead of
invoking die() by itself.
For now we'll move the relevant die() call to loose_object_info() and
read_loose_object() to keep this change smaller. In a subsequent
commit we'll make read_loose_object() return an error code instead of
dying. We should also address the "allow_unknown" case (should be
moved to builtin/cat-file.c), but for now I'll be leaving it.
For making parse_loose_header() not die() change its prototype to
accept a "struct object_info *" instead of the "unsigned long *sizep"
it accepted before. Its callers can now check the populated populated
"oi->typep".
Because of this we don't need to pass in the "unsigned int flags"
which we used for OBJECT_INFO_ALLOW_UNKNOWN_TYPE, we can instead do
that check in loose_object_info().
This also refactors some confusing control flow around the "status"
variable. In some cases we set it to the return value of "error()",
i.e. -1, and later checked if "status < 0" was true.
Since 93cff9a978e (sha1_loose_object_info: return error for corrupted
objects, 2017-04-01) the return value of loose_object_info() (then
named sha1_loose_object_info()) had been a "status" variable that be
any negative value, as we were expecting to return the "enum
object_type".
The only negative type happens to be OBJ_BAD, but the code still
assumed that more might be added. This was then used later in
e.g. c84a1f3ed4d (sha1_file: refactor read_object, 2017-06-21). Now
that parse_loose_header() will return 0 on success instead of the
type (which it'll stick into the "struct object_info") we don't need
to conflate these two cases in its callers.
Since parse_loose_header() doesn't need to return an arbitrary
"status" we only need to treat its "ret < 0" specially, but can
idiomatically overwrite it with our own error() return. This along
with having made unpack_loose_header() return an "enum
unpack_loose_header_result" in an earlier commit means that we can
move the previously nested if/else cases mostly into the "ULHR_OK"
branch of the "switch" statement.
We should be less silent if we reach that "status = -1" branch, which
happens if we've got trailing garbage in loose objects, see
f6371f92104 (sha1_file: add read_loose_object() function, 2017-01-13)
for a better way to handle it. For now let's punt on it, a subsequent
commit will address that edge case.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
-rw-r--r-- | cache.h | 11 | ||||
-rw-r--r-- | object-file.c | 67 | ||||
-rw-r--r-- | streaming.c | 3 |
3 files changed, 44 insertions, 37 deletions
@@ -1332,9 +1332,16 @@ enum unpack_loose_header_result unpack_loose_header(git_zstream *stream, unsigned long bufsiz, struct strbuf *hdrbuf); +/** + * parse_loose_header() parses the starting "<type> <len>\0" of an + * object. If it doesn't follow that format -1 is returned. To check + * the validity of the <type> populate the "typep" in the "struct + * object_info". It will be OBJ_BAD if the object type is unknown. The + * parsed <len> can be retrieved via "oi->sizep", and from there + * passed to unpack_loose_rest(). + */ struct object_info; -int parse_loose_header(const char *hdr, struct object_info *oi, - unsigned int flags); +int parse_loose_header(const char *hdr, struct object_info *oi); int check_object_signature(struct repository *r, const struct object_id *oid, void *buf, unsigned long size, const char *type); diff --git a/object-file.c b/object-file.c index 8abeb9ace8..e24fc4555d 100644 --- a/object-file.c +++ b/object-file.c @@ -1324,8 +1324,7 @@ static void *unpack_loose_rest(git_zstream *stream, * too permissive for what we want to check. So do an anal * object header parse by hand. */ -int parse_loose_header(const char *hdr, struct object_info *oi, - unsigned int flags) +int parse_loose_header(const char *hdr, struct object_info *oi) { const char *type_buf = hdr; unsigned long size; @@ -1347,15 +1346,6 @@ int parse_loose_header(const char *hdr, struct object_info *oi, type = type_from_string_gently(type_buf, type_len, 1); if (oi->type_name) strbuf_add(oi->type_name, type_buf, type_len); - /* - * Set type to 0 if its an unknown object and - * we're obtaining the type using '--allow-unknown-type' - * option. - */ - if ((flags & OBJECT_INFO_ALLOW_UNKNOWN_TYPE) && (type < 0)) - type = 0; - else if (type < 0) - die(_("invalid object type")); if (oi->typep) *oi->typep = type; @@ -1382,7 +1372,14 @@ int parse_loose_header(const char *hdr, struct object_info *oi, /* * The length must be followed by a zero byte */ - return *hdr ? -1 : type; + if (*hdr) + return -1; + + /* + * The format is valid, but the type may still be bogus. The + * Caller needs to check its oi->typep. + */ + return 0; } static int loose_object_info(struct repository *r, @@ -1396,6 +1393,7 @@ static int loose_object_info(struct repository *r, char hdr[MAX_HEADER_LEN]; struct strbuf hdrbuf = STRBUF_INIT; unsigned long size_scratch; + enum object_type type_scratch; int allow_unknown = flags & OBJECT_INFO_ALLOW_UNKNOWN_TYPE; if (oi->delta_base_oid) @@ -1427,6 +1425,8 @@ static int loose_object_info(struct repository *r, if (!oi->sizep) oi->sizep = &size_scratch; + if (!oi->typep) + oi->typep = &type_scratch; if (oi->disk_sizep) *oi->disk_sizep = mapsize; @@ -1434,6 +1434,18 @@ static int loose_object_info(struct repository *r, switch (unpack_loose_header(&stream, map, mapsize, hdr, sizeof(hdr), allow_unknown ? &hdrbuf : NULL)) { case ULHR_OK: + if (parse_loose_header(hdrbuf.len ? hdrbuf.buf : hdr, oi) < 0) + status = error(_("unable to parse %s header"), oid_to_hex(oid)); + else if (!allow_unknown && *oi->typep < 0) + die(_("invalid object type")); + + if (!oi->contentp) + break; + *oi->contentp = unpack_loose_rest(&stream, hdr, *oi->sizep, oid); + if (*oi->contentp) + goto cleanup; + + status = -1; break; case ULHR_BAD: status = error(_("unable to unpack %s header"), @@ -1445,31 +1457,16 @@ static int loose_object_info(struct repository *r, break; } - if (status < 0) { - /* Do nothing */ - } else if (hdrbuf.len) { - if ((status = parse_loose_header(hdrbuf.buf, oi, flags)) < 0) - status = error(_("unable to parse %s header with --allow-unknown-type"), - oid_to_hex(oid)); - } else if ((status = parse_loose_header(hdr, oi, flags)) < 0) - status = error(_("unable to parse %s header"), oid_to_hex(oid)); - - if (status >= 0 && oi->contentp) { - *oi->contentp = unpack_loose_rest(&stream, hdr, - *oi->sizep, oid); - if (!*oi->contentp) { - git_inflate_end(&stream); - status = -1; - } - } else - git_inflate_end(&stream); - + git_inflate_end(&stream); +cleanup: munmap(map, mapsize); if (oi->sizep == &size_scratch) oi->sizep = NULL; strbuf_release(&hdrbuf); + if (oi->typep == &type_scratch) + oi->typep = NULL; oi->whence = OI_LOOSE; - return (status < 0) ? status : 0; + return status; } int obj_read_use_lock = 0; @@ -2533,6 +2530,7 @@ int read_loose_object(const char *path, git_zstream stream; char hdr[MAX_HEADER_LEN]; struct object_info oi = OBJECT_INFO_INIT; + oi.typep = type; oi.sizep = size; *contents = NULL; @@ -2549,12 +2547,13 @@ int read_loose_object(const char *path, goto out; } - *type = parse_loose_header(hdr, &oi, 0); - if (*type < 0) { + if (parse_loose_header(hdr, &oi) < 0) { error(_("unable to parse header of %s"), path); git_inflate_end(&stream); goto out; } + if (*type < 0) + die(_("invalid object type")); if (*type == OBJ_BLOB && *size > big_file_threshold) { if (check_stream_oid(&stream, hdr, *size, path, expected_oid) < 0) diff --git a/streaming.c b/streaming.c index bd89c50e7b..fe54665d86 100644 --- a/streaming.c +++ b/streaming.c @@ -225,6 +225,7 @@ static int open_istream_loose(struct git_istream *st, struct repository *r, { struct object_info oi = OBJECT_INFO_INIT; oi.sizep = &st->size; + oi.typep = type; st->u.loose.mapped = map_loose_object(r, oid, &st->u.loose.mapsize); if (!st->u.loose.mapped) @@ -238,7 +239,7 @@ static int open_istream_loose(struct git_istream *st, struct repository *r, case ULHR_TOO_LONG: goto error; } - if (parse_loose_header(st->u.loose.hdr, &oi, 0) < 0) + if (parse_loose_header(st->u.loose.hdr, &oi) < 0 || *type < 0) goto error; st->u.loose.hdr_used = strlen(st->u.loose.hdr) + 1; |