From 74ad250a1ccc2bc2c9f50725f45544d0764c8664 Mon Sep 17 00:00:00 2001 From: Ævar Arnfjörð Bjarmason Date: Fri, 1 Oct 2021 11:16:45 +0200 Subject: object-file.c: don't set "typep" when returning non-zero MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the loose_object_info() function returns an error stop faking up the "oi->typep" to OBJ_BAD. Let the return value of the function itself suffice. This code cleanup simplifies subsequent changes. That we set this at all is a relic from the past. Before 052fe5eaca9 (sha1_loose_object_info: make type lookup optional, 2013-07-12) we would always return the type_from_string(type) via the parse_sha1_header() function, or -1 (i.e. OBJ_BAD) if we couldn't parse it. Then in a combination of 46f034483eb (sha1_file: support reading from a loose object of unknown type, 2015-05-03) and b3ea7dd32d6 (sha1_loose_object_info: handle errors from unpack_sha1_rest, 2017-10-05) our API drifted even further towards conflating the two again. Having read the code paths involved carefully I think this is OK. We are just about to return -1, and we have only one caller: do_oid_object_info_extended(). That function will in turn go on to return -1 when we return -1 here. This might be introducing a subtle bug where a caller of oid_object_info_extended() would inspect its "typep" and expect a meaningful value if the function returned -1. Such a problem would not occur for its simpler oid_object_info() sister function. That one always returns the "enum object_type", which in the case of -1 would be the OBJ_BAD. Having read the code for all the callers of these functions I don't believe any such bug is being introduced here, and in any case we'd likely already have such a bug for the "sizep" member (although blindly checking "typep" first would be a more common case). Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- object-file.c | 2 -- 1 file changed, 2 deletions(-) (limited to 'object-file.c') diff --git a/object-file.c b/object-file.c index f233b440b2..9210e2e6fe 100644 --- a/object-file.c +++ b/object-file.c @@ -1480,8 +1480,6 @@ static int loose_object_info(struct repository *r, git_inflate_end(&stream); munmap(map, mapsize); - if (status && oi->typep) - *oi->typep = status; if (oi->sizep == &size_scratch) oi->sizep = NULL; strbuf_release(&hdrbuf); -- cgit v1.3 From bfff2c48330e95244a0ebdd7e0ba82fb77327347 Mon Sep 17 00:00:00 2001 From: Ævar Arnfjörð Bjarmason Date: Fri, 1 Oct 2021 11:16:46 +0200 Subject: object-file.c: return -1, not "status" from unpack_loose_header() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Return a -1 when git_inflate() fails instead of whatever Z_* status we'd get from zlib.c. This makes no difference to any error we report, but makes it more obvious that we don't care about the specific zlib error codes here. See d21f8426907 (unpack_sha1_header(): detect malformed object header, 2016-09-25) for the commit that added the "return status" code. As far as I can tell there was never a real reason (e.g. different reporting) for carrying down the "status" as opposed to "-1". At the time that d21f8426907 was written there was a corresponding "ret < Z_OK" check right after the unpack_sha1_header() call (the "unpack_sha1_header()" function was later rename to our current "unpack_loose_header()"). However, that check was removed in c84a1f3ed4d (sha1_file: refactor read_object, 2017-06-21) without changing the corresponding return code. So let's do the minor cleanup of also changing this function to return a -1. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- object-file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'object-file.c') diff --git a/object-file.c b/object-file.c index 9210e2e6fe..3a7fe4fe96 100644 --- a/object-file.c +++ b/object-file.c @@ -1239,7 +1239,7 @@ int unpack_loose_header(git_zstream *stream, buffer, bufsiz); if (status < Z_OK) - return status; + return -1; /* Make sure we have the terminating NUL */ if (!memchr(buffer, '\0', stream->next_out - (unsigned char *)buffer)) -- cgit v1.3 From ddb3474b66ef36da40a4cf8346ec4655518243cb Mon Sep 17 00:00:00 2001 From: Ævar Arnfjörð Bjarmason Date: Fri, 1 Oct 2021 11:16:47 +0200 Subject: object-file.c: make parse_loose_header_extended() public MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Make the parse_loose_header_extended() function public and remove the parse_loose_header() wrapper. The only direct user of it outside of object-file.c itself was in streaming.c, that caller can simply pass the required "struct object-info *" instead. This change is being done in preparation for teaching read_loose_object() to accept a flag to pass to parse_loose_header(). It isn't strictly necessary for that change, we could simply use parse_loose_header_extended() there, but will leave the API in a better end state. It would be a better end-state to have already moved the declaration of these functions to object-store.h to avoid the forward declaration of "struct object_info" in cache.h, but let's leave that cleanup for some other time. 1. https://lore.kernel.org/git/patch-v6-09.22-5b9278e7bb4-20210907T104559Z-avarab@gmail.com/ Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- cache.h | 4 +++- object-file.c | 20 +++++++------------- streaming.c | 5 ++++- 3 files changed, 14 insertions(+), 15 deletions(-) (limited to 'object-file.c') diff --git a/cache.h b/cache.h index ba04ff8bd3..985caced24 100644 --- a/cache.h +++ b/cache.h @@ -1303,7 +1303,9 @@ char *xdg_cache_home(const char *filename); int git_open_cloexec(const char *name, int flags); #define git_open(name) git_open_cloexec(name, O_RDONLY) int unpack_loose_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, void *buffer, unsigned long bufsiz); -int parse_loose_header(const char *hdr, unsigned long *sizep); +struct object_info; +int parse_loose_header(const char *hdr, struct object_info *oi, + unsigned int flags); 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 3a7fe4fe96..6d97a6f69b 100644 --- a/object-file.c +++ b/object-file.c @@ -1340,8 +1340,8 @@ 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. */ -static int parse_loose_header_extended(const char *hdr, struct object_info *oi, - unsigned int flags) +int parse_loose_header(const char *hdr, struct object_info *oi, + unsigned int flags) { const char *type_buf = hdr; unsigned long size; @@ -1401,14 +1401,6 @@ static int parse_loose_header_extended(const char *hdr, struct object_info *oi, return *hdr ? -1 : type; } -int parse_loose_header(const char *hdr, unsigned long *sizep) -{ - struct object_info oi = OBJECT_INFO_INIT; - - oi.sizep = sizep; - return parse_loose_header_extended(hdr, &oi, 0); -} - static int loose_object_info(struct repository *r, const struct object_id *oid, struct object_info *oi, int flags) @@ -1463,10 +1455,10 @@ static int loose_object_info(struct repository *r, if (status < 0) ; /* Do nothing */ else if (hdrbuf.len) { - if ((status = parse_loose_header_extended(hdrbuf.buf, oi, flags)) < 0) + 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_extended(hdr, oi, flags)) < 0) + } 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) { @@ -2547,6 +2539,8 @@ int read_loose_object(const char *path, unsigned long mapsize; git_zstream stream; char hdr[MAX_HEADER_LEN]; + struct object_info oi = OBJECT_INFO_INIT; + oi.sizep = size; *contents = NULL; @@ -2561,7 +2555,7 @@ int read_loose_object(const char *path, goto out; } - *type = parse_loose_header(hdr, size); + *type = parse_loose_header(hdr, &oi, 0); if (*type < 0) { error(_("unable to parse header of %s"), path); git_inflate_end(&stream); diff --git a/streaming.c b/streaming.c index 5f480ad50c..8beac62cbb 100644 --- a/streaming.c +++ b/streaming.c @@ -223,6 +223,9 @@ static int open_istream_loose(struct git_istream *st, struct repository *r, const struct object_id *oid, enum object_type *type) { + struct object_info oi = OBJECT_INFO_INIT; + oi.sizep = &st->size; + st->u.loose.mapped = map_loose_object(r, oid, &st->u.loose.mapsize); if (!st->u.loose.mapped) return -1; @@ -231,7 +234,7 @@ static int open_istream_loose(struct git_istream *st, struct repository *r, st->u.loose.mapsize, st->u.loose.hdr, sizeof(st->u.loose.hdr)) < 0) || - (parse_loose_header(st->u.loose.hdr, &st->size) < 0)) { + (parse_loose_header(st->u.loose.hdr, &oi, 0) < 0)) { git_inflate_end(&st->z); munmap(st->u.loose.mapped, st->u.loose.mapsize); return -1; -- cgit v1.3 From 01cab9767929c6c3faf4f4ad3b348639655f04fd Mon Sep 17 00:00:00 2001 From: Ævar Arnfjörð Bjarmason Date: Fri, 1 Oct 2021 11:16:48 +0200 Subject: object-file.c: simplify unpack_loose_short_header() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Combine the unpack_loose_short_header(), unpack_loose_header_to_strbuf() and unpack_loose_header() functions into one. The unpack_loose_header_to_strbuf() function was added in 46f034483eb (sha1_file: support reading from a loose object of unknown type, 2015-05-03). Its code was mostly copy/pasted between it and both of unpack_loose_header() and unpack_loose_short_header(). We now have a single unpack_loose_header() function which accepts an optional "struct strbuf *" instead. I think the remaining unpack_loose_header() function could be further simplified, we're carrying some complexity just to be able to emit a garbage type longer than MAX_HEADER_LEN, we could alternatively just say "we found a garbage type ..." instead. But let's leave the current behavior in place for now. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- cache.h | 17 ++++++++++++++++- object-file.c | 58 ++++++++++++++++++++-------------------------------------- streaming.c | 3 ++- 3 files changed, 38 insertions(+), 40 deletions(-) (limited to 'object-file.c') diff --git a/cache.h b/cache.h index 985caced24..11d2482e30 100644 --- a/cache.h +++ b/cache.h @@ -1302,7 +1302,22 @@ char *xdg_cache_home(const char *filename); int git_open_cloexec(const char *name, int flags); #define git_open(name) git_open_cloexec(name, O_RDONLY) -int unpack_loose_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, void *buffer, unsigned long bufsiz); + +/** + * unpack_loose_header() initializes the data stream needed to unpack + * a loose object header. + * + * Returns 0 on success. Returns negative values on error. + * + * It will only parse up to MAX_HEADER_LEN bytes unless an optional + * "hdrbuf" argument is non-NULL. This is intended for use with + * OBJECT_INFO_ALLOW_UNKNOWN_TYPE to extract the bad type for (error) + * reporting. The full header will be extracted to "hdrbuf" for use + * with parse_loose_header(). + */ +int unpack_loose_header(git_zstream *stream, unsigned char *map, + unsigned long mapsize, void *buffer, + unsigned long bufsiz, struct strbuf *hdrbuf); struct object_info; int parse_loose_header(const char *hdr, struct object_info *oi, unsigned int flags); diff --git a/object-file.c b/object-file.c index 6d97a6f69b..59b38aac2b 100644 --- a/object-file.c +++ b/object-file.c @@ -1210,11 +1210,12 @@ void *map_loose_object(struct repository *r, return map_loose_object_1(r, NULL, oid, size); } -static int unpack_loose_short_header(git_zstream *stream, - unsigned char *map, unsigned long mapsize, - void *buffer, unsigned long bufsiz) +int unpack_loose_header(git_zstream *stream, + unsigned char *map, unsigned long mapsize, + void *buffer, unsigned long bufsiz, + struct strbuf *header) { - int ret; + int status; /* Get the data stream */ memset(stream, 0, sizeof(*stream)); @@ -1225,35 +1226,8 @@ static int unpack_loose_short_header(git_zstream *stream, git_inflate_init(stream); obj_read_unlock(); - ret = git_inflate(stream, 0); + status = git_inflate(stream, 0); obj_read_lock(); - - return ret; -} - -int unpack_loose_header(git_zstream *stream, - unsigned char *map, unsigned long mapsize, - void *buffer, unsigned long bufsiz) -{ - int status = unpack_loose_short_header(stream, map, mapsize, - buffer, bufsiz); - - if (status < Z_OK) - return -1; - - /* Make sure we have the terminating NUL */ - if (!memchr(buffer, '\0', stream->next_out - (unsigned char *)buffer)) - return -1; - return 0; -} - -static int unpack_loose_header_to_strbuf(git_zstream *stream, unsigned char *map, - unsigned long mapsize, void *buffer, - unsigned long bufsiz, struct strbuf *header) -{ - int status; - - status = unpack_loose_short_header(stream, map, mapsize, buffer, bufsiz); if (status < Z_OK) return -1; @@ -1263,6 +1237,14 @@ static int unpack_loose_header_to_strbuf(git_zstream *stream, unsigned char *map if (memchr(buffer, '\0', stream->next_out - (unsigned char *)buffer)) return 0; + /* + * We have a header longer than MAX_HEADER_LEN. The "header" + * here is only non-NULL when we run "cat-file + * --allow-unknown-type". + */ + if (!header) + return -1; + /* * buffer[0..bufsiz] was not large enough. Copy the partial * result out to header, and then append the result of further @@ -1412,6 +1394,7 @@ static int loose_object_info(struct repository *r, char hdr[MAX_HEADER_LEN]; struct strbuf hdrbuf = STRBUF_INIT; unsigned long size_scratch; + int allow_unknown = flags & OBJECT_INFO_ALLOW_UNKNOWN_TYPE; if (oi->delta_base_oid) oidclr(oi->delta_base_oid); @@ -1445,11 +1428,9 @@ static int loose_object_info(struct repository *r, if (oi->disk_sizep) *oi->disk_sizep = mapsize; - if ((flags & OBJECT_INFO_ALLOW_UNKNOWN_TYPE)) { - if (unpack_loose_header_to_strbuf(&stream, map, mapsize, hdr, sizeof(hdr), &hdrbuf) < 0) - status = error(_("unable to unpack %s header with --allow-unknown-type"), - oid_to_hex(oid)); - } else if (unpack_loose_header(&stream, map, mapsize, hdr, sizeof(hdr)) < 0) + + if (unpack_loose_header(&stream, map, mapsize, hdr, sizeof(hdr), + allow_unknown ? &hdrbuf : NULL) < 0) status = error(_("unable to unpack %s header"), oid_to_hex(oid)); if (status < 0) @@ -2550,7 +2531,8 @@ int read_loose_object(const char *path, goto out; } - if (unpack_loose_header(&stream, map, mapsize, hdr, sizeof(hdr)) < 0) { + if (unpack_loose_header(&stream, map, mapsize, hdr, sizeof(hdr), + NULL) < 0) { error(_("unable to unpack header of %s"), path); goto out; } diff --git a/streaming.c b/streaming.c index 8beac62cbb..cb3c3cf6ff 100644 --- a/streaming.c +++ b/streaming.c @@ -233,7 +233,8 @@ static int open_istream_loose(struct git_istream *st, struct repository *r, st->u.loose.mapped, st->u.loose.mapsize, st->u.loose.hdr, - sizeof(st->u.loose.hdr)) < 0) || + sizeof(st->u.loose.hdr), + NULL) < 0) || (parse_loose_header(st->u.loose.hdr, &oi, 0) < 0)) { git_inflate_end(&st->z); munmap(st->u.loose.mapped, st->u.loose.mapsize); -- cgit v1.3 From 3b6a8db3b03adb118bfafb90bbc710068dbd6d14 Mon Sep 17 00:00:00 2001 From: Ævar Arnfjörð Bjarmason Date: Fri, 1 Oct 2021 11:16:49 +0200 Subject: object-file.c: use "enum" return type for unpack_loose_header() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In a preceding commit we changed and documented unpack_loose_header() from its previous behavior of returning any negative value or zero, to only -1 or 0. Let's add an "enum unpack_loose_header_result" type and use it for these return values, and have the compiler assert that we're exhaustively covering all of them. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- cache.h | 19 +++++++++++++++---- object-file.c | 34 +++++++++++++++++++++------------- streaming.c | 23 +++++++++++++---------- 3 files changed, 49 insertions(+), 27 deletions(-) (limited to 'object-file.c') diff --git a/cache.h b/cache.h index 11d2482e30..f738275663 100644 --- a/cache.h +++ b/cache.h @@ -1307,7 +1307,10 @@ int git_open_cloexec(const char *name, int flags); * unpack_loose_header() initializes the data stream needed to unpack * a loose object header. * - * Returns 0 on success. Returns negative values on error. + * Returns: + * + * - ULHR_OK on success + * - ULHR_BAD on error * * It will only parse up to MAX_HEADER_LEN bytes unless an optional * "hdrbuf" argument is non-NULL. This is intended for use with @@ -1315,9 +1318,17 @@ int git_open_cloexec(const char *name, int flags); * reporting. The full header will be extracted to "hdrbuf" for use * with parse_loose_header(). */ -int unpack_loose_header(git_zstream *stream, unsigned char *map, - unsigned long mapsize, void *buffer, - unsigned long bufsiz, struct strbuf *hdrbuf); +enum unpack_loose_header_result { + ULHR_OK, + ULHR_BAD, +}; +enum unpack_loose_header_result unpack_loose_header(git_zstream *stream, + unsigned char *map, + unsigned long mapsize, + void *buffer, + unsigned long bufsiz, + struct strbuf *hdrbuf); + struct object_info; int parse_loose_header(const char *hdr, struct object_info *oi, unsigned int flags); diff --git a/object-file.c b/object-file.c index 59b38aac2b..ade5f33f3c 100644 --- a/object-file.c +++ b/object-file.c @@ -1210,10 +1210,12 @@ void *map_loose_object(struct repository *r, return map_loose_object_1(r, NULL, oid, size); } -int unpack_loose_header(git_zstream *stream, - unsigned char *map, unsigned long mapsize, - void *buffer, unsigned long bufsiz, - struct strbuf *header) +enum unpack_loose_header_result unpack_loose_header(git_zstream *stream, + unsigned char *map, + unsigned long mapsize, + void *buffer, + unsigned long bufsiz, + struct strbuf *header) { int status; @@ -1229,13 +1231,13 @@ int unpack_loose_header(git_zstream *stream, status = git_inflate(stream, 0); obj_read_lock(); if (status < Z_OK) - return -1; + return ULHR_BAD; /* * Check if entire header is unpacked in the first iteration. */ if (memchr(buffer, '\0', stream->next_out - (unsigned char *)buffer)) - return 0; + return ULHR_OK; /* * We have a header longer than MAX_HEADER_LEN. The "header" @@ -1243,7 +1245,7 @@ int unpack_loose_header(git_zstream *stream, * --allow-unknown-type". */ if (!header) - return -1; + return ULHR_BAD; /* * buffer[0..bufsiz] was not large enough. Copy the partial @@ -1264,7 +1266,7 @@ int unpack_loose_header(git_zstream *stream, stream->next_out = buffer; stream->avail_out = bufsiz; } while (status != Z_STREAM_END); - return -1; + return ULHR_BAD; } static void *unpack_loose_rest(git_zstream *stream, @@ -1429,13 +1431,19 @@ static int loose_object_info(struct repository *r, if (oi->disk_sizep) *oi->disk_sizep = mapsize; - if (unpack_loose_header(&stream, map, mapsize, hdr, sizeof(hdr), - allow_unknown ? &hdrbuf : NULL) < 0) + switch (unpack_loose_header(&stream, map, mapsize, hdr, sizeof(hdr), + allow_unknown ? &hdrbuf : NULL)) { + case ULHR_OK: + break; + case ULHR_BAD: status = error(_("unable to unpack %s header"), oid_to_hex(oid)); - if (status < 0) - ; /* Do nothing */ - else if (hdrbuf.len) { + 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)); diff --git a/streaming.c b/streaming.c index cb3c3cf6ff..6df0247a4c 100644 --- a/streaming.c +++ b/streaming.c @@ -229,17 +229,16 @@ static int open_istream_loose(struct git_istream *st, struct repository *r, st->u.loose.mapped = map_loose_object(r, oid, &st->u.loose.mapsize); if (!st->u.loose.mapped) return -1; - if ((unpack_loose_header(&st->z, - st->u.loose.mapped, - st->u.loose.mapsize, - st->u.loose.hdr, - sizeof(st->u.loose.hdr), - NULL) < 0) || - (parse_loose_header(st->u.loose.hdr, &oi, 0) < 0)) { - git_inflate_end(&st->z); - munmap(st->u.loose.mapped, st->u.loose.mapsize); - return -1; + switch (unpack_loose_header(&st->z, st->u.loose.mapped, + st->u.loose.mapsize, st->u.loose.hdr, + sizeof(st->u.loose.hdr), NULL)) { + case ULHR_OK: + break; + case ULHR_BAD: + goto error; } + if (parse_loose_header(st->u.loose.hdr, &oi, 0) < 0) + goto error; st->u.loose.hdr_used = strlen(st->u.loose.hdr) + 1; st->u.loose.hdr_avail = st->z.total_out; @@ -248,6 +247,10 @@ static int open_istream_loose(struct git_istream *st, struct repository *r, st->read = read_istream_loose; return 0; +error: + git_inflate_end(&st->z); + munmap(st->u.loose.mapped, st->u.loose.mapsize); + return -1; } -- cgit v1.3 From 5848fb11acd0b6aad6ba9e3e71bd91485e0d4c71 Mon Sep 17 00:00:00 2001 From: Ævar Arnfjörð Bjarmason Date: Fri, 1 Oct 2021 11:16:50 +0200 Subject: object-file.c: return ULHR_TOO_LONG on "header too long" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Split up the return code for "header too long" from the generic negative return value unpack_loose_header() returns, and report via error() if we exceed MAX_HEADER_LEN. As a test added earlier in this series in t1006-cat-file.sh shows we'll correctly emit zlib errors from zlib.c already in this case, so we have no need to carry those return codes further down the stack. Let's instead just return ULHR_TOO_LONG saying we ran into the MAX_HEADER_LEN limit, or other negative values for "unable to unpack header". Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- cache.h | 5 ++++- object-file.c | 8 ++++++-- streaming.c | 1 + t/t1006-cat-file.sh | 4 ++-- 4 files changed, 13 insertions(+), 5 deletions(-) (limited to 'object-file.c') diff --git a/cache.h b/cache.h index f738275663..e7d0cc3d3b 100644 --- a/cache.h +++ b/cache.h @@ -1311,16 +1311,19 @@ int git_open_cloexec(const char *name, int flags); * * - ULHR_OK on success * - ULHR_BAD on error + * - ULHR_TOO_LONG if the header was too long * * It will only parse up to MAX_HEADER_LEN bytes unless an optional * "hdrbuf" argument is non-NULL. This is intended for use with * OBJECT_INFO_ALLOW_UNKNOWN_TYPE to extract the bad type for (error) * reporting. The full header will be extracted to "hdrbuf" for use - * with parse_loose_header(). + * with parse_loose_header(), ULHR_TOO_LONG will still be returned + * from this function to indicate that the header was too long. */ enum unpack_loose_header_result { ULHR_OK, ULHR_BAD, + ULHR_TOO_LONG, }; enum unpack_loose_header_result unpack_loose_header(git_zstream *stream, unsigned char *map, diff --git a/object-file.c b/object-file.c index ade5f33f3c..8abeb9ace8 100644 --- a/object-file.c +++ b/object-file.c @@ -1245,7 +1245,7 @@ enum unpack_loose_header_result unpack_loose_header(git_zstream *stream, * --allow-unknown-type". */ if (!header) - return ULHR_BAD; + return ULHR_TOO_LONG; /* * buffer[0..bufsiz] was not large enough. Copy the partial @@ -1266,7 +1266,7 @@ enum unpack_loose_header_result unpack_loose_header(git_zstream *stream, stream->next_out = buffer; stream->avail_out = bufsiz; } while (status != Z_STREAM_END); - return ULHR_BAD; + return ULHR_TOO_LONG; } static void *unpack_loose_rest(git_zstream *stream, @@ -1439,6 +1439,10 @@ static int loose_object_info(struct repository *r, status = error(_("unable to unpack %s header"), oid_to_hex(oid)); break; + case ULHR_TOO_LONG: + status = error(_("header for %s too long, exceeds %d bytes"), + oid_to_hex(oid), MAX_HEADER_LEN); + break; } if (status < 0) { diff --git a/streaming.c b/streaming.c index 6df0247a4c..bd89c50e7b 100644 --- a/streaming.c +++ b/streaming.c @@ -235,6 +235,7 @@ static int open_istream_loose(struct git_istream *st, struct repository *r, case ULHR_OK: break; case ULHR_BAD: + case ULHR_TOO_LONG: goto error; } if (parse_loose_header(st->u.loose.hdr, &oi, 0) < 0) diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh index c89483f40b..4b55adf06a 100755 --- a/t/t1006-cat-file.sh +++ b/t/t1006-cat-file.sh @@ -356,12 +356,12 @@ do if test "$arg2" = "-p" then cat >expect <<-EOF - error: unable to unpack $bogus_long_sha1 header + error: header for $bogus_long_sha1 too long, exceeds 32 bytes fatal: Not a valid object name $bogus_long_sha1 EOF else cat >expect <<-EOF - error: unable to unpack $bogus_long_sha1 header + error: header for $bogus_long_sha1 too long, exceeds 32 bytes fatal: git cat-file: could not get object info EOF fi && -- cgit v1.3 From dccb32bf01411213297cde63cf689e476673a8ec Mon Sep 17 00:00:00 2001 From: Ævar Arnfjörð Bjarmason Date: Fri, 1 Oct 2021 11:16:51 +0200 Subject: object-file.c: stop dying in parse_loose_header() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Junio C Hamano --- cache.h | 11 ++++++++-- object-file.c | 67 +++++++++++++++++++++++++++++------------------------------ streaming.c | 3 ++- 3 files changed, 44 insertions(+), 37 deletions(-) (limited to 'object-file.c') diff --git a/cache.h b/cache.h index e7d0cc3d3b..1181304f3f 100644 --- a/cache.h +++ b/cache.h @@ -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 " \0" of an + * object. If it doesn't follow that format -1 is returned. To check + * the validity of the populate the "typep" in the "struct + * object_info". It will be OBJ_BAD if the object type is unknown. The + * parsed 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; -- cgit v1.3 From 31deb28f5e0c85e8bd556ba135e5f0e0926bad7a Mon Sep 17 00:00:00 2001 From: Ævar Arnfjörð Bjarmason Date: Fri, 1 Oct 2021 11:16:52 +0200 Subject: fsck: don't hard die on invalid object types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Change the error fsck emits on invalid object types, such as: $ git hash-object --stdin -w -t garbage --literally From the very ungraceful error of: $ git fsck fatal: invalid object type $ To: $ git fsck error: : object is of unknown type 'garbage': [ other fsck output ] We'll still exit with non-zero, but now we'll finish the rest of the traversal. The tests that's being added here asserts that we'll still complain about other fsck issues (e.g. an unrelated dangling blob). To do this we need to pass down the "OBJECT_INFO_ALLOW_UNKNOWN_TYPE" flag from read_loose_object() through to parse_loose_header(). Since the read_loose_object() function is only used in builtin/fsck.c we can simply change it to accept a "struct object_info" (which contains the OBJECT_INFO_ALLOW_UNKNOWN_TYPE in its flags). See f6371f92104 (sha1_file: add read_loose_object() function, 2017-01-13) for the introduction of read_loose_object(). Since we'll need a "struct strbuf" to hold the "type_name" let's pass it to the for_each_loose_file_in_objdir() callback to avoid allocating a new one for each loose object in the iteration. It also makes the memory management simpler than sticking it in fsck_loose() itself, as we'll only need to strbuf_reset() it, with no need to do a strbuf_release() before each "return". Before this commit we'd never check the "type" if read_loose_object() failed, but now we do. We therefore need to initialize it to OBJ_NONE to be able to tell the difference between e.g. its unpack_loose_header() having failed, and us getting past that and into parse_loose_header(). Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- builtin/fsck.c | 37 +++++++++++++++++++++++++++++++------ object-file.c | 18 ++++++------------ object-store.h | 6 +++--- t/t1450-fsck.sh | 18 +++++++++--------- 4 files changed, 49 insertions(+), 30 deletions(-) (limited to 'object-file.c') diff --git a/builtin/fsck.c b/builtin/fsck.c index 87a99b0108..f47b9234ed 100644 --- a/builtin/fsck.c +++ b/builtin/fsck.c @@ -592,18 +592,36 @@ static void get_default_heads(void) } } +struct for_each_loose_cb +{ + struct progress *progress; + struct strbuf obj_type; +}; + static int fsck_loose(const struct object_id *oid, const char *path, void *data) { + struct for_each_loose_cb *cb_data = data; struct object *obj; - enum object_type type; + enum object_type type = OBJ_NONE; unsigned long size; void *contents; int eaten; + struct object_info oi = OBJECT_INFO_INIT; + int err = 0; - if (read_loose_object(path, oid, &type, &size, &contents) < 0) { + strbuf_reset(&cb_data->obj_type); + oi.type_name = &cb_data->obj_type; + 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); + if (type != OBJ_NONE && type < 0) + err = error(_("%s: object is of unknown type '%s': %s"), + oid_to_hex(oid), cb_data->obj_type.buf, path); + if (err < 0) { errors_found |= ERROR_OBJECT; - error(_("%s: object corrupt or missing: %s"), - oid_to_hex(oid), path); return 0; /* keep checking other objects */ } @@ -639,8 +657,10 @@ static int fsck_cruft(const char *basename, const char *path, void *data) return 0; } -static int fsck_subdir(unsigned int nr, const char *path, void *progress) +static int fsck_subdir(unsigned int nr, const char *path, void *data) { + struct for_each_loose_cb *cb_data = data; + struct progress *progress = cb_data->progress; display_progress(progress, nr + 1); return 0; } @@ -648,6 +668,10 @@ static int fsck_subdir(unsigned int nr, const char *path, void *progress) static void fsck_object_dir(const char *path) { struct progress *progress = NULL; + struct for_each_loose_cb cb_data = { + .obj_type = STRBUF_INIT, + .progress = progress, + }; if (verbose) fprintf_ln(stderr, _("Checking object directory")); @@ -656,9 +680,10 @@ static void fsck_object_dir(const char *path) progress = start_progress(_("Checking object directories"), 256); for_each_loose_file_in_objdir(path, fsck_loose, fsck_cruft, fsck_subdir, - progress); + &cb_data); display_progress(progress, 256); stop_progress(&progress); + strbuf_release(&cb_data.obj_type); } static int fsck_head_link(const char *head_ref_name, diff --git a/object-file.c b/object-file.c index e24fc4555d..dd80d4b161 100644 --- a/object-file.c +++ b/object-file.c @@ -2520,18 +2520,15 @@ static int check_stream_oid(git_zstream *stream, int read_loose_object(const char *path, const struct object_id *expected_oid, - enum object_type *type, - unsigned long *size, - void **contents) + void **contents, + struct object_info *oi) { int ret = -1; void *map = NULL; unsigned long mapsize; git_zstream stream; char hdr[MAX_HEADER_LEN]; - struct object_info oi = OBJECT_INFO_INIT; - oi.typep = type; - oi.sizep = size; + unsigned long *size = oi->sizep; *contents = NULL; @@ -2547,15 +2544,13 @@ int read_loose_object(const char *path, goto out; } - if (parse_loose_header(hdr, &oi) < 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 (*oi->typep == OBJ_BLOB && *size > big_file_threshold) { if (check_stream_oid(&stream, hdr, *size, path, expected_oid) < 0) goto out; } else { @@ -2566,8 +2561,7 @@ int read_loose_object(const char *path, goto out; } if (check_object_signature(the_repository, expected_oid, - *contents, *size, - type_name(*type))) { + *contents, *size, oi->type_name->buf)) { error(_("hash mismatch for %s (expected %s)"), path, oid_to_hex(expected_oid)); free(*contents); diff --git a/object-store.h b/object-store.h index ec32c23dcb..3eb597a82a 100644 --- a/object-store.h +++ b/object-store.h @@ -236,6 +236,7 @@ int force_object_loose(const struct object_id *oid, time_t mtime); /* * Open the loose object at path, check its hash, and return the contents, + * use the "oi" argument to assert things about the object, or e.g. populate its * type, and size. If the object is a blob, then "contents" may return NULL, * to allow streaming of large blobs. * @@ -243,9 +244,8 @@ int force_object_loose(const struct object_id *oid, time_t mtime); */ int read_loose_object(const char *path, const struct object_id *expected_oid, - enum object_type *type, - unsigned long *size, - void **contents); + void **contents, + struct object_info *oi); /* Retry packed storage after checking packed and loose storage */ #define HAS_OBJECT_RECHECK_PACKED 1 diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh index 281ff8bdd8..faf0e98847 100755 --- a/t/t1450-fsck.sh +++ b/t/t1450-fsck.sh @@ -85,11 +85,10 @@ test_expect_success 'object with hash and type mismatch' ' cmt=$(echo bogus | git commit-tree $tree) && git update-ref refs/heads/bogus $cmt && - cat >expect <<-\EOF && - fatal: invalid object type - EOF - test_must_fail git fsck 2>actual && - test_cmp expect actual + + test_must_fail git fsck 2>out && + grep "^error: hash mismatch for " out && + grep "^error: $oid: object is of unknown type '"'"'garbage'"'"'" out ) ' @@ -910,19 +909,20 @@ test_expect_success 'detect corrupt index file in fsck' ' test_i18ngrep "bad index file" errors ' -test_expect_success 'fsck hard errors on an invalid object type' ' +test_expect_success 'fsck error and recovery on invalid object type' ' git init --bare garbage-type && ( cd garbage-type && - git hash-object --stdin -w -t garbage --literally err.expect <<-\EOF && fatal: invalid object type EOF test_must_fail git fsck >out 2>err && - test_cmp err.expect err && - test_must_be_empty out + grep -e "^error" -e "^fatal" err >errors && + test_line_count = 1 errors && + grep "$garbage_blob: object is of unknown type '"'"'garbage'"'"':" err ) ' -- cgit v1.3 From 96e41f58fe1a5aeadf2bf1c1850c53a1c1144bbc Mon Sep 17 00:00:00 2001 From: Ævar Arnfjörð Bjarmason Date: Fri, 1 Oct 2021 11:16:53 +0200 Subject: fsck: report invalid object type-path combinations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Improve the error that's emitted in cases where we find a loose object we parse, but which isn't at the location we expect it to be. Before this change we'd prefix the error with a not-a-OID derived from the path at which the object was found, due to an emergent behavior in how we'd end up with an "OID" in these codepaths. Now we'll instead say what object we hashed, and what path it was found at. Before this patch series e.g.: $ git hash-object --stdin -w -t blob >objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391 $ git fsck error: garbage at end of loose object 'e69d[...]' error: unable to unpack contents of ./objects/e6/9d[...] error: e69d[...]: object corrupt or missing: ./objects/e6/9d[...] There is currently some weird messaging in the edge case when the two are combined, i.e. because we're not explicitly passing along an error state about this specific scenario from check_stream_oid() via read_loose_object() we'll end up printing the null OID if an object is of an unknown type *and* it can't be unpacked by zlib, e.g.: $ git hash-object --stdin -w -t garbage --literally >objects/83/15a83d2acc4c174aed59430f9a9c4ed926440f $ /usr/bin/git fsck fatal: invalid object type $ ~/g/git/git fsck error: garbage at end of loose object '8315a83d2acc4c174aed59430f9a9c4ed926440f' error: unable to unpack contents of ./objects/83/15a83d2acc4c174aed59430f9a9c4ed926440f error: 8315a83d2acc4c174aed59430f9a9c4ed926440f: object corrupt or missing: ./objects/83/15a83d2acc4c174aed59430f9a9c4ed926440f error: 0000000000000000000000000000000000000000: object is of unknown type 'garbage': ./objects/83/15a83d2acc4c174aed59430f9a9c4ed926440f [...] I think it's OK to leave that for future improvements, which would involve enum-ifying more error state as we've done with "enum unpack_loose_header_result" in preceding commits. In these increasingly more obscure cases the worst that can happen is that we'll get slightly nonsensical or inapplicable error messages. There's other such potential edge cases, all of which might produce some confusing messaging, but still be handled correctly as far as passing along errors goes. E.g. if check_object_signature() returns and oideq(real_oid, null_oid()) is true, which could happen if it returns -1 due to the read_istream() call having failed. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- builtin/fast-export.c | 2 +- builtin/fsck.c | 15 +++++++++++---- builtin/index-pack.c | 2 +- builtin/mktag.c | 3 ++- cache.h | 3 ++- object-file.c | 21 ++++++++++----------- object-store.h | 1 + object.c | 4 ++-- pack-check.c | 3 ++- t/t1006-cat-file.sh | 2 +- t/t1450-fsck.sh | 8 +++++--- 11 files changed, 38 insertions(+), 26 deletions(-) (limited to 'object-file.c') diff --git a/builtin/fast-export.c b/builtin/fast-export.c index 3c20f164f0..48a3b6a7f8 100644 --- a/builtin/fast-export.c +++ b/builtin/fast-export.c @@ -312,7 +312,7 @@ static void export_blob(const struct object_id *oid) if (!buf) die("could not read blob %s", oid_to_hex(oid)); if (check_object_signature(the_repository, oid, buf, size, - type_name(type)) < 0) + type_name(type), NULL) < 0) die("oid mismatch in blob %s", oid_to_hex(oid)); object = parse_object_buffer(the_repository, oid, type, size, buf, &eaten); diff --git a/builtin/fsck.c b/builtin/fsck.c index f47b9234ed..1a023914a7 100644 --- a/builtin/fsck.c +++ b/builtin/fsck.c @@ -607,6 +607,7 @@ static int fsck_loose(const struct object_id *oid, const char *path, void *data) void *contents; int eaten; struct object_info oi = OBJECT_INFO_INIT; + struct object_id real_oid = *null_oid(); int err = 0; strbuf_reset(&cb_data->obj_type); @@ -614,12 +615,18 @@ static int fsck_loose(const struct object_id *oid, const char *path, void *data) 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); + if (read_loose_object(path, oid, &real_oid, &contents, &oi) < 0) { + if (contents && !oideq(&real_oid, oid)) + err = error(_("%s: hash-path mismatch, found at: %s"), + oid_to_hex(&real_oid), path); + else + err = error(_("%s: object corrupt or missing: %s"), + oid_to_hex(oid), path); + } if (type != OBJ_NONE && type < 0) err = error(_("%s: object is of unknown type '%s': %s"), - oid_to_hex(oid), cb_data->obj_type.buf, path); + oid_to_hex(&real_oid), cb_data->obj_type.buf, + path); if (err < 0) { errors_found |= ERROR_OBJECT; return 0; /* keep checking other objects */ diff --git a/builtin/index-pack.c b/builtin/index-pack.c index 3fbc5d7077..bf860b6555 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -1421,7 +1421,7 @@ static void fix_unresolved_deltas(struct hashfile *f) if (check_object_signature(the_repository, &d->oid, data, size, - type_name(type))) + type_name(type), NULL)) die(_("local object %s is corrupt"), oid_to_hex(&d->oid)); /* diff --git a/builtin/mktag.c b/builtin/mktag.c index dddcccdd36..3b2dbbb37e 100644 --- a/builtin/mktag.c +++ b/builtin/mktag.c @@ -62,7 +62,8 @@ static int verify_object_in_tag(struct object_id *tagged_oid, int *tagged_type) repl = lookup_replace_object(the_repository, tagged_oid); ret = check_object_signature(the_repository, repl, - buffer, size, type_name(*tagged_type)); + buffer, size, type_name(*tagged_type), + NULL); free(buffer); return ret; diff --git a/cache.h b/cache.h index 1181304f3f..4c0901f6e1 100644 --- a/cache.h +++ b/cache.h @@ -1344,7 +1344,8 @@ struct object_info; 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); + void *buf, unsigned long size, const char *type, + struct object_id *real_oidp); int finalize_object_file(const char *tmpfile, const char *filename); diff --git a/object-file.c b/object-file.c index dd80d4b161..4c258703a0 100644 --- a/object-file.c +++ b/object-file.c @@ -1039,9 +1039,11 @@ void *xmmap(void *start, size_t length, * the streaming interface and rehash it to do the same. */ int check_object_signature(struct repository *r, const struct object_id *oid, - void *map, unsigned long size, const char *type) + void *map, unsigned long size, const char *type, + struct object_id *real_oidp) { - struct object_id real_oid; + struct object_id tmp; + struct object_id *real_oid = real_oidp ? real_oidp : &tmp; enum object_type obj_type; struct git_istream *st; git_hash_ctx c; @@ -1049,8 +1051,8 @@ int check_object_signature(struct repository *r, const struct object_id *oid, int hdrlen; if (map) { - hash_object_file(r->hash_algo, map, size, type, &real_oid); - return !oideq(oid, &real_oid) ? -1 : 0; + hash_object_file(r->hash_algo, map, size, type, real_oid); + return !oideq(oid, real_oid) ? -1 : 0; } st = open_istream(r, oid, &obj_type, &size, NULL); @@ -1075,9 +1077,9 @@ int check_object_signature(struct repository *r, const struct object_id *oid, break; r->hash_algo->update_fn(&c, buf, readlen); } - r->hash_algo->final_oid_fn(&real_oid, &c); + r->hash_algo->final_oid_fn(real_oid, &c); close_istream(st); - return !oideq(oid, &real_oid) ? -1 : 0; + return !oideq(oid, real_oid) ? -1 : 0; } int git_open_cloexec(const char *name, int flags) @@ -2520,6 +2522,7 @@ static int check_stream_oid(git_zstream *stream, int read_loose_object(const char *path, const struct object_id *expected_oid, + struct object_id *real_oid, void **contents, struct object_info *oi) { @@ -2530,8 +2533,6 @@ int read_loose_object(const char *path, char hdr[MAX_HEADER_LEN]; unsigned long *size = oi->sizep; - *contents = NULL; - map = map_loose_object_1(the_repository, path, NULL, &mapsize); if (!map) { error_errno(_("unable to mmap %s"), path); @@ -2561,9 +2562,7 @@ int read_loose_object(const char *path, goto out; } if (check_object_signature(the_repository, expected_oid, - *contents, *size, oi->type_name->buf)) { - error(_("hash mismatch for %s (expected %s)"), path, - oid_to_hex(expected_oid)); + *contents, *size, oi->type_name->buf, real_oid)) { free(*contents); goto out; } diff --git a/object-store.h b/object-store.h index 3eb597a82a..6b9ffcffb2 100644 --- a/object-store.h +++ b/object-store.h @@ -244,6 +244,7 @@ int force_object_loose(const struct object_id *oid, time_t mtime); */ int read_loose_object(const char *path, const struct object_id *expected_oid, + struct object_id *real_oid, void **contents, struct object_info *oi); diff --git a/object.c b/object.c index 14188453c5..5467ead328 100644 --- a/object.c +++ b/object.c @@ -261,7 +261,7 @@ struct object *parse_object(struct repository *r, const struct object_id *oid) if ((obj && obj->type == OBJ_BLOB && repo_has_object_file(r, oid)) || (!obj && repo_has_object_file(r, oid) && oid_object_info(r, oid, NULL) == OBJ_BLOB)) { - if (check_object_signature(r, repl, NULL, 0, NULL) < 0) { + if (check_object_signature(r, repl, NULL, 0, NULL, NULL) < 0) { error(_("hash mismatch %s"), oid_to_hex(oid)); return NULL; } @@ -272,7 +272,7 @@ struct object *parse_object(struct repository *r, const struct object_id *oid) buffer = repo_read_object_file(r, oid, &type, &size); if (buffer) { if (check_object_signature(r, repl, buffer, size, - type_name(type)) < 0) { + type_name(type), NULL) < 0) { free(buffer); error(_("hash mismatch %s"), oid_to_hex(repl)); return NULL; diff --git a/pack-check.c b/pack-check.c index 4b089fe8ec..e6aa4442c9 100644 --- a/pack-check.c +++ b/pack-check.c @@ -142,7 +142,8 @@ static int verify_packfile(struct repository *r, err = error("cannot unpack %s from %s at offset %"PRIuMAX"", oid_to_hex(&oid), p->pack_name, (uintmax_t)entries[i].offset); - else if (check_object_signature(r, &oid, data, size, type_name(type))) + else if (check_object_signature(r, &oid, data, size, + type_name(type), NULL)) err = error("packed %s from %s is corrupt", oid_to_hex(&oid), p->pack_name); else if (fn) { diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh index 4b55adf06a..fe302f2818 100755 --- a/t/t1006-cat-file.sh +++ b/t/t1006-cat-file.sh @@ -512,7 +512,7 @@ test_expect_success 'cat-file -t and -s on corrupt loose object' ' # Swap the two to corrupt the repository mv -f "$other_path" "$empty_path" && test_must_fail git fsck 2>err.fsck && - grep "hash mismatch" err.fsck && + grep "hash-path mismatch" err.fsck && # confirm that cat-file is reading the new swapped-in # blob... diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh index faf0e98847..6337236fd8 100755 --- a/t/t1450-fsck.sh +++ b/t/t1450-fsck.sh @@ -54,6 +54,7 @@ test_expect_success 'object with hash mismatch' ' cd hash-mismatch && oid=$(echo blob | git hash-object -w --stdin) && + oldoid=$oid && old=$(test_oid_to_path "$oid") && new=$(dirname $old)/$(test_oid ff_2) && oid="$(dirname $new)$(basename $new)" && @@ -65,7 +66,7 @@ test_expect_success 'object with hash mismatch' ' git update-ref refs/heads/bogus $cmt && test_must_fail git fsck 2>out && - grep "$oid.*corrupt" out + grep "$oldoid: hash-path mismatch, found at: .*$new" out ) ' @@ -75,6 +76,7 @@ test_expect_success 'object with hash and type mismatch' ' cd hash-type-mismatch && oid=$(echo blob | git hash-object -w --stdin -t garbage --literally) && + oldoid=$oid && old=$(test_oid_to_path "$oid") && new=$(dirname $old)/$(test_oid ff_2) && oid="$(dirname $new)$(basename $new)" && @@ -87,8 +89,8 @@ test_expect_success 'object with hash and type mismatch' ' test_must_fail git fsck 2>out && - grep "^error: hash mismatch for " out && - grep "^error: $oid: object is of unknown type '"'"'garbage'"'"'" out + grep "^error: $oldoid: hash-path mismatch, found at: .*$new" out && + grep "^error: $oldoid: object is of unknown type '"'"'garbage'"'"'" out ) ' -- cgit v1.3