git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH 06/10] unpack_loose_header(): avoid numeric comparison of zlib status

From
Jeff King <peff@peff.net>
Date
Feb 25, 2025, 06:30 UTC
Message-ID
<20250225063056.GF1293961@coredump.intra.peff.net>
In-Reply-To
<20250225062518.GA1293854@coredump.intra.peff.net>

When unpacking a loose header, we try to inflate the first 32 bytes. We'd expect either Z_OK (we filled up the output buffer, but there are more bytes in the object) or Z_STREAM_END (this is a tiny object whose header and content fit in the buffer).

We check for that with "if (status < Z_OK)", making the assumption that all of the errors we'd see have negative values (as Z_OK itself is "0", and Z_STREAM_END is "1").

But there's at least one case this misses: Z_NEED_DICT is "2". This isn't something we'd ever expect to see, but if we do see it, we should consider it an error (since we have no dictionary to load).

Instead, the current code interprets Z_NEED_DICT as success and looks for the object header's terminating NUL in the bytes we've read. This will generaly be zero bytes if the dictionary is mentioned at the start of the stream. So we'll fail to find it and complain "the header is too long" (ULHR_LONG). But really, the problem is that the object is malformed, and we should return ULHR_BAD.

This is a minor bug, as we consider both cases to be an error. But it does mean we print the wrong error message. The test case added in the previous patch triggers this code, so we can just confirm the error message we see here.

Signed-off-by: Jeff King <peff@peff.net>
---
 object-file.c       | 2 +-
 t/t1006-cat-file.sh | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/object-file.c b/object-file.c
index 5b2446bfc1..5b347cfc2f 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1362,7 +1362,7 @@ enum unpack_loose_header_result unpack_loose_header(git_zstream *stream,
 	obj_read_unlock();
 	status = git_inflate(stream, 0);
 	obj_read_lock();
-	if (status < Z_OK)
+	if (status != Z_OK && status != Z_STREAM_END)
 		return ULHR_BAD;
 
 	/*
diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
index e493600aff..86a2825473 100755
--- a/t/t1006-cat-file.sh
+++ b/t/t1006-cat-file.sh
@@ -951,6 +951,8 @@ test_expect_success 'object reading handles zlib dictionary' - <<\EOT
 	printf '\170\273\017\112\003\143' >$objpath &&
 
 	test_must_fail git cat-file blob $blob 2>err &&
+	test_grep ! 'too long' err &&
+	test_grep 'error: unable to unpack' err &&
 	test_grep 'error: inflate: needs dictionary' err
 EOT
 
-- 
2.48.1.709.gf47ae731ff
Previous: Jeff KingNext: Taylor Blau
Message 21 of 34 in “some zlib inflating bug fixes”
  1. 0/10 some zlib inflating bug fixesJeff King, Feb 25, 2025
  2. 01/10 loose_object_info(): BUG() on inflating content with unknown typeJeff King, Feb 25, 2025
  3. Patrick SteinhardtFeb 25, 2025
  4. Junio C HamanoFeb 26, 2025
  5. Taylor BlauFeb 28, 2025
  6. Jeff KingMar 4, 2025
  7. Junio C HamanoMar 4, 2025
  8. Taylor BlauFeb 28, 2025
  9. 02/10 unpack_loose_header(): simplify next_out assignmentJeff King, Feb 25, 2025
  10. Taylor BlauFeb 28, 2025
  11. 03/10 unpack_loose_header(): report headers without NUL as "bad"Jeff King, Feb 25, 2025
  12. 04/10 unpack_loose_header(): fix infinite loop on broken zlib inputJeff King, Feb 25, 2025
  13. Patrick SteinhardtFeb 25, 2025
  14. Eric SunshineFeb 25, 2025
  15. Junio C HamanoFeb 26, 2025
  16. Taylor BlauFeb 28, 2025
  17. 05/10 git_inflate(): skip zlib_post_call() sanity check on Z_NEED_DICTJeff King, Feb 25, 2025
  18. Junio C HamanoFeb 26, 2025
  19. Taylor BlauFeb 28, 2025
  20. Jeff KingMar 4, 2025
  21. 06/10 unpack_loose_header(): avoid numeric comparison of zlib statusJeff King, Feb 25, 2025
  22. Taylor BlauFeb 28, 2025
  23. Jeff KingMar 4, 2025
  24. 07/10 unpack_loose_rest(): avoid numeric comparison of zlib statusJeff King, Feb 25, 2025
  25. 08/10 unpack_loose_rest(): never clean up zstreamJeff King, Feb 25, 2025
  26. Junio C HamanoFeb 26, 2025
  27. 09/10 unpack_loose_rest(): simplify error handlingJeff King, Feb 25, 2025
  28. Junio C HamanoFeb 26, 2025
  29. Taylor BlauFeb 28, 2025
  30. 10/10 unpack_loose_rest(): rewrite return handling for clarityJeff King, Feb 25, 2025
  31. Taylor BlauFeb 28, 2025
  32. Jeff KingMar 4, 2025
  33. Taylor BlauMar 4, 2025
  34. Taylor BlauFeb 28, 2025

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.