{"thread":{"id":"62997","subject":"[PATCH 0/10] some zlib inflating bug fixes","startedAt":"2025-02-25T06:25:20Z","lastAt":"2025-03-04T21:32:03Z","messageCount":34,"participants":["Jeff King","Patrick Steinhardt","Eric Sunshine","Junio C Hamano","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":10},"messages":[{"id":"512943","messageId":"20250225062518.GA1293854@coredump.intra.peff.net","threadId":"62997","inReplyTo":null,"subject":"[PATCH 0/10] some zlib inflating bug fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-02-25T06:25:18Z","receivedAt":"2025-02-25T06:25:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Here are a few bug fixes related to zlib-inflating objects. One is an\ninfinite loop, but triggering it requires writing to the local repo\nalong with running some seldom-used commands, so I think is not a\nsecurity risk. The other can be triggered by sending a specially\ncrafted pack, but it triggers a BUG(), so you'd only be crashing out\nyour own push.\n\nThe fixes themselves are in patches 4 and 5. The rest are related\ncleanups or clarifications in nearby code.\n\n [01/10]: loose_object_info(): BUG() on inflating content with unknown type\n [02/10]: unpack_loose_header(): simplify next_out assignment\n [03/10]: unpack_loose_header(): report headers without NUL as \"bad\"\n [04/10]: unpack_loose_header(): fix infinite loop on broken zlib input\n [05/10]: git_inflate(): skip zlib_post_call() sanity check on Z_NEED_DICT\n [06/10]: unpack_loose_header(): avoid numeric comparison of zlib status\n [07/10]: unpack_loose_rest(): avoid numeric comparison of zlib status\n [08/10]: unpack_loose_rest(): never clean up zstream\n [09/10]: unpack_loose_rest(): simplify error handling\n [10/10]: unpack_loose_rest(): rewrite return handling for clarity\n\n git-zlib.c | 27 +++++++++++++----------\n object-file.c | 48 ++++++++++++++++++++--------------------\n t/t1006-cat-file.sh | 53 +++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 92 insertions(+), 36 deletions(-)\n\n-Peff\n"},{"id":"512944","messageId":"20250225062824.GA1293961@coredump.intra.peff.net","threadId":"62997","inReplyTo":"20250225062518.GA1293854@coredump.intra.peff.net","subject":"[PATCH 01/10] loose_object_info(): BUG() on inflating content with unknown type","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-02-25T06:28:24Z","receivedAt":"2025-02-25T06:28:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"After unpack_loose_header() returns, it will have inflated not only the\nobject header, but possibly some bytes of the object content. When we\ncall unpack_loose_rest() to extract the actual content, it finds those\nextra bytes by skipping past the header's terminating NUL in the buffer.\nLike this:\n\n int bytes = strlen(buffer) + 1;\n n = stream->total_out - bytes;\n ...\n memcpy(buf, (char *) buffer + bytes, n);\n\nThis won't work with the OBJECT_INFO_ALLOW_UNKNOWN_TYPE flag, as there\nwe allow a header of arbitrary size. We put into a strbuf, but feed only\nthe final 32-byte chunk we read to unpack_loose_rest(). In that case\nstream->total_out may unexpectedly large, and thus our \"n\" will be\nlarge, causing an out-of-bounds read (we do check it against our\nallocated buffer size, which prevents an out-of-bounds write).\n\nProbably this could be made to work by feeding the strbuf to\nunpack_loose_rest(), along with adjusting some types (e.g., \"bytes\"\nwould need to be a size_t, since it is no longer operating on a 32-byte\nbuffer).\n\nBut I don't think it's possible to actually trigger this in practice.\nThe only caller who passes ALLOW_UNKNOWN_TYPE is cat-file, which only\nallows it with the \"-t\" and \"-s\" options (neither of which access the\ncontent). There is one way you can _almost_ trigger it: the oid compat\nroutines (i.e., accessing sha1 via sha256 names and vice versa) will\nconvert objects on the fly (which requires access to the content) using\nthe same flags that were passed in. So in theory this:\n\n t='some very large type field that causes an extra inflate call'\n sha1_oid=$(git hash-object -w -t \"$t\" file)\n sha256_oid=$(git rev-parse --output-object-format=sha256 $sha1_oid)\n git cat-file --allow-unknown-type -s $sha256_oid\n\nwould try to access the content. But it doesn't work, because using\ncompat objects requires an entry in the .git/objects/loose-object-idx\nfile, and we don't generate such an entry for non-standard types (see\nthe \"compat\" section of write_object_file_literally()).\n\nIf we use \"t=blob\" instead, then it does access the compat object, but\nit doesn't trigger the problem (because \"blob\" is a standard short type\nname, and it fits in the initial 32-byte buffer).\n\nSo given that this is almost a memory error bug, I think it's worth\naddressing. But because we can't actually trigger the situation, I'm\nhesitant to try a fix that we can't run. Instead let's document the\nrestriction and protect ourselves from the out-of-bounds read by adding\na BUG() check.\n\nSigned-off-by: Jeff King \n---\nI found this because I was tracing the code path after\nunpack_loose_header() returns to verify some assumptions in the other\npatches.\n\nIt really makes me wonder if this \"unknown type\" stuff has any value\nat all. You can create an object with any type using \"hash-object\n--literally -t\". And you can ask about its type and size. But you can\nnever retrieve the object content! Nor can you pack it or transfer it,\nsince packs use a numeric type field.\n\nThis code was added ~2015, but I don't think anybody built more on top\nof it. I wonder if we should just consider it a failed experiment and\nrip out the support.\n\n object-file.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/object-file.c b/object-file.c\nindex 00c3a4b910..45b251ba04 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1580,6 +1580,8 @@ static int loose_object_info(struct repository *r,\n \n \t\tif (!oi->contentp)\n \t\t\tbreak;\n+\t\tif (hdrbuf.len)\n+\t\t\tBUG(\"unpacking content with unknown types not yet supported\");\n \t\t*oi->contentp = unpack_loose_rest(&stream, hdr, *oi->sizep, oid);\n \t\tif (*oi->contentp)\n \t\t\tgoto cleanup;\n-- \n2.48.1.709.gf47ae731ff\n\n"},{"id":"512945","messageId":"20250225062900.GB1293961@coredump.intra.peff.net","threadId":"62997","inReplyTo":"20250225062518.GA1293854@coredump.intra.peff.net","subject":"[PATCH 02/10] unpack_loose_header(): simplify next_out assignment","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-02-25T06:29:00Z","receivedAt":"2025-02-25T06:29:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When using OBJECT_INFO_ALLOW_UNKNOWN_TYPE to unpack a header that\ndoesn't fit into our initial 32-byte buffer, we loop over calls\ngit_inflate(), feeding it our buffer to the \"next_out\" pointer each\ntime. As the code is written, we reset next_out after each inflate call\n(and after reading the output), ready for the next loop.\n\nThis isn't wrong, but there are a few advantages to setting up\n\"next_out\" right before each inflate call, rather than after:\n\n 1. It drops a few duplicated lines of code.\n\n 2. It makes it obvious that we always feed a fresh buffer on each call\n (and thus can never see Z_BUF_ERROR due to due to a lack of output\n space).\n\n 3. After we exit the loop, we'll leave stream->next_out pointing to\n the end of the fetched data (this is how zlib callers find out how\n much data is in the buffer). This doesn't matter in practice, since\n nobody looks at it again. But it's probably the least-surprising\n thing to do, as it matches how next_out is left when the whole\n thing fits in the initial 32-byte buffer (and we don't enter the\n loop at all).\n\nSigned-off-by: Jeff King \n---\nNot strictly necessary, but I think it makes some of the reasoning in\npatch 4 easier.\n\n object-file.c | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex 45b251ba04..0fd42981fb 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1385,18 +1385,17 @@ enum unpack_loose_header_result unpack_loose_header(git_zstream *stream,\n \t * reading the stream.\n \t */\n \tstrbuf_add(header, buffer, stream->next_out - (unsigned char *)buffer);\n-\tstream->next_out = buffer;\n-\tstream->avail_out = bufsiz;\n \n \tdo {\n+\t\tstream->next_out = buffer;\n+\t\tstream->avail_out = bufsiz;\n+\n \t\tobj_read_unlock();\n \t\tstatus = git_inflate(stream, 0);\n \t\tobj_read_lock();\n \t\tstrbuf_add(header, buffer, stream->next_out - (unsigned char *)buffer);\n \t\tif (memchr(buffer, '\\0', stream->next_out - (unsigned char *)buffer))\n \t\t\treturn 0;\n-\t\tstream->next_out = buffer;\n-\t\tstream->avail_out = bufsiz;\n \t} while (status != Z_STREAM_END);\n \treturn ULHR_TOO_LONG;\n }\n-- \n2.48.1.709.gf47ae731ff\n\n"},{"id":"512946","messageId":"20250225062940.GC1293961@coredump.intra.peff.net","threadId":"62997","inReplyTo":"20250225062518.GA1293854@coredump.intra.peff.net","subject":"[PATCH 03/10] unpack_loose_header(): report headers without NUL as \"bad\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-02-25T06:29:40Z","receivedAt":"2025-02-25T06:29:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If a caller asks us to read the whole loose object header value into a\nstrbuf (e.g., via \"cat-file --allow-unknown-type\"), we'll keep reading\nuntil we see a NUL byte marking the end of the header.\n\nIf we hit Z_STREAM_END before seeing the NUL, we obviously have to stop.\nBut we return ULHR_TOO_LONG, which doesn't make any sense. The \"too\nlong\" return code is used in the normal, 32-byte limited mode to\nindicate that we stopped looking. There is no such thing as \"too long\"\nhere, as we'd keep reading forever until we see the end of stream or the\nNUL.\n\nInstead, we should return ULHR_BAD. The loose object has no NUL marking\nthe end of header, so it is malformed. The behavior difference is\nslight; in either case we'd consider the object unreadable and refuse to\ngo further. The only difference is the specific error message we\nproduce.\n\nThere's no test case here, as we'd need to generate a valid zlib stream\nwithout a NUL. That's not something Git will do without writing new\ncustom code. And in the next patch we'll fix another bug in this area\nwhich will make this easier to do (and we will test it then).\n\nSigned-off-by: Jeff King \n---\nYou might be curious what happens if we see an error before the stream\nend. If so, read on to patch 4...\n\n object-file.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex 0fd42981fb..8c9295413b 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1397,7 +1397,7 @@ enum unpack_loose_header_result unpack_loose_header(git_zstream *stream,\n \t\tif (memchr(buffer, '\\0', stream->next_out - (unsigned char *)buffer))\n \t\t\treturn 0;\n \t} while (status != Z_STREAM_END);\n-\treturn ULHR_TOO_LONG;\n+\treturn ULHR_BAD;\n }\n \n static void *unpack_loose_rest(git_zstream *stream,\n-- \n2.48.1.709.gf47ae731ff\n\n"},{"id":"512947","messageId":"20250225062958.GD1293961@coredump.intra.peff.net","threadId":"62997","inReplyTo":"20250225062518.GA1293854@coredump.intra.peff.net","subject":"[PATCH 04/10] unpack_loose_header(): fix infinite loop on broken zlib input","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-02-25T06:29:58Z","receivedAt":"2025-02-25T06:29:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When reading a loose object, we first try to expand the first 32 bytes\nto read the type+size header. This is enough for any of the normal Git\ntypes. But since 46f034483e (sha1_file: support reading from a loose\nobject of unknown type, 2015-05-03), the caller can also ask us to parse\nany unknown names, which can be much longer. In this case we keep\ninflating until we find the NUL at the end of the header, or hit\nZ_STREAM_END.\n\nBut what if zlib can't make forward progress? For example, if the loose\nobject file is truncated, we'll have no more data to feed it. It will\nreturn Z_BUF_ERROR, and we'll just loop infinitely, calling\ngit_inflate() over and over but never seeing new bytes nor an\nend-of-stream marker.\n\nWe can fix this by only looping when we think we can make forward\nprogress. This will always be Z_OK in this case. In other code we might\nalso be able to continue on Z_BUF_ERROR, but:\n\n - We will never see Z_BUF_ERROR because the output buffer is full; we\n always feed a fresh 32-byte buffer on each call to git_inflate().\n\n - We may see Z_BUF_ERROR if we run out of input. But since we've fed\n the whole mmap'd buffer to zlib, if it runs out of input there is\n nothing more we can do.\n\nSo if we don't see Z_OK (and didn't see the end-of-header NUL, otherwise\nwe'd have broken out of the loop), then we should stop looping and\nreturn an error.\n\nThe test case shows an example where the input is truncated (which gives\nus the input Z_BUF_ERROR case above).\n\nAlthough we do operate on objects we might get from an untrusted remote,\nI don't think the security implications of this bug are too great. It\ncan only trigger if both of these are true:\n\n - You're reading a loose object whose on-disk representation was\n written by an attacker. So fetching an object (or receiving a push)\n are mostly OK, because even with unpack-objects it is our local,\n trusted code that writes out the object file. The exception may be\n fetching from an untrusted local repo, or using dumb-http, which\n copies objects verbatim. But...\n\n - The only code path which triggers the inflate loop is cat-file's\n --allow-unknown-type option. This is unlikely to be called at all\n outside of debugging. But I also suspect that objects with\n non-standard types (or that are truncated) would not survive the\n usual fetch/receive checks in the first place.\n\nSo I think it would be quite hard to trick somebody into running the\ninfinite loop, and we can just fix the bug.\n\nCo-authored-by: Taylor Blau \nSigned-off-by: Jeff King \n---\n object-file.c | 2 +-\n t/t1006-cat-file.sh | 19 +++++++++++++++++++\n 2 files changed, 20 insertions(+), 1 deletion(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex 8c9295413b..5b2446bfc1 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1396,7 +1396,7 @@ enum unpack_loose_header_result unpack_loose_header(git_zstream *stream,\n \t\tstrbuf_add(header, buffer, stream->next_out - (unsigned char *)buffer);\n \t\tif (memchr(buffer, '\\0', stream->next_out - (unsigned char *)buffer))\n \t\t\treturn 0;\n-\t} while (status != Z_STREAM_END);\n+\t} while (status == Z_OK);\n \treturn ULHR_BAD;\n }\n \ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex 398865d6eb..0b0d915773 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -903,6 +903,25 @@ test_expect_success 'cat-file -t and -s on corrupt loose object' '\n \t)\n '\n \n+test_expect_success 'truncated object with --allow-unknown-type' - <<\\EOT\n+\tobjtype='a really long type name that exceeds the 32-byte limit' &&\n+\tblob=$(git hash-object -w --literally -t \"$objtype\" /dev/null) &&\n+\tobjpath=.git/objects/$(test_oid_to_path \"$blob\") &&\n+\n+\t# We want to truncate the object far enough in that we don't hit the\n+\t# end while inflating the first 32 bytes (since we want to have to dig\n+\t# for the trailing NUL of the header). But we don't want to go too far,\n+\t# since our header isn't very big. And of course we are counting\n+\t# deflated zlib bytes in the on-disk file, so it's a bit of a guess.\n+\t# Empirically 50 seems to work.\n+\tmv \"$objpath\" obj.bak &&\n+\ttest_when_finished 'mv obj.bak \"$objpath\"' &&\n+\ttest_copy_bytes 50 \"$objpath\" &&\n+\n+\ttest_must_fail git cat-file --allow-unknown-type -t $blob 2>err &&\n+\ttest_grep \"unable to unpack $blob header\" err\n+EOT\n+\n # Tests for git cat-file --follow-symlinks\n test_expect_success 'prep for symlink tests' '\n \techo_without_newline \"$hello_content\" >morx &&\n-- \n2.48.1.709.gf47ae731ff\n\n"},{"id":"512948","messageId":"20250225063026.GE1293961@coredump.intra.peff.net","threadId":"62997","inReplyTo":"20250225062518.GA1293854@coredump.intra.peff.net","subject":"[PATCH 05/10] git_inflate(): skip zlib_post_call() sanity check on Z_NEED_DICT","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-02-25T06:30:26Z","receivedAt":"2025-02-25T06:30:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This fixes a case where malformed object input can cause us to hit a\nBUG() call in the git-zlib.c code.\n\nThe zlib format allows the use of preset dictionaries to reduce the size\nof deflated data. The checksum of the dictionary is computed by the\ndeflate code and goes into the stream. On the inflating side, zlib sees\nthe dictionary checksum and returns Z_NEED_DICT, asking the caller to\nprovide the dictionary data via inflateSetDictionary().\n\nThis should never happen in Git, because we never provide a dictionary\nfor deflating (and if we get a stream that mentions a dictionary, we\nhave no idea how to provide it). So normally Z_NEED_DICT is a hard error\nfor us. But something interesting happens if we _do_ happen to see it\n(e.g., because of a corrupt or malicious input).\n\nIn git_inflate() as we loop over calls to zlib's inflate(), we translate\nbetween our large-integer git_zstream values and zlib's native z_stream\ntypes, copying in and out with zlib_pre_call() and zlib_post_call(). In\nzlib_post_call() we have a few sanity checks, including one that checks\nthat the number of bytes consumed by zlib (as measured by it moving the\n\"next_in\" pointer) is equal to the movement of its \"total_in\" count.\n\nBut these do not correspond when we see Z_NEED_DICT! Zlib consumes the\nbytes from the input buffer but it does not increment total_in. And so\nwe hit the BUG(\"total_in mismatch\") call.\n\nThere are a few options here:\n\n - We could ditch that BUG() check. It is making too many assumptions\n about how zlib updates these values. But it does have value in most\n cases as a sanity check on the values we're copying.\n\n - We could skip the zlib_post_call() entirely when we see Z_NEED_DICT.\n We know that it's hard error for us, so we should just send the\n status up the stack and let the caller bail.\n\n The downside is that if we ever did want to support dictionaries,\n we couldn't (the git_zstream will be out of sync, since we never\n copied its values back from the z_stream).\n\n - We could continue to call zlib_post_call(), but skip just that BUG()\n check if the status is Z_NEED_DICT. This keeps git_inflate() as a\n thin wrapper around inflate(), and would let us later support\n dictionaries for some calls if we wanted to.\n\nThis patch uses the third approach. It seems like the least-surprising\nthing to keep git_inflate() a close to inflate() as possible. And while\nit makes the diff a bit larger (since we have to pass the status down to\nto the zlib_post_call() function), it's a static local function, and\nevery caller by definition will have just made a zlib call (and so will\nhave a status integer).\n\nCo-authored-by: Taylor Blau \nSigned-off-by: Jeff King \n---\n git-zlib.c | 27 ++++++++++++++++-----------\n t/t1006-cat-file.sh | 32 ++++++++++++++++++++++++++++++++\n 2 files changed, 48 insertions(+), 11 deletions(-)\n\ndiff --git a/git-zlib.c b/git-zlib.c\nindex 651dd9e07c..df9604910e 100644\n--- a/git-zlib.c\n+++ b/git-zlib.c\n@@ -45,7 +45,7 @@ static void zlib_pre_call(git_zstream *s)\n \ts->z.avail_out = zlib_buf_cap(s->avail_out);\n }\n \n-static void zlib_post_call(git_zstream *s)\n+static void zlib_post_call(git_zstream *s, int status)\n {\n \tunsigned long bytes_consumed;\n \tunsigned long bytes_produced;\n@@ -54,7 +54,12 @@ static void zlib_post_call(git_zstream *s)\n \tbytes_produced = s->z.next_out - s->next_out;\n \tif (s->z.total_out != s->total_out + bytes_produced)\n \t\tBUG(\"total_out mismatch\");\n-\tif (s->z.total_in != s->total_in + bytes_consumed)\n+\t/*\n+\t * zlib does not update total_in when it returns Z_NEED_DICT,\n+\t * causing a mismatch here. Skip the sanity check in that case.\n+\t */\n+\tif (status != Z_NEED_DICT &&\n+\t s->z.total_in != s->total_in + bytes_consumed)\n \t\tBUG(\"total_in mismatch\");\n \n \ts->total_out = s->z.total_out;\n@@ -72,7 +77,7 @@ void git_inflate_init(git_zstream *strm)\n \n \tzlib_pre_call(strm);\n \tstatus = inflateInit(&strm->z);\n-\tzlib_post_call(strm);\n+\tzlib_post_call(strm, status);\n \tif (status == Z_OK)\n \t\treturn;\n \tdie(\"inflateInit: %s (%s)\", zerr_to_string(status),\n@@ -90,7 +95,7 @@ void git_inflate_init_gzip_only(git_zstream *strm)\n \n \tzlib_pre_call(strm);\n \tstatus = inflateInit2(&strm->z, windowBits);\n-\tzlib_post_call(strm);\n+\tzlib_post_call(strm, status);\n \tif (status == Z_OK)\n \t\treturn;\n \tdie(\"inflateInit2: %s (%s)\", zerr_to_string(status),\n@@ -103,7 +108,7 @@ void git_inflate_end(git_zstream *strm)\n \n \tzlib_pre_call(strm);\n \tstatus = inflateEnd(&strm->z);\n-\tzlib_post_call(strm);\n+\tzlib_post_call(strm, status);\n \tif (status == Z_OK)\n \t\treturn;\n \terror(\"inflateEnd: %s (%s)\", zerr_to_string(status),\n@@ -122,7 +127,7 @@ int git_inflate(git_zstream *strm, int flush)\n \t\t\t\t ? 0 : flush);\n \t\tif (status == Z_MEM_ERROR)\n \t\t\tdie(\"inflate: out of memory\");\n-\t\tzlib_post_call(strm);\n+\t\tzlib_post_call(strm, status);\n \n \t\t/*\n \t\t * Let zlib work another round, while we can still\n@@ -160,7 +165,7 @@ void git_deflate_init(git_zstream *strm, int level)\n \tmemset(strm, 0, sizeof(*strm));\n \tzlib_pre_call(strm);\n \tstatus = deflateInit(&strm->z, level);\n-\tzlib_post_call(strm);\n+\tzlib_post_call(strm, status);\n \tif (status == Z_OK)\n \t\treturn;\n \tdie(\"deflateInit: %s (%s)\", zerr_to_string(status),\n@@ -176,7 +181,7 @@ static void do_git_deflate_init(git_zstream *strm, int level, int windowBits)\n \tstatus = deflateInit2(&strm->z, level,\n \t\t\t\t Z_DEFLATED, windowBits,\n \t\t\t\t 8, Z_DEFAULT_STRATEGY);\n-\tzlib_post_call(strm);\n+\tzlib_post_call(strm, status);\n \tif (status == Z_OK)\n \t\treturn;\n \tdie(\"deflateInit2: %s (%s)\", zerr_to_string(status),\n@@ -207,7 +212,7 @@ int git_deflate_abort(git_zstream *strm)\n \n \tzlib_pre_call(strm);\n \tstatus = deflateEnd(&strm->z);\n-\tzlib_post_call(strm);\n+\tzlib_post_call(strm, status);\n \treturn status;\n }\n \n@@ -227,7 +232,7 @@ int git_deflate_end_gently(git_zstream *strm)\n \n \tzlib_pre_call(strm);\n \tstatus = deflateEnd(&strm->z);\n-\tzlib_post_call(strm);\n+\tzlib_post_call(strm, status);\n \treturn status;\n }\n \n@@ -244,7 +249,7 @@ int git_deflate(git_zstream *strm, int flush)\n \t\t\t\t ? 0 : flush);\n \t\tif (status == Z_MEM_ERROR)\n \t\t\tdie(\"deflate: out of memory\");\n-\t\tzlib_post_call(strm);\n+\t\tzlib_post_call(strm, status);\n \n \t\t/*\n \t\t * Let zlib work another round, while we can still\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex 0b0d915773..e493600aff 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -922,6 +922,38 @@ test_expect_success 'truncated object with --allow-unknown-type' - <<\\EOT\n \ttest_grep \"unable to unpack $blob header\" err\n EOT\n \n+test_expect_success 'object reading handles zlib dictionary' - <<\\EOT\n+\techo 'content that will be recompressed' >file &&\n+\tblob=$(git hash-object -w file) &&\n+\tobjpath=.git/objects/$(test_oid_to_path \"$blob\") &&\n+\n+\t# Recompress a loose object using a precomputed zlib dictionary.\n+\t# This was originally done with:\n+\t#\n+\t# perl -MCompress::Raw::Zlib -e '\n+\t# binmode STDIN;\n+\t# binmode STDOUT;\n+\t# my $data = do { local $/; };\n+\t# my $in = new Compress::Raw::Zlib::Inflate;\n+\t# my $de = new Compress::Raw::Zlib::Deflate(\n+\t# -Dictionary => \"anything\"\n+\t# );\n+\t# $in->inflate($data, $raw);\n+\t# $de->deflate($raw, $out);\n+\t# print $out;\n+\t# ' $objpath\n+\t#\n+\t# but we do not want to require the perl module for all test runs (nor\n+\t# carry a custom t/helper program that uses zlib features we don't\n+\t# otherwise care about).\n+\tmv \"$objpath\" obj.bak &&\n+\ttest_when_finished 'mv obj.bak \"$objpath\"' &&\n+\tprintf '\\170\\273\\017\\112\\003\\143' >$objpath &&\n+\n+\ttest_must_fail git cat-file blob $blob 2>err &&\n+\ttest_grep 'error: inflate: needs dictionary' err\n+EOT\n+\n # Tests for git cat-file --follow-symlinks\n test_expect_success 'prep for symlink tests' '\n \techo_without_newline \"$hello_content\" >morx &&\n-- \n2.48.1.709.gf47ae731ff\n\n"},{"id":"512949","messageId":"20250225063056.GF1293961@coredump.intra.peff.net","threadId":"62997","inReplyTo":"20250225062518.GA1293854@coredump.intra.peff.net","subject":"[PATCH 06/10] unpack_loose_header(): avoid numeric comparison of zlib status","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-02-25T06:30:56Z","receivedAt":"2025-02-25T06:30:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When unpacking a loose header, we try to inflate the first 32 bytes.\nWe'd expect either Z_OK (we filled up the output buffer, but there are\nmore bytes in the object) or Z_STREAM_END (this is a tiny object whose\nheader and content fit in the buffer).\n\nWe check for that with \"if (status < Z_OK)\", making the assumption that\nall of the errors we'd see have negative values (as Z_OK itself is \"0\",\nand Z_STREAM_END is \"1\").\n\nBut there's at least one case this misses: Z_NEED_DICT is \"2\". This\nisn't something we'd ever expect to see, but if we do see it, we should\nconsider it an error (since we have no dictionary to load).\n\nInstead, the current code interprets Z_NEED_DICT as success and looks\nfor the object header's terminating NUL in the bytes we've read. This\nwill generaly be zero bytes if the dictionary is mentioned at the start\nof the stream. So we'll fail to find it and complain \"the header is too\nlong\" (ULHR_LONG). But really, the problem is that the object is\nmalformed, and we should return ULHR_BAD.\n\nThis is a minor bug, as we consider both cases to be an error. But it\ndoes mean we print the wrong error message. The test case added in the\nprevious patch triggers this code, so we can just confirm the error\nmessage we see here.\n\nSigned-off-by: Jeff King \n---\n object-file.c | 2 +-\n t/t1006-cat-file.sh | 2 ++\n 2 files changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex 5b2446bfc1..5b347cfc2f 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1362,7 +1362,7 @@ enum unpack_loose_header_result unpack_loose_header(git_zstream *stream,\n \tobj_read_unlock();\n \tstatus = git_inflate(stream, 0);\n \tobj_read_lock();\n-\tif (status < Z_OK)\n+\tif (status != Z_OK && status != Z_STREAM_END)\n \t\treturn ULHR_BAD;\n \n \t/*\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex e493600aff..86a2825473 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -951,6 +951,8 @@ test_expect_success 'object reading handles zlib dictionary' - <<\\EOT\n \tprintf '\\170\\273\\017\\112\\003\\143' >$objpath &&\n \n \ttest_must_fail git cat-file blob $blob 2>err &&\n+\ttest_grep ! 'too long' err &&\n+\ttest_grep 'error: unable to unpack' err &&\n \ttest_grep 'error: inflate: needs dictionary' err\n EOT\n \n-- \n2.48.1.709.gf47ae731ff\n\n"},{"id":"512950","messageId":"20250225063115.GG1293961@coredump.intra.peff.net","threadId":"62997","inReplyTo":"20250225062518.GA1293854@coredump.intra.peff.net","subject":"[PATCH 07/10] unpack_loose_rest(): avoid numeric comparison of zlib status","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-02-25T06:31:15Z","receivedAt":"2025-02-25T06:31:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When unpacking the actual content of a loose object file, we insist both\nthat the status code we got is Z_STREAM_END, and that we consumed all\nbytes.\n\nIf we didn't, we'll return an error, but the specific error message we\nproduce depends on which of the two error conditions we saw. So we'll\ncheck both a second time to decide which error to produce. But this\nsecond time, our status code check is loose: it checks for a negative\nstatus value.\n\nThis can get confused by zlib codes which are not negative, such as\nZ_NEED_DICT. In this case we'd erroneously print nothing at all, when we\nshould say \"corrupt loose object\".\n\nInstead, this second check should check explicitly against Z_STREAM_END.\n\nNote that Z_OK is \"0\", so the existing code also produced no message for\nZ_OK. But it's impossible to see that status, since we only break out of\nthe inflate loop when we stop seeing Z_OK (so a stream which has more\nbytes than its object header claims would eventually yield Z_BUF_ERROR).\n\nThere's no test here, as it would require a loose object whose zlib\nstream returns Z_NEED_DICT in the middle of the object content. I think\nthat is probably possible, but even our Z_NEED_DICT test in t1006 does\nnot trigger this, because we hit that error while reading the header. I\nfound this bug while reviewing all callers of git_inflate() for bugs\nsimilar to the one we saw in unpack_loose_header(). This was the only\nother case that did a numeric comparison rather than explicitly checking\nfor Z_STREAM_END.\n\nSigned-off-by: Jeff King \n---\n object-file.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex 5b347cfc2f..859888eb2a 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1441,7 +1441,7 @@ static void *unpack_loose_rest(git_zstream *stream,\n \t\treturn buf;\n \t}\n \n-\tif (status < 0)\n+\tif (status != Z_STREAM_END)\n \t\terror(_(\"corrupt loose object '%s'\"), oid_to_hex(oid));\n \telse if (stream->avail_in)\n \t\terror(_(\"garbage at end of loose object '%s'\"),\n-- \n2.48.1.709.gf47ae731ff\n\n"},{"id":"512951","messageId":"20250225063312.GH1293961@coredump.intra.peff.net","threadId":"62997","inReplyTo":"20250225062518.GA1293854@coredump.intra.peff.net","subject":"[PATCH 08/10] unpack_loose_rest(): never clean up zstream","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-02-25T06:33:12Z","receivedAt":"2025-02-25T06:33:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The unpack_loose_rest() function has funny ownership semantics: we pass\nin a z_stream opened by the caller, but then only _sometimes_ close it.\n\nThis oddity has developed over time. When the function was originally\nsplit out in 5180cacc20 (Split up unpack_sha1_file() some more,\n2005-06-02), it always called inflateEnd() to clean up the stream\n(though nowadays it is a git_zstream and we call git_inflate_end()).\n\nBut in 7efbff7531 (unpack_sha1_file(): detect corrupt loose object\nfiles., 2007-03-05) we added error code paths which don't close the\nstream. This makes some sense, as we'd still look at parts of the stream\nstruct to decide which error to show (though I am not sure in practice\nif inflateEnd() even touches those fields).\n\nThis subtlety makes it hard to know when the caller has to clean up the\nstream and when it does not. That led to the leak fixed by aa9ef614dc\n(object-file: fix memory leak when reading corrupted headers,\n2024-08-14).\n\nLet's instead always leave the stream intact, forcing the caller to\nclean it up. You might think that would create more work for the\ncallers, but it actually ends up simplifying them, since they can put\nthe call to git_inflate_end() in the common cleanup code path.\n\nTwo things to note, though:\n\n - The check_stream_oid() function is used as a replacement for\n unpack_loose_rest() in read_loose_object() to read blobs. It\n inherited the same funny semantics, and we should fix it here, too\n (to keep the cleanup in read_loose_object() consistent).\n\n - In read_loose_object() we need a second \"out\" label, as we can jump\n to the existing label before opening the stream at all (and since\n the struct is opaque, there is no way to if it was initialized or\n not, so we must not call git_inflate_end() in that case).\n\nSigned-off-by: Jeff King \n---\nThis patch and the two after are pure cleanups which should have no\nbehavior effect. I think they make things better, but one could argue\nthey are churn.\n\nI didn't reproduce it, but I think this is also fixing a leak when\ncheck_stream_oid() returned an error.\n\n object-file.c | 19 ++++++++-----------\n 1 file changed, 8 insertions(+), 11 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex 859888eb2a..8cf87caef5 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1437,7 +1437,6 @@ static void *unpack_loose_rest(git_zstream *stream,\n \t\t}\n \t}\n \tif (status == Z_STREAM_END && !stream->avail_in) {\n-\t\tgit_inflate_end(stream);\n \t\treturn buf;\n \t}\n \n@@ -1601,8 +1600,8 @@ static int loose_object_info(struct repository *r,\n \t\tdie(_(\"loose object %s (stored in %s) is corrupt\"),\n \t\t oid_to_hex(oid), path);\n \n-\tgit_inflate_end(&stream);\n cleanup:\n+\tgit_inflate_end(&stream);\n \tmunmap(map, mapsize);\n \tif (oi->sizep == &size_scratch)\n \t\toi->sizep = NULL;\n@@ -3081,7 +3080,6 @@ static int check_stream_oid(git_zstream *stream,\n \t\tgit_hash_update(&c, buf, stream->next_out - buf);\n \t\ttotal_read += stream->next_out - buf;\n \t}\n-\tgit_inflate_end(stream);\n \n \tif (status != Z_STREAM_END) {\n \t\terror(_(\"corrupt loose object '%s'\"), oid_to_hex(expected_oid));\n@@ -3128,35 +3126,34 @@ int read_loose_object(const char *path,\n \tif (unpack_loose_header(&stream, map, mapsize, hdr, sizeof(hdr),\n \t\t\t\tNULL) != ULHR_OK) {\n \t\terror(_(\"unable to unpack header of %s\"), path);\n-\t\tgit_inflate_end(&stream);\n-\t\tgoto out;\n+\t\tgoto out_inflate;\n \t}\n \n \tif (parse_loose_header(hdr, oi) < 0) {\n \t\terror(_(\"unable to parse header of %s\"), path);\n-\t\tgit_inflate_end(&stream);\n-\t\tgoto out;\n+\t\tgoto out_inflate;\n \t}\n \n \tif (*oi->typep == OBJ_BLOB && *size > big_file_threshold) {\n \t\tif (check_stream_oid(&stream, hdr, *size, path, expected_oid) < 0)\n-\t\t\tgoto out;\n+\t\t\tgoto out_inflate;\n \t} else {\n \t\t*contents = unpack_loose_rest(&stream, hdr, *size, expected_oid);\n \t\tif (!*contents) {\n \t\t\terror(_(\"unable to unpack contents of %s\"), path);\n-\t\t\tgit_inflate_end(&stream);\n-\t\t\tgoto out;\n+\t\t\tgoto out_inflate;\n \t\t}\n \t\thash_object_file_literally(the_repository->hash_algo,\n \t\t\t\t\t *contents, *size,\n \t\t\t\t\t oi->type_name->buf, real_oid);\n \t\tif (!oideq(expected_oid, real_oid))\n-\t\t\tgoto out;\n+\t\t\tgoto out_inflate;\n \t}\n \n \tret = 0; /* everything checks out */\n \n+out_inflate:\n+\tgit_inflate_end(&stream);\n out:\n \tif (map)\n \t\tmunmap(map, mapsize);\n-- \n2.48.1.709.gf47ae731ff\n\n"},{"id":"512952","messageId":"20250225063351.GI1293961@coredump.intra.peff.net","threadId":"62997","inReplyTo":"20250225062518.GA1293854@coredump.intra.peff.net","subject":"[PATCH 09/10] unpack_loose_rest(): simplify error handling","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-02-25T06:33:51Z","receivedAt":"2025-02-25T06:33:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Inflating a loose object is considered successful only if we got\nZ_STREAM_END and there were no more bytes. We check both of those\nconditions and return success, but then have to check them a second time\nto decide which error message to produce.\n\nI.e., we do something like this:\n\n if (!error_1 && !error_2)\n ...return success...\n\n if (error_1)\n ...handle error1...\n else if (error_2)\n ...handle error2...\n ...common error handling...\n\nThis repetition was the source of a small bug fixed in an earlier commit\n(our Z_STREAM_END check was not the same in the two conditionals).\n\nInstead we can chain them all into a single if/else cascade, which\navoids repeating ourselves:\n\n if (error_1)\n ...handle error1...\n else if (error_2)\n ...handle error2....\n else\n ...return success...\n ...common error handling...\n\nSigned-off-by: Jeff King \n---\n object-file.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex 8cf87caef5..b7928fb74e 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1436,15 +1436,15 @@ static void *unpack_loose_rest(git_zstream *stream,\n \t\t\tobj_read_lock();\n \t\t}\n \t}\n-\tif (status == Z_STREAM_END && !stream->avail_in) {\n-\t\treturn buf;\n-\t}\n \n \tif (status != Z_STREAM_END)\n \t\terror(_(\"corrupt loose object '%s'\"), oid_to_hex(oid));\n \telse if (stream->avail_in)\n \t\terror(_(\"garbage at end of loose object '%s'\"),\n \t\t oid_to_hex(oid));\n+\telse\n+\t\treturn buf;\n+\n \tfree(buf);\n \treturn NULL;\n }\n-- \n2.48.1.709.gf47ae731ff\n\n"},{"id":"512953","messageId":"20250225063421.GJ1293961@coredump.intra.peff.net","threadId":"62997","inReplyTo":"20250225062518.GA1293854@coredump.intra.peff.net","subject":"[PATCH 10/10] unpack_loose_rest(): rewrite return handling for clarity","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-02-25T06:34:21Z","receivedAt":"2025-02-25T06:34:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We have a pattern like:\n\n if (error1)\n ...handle error 1...\n else if (error2)\n ...handle error 2...\n else\n ...return buf...\n ...free buf and return NULL...\n\nThis is a little subtle because it is the return in the success block\nthat lets us skip the common error handling. Rewrite this instead to\nfree the buffer in each error path, marking it as NULL, and then all\ncode paths can use the common return.\n\nThis should make the logic a bit easier to follow. It does mean\nduplicating the buf cleanup for errors, but it's a single line.\n\nSigned-off-by: Jeff King \n---\nObviously could be squashed into the previous one, but I thought the\nsequence of diffs made it easier to understand what was being changed.\n\n object-file.c | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex b7928fb74e..1df8870578 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1437,16 +1437,16 @@ static void *unpack_loose_rest(git_zstream *stream,\n \t\t}\n \t}\n \n-\tif (status != Z_STREAM_END)\n+\tif (status != Z_STREAM_END) {\n \t\terror(_(\"corrupt loose object '%s'\"), oid_to_hex(oid));\n-\telse if (stream->avail_in)\n+\t\tFREE_AND_NULL(buf);\n+\t} else if (stream->avail_in) {\n \t\terror(_(\"garbage at end of loose object '%s'\"),\n \t\t oid_to_hex(oid));\n-\telse\n-\t\treturn buf;\n+\t\tFREE_AND_NULL(buf);\n+\t}\n \n-\tfree(buf);\n-\treturn NULL;\n+\treturn buf;\n }\n \n /*\n-- \n2.48.1.709.gf47ae731ff\n"},{"id":"513005","messageId":"Z72sns9I-zq_KoNr@pks.im","threadId":"62997","inReplyTo":"20250225062824.GA1293961@coredump.intra.peff.net","subject":"Re: [PATCH 01/10] loose_object_info(): BUG() on inflating content with unknown type","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-25T11:42:22Z","receivedAt":"2025-02-25T11:42:31Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Feb 25, 2025 at 01:28:24AM -0500, Jeff King wrote:\n> After unpack_loose_header() returns, it will have inflated not only the\n> object header, but possibly some bytes of the object content. When we\n> call unpack_loose_rest() to extract the actual content, it finds those\n> extra bytes by skipping past the header's terminating NUL in the buffer.\n> Like this:\n> \n> int bytes = strlen(buffer) + 1;\n> n = stream->total_out - bytes;\n> ...\n> memcpy(buf, (char *) buffer + bytes, n);\n> \n> This won't work with the OBJECT_INFO_ALLOW_UNKNOWN_TYPE flag, as there\n> we allow a header of arbitrary size. We put into a strbuf, but feed only\n\ns/into/it &/\n\n> the final 32-byte chunk we read to unpack_loose_rest(). In that case\n> stream->total_out may unexpectedly large, and thus our \"n\" will be\n\ns/may/& be/\n\n> large, causing an out-of-bounds read (we do check it against our\n> allocated buffer size, which prevents an out-of-bounds write).\n> \n> Probably this could be made to work by feeding the strbuf to\n> unpack_loose_rest(), along with adjusting some types (e.g., \"bytes\"\n> would need to be a size_t, since it is no longer operating on a 32-byte\n> buffer).\n\nI was a bit confused initially as I was thinking in terms of `size_t`\nand `uint32_t` as I misread 32-byte for 32-bit, which is an immediate\nshortcut that my brain took because 32 bit is something you read all the\ntime. I don't really have a great idea for how to introduce the byte\nchunk better though to avoid this.\n\n> Signed-off-by: Jeff King \n> ---\n> I found this because I was tracing the code path after\n> unpack_loose_header() returns to verify some assumptions in the other\n> patches.\n> \n> It really makes me wonder if this \"unknown type\" stuff has any value\n> at all. You can create an object with any type using \"hash-object\n> --literally -t\". And you can ask about its type and size. But you can\n> never retrieve the object content! Nor can you pack it or transfer it,\n> since packs use a numeric type field.\n> \n> This code was added ~2015, but I don't think anybody built more on top\n> of it. I wonder if we should just consider it a failed experiment and\n> rip out the support.\n\nI certainly do not know and cannot think of any usecase for this. I also\nexpect that a repository with unknown object types is a recipe for weird\nedge cases in case they are being read somehow.\n\nI guess we'll know a bit more when this patch series lands? In case\nnobody complains it is another indicator that unknown object types\naren't being used out there in the wild.\n\n> object-file.c | 2 ++\n> 1 file changed, 2 insertions(+)\n> \n> diff --git a/object-file.c b/object-file.c\n> index 00c3a4b910..45b251ba04 100644\n> --- a/object-file.c\n> +++ b/object-file.c\n> @@ -1580,6 +1580,8 @@ static int loose_object_info(struct repository *r,\n> \n> \t\tif (!oi->contentp)\n> \t\t\tbreak;\n> +\t\tif (hdrbuf.len)\n> +\t\t\tBUG(\"unpacking content with unknown types not yet supported\");\n> \t\t*oi->contentp = unpack_loose_rest(&stream, hdr, *oi->sizep, oid);\n> \t\tif (*oi->contentp)\n> \t\t\tgoto cleanup;\n\nOkay. I was wondering whether we still need `hdrbuf`, but we of course\ndo in order to continue reading the type and length itself. The only\nthing we restrict is reading the contents of such objects.\n\nPatrick\n"},{"id":"513006","messageId":"Z72sp639o8rxG1Qf@pks.im","threadId":"62997","inReplyTo":"20250225062958.GD1293961@coredump.intra.peff.net","subject":"Re: [PATCH 04/10] unpack_loose_header(): fix infinite loop on broken zlib input","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-25T11:42:31Z","receivedAt":"2025-02-25T11:42:35Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Feb 25, 2025 at 01:29:58AM -0500, Jeff King wrote:\n> diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\n> index 398865d6eb..0b0d915773 100755\n> --- a/t/t1006-cat-file.sh\n> +++ b/t/t1006-cat-file.sh\n> @@ -903,6 +903,25 @@ test_expect_success 'cat-file -t and -s on corrupt loose object' '\n> \t)\n> '\n> \n> +test_expect_success 'truncated object with --allow-unknown-type' - <<\\EOT\n> +\tobjtype='a really long type name that exceeds the 32-byte limit' &&\n> +\tblob=$(git hash-object -w --literally -t \"$objtype\" /dev/null) &&\n> +\tobjpath=.git/objects/$(test_oid_to_path \"$blob\") &&\n> +\n> +\t# We want to truncate the object far enough in that we don't hit the\n> +\t# end while inflating the first 32 bytes (since we want to have to dig\n> +\t# for the trailing NUL of the header). But we don't want to go too far,\n> +\t# since our header isn't very big. And of course we are counting\n> +\t# deflated zlib bytes in the on-disk file, so it's a bit of a guess.\n> +\t# Empirically 50 seems to work.\n> +\tmv \"$objpath\" obj.bak &&\n> +\ttest_when_finished 'mv obj.bak \"$objpath\"' &&\n\nThe order should probably be reversed here, as we nowadays tend to first\nqueue the cleanup before doing the actual work. Not that it really\nmatters in this case.\n\nPatrick\n"},{"id":"513035","messageId":"CAPig+cSq89192SvKQRcPqDTnBEkVtLyrVMPmU989c5gwkO1tUw@mail.gmail.com","threadId":"62997","inReplyTo":"Z72sp639o8rxG1Qf@pks.im","subject":"Re: [PATCH 04/10] unpack_loose_header(): fix infinite loop on broken zlib input","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-02-25T19:00:05Z","receivedAt":"2025-02-25T19:00:19Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Feb 25, 2025 at 6:46 AM Patrick Steinhardt wrote:\n> On Tue, Feb 25, 2025 at 01:29:58AM -0500, Jeff King wrote:\n> > + mv \"$objpath\" obj.bak &&\n> > + test_when_finished 'mv obj.bak \"$objpath\"' &&\n>\n> The order should probably be reversed here, as we nowadays tend to first\n> queue the cleanup before doing the actual work. Not that it really\n> matters in this case.\n\nI'd say this case is fine as-is since that particular cleanup *only*\nmakes sense if `mv` succeeded. Moreover, if these statements were to\nbe reversed, we'd need to take extra precaution (using `||:`, for\ninstance) against pointless failure of the cleanup code itself; i.e.:\n\n test_when_finished 'mv obj.bak \"$objpath\" ||:' &&\n mv \"$objpath\" obj.bak &&\n"},{"id":"513048","messageId":"xmqqv7sxh3xv.fsf@gitster.g","threadId":"62997","inReplyTo":"20250225062824.GA1293961@coredump.intra.peff.net","subject":"Re: [PATCH 01/10] loose_object_info(): BUG() on inflating content with unknown type","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-02-26T01:47:56Z","receivedAt":"2025-02-26T01:47:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> It really makes me wonder if this \"unknown type\" stuff has any value\n> at all. You can create an object with any type using \"hash-object\n> --literally -t\". And you can ask about its type and size. But you can\n> never retrieve the object content! Nor can you pack it or transfer it,\n> since packs use a numeric type field.\n\nCorrect. IIRC, the \"--literally\" support was mostly for debugging,\nand as you noticed, is very much limited because it can only create\nfunny objects that are loose. And the debugging was not really about\nadding more object types, but was more about \"what would our code do\nwhen we see an object that is corrupt whose type we do not recognise\".\n\nI personally think the \"--literally\" should not survive the Git 3.0\nboundary.\n\nThanks.\n"},{"id":"513077","messageId":"xmqq34g0hnkr.fsf@gitster.g","threadId":"62997","inReplyTo":"20250225062958.GD1293961@coredump.intra.peff.net","subject":"Re: [PATCH 04/10] unpack_loose_header(): fix infinite loop on broken zlib input","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-02-26T12:56:04Z","receivedAt":"2025-02-26T12:56:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> diff --git a/object-file.c b/object-file.c\n> index 8c9295413b..5b2446bfc1 100644\n> --- a/object-file.c\n> +++ b/object-file.c\n> @@ -1396,7 +1396,7 @@ enum unpack_loose_header_result unpack_loose_header(git_zstream *stream,\n> \t\tstrbuf_add(header, buffer, stream->next_out - (unsigned char *)buffer);\n> \t\tif (memchr(buffer, '\\0', stream->next_out - (unsigned char *)buffer))\n> \t\t\treturn 0;\n> -\t} while (status != Z_STREAM_END);\n> +\t} while (status == Z_OK);\n> \treturn ULHR_BAD;\n> }\n\nAh, fond memories. I do recall we had very similar bugs whose\ncorrect solution was \"instead of stopping with Z_something, just\nwrite your loop to keep ging while you get Z_OK\" every time we\nencountered them.\n\n"},{"id":"513078","messageId":"xmqqwmdcg82z.fsf@gitster.g","threadId":"62997","inReplyTo":"20250225063312.GH1293961@coredump.intra.peff.net","subject":"Re: [PATCH 08/10] unpack_loose_rest(): never clean up zstream","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-02-26T13:16:04Z","receivedAt":"2025-02-26T13:16:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> This patch and the two after are pure cleanups which should have no\n> behavior effect. I think they make things better, but one could argue\n> they are churn.\n>\n> I didn't reproduce it, but I think this is also fixing a leak when\n> check_stream_oid() returned an error.\n\nLooking good. Thanks for paying down our technical debt.\n\n"},{"id":"513079","messageId":"xmqqr03kg7mb.fsf@gitster.g","threadId":"62997","inReplyTo":"20250225063026.GE1293961@coredump.intra.peff.net","subject":"Re: [PATCH 05/10] git_inflate(): skip zlib_post_call() sanity check on Z_NEED_DICT","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-02-26T13:26:04Z","receivedAt":"2025-02-26T13:26:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> But these do not correspond when we see Z_NEED_DICT! Zlib consumes the\n> bytes from the input buffer but it does not increment total_in. And so\n> we hit the BUG(\"total_in mismatch\") call.\n>\n> There are a few options here:\n>\n> - We could ditch that BUG() check. It is making too many assumptions\n> about how zlib updates these values. But it does have value in most\n> cases as a sanity check on the values we're copying.\n>\n> - We could skip the zlib_post_call() entirely when we see Z_NEED_DICT.\n> We know that it's hard error for us, so we should just send the\n> status up the stack and let the caller bail.\n>\n> The downside is that if we ever did want to support dictionaries,\n> we couldn't (the git_zstream will be out of sync, since we never\n> copied its values back from the z_stream).\n>\n> - We could continue to call zlib_post_call(), but skip just that BUG()\n> check if the status is Z_NEED_DICT. This keeps git_inflate() as a\n> thin wrapper around inflate(), and would let us later support\n> dictionaries for some calls if we wanted to.\n>\n> This patch uses the third approach. It seems like the least-surprising\n> thing to keep git_inflate() a close to inflate() as possible. And while\n> it makes the diff a bit larger (since we have to pass the status down to\n> to the zlib_post_call() function), it's a static local function, and\n> every caller by definition will have just made a zlib call (and so will\n> have a status integer).\n\nOuch. I am not sure if I would have made the choice you guys made\nif I were writing this fix, but that is mostly because I do not\nanticipate that we would ever support NEED_DICT and the other two\noptions seem simpler, and certainly not because I have any concrete\nreason to oppose the third approach.\n\n> +test_expect_success 'object reading handles zlib dictionary' - <<\\EOT\n> +\techo 'content that will be recompressed' >file &&\n> +\tblob=$(git hash-object -w file) &&\n> +\tobjpath=.git/objects/$(test_oid_to_path \"$blob\") &&\n> +\n> +\t# Recompress a loose object using a precomputed zlib dictionary.\n> +\t# This was originally done with:\n> +\t#\n> +\t# perl -MCompress::Raw::Zlib -e '\n> +\t# binmode STDIN;\n> +\t# binmode STDOUT;\n> +\t# my $data = do { local $/; };\n> +\t# my $in = new Compress::Raw::Zlib::Inflate;\n> +\t# my $de = new Compress::Raw::Zlib::Deflate(\n> +\t# -Dictionary => \"anything\"\n> +\t# );\n> +\t# $in->inflate($data, $raw);\n> +\t# $de->deflate($raw, $out);\n> +\t# print $out;\n> +\t# ' $objpath\n> +\t#\n> +\t# but we do not want to require the perl module for all test runs (nor\n> +\t# carry a custom t/helper program that uses zlib features we don't\n> +\t# otherwise care about).\n> +\tmv \"$objpath\" obj.bak &&\n> +\ttest_when_finished 'mv obj.bak \"$objpath\"' &&\n> +\tprintf '\\170\\273\\017\\112\\003\\143' >$objpath &&\n> +\n> +\ttest_must_fail git cat-file blob $blob 2>err &&\n> +\ttest_grep 'error: inflate: needs dictionary' err\n> +EOT\n> +\n\nWheee. I guess an ugly and down-to-bit-sequence test is much better\nthan having no test at all ;-)\n\nWe'd want to say >\"$objpath\" to adhere to CodingGuidelines, but\nI guess those particular versions of bash has not been seen in the\nwild for quite some time and we may want to loosen the rule there.\n\nAnyway, looking good. Thanks.\n"},{"id":"513080","messageId":"xmqqldtsg6oz.fsf@gitster.g","threadId":"62997","inReplyTo":"20250225063351.GI1293961@coredump.intra.peff.net","subject":"Re: [PATCH 09/10] unpack_loose_rest(): simplify error handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-02-26T13:46:04Z","receivedAt":"2025-02-26T13:46:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Inflating a loose object is considered successful only if we got\n> Z_STREAM_END and there were no more bytes. We check both of those\n> conditions and return success, but then have to check them a second time\n> to decide which error message to produce.\n>\n> I.e., we do something like this:\n>\n> if (!error_1 && !error_2)\n> ...return success...\n>\n> if (error_1)\n> ...handle error1...\n> else if (error_2)\n> ...handle error2...\n> ...common error handling...\n>\n> This repetition was the source of a small bug fixed in an earlier commit\n> (our Z_STREAM_END check was not the same in the two conditionals).\n>\n> Instead we can chain them all into a single if/else cascade, which\n> avoids repeating ourselves:\n>\n> if (error_1)\n> ...handle error1...\n> else if (error_2)\n> ...handle error2....\n> else\n> ...return success...\n> ...common error handling...\n>\n> Signed-off-by: Jeff King \n> ---\n> object-file.c | 6 +++---\n> 1 file changed, 3 insertions(+), 3 deletions(-)\n\nOf course the resulting code is so much cleaner and more obvious.\nThanks for cleaning it up.\n\n\n> diff --git a/object-file.c b/object-file.c\n> index 8cf87caef5..b7928fb74e 100644\n> --- a/object-file.c\n> +++ b/object-file.c\n> @@ -1436,15 +1436,15 @@ static void *unpack_loose_rest(git_zstream *stream,\n> \t\t\tobj_read_lock();\n> \t\t}\n> \t}\n> -\tif (status == Z_STREAM_END && !stream->avail_in) {\n> -\t\treturn buf;\n> -\t}\n> \n> \tif (status != Z_STREAM_END)\n> \t\terror(_(\"corrupt loose object '%s'\"), oid_to_hex(oid));\n> \telse if (stream->avail_in)\n> \t\terror(_(\"garbage at end of loose object '%s'\"),\n> \t\t oid_to_hex(oid));\n> +\telse\n> +\t\treturn buf;\n> +\n> \tfree(buf);\n> \treturn NULL;\n> }\n"},{"id":"513203","messageId":"Z8D/5Kmsn6XqXgaZ@nand.local","threadId":"62997","inReplyTo":"20250225062824.GA1293961@coredump.intra.peff.net","subject":"Re: [PATCH 01/10] loose_object_info(): BUG() on inflating content with unknown type","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-02-28T00:14:28Z","receivedAt":"2025-02-28T00:14:30Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Feb 25, 2025 at 01:28:24AM -0500, Jeff King wrote:\n> It really makes me wonder if this \"unknown type\" stuff has any value\n> at all. You can create an object with any type using \"hash-object\n> --literally -t\". And you can ask about its type and size. But you can\n> never retrieve the object content! Nor can you pack it or transfer it,\n> since packs use a numeric type field.\n>\n> This code was added ~2015, but I don't think anybody built more on top\n> of it. I wonder if we should just consider it a failed experiment and\n> rip out the support.\n\nYeah, I was wondering the same thing. I think 10 years seems like a long\nenough time to declare that it was a failed experiment ;-).\n\nThanks,\nTaylor\n"},{"id":"513204","messageId":"Z8EAXIHB4dVYS9t/@nand.local","threadId":"62997","inReplyTo":"xmqqv7sxh3xv.fsf@gitster.g","subject":"Re: [PATCH 01/10] loose_object_info(): BUG() on inflating content with unknown type","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-02-28T00:16:28Z","receivedAt":"2025-02-28T00:16:31Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Feb 25, 2025 at 05:47:56PM -0800, Junio C Hamano wrote:\n> Jeff King writes:\n>\n> > It really makes me wonder if this \"unknown type\" stuff has any value\n> > at all. You can create an object with any type using \"hash-object\n> > --literally -t\". And you can ask about its type and size. But you can\n> > never retrieve the object content! Nor can you pack it or transfer it,\n> > since packs use a numeric type field.\n>\n> Correct. IIRC, the \"--literally\" support was mostly for debugging,\n> and as you noticed, is very much limited because it can only create\n> funny objects that are loose. And the debugging was not really about\n> adding more object types, but was more about \"what would our code do\n> when we see an object that is corrupt whose type we do not recognise\".\n>\n> I personally think the \"--literally\" should not survive the Git 3.0\n> boundary.\n\nIt is quite useful for testing intentionally broken objects, like\ncommits with malformed author/committer lines, or trees with\nout-of-order entries, etc.\n\nPerhaps we could replace that with a test helper that is only accessible\nwithin the test suite that acts like \"hash-object --literally\" and\nremove \"--literally\" from the plumbing interface? I dunno.\n\nThanks,\nTaylor\n"},{"id":"513205","messageId":"Z8EA6SGpwiV5pv7J@nand.local","threadId":"62997","inReplyTo":"20250225062900.GB1293961@coredump.intra.peff.net","subject":"Re: [PATCH 02/10] unpack_loose_header(): simplify next_out assignment","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-02-28T00:18:49Z","receivedAt":"2025-02-28T00:18:52Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Feb 25, 2025 at 01:29:00AM -0500, Jeff King wrote:\n> When using OBJECT_INFO_ALLOW_UNKNOWN_TYPE to unpack a header that\n> doesn't fit into our initial 32-byte buffer, we loop over calls\n> git_inflate(), feeding it our buffer to the \"next_out\" pointer each\n> time. As the code is written, we reset next_out after each inflate call\n> (and after reading the output), ready for the next loop.\n>\n> This isn't wrong, but there are a few advantages to setting up\n> \"next_out\" right before each inflate call, rather than after:\n>\n> 1. It drops a few duplicated lines of code.\n>\n> 2. It makes it obvious that we always feed a fresh buffer on each call\n> (and thus can never see Z_BUF_ERROR due to due to a lack of output\n> space).\n>\n> 3. After we exit the loop, we'll leave stream->next_out pointing to\n> the end of the fetched data (this is how zlib callers find out how\n> much data is in the buffer). This doesn't matter in practice, since\n> nobody looks at it again. But it's probably the least-surprising\n> thing to do, as it matches how next_out is left when the whole\n> thing fits in the initial 32-byte buffer (and we don't enter the\n> loop at all).\n\nThanks for calling (3) out. There is definitely a subtle behavior change\nthere, but having the context that it doesn't matter in practice is\nuseful in judging whether or not the refactoring is OK.\n\nIn any event, I agree that this is a much cleaner read, and recall\nthinking something along the same lines as (1) when we were working on\nthis together.\n\nThanks,\nTaylor\n"},{"id":"513206","messageId":"Z8EBcqgn60tmXDGj@nand.local","threadId":"62997","inReplyTo":"20250225062958.GD1293961@coredump.intra.peff.net","subject":"Re: [PATCH 04/10] unpack_loose_header(): fix infinite loop on broken zlib input","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-02-28T00:21:06Z","receivedAt":"2025-02-28T00:21:08Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Feb 25, 2025 at 01:29:58AM -0500, Jeff King wrote:\n> Co-authored-by: Taylor Blau \n> Signed-off-by: Jeff King \n> ---\n> object-file.c | 2 +-\n> t/t1006-cat-file.sh | 19 +++++++++++++++++++\n> 2 files changed, 20 insertions(+), 1 deletion(-)\n\nI don't have much more to add here having helped write this patch and\nreviewed an early copy of it on the git-security list. So the series up\nto this point LGTM, let's keep reading on...\n\nThanks,\nTaylor\n"},{"id":"513212","messageId":"Z8EDxd3f/U+EdoLO@nand.local","threadId":"62997","inReplyTo":"xmqqr03kg7mb.fsf@gitster.g","subject":"Re: [PATCH 05/10] git_inflate(): skip zlib_post_call() sanity check on Z_NEED_DICT","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-02-28T00:31:01Z","receivedAt":"2025-02-28T00:31:03Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Feb 26, 2025 at 05:26:04AM -0800, Junio C Hamano wrote:\n> Jeff King writes:\n>\n> > But these do not correspond when we see Z_NEED_DICT! Zlib consumes the\n> > bytes from the input buffer but it does not increment total_in. And so\n> > we hit the BUG(\"total_in mismatch\") call.\n> >\n> > There are a few options here:\n> >\n> > - We could ditch that BUG() check. It is making too many assumptions\n> > about how zlib updates these values. But it does have value in most\n> > cases as a sanity check on the values we're copying.\n> >\n> > - We could skip the zlib_post_call() entirely when we see Z_NEED_DICT.\n> > We know that it's hard error for us, so we should just send the\n> > status up the stack and let the caller bail.\n> >\n> > The downside is that if we ever did want to support dictionaries,\n> > we couldn't (the git_zstream will be out of sync, since we never\n> > copied its values back from the z_stream).\n> >\n> > - We could continue to call zlib_post_call(), but skip just that BUG()\n> > check if the status is Z_NEED_DICT. This keeps git_inflate() as a\n> > thin wrapper around inflate(), and would let us later support\n> > dictionaries for some calls if we wanted to.\n> >\n> > This patch uses the third approach. It seems like the least-surprising\n> > thing to keep git_inflate() a close to inflate() as possible. And while\n> > it makes the diff a bit larger (since we have to pass the status down to\n> > to the zlib_post_call() function), it's a static local function, and\n> > every caller by definition will have just made a zlib call (and so will\n> > have a status integer).\n>\n> Ouch. I am not sure if I would have made the choice you guys made\n> if I were writing this fix, but that is mostly because I do not\n> anticipate that we would ever support NEED_DICT and the other two\n> options seem simpler, and certainly not because I have any concrete\n> reason to oppose the third approach.\n\nIt's been a few weeks since I last looked at this patch, but I have a\nvague recollection that we chose the second approach while discussing\nthis together.\n\nAnd indeed, looking at the copy I have from that session, it looks like\nwe did\n\n--- 8< ---\ndiff --git a/git-zlib.c b/git-zlib.c\nindex 651dd9e07c..265d3074e1 100644\n--- a/git-zlib.c\n+++ b/git-zlib.c\n@@ -122,6 +122,8 @@ int git_inflate(git_zstream *strm, int flush)\n ? 0 : flush);\n if (status == Z_MEM_ERROR)\n die(\"inflate: out of memory\");\n+ if (status == Z_NEED_DICT)\n+ break;\n zlib_post_call(strm);\n\n /*\n--- >8 ---\n\nI actually have a vague preference towards that approach, since I too do\nnot anticipate that we'd ever need/want to support Z_NEED_DICT (and if\nwe did, we could always change from option (2) to (3) later on).\nLikewise, the resulting diff is considerably smaller by line count,\nthough on the other hand this diff is not all that more complicated\noverall.\n\nI don't have a strong enough preference to suggest you make any changes\nhere, but I thought I'd mention it nonetheless.\n\n> > +test_expect_success 'object reading handles zlib dictionary' - <<\\EOT\n> > +\techo 'content that will be recompressed' >file &&\n> > +\tblob=$(git hash-object -w file) &&\n> > +\tobjpath=.git/objects/$(test_oid_to_path \"$blob\") &&\n> > +\n> > +\t# Recompress a loose object using a precomputed zlib dictionary.\n> > +\t# This was originally done with:\n> > +\t#\n> > +\t# perl -MCompress::Raw::Zlib -e '\n> > +\t# binmode STDIN;\n> > +\t# binmode STDOUT;\n> > +\t# my $data = do { local $/; };\n> > +\t# my $in = new Compress::Raw::Zlib::Inflate;\n> > +\t# my $de = new Compress::Raw::Zlib::Deflate(\n> > +\t# -Dictionary => \"anything\"\n> > +\t# );\n> > +\t# $in->inflate($data, $raw);\n> > +\t# $de->deflate($raw, $out);\n> > +\t# print $out;\n> > +\t# ' $objpath\n> > +\t#\n> > +\t# but we do not want to require the perl module for all test runs (nor\n> > +\t# carry a custom t/helper program that uses zlib features we don't\n> > +\t# otherwise care about).\n> > +\tmv \"$objpath\" obj.bak &&\n> > +\ttest_when_finished 'mv obj.bak \"$objpath\"' &&\n> > +\tprintf '\\170\\273\\017\\112\\003\\143' >$objpath &&\n> > +\n> > +\ttest_must_fail git cat-file blob $blob 2>err &&\n> > +\ttest_grep 'error: inflate: needs dictionary' err\n> > +EOT\n> > +\n>\n> Wheee. I guess an ugly and down-to-bit-sequence test is much better\n> than having no test at all ;-)\n\nAs a fun aside, Peff wrote this Perl script (unsurprisingly, I strongly\ndislike writing Perl), and amazingly it worked the first try when we\nwere still hypothesizing about what this bug would look like. It was\nquite the surprise to both of us, I think, that it worked so well.\n\nThanks,\nTaylor\n"},{"id":"513213","messageId":"Z8EEGd4Q+Py3Ym0l@nand.local","threadId":"62997","inReplyTo":"20250225063056.GF1293961@coredump.intra.peff.net","subject":"Re: [PATCH 06/10] unpack_loose_header(): avoid numeric comparison of zlib status","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-02-28T00:32:25Z","receivedAt":"2025-02-28T00:32:28Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Feb 25, 2025 at 01:30:56AM -0500, Jeff King wrote:\n> diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\n> index e493600aff..86a2825473 100755\n> --- a/t/t1006-cat-file.sh\n> +++ b/t/t1006-cat-file.sh\n> @@ -951,6 +951,8 @@ test_expect_success 'object reading handles zlib dictionary' - <<\\EOT\n> \tprintf '\\170\\273\\017\\112\\003\\143' >$objpath &&\n>\n> \ttest_must_fail git cat-file blob $blob 2>err &&\n> +\ttest_grep ! 'too long' err &&\n> +\ttest_grep 'error: unable to unpack' err &&\n> \ttest_grep 'error: inflate: needs dictionary' err\n> EOT\n\nAll looking good here, too.\n\nI think the test_grep is hiding what is a fairly unpleasant error\nmessage that says the same thing a few times from different points in\nthe call-stack. But that isn't anything new from this series, and I'm\ncontent to let it be a problem for another day ;-).\n\nThanks,\nTaylor\n"},{"id":"513214","messageId":"Z8EEn2LGetE7B4Sp@nand.local","threadId":"62997","inReplyTo":"20250225063351.GI1293961@coredump.intra.peff.net","subject":"Re: [PATCH 09/10] unpack_loose_rest(): simplify error handling","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-02-28T00:34:39Z","receivedAt":"2025-02-28T00:34:41Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Feb 25, 2025 at 01:33:51AM -0500, Jeff King wrote:\n> ---\n> object-file.c | 6 +++---\n> 1 file changed, 3 insertions(+), 3 deletions(-)\n\nMuch cleaner indeed. Well spotted, and thanks for fixing it up.\n\nThanks,\nTaylor\n"},{"id":"513215","messageId":"Z8EFGnUmyAAJ6DUY@nand.local","threadId":"62997","inReplyTo":"20250225063421.GJ1293961@coredump.intra.peff.net","subject":"Re: [PATCH 10/10] unpack_loose_rest(): rewrite return handling for clarity","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-02-28T00:36:42Z","receivedAt":"2025-02-28T00:36:44Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Feb 25, 2025 at 01:34:21AM -0500, Jeff King wrote:\n> This should make the logic a bit easier to follow. It does mean\n> duplicating the buf cleanup for errors, but it's a single line.\n\nAt least to my eyes, I actually prefer the state after 9/10 and would\nprobably be OK to see this patch get dropped. I wish I had a compelling\nreason *why* I felt that way, but I think it may too subjective.\n\nI don't feel strongly about it either way, though.\n\nThanks,\nTaylor\n"},{"id":"513216","messageId":"Z8EFkLEiRCztQA8l@nand.local","threadId":"62997","inReplyTo":"20250225062518.GA1293854@coredump.intra.peff.net","subject":"Re: [PATCH 0/10] some zlib inflating bug fixes","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-02-28T00:38:40Z","receivedAt":"2025-02-28T00:38:42Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Feb 25, 2025 at 01:25:18AM -0500, Jeff King wrote:\n> git-zlib.c | 27 +++++++++++++----------\n> object-file.c | 48 ++++++++++++++++++++--------------------\n> t/t1006-cat-file.sh | 53 +++++++++++++++++++++++++++++++++++++++++++++\n> 3 files changed, 92 insertions(+), 36 deletions(-)\n\nThanks for putting these together and sending them out. I didn't have a\nton to add throughout since you and I wrote patches 4 and 5 together,\nbut I re-read things to make sure that everything still seemed sane a\nweek or two out.\n\nThe remainder of the series looks good to me too, though I am not in\nlove with the final patch. Like I mentioned in my response there, I\nthink it's pretty subjective, but I would be OK to see the first nine\npatches queued and the last one dropped. OTOH, I wouldn't be upset to\nsee it included either.\n\nThanks again for working on this :-).\n\nThanks,\nTaylor\n"},{"id":"513493","messageId":"20250304064308.GA1283901@coredump.intra.peff.net","threadId":"62997","inReplyTo":"Z8EAXIHB4dVYS9t/@nand.local","subject":"Re: [PATCH 01/10] loose_object_info(): BUG() on inflating content with unknown type","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-03-04T06:43:08Z","receivedAt":"2025-03-04T06:43:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 27, 2025 at 07:16:28PM -0500, Taylor Blau wrote:\n\n> On Tue, Feb 25, 2025 at 05:47:56PM -0800, Junio C Hamano wrote:\n> > Jeff King writes:\n> >\n> > > It really makes me wonder if this \"unknown type\" stuff has any value\n> > > at all. You can create an object with any type using \"hash-object\n> > > --literally -t\". And you can ask about its type and size. But you can\n> > > never retrieve the object content! Nor can you pack it or transfer it,\n> > > since packs use a numeric type field.\n> >\n> > Correct. IIRC, the \"--literally\" support was mostly for debugging,\n> > and as you noticed, is very much limited because it can only create\n> > funny objects that are loose. And the debugging was not really about\n> > adding more object types, but was more about \"what would our code do\n> > when we see an object that is corrupt whose type we do not recognise\".\n> >\n> > I personally think the \"--literally\" should not survive the Git 3.0\n> > boundary.\n> \n> It is quite useful for testing intentionally broken objects, like\n> commits with malformed author/committer lines, or trees with\n> out-of-order entries, etc.\n> \n> Perhaps we could replace that with a test helper that is only accessible\n> within the test suite that acts like \"hash-object --literally\" and\n> remove \"--literally\" from the plumbing interface? I dunno.\n\nI don't want to get rid of --literally. It is great not only for testing\nand debugging, but also if you needed to recreate a slightly broken\nobject (e.g., to recover from corruption). Hash-object will complain\nabout objects that fail fsck, even though they are perfectly usable (and\nmay be embedded in existing history).\n\nBut this unknown type stuff is not and has never been usable. So I'd\npropose to keep --literally but always reject unknown types.\n\n-Peff\n"},{"id":"513494","messageId":"20250304065501.GB1283901@coredump.intra.peff.net","threadId":"62997","inReplyTo":"Z8EEGd4Q+Py3Ym0l@nand.local","subject":"Re: [PATCH 06/10] unpack_loose_header(): avoid numeric comparison of zlib status","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-03-04T06:55:01Z","receivedAt":"2025-03-04T06:55:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 27, 2025 at 07:32:25PM -0500, Taylor Blau wrote:\n\n> On Tue, Feb 25, 2025 at 01:30:56AM -0500, Jeff King wrote:\n> > diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\n> > index e493600aff..86a2825473 100755\n> > --- a/t/t1006-cat-file.sh\n> > +++ b/t/t1006-cat-file.sh\n> > @@ -951,6 +951,8 @@ test_expect_success 'object reading handles zlib dictionary' - <<\\EOT\n> > \tprintf '\\170\\273\\017\\112\\003\\143' >$objpath &&\n> >\n> > \ttest_must_fail git cat-file blob $blob 2>err &&\n> > +\ttest_grep ! 'too long' err &&\n> > +\ttest_grep 'error: unable to unpack' err &&\n> > \ttest_grep 'error: inflate: needs dictionary' err\n> > EOT\n> \n> All looking good here, too.\n> \n> I think the test_grep is hiding what is a fairly unpleasant error\n> message that says the same thing a few times from different points in\n> the call-stack. But that isn't anything new from this series, and I'm\n> content to let it be a problem for another day ;-).\n\nYeah, we get one set of errors when we ask for the type to find out if\nwe need to peel a tag (it's not a tag, it's OBJ_ERR ;) ). And then\nagain we ask if it's a blob to try streaming. It's still OBJ_ERR. And\nthen we fall back to the non-streaming case.\n\nWe should probably check for errors earlier. And also avoid asking for\nthe type twice when we didn't peel a tag, which is just stupidly\ninefficient.\n\nPerhaps something like this (untested):\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 9de1016acd..e1dbbfeb43 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -236,7 +236,13 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,\n \n \t\tif (exp_type_id == OBJ_BLOB) {\n \t\t\tstruct object_id blob_oid;\n-\t\t\tif (oid_object_info(the_repository, &oid, NULL) == OBJ_TAG) {\n+\t\t\tenum object_type found_type = oid_object_info(the_repository,\n+\t\t\t\t\t\t\t\t &oid, NULL);\n+\n+\t\t\tif (found_type < 0)\n+\t\t\t\tdie(_(\"unable to read %s\"), oid_to_hex(&oid));\n+\n+\t\t\tif (found_type == OBJ_TAG) {\n \t\t\t\tchar *buffer = repo_read_object_file(the_repository,\n \t\t\t\t\t\t\t\t &oid,\n \t\t\t\t\t\t\t\t &type,\n@@ -251,10 +257,11 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,\n \t\t\t\t\t\t &hash_algos[oid.algo]))\n \t\t\t\t\tdie(\"%s not a valid tag\", oid_to_hex(&oid));\n \t\t\t\tfree(buffer);\n+\t\t\t\tfound_type = type;\n \t\t\t} else\n \t\t\t\toidcpy(&blob_oid, &oid);\n \n-\t\t\tif (oid_object_info(the_repository, &blob_oid, NULL) == OBJ_BLOB) {\n+\t\t\tif (found_type == OBJ_BLOB) {\n \t\t\t\tret = stream_blob(&blob_oid);\n \t\t\t\tgoto cleanup;\n \t\t\t}\n\nBut yes, this is all way out of scope for this series, and is true of\nany corruption.\n\n-Peff\n"},{"id":"513497","messageId":"20250304070818.GA1283943@coredump.intra.peff.net","threadId":"62997","inReplyTo":"Z8EDxd3f/U+EdoLO@nand.local","subject":"Re: [PATCH 05/10] git_inflate(): skip zlib_post_call() sanity check on Z_NEED_DICT","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-03-04T07:08:18Z","receivedAt":"2025-03-04T07:08:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 27, 2025 at 07:31:01PM -0500, Taylor Blau wrote:\n\n> It's been a few weeks since I last looked at this patch, but I have a\n> vague recollection that we chose the second approach while discussing\n> this together.\n> \n> And indeed, looking at the copy I have from that session, it looks like\n> we did\n> \n> --- 8< ---\n> diff --git a/git-zlib.c b/git-zlib.c\n> index 651dd9e07c..265d3074e1 100644\n> --- a/git-zlib.c\n> +++ b/git-zlib.c\n> @@ -122,6 +122,8 @@ int git_inflate(git_zstream *strm, int flush)\n> ? 0 : flush);\n> if (status == Z_MEM_ERROR)\n> die(\"inflate: out of memory\");\n> + if (status == Z_NEED_DICT)\n> + break;\n> zlib_post_call(strm);\n> \n> /*\n> --- >8 ---\n> \n> I actually have a vague preference towards that approach, since I too do\n> not anticipate that we'd ever need/want to support Z_NEED_DICT (and if\n> we did, we could always change from option (2) to (3) later on).\n> Likewise, the resulting diff is considerably smaller by line count,\n> though on the other hand this diff is not all that more complicated\n> overall.\n\nYes, that was definitely the patch we initially wrote. And I do agree\nthat it's pretty unlikely that we'd use Z_NEED_DICT ever. But what I\nreally disliked is that it leaves the git_zstream in a totally broken\nstate, unsynced with the underlying z_stream.\n\nIf the caller takes it as a hard error and immediately throws away the\nstream that's not too bad. Although even then it gets a little weird: we\nhave to call git_inflated_end(), which itself does the usual zlib\npre/post calls. The \"pre\" call overwrites the zstream with old values,\nwhich zlib's inflateEnd() then operates on.\n\nIt seems to be OK in practice, but we are making assumptions about which\nparts of the struct zlib cares about. I guess it's unlikely that\nunexpectedly rewinding total_in would cause zlib to misbehave, but it's\nprobably better not to be too intimate with it.\n\nAnd of course if somebody ever does want to play with Z_NEED_DICT, it's\na bit of a booby trap we've left for them. We know today that they will\nneed to change from option (2) to (3), but in that hypothetical future\nthey will have to first figure out why their z_stream is corrupted. ;)\n\n-Peff\n"},{"id":"513498","messageId":"20250304071011.GB1283943@coredump.intra.peff.net","threadId":"62997","inReplyTo":"Z8EFGnUmyAAJ6DUY@nand.local","subject":"Re: [PATCH 10/10] unpack_loose_rest(): rewrite return handling for clarity","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-03-04T07:10:11Z","receivedAt":"2025-03-04T07:10:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 27, 2025 at 07:36:42PM -0500, Taylor Blau wrote:\n\n> On Tue, Feb 25, 2025 at 01:34:21AM -0500, Jeff King wrote:\n> > This should make the logic a bit easier to follow. It does mean\n> > duplicating the buf cleanup for errors, but it's a single line.\n> \n> At least to my eyes, I actually prefer the state after 9/10 and would\n> probably be OK to see this patch get dropped. I wish I had a compelling\n> reason *why* I felt that way, but I think it may too subjective.\n> \n> I don't feel strongly about it either way, though.\n\nI also don't have a super strong feeling, though I fall on the other\nside of the line (which is why I bothered sending the patch).\n\nIf we didn't do that, I think the alternative is probably a comment\nlike:\n\n if (error1)\n\terror(describe error1);\n else if (error2)\n\terror(describe error2);\n else\n\treturn buf;\n\n /* if we didn't return above, we saw some error */\n free(buf);\n return NULL;\n\nI dunno. I'd probably stick with what I send. ;)\n\n-Peff\n"},{"id":"513535","messageId":"xmqq7c54u7lf.fsf@gitster.g","threadId":"62997","inReplyTo":"20250304064308.GA1283901@coredump.intra.peff.net","subject":"Re: [PATCH 01/10] loose_object_info(): BUG() on inflating content with unknown type","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-04T15:41:00Z","receivedAt":"2025-03-04T15:41:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> But this unknown type stuff is not and has never been usable. So I'd\n> propose to keep --literally but always reject unknown types.\n\nThat sounds like a good way to go. Thanks.\n"},{"id":"513571","messageId":"Z8dxUc1rUeOp13Pb@nand.local","threadId":"62997","inReplyTo":"20250304071011.GB1283943@coredump.intra.peff.net","subject":"Re: [PATCH 10/10] unpack_loose_rest(): rewrite return handling for clarity","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-04T21:32:01Z","receivedAt":"2025-03-04T21:32:03Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Mar 04, 2025 at 02:10:11AM -0500, Jeff King wrote:\n> On Thu, Feb 27, 2025 at 07:36:42PM -0500, Taylor Blau wrote:\n>\n> > On Tue, Feb 25, 2025 at 01:34:21AM -0500, Jeff King wrote:\n> > > This should make the logic a bit easier to follow. It does mean\n> > > duplicating the buf cleanup for errors, but it's a single line.\n> >\n> > At least to my eyes, I actually prefer the state after 9/10 and would\n> > probably be OK to see this patch get dropped. I wish I had a compelling\n> > reason *why* I felt that way, but I think it may too subjective.\n> >\n> > I don't feel strongly about it either way, though.\n>\n> I also don't have a super strong feeling, though I fall on the other\n> side of the line (which is why I bothered sending the patch).\n>\n> If we didn't do that, I think the alternative is probably a comment\n> like:\n>\n> if (error1)\n> \terror(describe error1);\n> else if (error2)\n> \terror(describe error2);\n> else\n> \treturn buf;\n>\n> /* if we didn't return above, we saw some error */\n> free(buf);\n> return NULL;\n>\n> I dunno. I'd probably stick with what I send. ;)\n\nFair enough ;-).\n\nThanks,\nTaylor\n"}]}