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>

Ævar Arnfjörð Bjarmason committed Oct 1, 2021 at 11:16 UTC dccb32bf01411213297cde63cf689e476673a8ec
3 files changed +44 -37
cache.h
+9 -2
@@ -1332,9 +1332,16 @@ enum unpack_loose_header_result unpack_loose_header(git_zstream *stream,
1332 unsigned long bufsiz,
1333 struct strbuf *hdrbuf);
1334
1335 +/**
1336 + * parse_loose_header() parses the starting "<type> <len>\0" of an
1337 + * object. If it doesn't follow that format -1 is returned. To check
1338 + * the validity of the <type> populate the "typep" in the "struct
1339 + * object_info". It will be OBJ_BAD if the object type is unknown. The
1340 + * parsed <len> can be retrieved via "oi->sizep", and from there
1341 + * passed to unpack_loose_rest().
1342 + */
1343 struct object_info;
1336 -int parse_loose_header(const char *hdr, struct object_info *oi,
1337 - unsigned int flags);
1344 +int parse_loose_header(const char *hdr, struct object_info *oi);
1345
1346 int check_object_signature(struct repository *r, const struct object_id *oid,
1347 void *buf, unsigned long size, const char *type);
object-file.c
+33 -34
@@ -1324,8 +1324,7 @@ static void *unpack_loose_rest(git_zstream *stream,
1324 * too permissive for what we want to check. So do an anal
1325 * object header parse by hand.
1326 */
1327 -int parse_loose_header(const char *hdr, struct object_info *oi,
1328 - unsigned int flags)
1327 +int parse_loose_header(const char *hdr, struct object_info *oi)
1328 {
1329 const char *type_buf = hdr;
1330 unsigned long size;
@@ -1347,15 +1346,6 @@ int parse_loose_header(const char *hdr, struct object_info *oi,
1346 type = type_from_string_gently(type_buf, type_len, 1);
1347 if (oi->type_name)
1348 strbuf_add(oi->type_name, type_buf, type_len);
1350 - /*
1351 - * Set type to 0 if its an unknown object and
1352 - * we're obtaining the type using '--allow-unknown-type'
1353 - * option.
1354 - */
1355 - if ((flags & OBJECT_INFO_ALLOW_UNKNOWN_TYPE) && (type < 0))
1356 - type = 0;
1357 - else if (type < 0)
1358 - die(_("invalid object type"));
1349 if (oi->typep)
1350 *oi->typep = type;
1351
@@ -1382,7 +1372,14 @@ int parse_loose_header(const char *hdr, struct object_info *oi,
1372 /*
1373 * The length must be followed by a zero byte
1374 */
1385 - return *hdr ? -1 : type;
1375 + if (*hdr)
1376 + return -1;
1377 +
1378 + /*
1379 + * The format is valid, but the type may still be bogus. The
1380 + * Caller needs to check its oi->typep.
1381 + */
1382 + return 0;
1383 }
1384
1385 static int loose_object_info(struct repository *r,
@@ -1396,6 +1393,7 @@ static int loose_object_info(struct repository *r,
1393 char hdr[MAX_HEADER_LEN];
1394 struct strbuf hdrbuf = STRBUF_INIT;
1395 unsigned long size_scratch;
1396 + enum object_type type_scratch;
1397 int allow_unknown = flags & OBJECT_INFO_ALLOW_UNKNOWN_TYPE;
1398
1399 if (oi->delta_base_oid)
@@ -1427,6 +1425,8 @@ static int loose_object_info(struct repository *r,
1425
1426 if (!oi->sizep)
1427 oi->sizep = &size_scratch;
1428 + if (!oi->typep)
1429 + oi->typep = &type_scratch;
1430
1431 if (oi->disk_sizep)
1432 *oi->disk_sizep = mapsize;
@@ -1434,6 +1434,18 @@ static int loose_object_info(struct repository *r,
1434 switch (unpack_loose_header(&stream, map, mapsize, hdr, sizeof(hdr),
1435 allow_unknown ? &hdrbuf : NULL)) {
1436 case ULHR_OK:
1437 + if (parse_loose_header(hdrbuf.len ? hdrbuf.buf : hdr, oi) < 0)
1438 + status = error(_("unable to parse %s header"), oid_to_hex(oid));
1439 + else if (!allow_unknown && *oi->typep < 0)
1440 + die(_("invalid object type"));
1441 +
1442 + if (!oi->contentp)
1443 + break;
1444 + *oi->contentp = unpack_loose_rest(&stream, hdr, *oi->sizep, oid);
1445 + if (*oi->contentp)
1446 + goto cleanup;
1447 +
1448 + status = -1;
1449 break;
1450 case ULHR_BAD:
1451 status = error(_("unable to unpack %s header"),
@@ -1445,31 +1457,16 @@ static int loose_object_info(struct repository *r,
1457 break;
1458 }
1459
1448 - if (status < 0) {
1449 - /* Do nothing */
1450 - } else if (hdrbuf.len) {
1451 - if ((status = parse_loose_header(hdrbuf.buf, oi, flags)) < 0)
1452 - status = error(_("unable to parse %s header with --allow-unknown-type"),
1453 - oid_to_hex(oid));
1454 - } else if ((status = parse_loose_header(hdr, oi, flags)) < 0)
1455 - status = error(_("unable to parse %s header"), oid_to_hex(oid));
1456 -
1457 - if (status >= 0 && oi->contentp) {
1458 - *oi->contentp = unpack_loose_rest(&stream, hdr,
1459 - *oi->sizep, oid);
1460 - if (!*oi->contentp) {
1461 - git_inflate_end(&stream);
1462 - status = -1;
1463 - }
1464 - } else
1465 - git_inflate_end(&stream);
1466 -
1460 + git_inflate_end(&stream);
1461 +cleanup:
1462 munmap(map, mapsize);
1463 if (oi->sizep == &size_scratch)
1464 oi->sizep = NULL;
1465 strbuf_release(&hdrbuf);
1466 + if (oi->typep == &type_scratch)
1467 + oi->typep = NULL;
1468 oi->whence = OI_LOOSE;
1472 - return (status < 0) ? status : 0;
1469 + return status;
1470 }
1471
1472 int obj_read_use_lock = 0;
@@ -2533,6 +2530,7 @@ int read_loose_object(const char *path,
2530 git_zstream stream;
2531 char hdr[MAX_HEADER_LEN];
2532 struct object_info oi = OBJECT_INFO_INIT;
2533 + oi.typep = type;
2534 oi.sizep = size;
2535
2536 *contents = NULL;
@@ -2549,12 +2547,13 @@ int read_loose_object(const char *path,
2547 goto out;
2548 }
2549
2552 - *type = parse_loose_header(hdr, &oi, 0);
2553 - if (*type < 0) {
2550 + if (parse_loose_header(hdr, &oi) < 0) {
2551 error(_("unable to parse header of %s"), path);
2552 git_inflate_end(&stream);
2553 goto out;
2554 }
2555 + if (*type < 0)
2556 + die(_("invalid object type"));
2557
2558 if (*type == OBJ_BLOB && *size > big_file_threshold) {
2559 if (check_stream_oid(&stream, hdr, *size, path, expected_oid) < 0)
streaming.c
+2 -1
@@ -225,6 +225,7 @@ static int open_istream_loose(struct git_istream *st, struct repository *r,
225 {
226 struct object_info oi = OBJECT_INFO_INIT;
227 oi.sizep = &st->size;
228 + oi.typep = type;
229
230 st->u.loose.mapped = map_loose_object(r, oid, &st->u.loose.mapsize);
231 if (!st->u.loose.mapped)
@@ -238,7 +239,7 @@ static int open_istream_loose(struct git_istream *st, struct repository *r,
239 case ULHR_TOO_LONG:
240 goto error;
241 }
241 - if (parse_loose_header(st->u.loose.hdr, &oi, 0) < 0)
242 + if (parse_loose_header(st->u.loose.hdr, &oi) < 0 || *type < 0)
243 goto error;
244
245 st->u.loose.hdr_used = strlen(st->u.loose.hdr) + 1;