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

[PATCH 11/11] packfile: use oidread() instead of hashcpy() to fill object_id

From
Jeff King <peff@peff.net>
Date
Oct 25, 2024, 07:08 UTC
Message-ID
<20241025070810.GK2110355@coredump.intra.peff.net>
In-Reply-To
<20241025064148.GA2110169@coredump.intra.peff.net>

When chasing a REF_DELTA, we need to pull the raw hash bytes out of the mmap'd packfile into an object_id struct. We do that with a raw hashcpy() of the appropriate length (that happens directly now, though before the previous commit it happened inside find_pack_entry_one(), also using a hashcpy).

But I think this creates a potentially dangerous situation due to d4d364b2c7 (hash: convert `oidcmp()` and `oideq()` to compare whole hash, 2024-06-14). When using sha1, we'll have uninitialized bytes in the latter part of the object_id.hash buffer, which could fool oideq(), etc.

We should use oidread() instead, which correctly zero-pads the extra bytes, as of c98d762ed9 (global: ensure that object IDs are always padded, 2024-06-14).

As far as I can see, this has not been a problem in practice because the object_id we feed to find_pack_entry_one() is never used with oideq(), etc. It is being compared to the bytes mmap'd from a pack idx file, which of course do not have the extra padding bytes themselves. So there's no bug here, but this just puzzled me while looking at the code. We should do the more obviously safe thing, both for future-proofing and to avoid confusing readers.

Signed-off-by: Jeff King <peff@peff.net>
---
+cc Patrick for any wisdom here. I'd guess that the conversions from
c98d762ed9 were found by using ASan, valgrind, or similar with the new
oideq() implementation.  And so this case, because it actually is safe,
was not flagged.
 packfile.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/packfile.c b/packfile.c
index 005ca670b4..9560f0a33c 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1240,7 +1240,7 @@ off_t get_delta_base(struct packed_git *p,
 	} else if (type == OBJ_REF_DELTA) {
 		/* The base entry _must_ be in the same pack */
 		struct object_id oid;
-		hashcpy(oid.hash, base_info, the_repository->hash_algo);
+		oidread(&oid, base_info, the_repository->hash_algo);
 		base_offset = find_pack_entry_one(&oid, p);
 		*curpos += the_hash_algo->rawsz;
 	} else
-- 
2.47.0.363.g6e72b256be
Previous: Taylor BlauNext: Taylor Blau
Message 27 of 31 in “Bug report: v2.47.0 cannot fetch version 1 pack indexes”
  1. foxOct 19, 2024
  2. Eric SunshineOct 20, 2024
  3. Taylor BlauOct 21, 2024
  4. Jeff KingOct 20, 2024
  5. Jeff KingOct 20, 2024
  6. Taylor BlauOct 21, 2024
  7. Jeff KingOct 22, 2024
  8. Taylor BlauOct 22, 2024
  9. 0/11 dumb-http pack index v1 regression + cleanupsJeff King, Oct 25, 2024
  10. 01/11 midx: avoid duplicate packed_git entriesJeff King, Oct 25, 2024
  11. Taylor BlauOct 25, 2024
  12. 02/11 t5550: count fetches in "previously-fetched .idx" testJeff King, Oct 25, 2024
  13. 03/11 dumb-http: store downloaded pack idx as tempfileJeff King, Oct 25, 2024
  14. Taylor BlauOct 25, 2024
  15. Jeff KingOct 26, 2024
  16. Taylor BlauOct 28, 2024
  17. 04/11 packfile: drop has_pack_index()Jeff King, Oct 25, 2024
  18. Taylor BlauOct 25, 2024
  19. 05/11 packfile: drop sha1_pack_name()Jeff King, Oct 25, 2024
  20. 06/11 packfile: drop sha1_pack_index_name()Jeff King, Oct 25, 2024
  21. 07/11 packfile: warn people away from parse_packed_git()Jeff King, Oct 25, 2024
  22. Taylor BlauOct 25, 2024
  23. 08/11 http-walker: use object_id instead of bare hashJeff King, Oct 25, 2024
  24. 09/11 packfile: convert find_sha1_pack() to use object_idJeff King, Oct 25, 2024
  25. 10/11 packfile: use object_id in find_pack_entry_one()Jeff King, Oct 25, 2024
  26. Taylor BlauOct 25, 2024
  27. 11/11 packfile: use oidread() instead of hashcpy() to fill object_idJeff King, Oct 25, 2024
  28. Taylor BlauOct 25, 2024
  29. Taylor BlauOct 21, 2024
  30. Jeff KingOct 22, 2024
  31. Taylor BlauOct 22, 2024

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.