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

Re: [PATCH 6/6] pack-bitmap.c: factor out `bitmap_index_seek_commit()`

From
Jeff King <peff@peff.net>
Date
Mar 21, 2023, 18:13 UTC
Message-ID
<20230321181315.GH3119834@coredump.intra.peff.net>
In-Reply-To
<82049ed09e1695db644d1d4cf17557214e54dcea.1679342296.git.me@ttaylorr.com>
On Mon, Mar 20, 2023 at 04:02:55PM -0400, Taylor Blau wrote:
Show 7 quoted lines
> Factor out a common pattern within `lazy_bitmap_for_commit()` where we
> seek to a given position (expecting to read the start of an individual
> bitmap entry).
> 
> Both spots within `lazy_bitmap_for_commit()` emit a common error, so
> factor out the whole routine into its own function to DRY things up a
> little.

OK, so this case makes more sense to me as a bounds-check, because we are seeking to an arbitrary offset.

But...
Show 14 quoted lines
> +static int bitmap_index_seek_commit(struct bitmap_index *bitmap_git,
> +				     struct object_id *oid,
> +				     size_t pos)
> +{
> +	const int bitmap_header_size = 6;
> +
> +	bitmap_index_seek(bitmap_git, pos, SEEK_SET);
> +
> +	if (bitmap_git->map_size - bitmap_git->map_pos < bitmap_header_size)
> +		return error(_("corrupt ewah bitmap: truncated header for "
> +			       "bitmap of commit \"%s\""),
> +			oid_to_hex(oid));
> +	return 0;
> +}

So here we seek to the position, and then make sure we have 6 bytes to read, which makes sense. But in the seek step, are we worried that we will seek to somewhere bogus? If so, we'll get a BUG(). But is that the right thing if somebody feeds a bogus "pos" that moves past truncation?

I'm not 100% sure on where these offsets come from. But it looks like they're coming from the bitmap lookup table. In which case a bogus value there should be an error(), and not a BUG(), I would think.

-Peff
Previous: Taylor BlauNext: Taylor Blau
Message 26 of 29 in “pack-bitmap: miscellaneous mmap read hardening”
  1. 0/6 pack-bitmap: miscellaneous mmap read hardeningTaylor Blau, Mar 20, 2023
  2. 1/6 pack-bitmap.c: hide bitmap internals in `read_u8()`Taylor Blau, Mar 20, 2023
  3. Jeff KingMar 21, 2023
  4. Derrick StoleeMar 24, 2023
  5. 2/6 pack-bitmap.c: hide bitmap internals in `read_be32()`Taylor Blau, Mar 20, 2023
  6. 3/6 pack-bitmap.c: drop unnecessary 'inline'sTaylor Blau, Mar 20, 2023
  7. Jeff KingMar 21, 2023
  8. 4/6 pack-bitmap.c: factor out manual `map_pos` manipulationTaylor Blau, Mar 20, 2023
  9. Jeff KingMar 21, 2023
  10. Derrick StoleeMar 24, 2023
  11. Jeff KingMar 24, 2023
  12. Taylor BlauMar 24, 2023
  13. Jeff KingMar 25, 2023
  14. Taylor BlauMar 24, 2023
  15. Taylor BlauMar 24, 2023
  16. Taylor BlauMar 24, 2023
  17. 5/6 pack-bitmap.c: use `bitmap_index_seek()` where possibleTaylor Blau, Mar 20, 2023
  18. Jeff KingMar 21, 2023
  19. Derrick StoleeMar 24, 2023
  20. Jeff KingMar 24, 2023
  21. Junio C HamanoMar 24, 2023
  22. Jeff KingMar 24, 2023
  23. Junio C HamanoMar 24, 2023
  24. Taylor BlauMar 24, 2023
  25. 6/6 pack-bitmap.c: factor out `bitmap_index_seek_commit()`Taylor Blau, Mar 20, 2023
  26. Jeff KingMar 21, 2023
  27. Taylor BlauMar 21, 2023
  28. Jeff KingMar 21, 2023
  29. Derrick StoleeMar 24, 2023

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.