{"thread":{"id":"59424","subject":"[PATCH 0/6] pack-bitmap: miscellaneous mmap read hardening","startedAt":"2023-03-20T20:03:03Z","lastAt":"2023-03-25T04:57:14Z","messageCount":29,"participants":["Taylor Blau","Jeff King","Derrick Stolee","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"473802","messageId":"cover.1679342296.git.me@ttaylorr.com","threadId":"59424","inReplyTo":null,"subject":"[PATCH 0/6] pack-bitmap: miscellaneous mmap read hardening","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-03-20T20:02:37Z","receivedAt":"2023-03-20T20:03:03Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This short series has a few minor refactorings and hardenings I noticed\nwhile reading the pack-bitmap.c code recently.\n\nThe series is structured as follows:\n\n  - The first three are cleanups to the read_be32() and read_u8()\n    functions that could be taken on their own.\n\n  - The last three replace manually reading from `bitmap_git->map` and\n    adjusting `bitmap_git->map_pos` accordingly with a new\n    `bitmap_index_seek()` wrapper that accomplishes the same with\n    additional bounds- and overflow-checking.\n\nThanks in advance for your review.\n\nTaylor Blau (6):\n  pack-bitmap.c: hide bitmap internals in `read_u8()`\n  pack-bitmap.c: hide bitmap internals in `read_be32()`\n  pack-bitmap.c: drop unnecessary 'inline's\n  pack-bitmap.c: factor out manual `map_pos` manipulation\n  pack-bitmap.c: use `bitmap_index_seek()` where possible\n  pack-bitmap.c: factor out `bitmap_index_seek_commit()`\n\n pack-bitmap.c | 84 ++++++++++++++++++++++++++++++++++++---------------\n 1 file changed, 59 insertions(+), 25 deletions(-)\n\n-- \n2.40.0.77.gd564125b3f\n"},{"id":"473803","messageId":"8c70c94a3f5f0463041dbaaa542fd69bd159428b.1679342296.git.me@ttaylorr.com","threadId":"59424","inReplyTo":"cover.1679342296.git.me@ttaylorr.com","subject":"[PATCH 1/6] pack-bitmap.c: hide bitmap internals in `read_u8()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-03-20T20:02:40Z","receivedAt":"2023-03-20T20:03:08Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The `read_u8()` helper function internal to pack-bitmap.c was defined in\nb5007211b6 (pack-bitmap: do not use gcc packed attribute, 2014-11-27).\n\nPrior to b5007211b6, callers within pack-bitmap.c would read an\nindividual unsigned integer by doing something like:\n\n    struct bitmap_disk_entry *e;\n\n    e = (struct bitmap_disk_entry *)(index->map + index->map_pos);\n    index->map_pos += sizeof(*e);\n\n...which relied on the fact that the `bitmap_disk_entry` struct was\ndefined with `__attribute((packed))`, which b5007211b6 sought to get rid\nof since the `__attribute__` flag is a noop on some compilers (which\nmakes the above code rely on the absence of padding to be correct).\n\nSo b5007211b6 got rid of the above convention and replaced it by reading\nindividual fields of that structure with a `read_u8()` helper that reads\nfrom the region of memory pointed to by `->map`, and updates the\n`->map_pos` pointer accordingly.\n\nBut this forces callers to be intimately aware of `bitmap_git->map` and\n`bitmap_git->map_pos`. Instead, teach `read_u8()` to take a `struct\nbitmap_index *` directly, and avoid having callers deal with the\ninternals themselves.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n pack-bitmap.c | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex ca7c81b5c9..d8ba252ba1 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -251,9 +251,9 @@ static inline uint32_t read_be32(const unsigned char *buffer, size_t *pos)\n \treturn result;\n }\n \n-static inline uint8_t read_u8(const unsigned char *buffer, size_t *pos)\n+static inline uint8_t read_u8(struct bitmap_index *bitmap_git)\n {\n-\treturn buffer[(*pos)++];\n+\treturn bitmap_git->map[bitmap_git->map_pos++];\n }\n \n #define MAX_XOR_OFFSET 160\n@@ -283,8 +283,8 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)\n \t\t\treturn error(_(\"corrupt ewah bitmap: truncated header for entry %d\"), i);\n \n \t\tcommit_idx_pos = read_be32(index->map, &index->map_pos);\n-\t\txor_offset = read_u8(index->map, &index->map_pos);\n-\t\tflags = read_u8(index->map, &index->map_pos);\n+\t\txor_offset = read_u8(index);\n+\t\tflags = read_u8(index);\n \n \t\tif (nth_bitmap_object_oid(index, &oid, commit_idx_pos) < 0)\n \t\t\treturn error(_(\"corrupt ewah bitmap: commit index %u out of range\"),\n@@ -780,7 +780,7 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n \t\t}\n \n \t\tbitmap_git->map_pos += sizeof(uint32_t) + sizeof(uint8_t);\n-\t\txor_flags = read_u8(bitmap_git->map, &bitmap_git->map_pos);\n+\t\txor_flags = read_u8(bitmap_git);\n \t\tbitmap = read_bitmap_1(bitmap_git);\n \n \t\tif (!bitmap)\n@@ -821,7 +821,7 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n \t * ewah bitmap.\n \t */\n \tbitmap_git->map_pos += sizeof(uint32_t) + sizeof(uint8_t);\n-\tflags = read_u8(bitmap_git->map, &bitmap_git->map_pos);\n+\tflags = read_u8(bitmap_git);\n \tbitmap = read_bitmap_1(bitmap_git);\n \n \tif (!bitmap)\n-- \n2.40.0.77.gd564125b3f\n\n"},{"id":"473804","messageId":"4914c5b72c3aab9f59589daf8daff5e2271c52fb.1679342296.git.me@ttaylorr.com","threadId":"59424","inReplyTo":"cover.1679342296.git.me@ttaylorr.com","subject":"[PATCH 2/6] pack-bitmap.c: hide bitmap internals in `read_be32()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-03-20T20:02:43Z","receivedAt":"2023-03-20T20:03:15Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In a similar fashion as the previous commit, update `read_be32()` to\ntake a `struct bitmap_index *` instead of copies of and pointers to\nvariables within that structure.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n pack-bitmap.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex d8ba252ba1..794aaf5b02 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -244,10 +244,10 @@ static struct stored_bitmap *store_bitmap(struct bitmap_index *index,\n \treturn stored;\n }\n \n-static inline uint32_t read_be32(const unsigned char *buffer, size_t *pos)\n+static inline uint32_t read_be32(struct bitmap_index *bitmap_git)\n {\n-\tuint32_t result = get_be32(buffer + *pos);\n-\t(*pos) += sizeof(result);\n+\tuint32_t result = get_be32(bitmap_git->map + bitmap_git->map_pos);\n+\tbitmap_git->map_pos += sizeof(result);\n \treturn result;\n }\n \n@@ -282,7 +282,7 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)\n \t\tif (index->map_size - index->map_pos < 6)\n \t\t\treturn error(_(\"corrupt ewah bitmap: truncated header for entry %d\"), i);\n \n-\t\tcommit_idx_pos = read_be32(index->map, &index->map_pos);\n+\t\tcommit_idx_pos = read_be32(index);\n \t\txor_offset = read_u8(index);\n \t\tflags = read_u8(index);\n \n-- \n2.40.0.77.gd564125b3f\n\n"},{"id":"473805","messageId":"2e3e3a7145a5851fcf5c485b38d14344c9b824d7.1679342296.git.me@ttaylorr.com","threadId":"59424","inReplyTo":"cover.1679342296.git.me@ttaylorr.com","subject":"[PATCH 3/6] pack-bitmap.c: drop unnecessary 'inline's","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-03-20T20:02:46Z","receivedAt":"2023-03-20T20:03:19Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Both `read_be32()` and `read_u8()` are defined as inline dating back\nto b5007211b6 (pack-bitmap: do not use gcc packed attribute,\n2014-11-27), though that commit does not hint at why the functions were\ndefined with that attribute.\n\nHowever (at least with GCC 12.2.0, at the time of writing), the\nresulting pack-bitmap.o contains the same instructions with or without\nthe inline attribute applied to these functions:\n\n    $ make O=3 pack-bitmap.o && mv pack-bitmap.o{,.before}\n    [ apply this patch ]\n    $ make O=3 pack-bitmap.o && mv pack-bitmap.o{,.after}\n    $ objdump -d pack-bitmap.o.before >before\n    $ objdump -d pack-bitmap.o.after >after\n    $ diff -u pack-bitmap.o.{before,after}\n    --- before\t2023-03-15 18:54:17.021580095 -0400\n    +++ after\t2023-03-15 18:54:21.853552218 -0400\n    @@ -1,5 +1,5 @@\n\n    -pack-bitmap.o.before:     file format elf64-x86-64\n    +pack-bitmap.o.after:     file format elf64-x86-64\n\n     Disassembly of section .text:\n\nSo defining these functions as inline is at best a noop, and at worst\nconfuses the reader into thinking that there is some trickier reason\nthat they are defined as inline when there isn't.\n\nSince this pair of functions does not need to be inlined, let's drop\nthat attribute from both `read_u8()` and `read_be32()`.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n pack-bitmap.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 794aaf5b02..1d12f90ff9 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -244,14 +244,14 @@ static struct stored_bitmap *store_bitmap(struct bitmap_index *index,\n \treturn stored;\n }\n \n-static inline uint32_t read_be32(struct bitmap_index *bitmap_git)\n+static uint32_t read_be32(struct bitmap_index *bitmap_git)\n {\n \tuint32_t result = get_be32(bitmap_git->map + bitmap_git->map_pos);\n \tbitmap_git->map_pos += sizeof(result);\n \treturn result;\n }\n \n-static inline uint8_t read_u8(struct bitmap_index *bitmap_git)\n+static uint8_t read_u8(struct bitmap_index *bitmap_git)\n {\n \treturn bitmap_git->map[bitmap_git->map_pos++];\n }\n-- \n2.40.0.77.gd564125b3f\n\n"},{"id":"473806","messageId":"0decf13869df6216914044a560d94968126836f4.1679342296.git.me@ttaylorr.com","threadId":"59424","inReplyTo":"cover.1679342296.git.me@ttaylorr.com","subject":"[PATCH 4/6] pack-bitmap.c: factor out manual `map_pos` manipulation","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-03-20T20:02:49Z","receivedAt":"2023-03-20T20:03:27Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The pack-bitmap internals use a combination of a `map` and `map_pos`\nvariable to keep a pointer to a memory mapped region of the `.bitmap`\nfile, as well as the position within that file, respectively.\n\nReads within the memory mapped region are meant to mimic file reads,\nwhere each read adjusts the offset of the file descriptor accordingly.\nThis functionality is implemented by adjusting the `map_pos` variable\nafter each read.\n\nFactor out a function similar to seek() which adjusts the `map_pos`\nvariable for us. Since the bitmap code only needs to adjust relative to\nthe beginning of the file as well as the current position, we do not\nneed to support any \"whence\" values outside of SEEK_SET and SEEK_CUR.\n\nExtracting out this operation into a separate function allows us to add\nsome additional safety checks, such as ensuring that adding to `map_pos`\ndoes not overflow `size_t`, and that the resulting `map_pos` value is\nin bounds of the mapped region.\n\nThe subsequent commit will rewrite all manual manipulation of the\n`map_pos` variable in terms of the new `bitmap_index_seek()`.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n pack-bitmap.c | 22 ++++++++++++++++++++++\n 1 file changed, 22 insertions(+)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 1d12f90ff9..fabcf01c14 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -134,6 +134,28 @@ static struct ewah_bitmap *lookup_stored_bitmap(struct stored_bitmap *st)\n \treturn composed;\n }\n \n+static size_t bitmap_index_seek(struct bitmap_index *bitmap_git, size_t offset,\n+\t\t\t\tint whence)\n+{\n+\tswitch (whence) {\n+\tcase SEEK_SET:\n+\t\tbitmap_git->map_pos = offset;\n+\t\tbreak;\n+\tcase SEEK_CUR:\n+\t\tbitmap_git->map_pos = st_add(bitmap_git->map_pos, offset);\n+\t\tbreak;\n+\tdefault:\n+\t\tBUG(\"unhandled seek whence: %d\", whence);\n+\t}\n+\n+\tif (bitmap_git->map_pos >= bitmap_git->map_size)\n+\t\tBUG(\"bitmap position exceeds size (%\"PRIuMAX\" >= %\"PRIuMAX\")\",\n+\t\t    (uintmax_t)bitmap_git->map_pos,\n+\t\t    (uintmax_t)bitmap_git->map_size);\n+\n+\treturn bitmap_git->map_pos;\n+}\n+\n /*\n  * Read a bitmap from the current read position on the mmaped\n  * index, and increase the read position accordingly\n-- \n2.40.0.77.gd564125b3f\n\n"},{"id":"473807","messageId":"9a3e45b78b7810e0116848f1de80096b04285a55.1679342296.git.me@ttaylorr.com","threadId":"59424","inReplyTo":"cover.1679342296.git.me@ttaylorr.com","subject":"[PATCH 5/6] pack-bitmap.c: use `bitmap_index_seek()` where possible","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-03-20T20:02:52Z","receivedAt":"2023-03-20T20:03:29Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"As described in the previous commit, now that we have a functional\n`bitmap_index_seek()`, rewrite all callers that manually manipulate the\n`map_pos` variable with calls to `bitmap_index_seek()`.\n\nThis means that all callers that adjust the value of `map_pos` have\nthose changes automatically bounds- and overflow-checked.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n pack-bitmap.c | 21 +++++++++++++--------\n 1 file changed, 13 insertions(+), 8 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex fabcf01c14..38a3c6a3f9 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -174,7 +174,7 @@ static struct ewah_bitmap *read_bitmap_1(struct bitmap_index *index)\n \t\treturn NULL;\n \t}\n \n-\tindex->map_pos += bitmap_size;\n+\tbitmap_index_seek(index, bitmap_size, SEEK_CUR);\n \treturn b;\n }\n \n@@ -230,7 +230,7 @@ static int load_bitmap_header(struct bitmap_index *index)\n \n \tindex->entry_count = ntohl(header->entry_count);\n \tindex->checksum = header->checksum;\n-\tindex->map_pos += header_size;\n+\tbitmap_index_seek(index, header_size, SEEK_CUR);\n \treturn 0;\n }\n \n@@ -269,13 +269,15 @@ static struct stored_bitmap *store_bitmap(struct bitmap_index *index,\n static uint32_t read_be32(struct bitmap_index *bitmap_git)\n {\n \tuint32_t result = get_be32(bitmap_git->map + bitmap_git->map_pos);\n-\tbitmap_git->map_pos += sizeof(result);\n+\tbitmap_index_seek(bitmap_git, sizeof(uint32_t), SEEK_CUR);\n \treturn result;\n }\n \n static uint8_t read_u8(struct bitmap_index *bitmap_git)\n {\n-\treturn bitmap_git->map[bitmap_git->map_pos++];\n+\tuint8_t result = bitmap_git->map[bitmap_git->map_pos];\n+\tbitmap_index_seek(bitmap_git, sizeof(uint8_t), SEEK_CUR);\n+\treturn result;\n }\n \n #define MAX_XOR_OFFSET 160\n@@ -794,14 +796,16 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n \n \twhile (xor_items_nr) {\n \t\txor_item = &xor_items[xor_items_nr - 1];\n-\t\tbitmap_git->map_pos = xor_item->offset;\n+\t\tbitmap_index_seek(bitmap_git, xor_item->offset, SEEK_SET);\n+\n \t\tif (bitmap_git->map_size - bitmap_git->map_pos < bitmap_header_size) {\n \t\t\terror(_(\"corrupt ewah bitmap: truncated header for bitmap of commit \\\"%s\\\"\"),\n \t\t\t\toid_to_hex(&xor_item->oid));\n \t\t\tgoto corrupt;\n \t\t}\n \n-\t\tbitmap_git->map_pos += sizeof(uint32_t) + sizeof(uint8_t);\n+\t\tbitmap_index_seek(bitmap_git,\n+\t\t\t\t  sizeof(uint32_t) + sizeof(uint8_t), SEEK_CUR);\n \t\txor_flags = read_u8(bitmap_git);\n \t\tbitmap = read_bitmap_1(bitmap_git);\n \n@@ -812,7 +816,7 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n \t\txor_items_nr--;\n \t}\n \n-\tbitmap_git->map_pos = offset;\n+\tbitmap_index_seek(bitmap_git, offset, SEEK_SET);\n \tif (bitmap_git->map_size - bitmap_git->map_pos < bitmap_header_size) {\n \t\terror(_(\"corrupt ewah bitmap: truncated header for bitmap of commit \\\"%s\\\"\"),\n \t\t\toid_to_hex(oid));\n@@ -842,7 +846,8 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n \t * Instead, we can skip ahead and immediately read the flags and\n \t * ewah bitmap.\n \t */\n-\tbitmap_git->map_pos += sizeof(uint32_t) + sizeof(uint8_t);\n+\tbitmap_index_seek(bitmap_git, sizeof(uint32_t) + sizeof(uint8_t),\n+\t\t\t  SEEK_CUR);\n \tflags = read_u8(bitmap_git);\n \tbitmap = read_bitmap_1(bitmap_git);\n \n-- \n2.40.0.77.gd564125b3f\n\n"},{"id":"473808","messageId":"82049ed09e1695db644d1d4cf17557214e54dcea.1679342296.git.me@ttaylorr.com","threadId":"59424","inReplyTo":"cover.1679342296.git.me@ttaylorr.com","subject":"[PATCH 6/6] pack-bitmap.c: factor out `bitmap_index_seek_commit()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-03-20T20:02:55Z","receivedAt":"2023-03-20T20:03:31Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Factor out a common pattern within `lazy_bitmap_for_commit()` where we\nseek to a given position (expecting to read the start of an individual\nbitmap entry).\n\nBoth spots within `lazy_bitmap_for_commit()` emit a common error, so\nfactor out the whole routine into its own function to DRY things up a\nlittle.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n pack-bitmap.c | 29 ++++++++++++++++++-----------\n 1 file changed, 18 insertions(+), 11 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 38a3c6a3f9..9859f61a5a 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -156,6 +156,21 @@ static size_t bitmap_index_seek(struct bitmap_index *bitmap_git, size_t offset,\n \treturn bitmap_git->map_pos;\n }\n \n+static int bitmap_index_seek_commit(struct bitmap_index *bitmap_git,\n+\t\t\t\t     struct object_id *oid,\n+\t\t\t\t     size_t pos)\n+{\n+\tconst int bitmap_header_size = 6;\n+\n+\tbitmap_index_seek(bitmap_git, pos, SEEK_SET);\n+\n+\tif (bitmap_git->map_size - bitmap_git->map_pos < bitmap_header_size)\n+\t\treturn error(_(\"corrupt ewah bitmap: truncated header for \"\n+\t\t\t       \"bitmap of commit \\\"%s\\\"\"),\n+\t\t\toid_to_hex(oid));\n+\treturn 0;\n+}\n+\n /*\n  * Read a bitmap from the current read position on the mmaped\n  * index, and increase the read position accordingly\n@@ -737,7 +752,6 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n \tstruct object_id *oid = &commit->object.oid;\n \tstruct ewah_bitmap *bitmap;\n \tstruct stored_bitmap *xor_bitmap = NULL;\n-\tconst int bitmap_header_size = 6;\n \tstatic struct bitmap_lookup_table_xor_item *xor_items = NULL;\n \tstatic size_t xor_items_nr = 0, xor_items_alloc = 0;\n \tstatic int is_corrupt = 0;\n@@ -796,13 +810,10 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n \n \twhile (xor_items_nr) {\n \t\txor_item = &xor_items[xor_items_nr - 1];\n-\t\tbitmap_index_seek(bitmap_git, xor_item->offset, SEEK_SET);\n \n-\t\tif (bitmap_git->map_size - bitmap_git->map_pos < bitmap_header_size) {\n-\t\t\terror(_(\"corrupt ewah bitmap: truncated header for bitmap of commit \\\"%s\\\"\"),\n-\t\t\t\toid_to_hex(&xor_item->oid));\n+\t\tif (bitmap_index_seek_commit(bitmap_git, &xor_item->oid,\n+\t\t\t\t\t     xor_item->offset) < 0)\n \t\t\tgoto corrupt;\n-\t\t}\n \n \t\tbitmap_index_seek(bitmap_git,\n \t\t\t\t  sizeof(uint32_t) + sizeof(uint8_t), SEEK_CUR);\n@@ -816,12 +827,8 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n \t\txor_items_nr--;\n \t}\n \n-\tbitmap_index_seek(bitmap_git, offset, SEEK_SET);\n-\tif (bitmap_git->map_size - bitmap_git->map_pos < bitmap_header_size) {\n-\t\terror(_(\"corrupt ewah bitmap: truncated header for bitmap of commit \\\"%s\\\"\"),\n-\t\t\toid_to_hex(oid));\n+\tif (bitmap_index_seek_commit(bitmap_git, oid, offset) < 0)\n \t\tgoto corrupt;\n-\t}\n \n \t/*\n \t * Don't bother reading the commit's index position or its xor\n-- \n2.40.0.77.gd564125b3f\n"},{"id":"473856","messageId":"20230321173556.GD3119834@coredump.intra.peff.net","threadId":"59424","inReplyTo":"8c70c94a3f5f0463041dbaaa542fd69bd159428b.1679342296.git.me@ttaylorr.com","subject":"Re: [PATCH 1/6] pack-bitmap.c: hide bitmap internals in `read_u8()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-21T17:35:56Z","receivedAt":"2023-03-21T17:38:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 20, 2023 at 04:02:40PM -0400, Taylor Blau wrote:\n\n> So b5007211b6 got rid of the above convention and replaced it by reading\n> individual fields of that structure with a `read_u8()` helper that reads\n> from the region of memory pointed to by `->map`, and updates the\n> `->map_pos` pointer accordingly.\n> \n> But this forces callers to be intimately aware of `bitmap_git->map` and\n> `bitmap_git->map_pos`. Instead, teach `read_u8()` to take a `struct\n> bitmap_index *` directly, and avoid having callers deal with the\n> internals themselves.\n\nOK. I always felt like this read_u8() and read_be32() were trying to\nmatch get_be32(), etc, just with an auto-incrementing \"pos\" pointer. And\nthis patch makes them a lot less generic.\n\nBut that is probably OK. They are static-local to the bitmap file, and\nwe have not found another caller who wanted them in the intervening\nyears. Arguably they could be given more descriptive names, like\nread_bitmap_u8() or something, but again, being static-local to the\nfile, the generic names are fine.\n\n-Peff\n"},{"id":"473857","messageId":"20230321174033.GE3119834@coredump.intra.peff.net","threadId":"59424","inReplyTo":"2e3e3a7145a5851fcf5c485b38d14344c9b824d7.1679342296.git.me@ttaylorr.com","subject":"Re: [PATCH 3/6] pack-bitmap.c: drop unnecessary 'inline's","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-21T17:40:33Z","receivedAt":"2023-03-21T17:41:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 20, 2023 at 04:02:46PM -0400, Taylor Blau wrote:\n\n> Both `read_be32()` and `read_u8()` are defined as inline dating back\n> to b5007211b6 (pack-bitmap: do not use gcc packed attribute,\n> 2014-11-27), though that commit does not hint at why the functions were\n> defined with that attribute.\n\nI think any non-header inline like this can implicitly be assumed to be\n\"I thought it would make things faster\". ;)\n\n> However (at least with GCC 12.2.0, at the time of writing), the\n> resulting pack-bitmap.o contains the same instructions with or without\n> the inline attribute applied to these functions:\n> \n>     $ make O=3 pack-bitmap.o && mv pack-bitmap.o{,.before}\n>     [ apply this patch ]\n>     $ make O=3 pack-bitmap.o && mv pack-bitmap.o{,.after}\n>     $ objdump -d pack-bitmap.o.before >before\n>     $ objdump -d pack-bitmap.o.after >after\n>     $ diff -u pack-bitmap.o.{before,after}\n>     --- before\t2023-03-15 18:54:17.021580095 -0400\n>     +++ after\t2023-03-15 18:54:21.853552218 -0400\n>     @@ -1,5 +1,5 @@\n> \n>     -pack-bitmap.o.before:     file format elf64-x86-64\n>     +pack-bitmap.o.after:     file format elf64-x86-64\n> \n>      Disassembly of section .text:\n> \n> So defining these functions as inline is at best a noop, and at worst\n> confuses the reader into thinking that there is some trickier reason\n> that they are defined as inline when there isn't.\n\nNice digging. The \"inline\" is really just a hint to the compiler here,\nand obviously it does not need that hint. I do wonder if that is still\ntrue after you make them more complicated in a later patch in the\nseries.\n\nOn the other hand, I doubt that these need to be very optimized at all.\nIf there were a tight loop of single-byte reads, the function overhead\nmight be noticeable. But generally we're reading only a few items from\nthe beginning of each entry, and then reading the bulk of the data via\newah_read_mmap().\n\nSo I think the overall argument is \"let the compiler decide what is good\nto inline and what is not\".\n\n-Peff\n"},{"id":"473862","messageId":"20230321175612.GF3119834@coredump.intra.peff.net","threadId":"59424","inReplyTo":"0decf13869df6216914044a560d94968126836f4.1679342296.git.me@ttaylorr.com","subject":"Re: [PATCH 4/6] pack-bitmap.c: factor out manual `map_pos` manipulation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-21T17:56:12Z","receivedAt":"2023-03-21T17:56:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 20, 2023 at 04:02:49PM -0400, Taylor Blau wrote:\n\n> The pack-bitmap internals use a combination of a `map` and `map_pos`\n> variable to keep a pointer to a memory mapped region of the `.bitmap`\n> file, as well as the position within that file, respectively.\n> \n> Reads within the memory mapped region are meant to mimic file reads,\n> where each read adjusts the offset of the file descriptor accordingly.\n> This functionality is implemented by adjusting the `map_pos` variable\n> after each read.\n> \n> Factor out a function similar to seek() which adjusts the `map_pos`\n> variable for us. Since the bitmap code only needs to adjust relative to\n> the beginning of the file as well as the current position, we do not\n> need to support any \"whence\" values outside of SEEK_SET and SEEK_CUR.\n\nI like the idea of centralizing the bounds checks here.\n\nI do think copying lseek() is a little awkward, though. As you note, we\nonly care about SEEK_SET and SEEK_CUR, and we have to BUG() on anything\nelse. Which means we have a run-time check, rather than a compile time\ncheck. If we had two functions like:\n\n  void bitmap_index_seek_to(struct bitmap_index *bitmap_git, size_t pos)\n  {\n\tbitmap_git->map_pos = pos;\n\tif (bitmap_git->map_pos >= bitmap_git->map_size)\n\t   ...etc...\n  }\n\n  void bitmap_index_incr(struct bitmap_index *bitmap_git, size_t offset)\n  {\n\tbitmap_index_seek_to(bitmap_git, st_add(bitmap_git->map_pos, offset));\n  }\n\nthen the compiler can see all cases directly. I dunno how much it\nmatters.\n\nThe other thing that's interesting from a bounds-checking perspective is\nthat you're checking the seek _after_ you've read an item. Which has two\nimplications:\n\n  - I don't think we could ever overflow size_t here. We are working\n    with a buffer that we got from mmap(), so it's already probably\n    bounded to some much smaller subset of size_t. And even if we\n    imagine that you really could get a buffer that stretches for the\n    whole of the memory space, and that incrementing it by 4 bytes would\n    overflow, we'd by definition have just overflowed the memory space\n    itself by reading 4 bytes (and presumably segfaulted). So I really\n    doubt this st_add() is doing anything.\n\n  - The more interesting case is that we are not overflowing size_t, but\n    simply walking past bitmap_git->map_size. But again, we are reading\n    first and then seeking. So if our seek does go past, then by\n    definition we just read garbage bytes, which is undefined behavior.\n\n    For a bounds-check, wouldn't we want it the other way around? To\n    make sure we have 4 bytes available, and then if so read them and\n    increment the offset?\n\n> +\tif (bitmap_git->map_pos >= bitmap_git->map_size)\n> +\t\tBUG(\"bitmap position exceeds size (%\"PRIuMAX\" >= %\"PRIuMAX\")\",\n> +\t\t    (uintmax_t)bitmap_git->map_pos,\n> +\t\t    (uintmax_t)bitmap_git->map_size);\n\nThis uses \">=\", which is good, because it is valid to walk the pointer\nto one past the end of the map, which is effectively EOF. But as above,\nin that condition the callers should be checking for this EOF state\nbefore reading the bytes.\n\n-Peff\n"},{"id":"473863","messageId":"20230321180525.GG3119834@coredump.intra.peff.net","threadId":"59424","inReplyTo":"9a3e45b78b7810e0116848f1de80096b04285a55.1679342296.git.me@ttaylorr.com","subject":"Re: [PATCH 5/6] pack-bitmap.c: use `bitmap_index_seek()` where possible","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-21T18:05:25Z","receivedAt":"2023-03-21T18:05:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 20, 2023 at 04:02:52PM -0400, Taylor Blau wrote:\n\n> --- a/pack-bitmap.c\n> +++ b/pack-bitmap.c\n> @@ -174,7 +174,7 @@ static struct ewah_bitmap *read_bitmap_1(struct bitmap_index *index)\n>  \t\treturn NULL;\n>  \t}\n>  \n> -\tindex->map_pos += bitmap_size;\n> +\tbitmap_index_seek(index, bitmap_size, SEEK_CUR);\n>  \treturn b;\n\nAs an aside, I notice none of the callers here or in the next patch\ncheck the return value of bitmap_index_seek(). I guess you included it\nto match the return value of lseek(), but maybe it is better to return\nvoid if nobody is looking at it.\n\nBut getting back to the bounds-checking: I think we are already\ncorrectly bounds-checked here, because ewah_read_mmap() will make sure\nthat it doesn't read too far (and will return an error if there's\ntruncation). And if it didn't, this check-on-seek doesn't help us,\nbecause we'll already have done out-of-bounds reads.\n\n> @@ -230,7 +230,7 @@ static int load_bitmap_header(struct bitmap_index *index)\n>  \n>  \tindex->entry_count = ntohl(header->entry_count);\n>  \tindex->checksum = header->checksum;\n> -\tindex->map_pos += header_size;\n> +\tbitmap_index_seek(index, header_size, SEEK_CUR);\n>  \treturn 0;\n>  }\n\nLikewise this function already has bounds checks at the top:\n\n\tif (index->map_size < header_size + the_hash_algo->rawsz)\n\t\treturn error(_(\"corrupted bitmap index (too small)\"));\n\nI'd be perfectly happy if we swapped that our for checking the bounds on\nindividual reads, but the extra checking in the seek step here just\nseems redundant (and again, too late).\n\n> @@ -269,13 +269,15 @@ static struct stored_bitmap *store_bitmap(struct bitmap_index *index,\n>  static uint32_t read_be32(struct bitmap_index *bitmap_git)\n>  {\n>  \tuint32_t result = get_be32(bitmap_git->map + bitmap_git->map_pos);\n> -\tbitmap_git->map_pos += sizeof(result);\n> +\tbitmap_index_seek(bitmap_git, sizeof(uint32_t), SEEK_CUR);\n>  \treturn result;\n>  }\n\nThe function doesn't do bounds-checks itself, but the callers do, like:\n\n                if (index->map_size - index->map_pos < 6)\n                        return error(_(\"corrupt ewah bitmap: truncated header for entry %d\"), i);\n\n                commit_idx_pos = read_be32(index->map, &index->map_pos);\n                xor_offset = read_u8(index->map, &index->map_pos);\n                flags = read_u8(index->map, &index->map_pos);\n\n(and again, I'd be happy to see this magic \"6\" go away in favor of\nchecking as we read each item).\n\nMaybe I'm misunderstanding the purpose of your series. I thought it was\nto avoid reading out of bounds. But since bitmap_index_seek() triggers a\nBUG(), it's not good for detecting truncated files, etc. So is it really\njust meant to be a belt-and-suspenders check on the existing\nbounds-checks? I guess that makes more sense with the way it is written,\nbut I'm just a little skeptical that it's really useful.\n\n-Peff\n"},{"id":"473865","messageId":"20230321181315.GH3119834@coredump.intra.peff.net","threadId":"59424","inReplyTo":"82049ed09e1695db644d1d4cf17557214e54dcea.1679342296.git.me@ttaylorr.com","subject":"Re: [PATCH 6/6] pack-bitmap.c: factor out `bitmap_index_seek_commit()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-21T18:13:15Z","receivedAt":"2023-03-21T18:13:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 20, 2023 at 04:02:55PM -0400, Taylor Blau wrote:\n\n> Factor out a common pattern within `lazy_bitmap_for_commit()` where we\n> seek to a given position (expecting to read the start of an individual\n> bitmap entry).\n> \n> Both spots within `lazy_bitmap_for_commit()` emit a common error, so\n> factor out the whole routine into its own function to DRY things up a\n> little.\n\nOK, so this case makes more sense to me as a bounds-check, because we\nare seeking to an arbitrary offset.\n\nBut...\n\n> +static int bitmap_index_seek_commit(struct bitmap_index *bitmap_git,\n> +\t\t\t\t     struct object_id *oid,\n> +\t\t\t\t     size_t pos)\n> +{\n> +\tconst int bitmap_header_size = 6;\n> +\n> +\tbitmap_index_seek(bitmap_git, pos, SEEK_SET);\n> +\n> +\tif (bitmap_git->map_size - bitmap_git->map_pos < bitmap_header_size)\n> +\t\treturn error(_(\"corrupt ewah bitmap: truncated header for \"\n> +\t\t\t       \"bitmap of commit \\\"%s\\\"\"),\n> +\t\t\toid_to_hex(oid));\n> +\treturn 0;\n> +}\n\nSo here we seek to the position, and then make sure we have 6 bytes to\nread, which makes sense. But in the seek step, are we worried that we\nwill seek to somewhere bogus? If so, we'll get a BUG(). But is that the\nright thing if somebody feeds a bogus \"pos\" that moves past truncation?\n\nI'm not 100% sure on where these offsets come from. But it looks like\nthey're coming from the bitmap lookup table. In which case a bogus value\nthere should be an error(), and not a BUG(), I would think.\n\n-Peff\n"},{"id":"473866","messageId":"ZBn0iFE5qOJKVeel@nand.local","threadId":"59424","inReplyTo":"20230321181315.GH3119834@coredump.intra.peff.net","subject":"Re: [PATCH 6/6] pack-bitmap.c: factor out `bitmap_index_seek_commit()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-03-21T18:16:40Z","receivedAt":"2023-03-21T18:16:48Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Mar 21, 2023 at 02:13:15PM -0400, Jeff King wrote:\n> I'm not 100% sure on where these offsets come from. But it looks like\n> they're coming from the bitmap lookup table. In which case a bogus value\n> there should be an error(), and not a BUG(), I would think.\n\nThey do come from the lookup table, yes. I'm not sure that I agree that\nbogus values here should be an error() or a BUG(), or if I even have a\nstrong preference between one and the other.\n\nBut I do think that trying to make it an error() makes it awkward for\nall of the other callers that want it to be a BUG(), since the detail of\nwhether to call one or the other is private to bitmap_index_seek().\n\nWe *could* open-code it, introduce a variant of bitmap_index_seek(),\nmake it take an additional parameter specifying whether to call one over\nthe other, *or* check the bounds ourselves before even calling\nbitmap_index_seek().\n\nBut none of those seem like great options to me, TBH. I would be just as\nhappy to leave this as a BUG(), to be honest.\n\nThanks,\nTaylor\n"},{"id":"473871","messageId":"20230321182732.GK3119834@coredump.intra.peff.net","threadId":"59424","inReplyTo":"ZBn0iFE5qOJKVeel@nand.local","subject":"Re: [PATCH 6/6] pack-bitmap.c: factor out `bitmap_index_seek_commit()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-21T18:27:32Z","receivedAt":"2023-03-21T18:28:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 21, 2023 at 02:16:40PM -0400, Taylor Blau wrote:\n\n> On Tue, Mar 21, 2023 at 02:13:15PM -0400, Jeff King wrote:\n> > I'm not 100% sure on where these offsets come from. But it looks like\n> > they're coming from the bitmap lookup table. In which case a bogus value\n> > there should be an error(), and not a BUG(), I would think.\n> \n> They do come from the lookup table, yes. I'm not sure that I agree that\n> bogus values here should be an error() or a BUG(), or if I even have a\n> strong preference between one and the other.\n\nThe usual philosophy we've applied is: a BUG() should not be\ntrigger-able, even if Git is fed bad data. A BUG() should indicate an\nerror in the program logic, and if we see one, there should be a code\nfix that handles the case.\n\nWhereas if I understand this correctly, if I corrupt the bitmap file on\ndisk, we'd trigger this BUG().\n\nIn many cases I think one could argue that it's kind of academic. But in\nthis case we should be able to say \"oops, the bitmap file seems corrupt\"\nand skip using it, rather than bailing completely from the process.\n\n> But I do think that trying to make it an error() makes it awkward for\n> all of the other callers that want it to be a BUG(), since the detail of\n> whether to call one or the other is private to bitmap_index_seek().\n>\n> We *could* open-code it, introduce a variant of bitmap_index_seek(),\n> make it take an additional parameter specifying whether to call one over\n> the other, *or* check the bounds ourselves before even calling\n> bitmap_index_seek().\n\nI'm mostly unconvinced of the value of bitmap_index_seek() doing\nchecking at all, because it is too late in most of the cases. In fact it\nis only in this case that it is doing something useful, which makes me\nthink that the check should be open-coded here.\n\n-Peff\n"},{"id":"474104","messageId":"9e043d78-a225-4b6a-4567-83ac27b022b2@github.com","threadId":"59424","inReplyTo":"20230321173556.GD3119834@coredump.intra.peff.net","subject":"Re: [PATCH 1/6] pack-bitmap.c: hide bitmap internals in `read_u8()`","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-03-24T17:52:15Z","receivedAt":"2023-03-24T17:53:37Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/21/2023 1:35 PM, Jeff King wrote:\n> On Mon, Mar 20, 2023 at 04:02:40PM -0400, Taylor Blau wrote:\n> \n>> So b5007211b6 got rid of the above convention and replaced it by reading\n>> individual fields of that structure with a `read_u8()` helper that reads\n>> from the region of memory pointed to by `->map`, and updates the\n>> `->map_pos` pointer accordingly.\n>>\n>> But this forces callers to be intimately aware of `bitmap_git->map` and\n>> `bitmap_git->map_pos`. Instead, teach `read_u8()` to take a `struct\n>> bitmap_index *` directly, and avoid having callers deal with the\n>> internals themselves.\n> \n> OK. I always felt like this read_u8() and read_be32() were trying to\n> match get_be32(), etc, just with an auto-incrementing \"pos\" pointer. And\n> this patch makes them a lot less generic.\n> \n> But that is probably OK. They are static-local to the bitmap file, and\n> we have not found another caller who wanted them in the intervening\n> years. Arguably they could be given more descriptive names, like\n> read_bitmap_u8() or something, but again, being static-local to the\n> file, the generic names are fine.\n\nSince the names don't match the \"get\" of get_be32(), we don't run the\nrisk of a future public get_be8() colliding with these static-local\nmethods, so I agree.\n\nThanks,\n-Stolee\n\n"},{"id":"474105","messageId":"61ae4ad5-4d4d-933c-a2cb-e7e2cd734401@github.com","threadId":"59424","inReplyTo":"20230321175612.GF3119834@coredump.intra.peff.net","subject":"Re: [PATCH 4/6] pack-bitmap.c: factor out manual `map_pos` manipulation","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-03-24T18:04:05Z","receivedAt":"2023-03-24T18:06:20Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/21/2023 1:56 PM, Jeff King wrote:\n> On Mon, Mar 20, 2023 at 04:02:49PM -0400, Taylor Blau wrote:\n> \n>> The pack-bitmap internals use a combination of a `map` and `map_pos`\n>> variable to keep a pointer to a memory mapped region of the `.bitmap`\n>> file, as well as the position within that file, respectively.\n>>\n>> Reads within the memory mapped region are meant to mimic file reads,\n>> where each read adjusts the offset of the file descriptor accordingly.\n>> This functionality is implemented by adjusting the `map_pos` variable\n>> after each read.\n>>\n>> Factor out a function similar to seek() which adjusts the `map_pos`\n>> variable for us. Since the bitmap code only needs to adjust relative to\n>> the beginning of the file as well as the current position, we do not\n>> need to support any \"whence\" values outside of SEEK_SET and SEEK_CUR.\n> \n> I like the idea of centralizing the bounds checks here.\n> \n> I do think copying lseek() is a little awkward, though. As you note, we\n> only care about SEEK_SET and SEEK_CUR, and we have to BUG() on anything\n> else. Which means we have a run-time check, rather than a compile time\n> check. If we had two functions like:\n> \n>   void bitmap_index_seek_to(struct bitmap_index *bitmap_git, size_t pos)\n>   {\n> \tbitmap_git->map_pos = pos;\n> \tif (bitmap_git->map_pos >= bitmap_git->map_size)\n> \t   ...etc...\n>   }\n> \n>   void bitmap_index_incr(struct bitmap_index *bitmap_git, size_t offset)\n>   {\n> \tbitmap_index_seek_to(bitmap_git, st_add(bitmap_git->map_pos, offset));\n>   }\n> \n> then the compiler can see all cases directly. I dunno how much it\n> matters.\n\nWhenever the compiler can help us, I'm usually in favor.\n\nIn this case, I'd call them bitmap_index_seek() and bitmap_index_increment(),\nwhich should be clear enough.\n\nThe other alternative would be to use an enum instead of an arbitrary int.\nThe compiler can warn to some extent, but it's not as helpful as a method\nname distinction.\n\n>> +\tif (bitmap_git->map_pos >= bitmap_git->map_size)\n>> +\t\tBUG(\"bitmap position exceeds size (%\"PRIuMAX\" >= %\"PRIuMAX\")\",\n>> +\t\t    (uintmax_t)bitmap_git->map_pos,\n>> +\t\t    (uintmax_t)bitmap_git->map_size);\n> \n> This uses \">=\", which is good, because it is valid to walk the pointer\n> to one past the end of the map, which is effectively EOF. But as above,\n> in that condition the callers should be checking for this EOF state\n> before reading the bytes.\n\nIn other words, it would be too easy for a strange data shape to trigger\nthis BUG() statement, which should be reserved for the most-extreme cases\nof developer mistake. Things like \"this is an unacceptable 'whence' value\"\nor \"this should never be NULL\" make sense, but this is too likely to be\nhit due to a runtime error.\n\nWould it make sense to return an 'int' instead of the size_t of map_pos?\nThat way we could return in error if this is exceeded, and then all\ncallers can respond \"oh wait, that move would exceed the file size, so\nI should fail in my own way...\"?\n\nThanks,\n-Stolee\n"},{"id":"474106","messageId":"8a7cf287-421c-9a7f-0788-c5e219e6a8e3@github.com","threadId":"59424","inReplyTo":"20230321180525.GG3119834@coredump.intra.peff.net","subject":"Re: [PATCH 5/6] pack-bitmap.c: use `bitmap_index_seek()` where possible","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-03-24T18:06:43Z","receivedAt":"2023-03-24T18:08:05Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/21/2023 2:05 PM, Jeff King wrote:\n> On Mon, Mar 20, 2023 at 04:02:52PM -0400, Taylor Blau wrote:\n> \n>> --- a/pack-bitmap.c\n>> +++ b/pack-bitmap.c\n>> @@ -174,7 +174,7 @@ static struct ewah_bitmap *read_bitmap_1(struct bitmap_index *index)\n>>  \t\treturn NULL;\n>>  \t}\n>>  \n>> -\tindex->map_pos += bitmap_size;\n>> +\tbitmap_index_seek(index, bitmap_size, SEEK_CUR);\n>>  \treturn b;\n> \n> As an aside, I notice none of the callers here or in the next patch\n> check the return value of bitmap_index_seek(). I guess you included it\n> to match the return value of lseek(), but maybe it is better to return\n> void if nobody is looking at it.\n> \n> But getting back to the bounds-checking: I think we are already\n> correctly bounds-checked here, because ewah_read_mmap() will make sure\n> that it doesn't read too far (and will return an error if there's\n> truncation). And if it didn't, this check-on-seek doesn't help us,\n> because we'll already have done out-of-bounds reads.\n\n>> +\tbitmap_index_seek(index, header_size, SEEK_CUR);\n>>  \treturn 0;\n>>  }\n> \n> Likewise this function already has bounds checks at the top:\n> \n> \tif (index->map_size < header_size + the_hash_algo->rawsz)\n> \t\treturn error(_(\"corrupted bitmap index (too small)\"));\n> \n> I'd be perfectly happy if we swapped that our for checking the bounds on\n> individual reads, but the extra checking in the seek step here just\n> seems redundant (and again, too late).\n\nI think it would be nice to replace all of these custom bounds\nchecks with a check within bitmap_index_seek() and error conditions\ndone in response to an error code returned by that method. It keeps\nthe code more consistent in the potential future of changing the\namount to move the map_pos and the amount checked in these conditions.\n\nThanks,\n-Stolee\n"},{"id":"474107","messageId":"2ac96251-1b91-3470-b0a1-cbe8fa0cc4bb@github.com","threadId":"59424","inReplyTo":"20230321182732.GK3119834@coredump.intra.peff.net","subject":"Re: [PATCH 6/6] pack-bitmap.c: factor out `bitmap_index_seek_commit()`","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-03-24T18:09:58Z","receivedAt":"2023-03-24T18:10:18Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/21/2023 2:27 PM, Jeff King wrote:\n> On Tue, Mar 21, 2023 at 02:16:40PM -0400, Taylor Blau wrote:\n> \n>> On Tue, Mar 21, 2023 at 02:13:15PM -0400, Jeff King wrote:\n>>> I'm not 100% sure on where these offsets come from. But it looks like\n>>> they're coming from the bitmap lookup table. In which case a bogus value\n>>> there should be an error(), and not a BUG(), I would think.\n>>\n>> They do come from the lookup table, yes. I'm not sure that I agree that\n>> bogus values here should be an error() or a BUG(), or if I even have a\n>> strong preference between one and the other.\n> \n> The usual philosophy we've applied is: a BUG() should not be\n> trigger-able, even if Git is fed bad data. A BUG() should indicate an\n> error in the program logic, and if we see one, there should be a code\n> fix that handles the case.\n> \n> Whereas if I understand this correctly, if I corrupt the bitmap file on\n> disk, we'd trigger this BUG().\n> \n> In many cases I think one could argue that it's kind of academic. But in\n> this case we should be able to say \"oops, the bitmap file seems corrupt\"\n> and skip using it, rather than bailing completely from the process.\n\nIt's not just academic. BUG() statements kill the process without running\nimportant cleanup steps like deleting open .lock files or outputting the\nfinal traces. This can be especially problematic when we count on those\noperations in order to recover a repository from such errors.\n \n>> But I do think that trying to make it an error() makes it awkward for\n>> all of the other callers that want it to be a BUG(), since the detail of\n>> whether to call one or the other is private to bitmap_index_seek().\n>>\n>> We *could* open-code it, introduce a variant of bitmap_index_seek(),\n>> make it take an additional parameter specifying whether to call one over\n>> the other, *or* check the bounds ourselves before even calling\n>> bitmap_index_seek().\n> \n> I'm mostly unconvinced of the value of bitmap_index_seek() doing\n> checking at all, because it is too late in most of the cases. In fact it\n> is only in this case that it is doing something useful, which makes me\n> think that the check should be open-coded here.\n\nIf we universally check whether bitmap_index_seek() works, then there\nis value. It avoids the existing ad-hoc checks in favor of always-on\nchecks (as well as avoiding potential disconnects between the check\nand the seeked position in the future).\n\nThanks,\n-Stolee\n"},{"id":"474110","messageId":"20230324182929.GA536252@coredump.intra.peff.net","threadId":"59424","inReplyTo":"61ae4ad5-4d4d-933c-a2cb-e7e2cd734401@github.com","subject":"Re: [PATCH 4/6] pack-bitmap.c: factor out manual `map_pos` manipulation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-24T18:29:29Z","receivedAt":"2023-03-24T18:29:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 24, 2023 at 02:04:05PM -0400, Derrick Stolee wrote:\n\n> >> +\tif (bitmap_git->map_pos >= bitmap_git->map_size)\n> >> +\t\tBUG(\"bitmap position exceeds size (%\"PRIuMAX\" >= %\"PRIuMAX\")\",\n> >> +\t\t    (uintmax_t)bitmap_git->map_pos,\n> >> +\t\t    (uintmax_t)bitmap_git->map_size);\n> > \n> > This uses \">=\", which is good, because it is valid to walk the pointer\n> > to one past the end of the map, which is effectively EOF. But as above,\n> > in that condition the callers should be checking for this EOF state\n> > before reading the bytes.\n> \n> In other words, it would be too easy for a strange data shape to trigger\n> this BUG() statement, which should be reserved for the most-extreme cases\n> of developer mistake. Things like \"this is an unacceptable 'whence' value\"\n> or \"this should never be NULL\" make sense, but this is too likely to be\n> hit due to a runtime error.\n\nSort of. AFAICT in all of the \"increment\" cases we'll already have done\nbounds-checks, so this really is a BUG() condition. But in that case I\nquestion whether it's even worthwhile. The calling code ends up being\nsomething like:\n\n  /* check that we have enough bytes */\n  if (total - pos < 4)\n\treturn error(...);\n\n  /* read those bytes */\n  get_be32() or whatever...\n\n  /* now advance pos, making sure we...had enough bytes? */\n  bitmap_index_seek(..., 4);\n\nWe know the advance will succeed because we checked ahead of time that\nwe had enough bytes. So it really is a BUG() if we don't, as it would\nindicate somebody missed the earlier check. On the other hand, it is a\nweird spot for an extra check, because by definition we'll have just\nread off the array just before the seek.\n\n> Would it make sense to return an 'int' instead of the size_t of map_pos?\n> That way we could return in error if this is exceeded, and then all\n> callers can respond \"oh wait, that move would exceed the file size, so\n> I should fail in my own way...\"?\n\nYou can die() to avoid returning an error. But given that this is bitmap\ncode, and we can generally complete the operation even if it is broken\n(albeit slower), usually we'd try to return the error up the call chain\n(like bitmap_index_seek_commit() does later in the series). Plus that\nfollows our libification trend of not killing the process in low-level\ncode.\n\nIt does make the callers more complicated, though. If this were\n_replacing_ the existing bounds-checks that might be worth it, but\ncoming after like this, I guess I just don't see it as adding much.\n\nThe case where we _do_ seek directly to a file-provided offset, rather\nthan incrementing, is an important check that this series adds, but that\none should be a die() and not a BUG().\n\n-Peff\n"},{"id":"474111","messageId":"20230324183514.GB536252@coredump.intra.peff.net","threadId":"59424","inReplyTo":"8a7cf287-421c-9a7f-0788-c5e219e6a8e3@github.com","subject":"Re: [PATCH 5/6] pack-bitmap.c: use `bitmap_index_seek()` where possible","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-24T18:35:14Z","receivedAt":"2023-03-24T18:35:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 24, 2023 at 02:06:43PM -0400, Derrick Stolee wrote:\n\n> >> +\tbitmap_index_seek(index, header_size, SEEK_CUR);\n> >>  \treturn 0;\n> >>  }\n> > \n> > Likewise this function already has bounds checks at the top:\n> > \n> > \tif (index->map_size < header_size + the_hash_algo->rawsz)\n> > \t\treturn error(_(\"corrupted bitmap index (too small)\"));\n> > \n> > I'd be perfectly happy if we swapped that our for checking the bounds on\n> > individual reads, but the extra checking in the seek step here just\n> > seems redundant (and again, too late).\n> \n> I think it would be nice to replace all of these custom bounds\n> checks with a check within bitmap_index_seek() and error conditions\n> done in response to an error code returned by that method. It keeps\n> the code more consistent in the potential future of changing the\n> amount to move the map_pos and the amount checked in these conditions.\n\nYeah, that's what I was getting at. But doing it at seek time is too\nlate. We'll have just read off the end of the array.\n\nYou really need an interface more like \"make sure there are N bytes for\nme to read at offset X\". Then you can read and advance past them.\n\nFor individual items where you want to copy the bytes out anyway (like a\nbe32) you can have a nice interface like:\n\n  if (read_be32(bitmap_git, &commit_idx_pos) < 0 ||\n      read_u8(bitmap_git, &xor_offset) < 0 ||\n      read_u8(bitmap_git, &flags) < 0)\n\treturn error(\"truncated bitmap entry\");\n\nBut given that there is only one spot that calls these, that kind of\nrefactoring might not be worth it (right now it just uses the magic\nnumber \"6\" right before grabbing the data).\n\n-Peff\n"},{"id":"474118","messageId":"xmqqr0tedjf1.fsf@gitster.g","threadId":"59424","inReplyTo":"20230324183514.GB536252@coredump.intra.peff.net","subject":"Re: [PATCH 5/6] pack-bitmap.c: use `bitmap_index_seek()` where possible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-24T19:43:46Z","receivedAt":"2023-03-24T19:43:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Fri, Mar 24, 2023 at 02:06:43PM -0400, Derrick Stolee wrote:\n>\n>> >> +\tbitmap_index_seek(index, header_size, SEEK_CUR);\n>> >>  \treturn 0;\n>> >>  }\n>> > \n>> > Likewise this function already has bounds checks at the top:\n>> > \n>> > \tif (index->map_size < header_size + the_hash_algo->rawsz)\n>> > \t\treturn error(_(\"corrupted bitmap index (too small)\"));\n>> > \n>> > I'd be perfectly happy if we swapped that our for checking the bounds on\n>> > individual reads, but the extra checking in the seek step here just\n>> > seems redundant (and again, too late).\n>> \n>> I think it would be nice to replace all of these custom bounds\n>> checks with a check within bitmap_index_seek() and error conditions\n>> done in response to an error code returned by that method. It keeps\n>> the code more consistent in the potential future of changing the\n>> amount to move the map_pos and the amount checked in these conditions.\n>\n> Yeah, that's what I was getting at. But doing it at seek time is too\n> late. We'll have just read off the end of the array.\n\nYup.  You illustrated it nicely in your response for the previous\nstep of the series.  If the typical access pattern is to check, read\nand then advance to the next position, and by the time you are ready\nto advance to the next position, you'd better have done the checking.\n\n> You really need an interface more like \"make sure there are N bytes for\n> me to read at offset X\". Then you can read and advance past them.\n>\n> For individual items where you want to copy the bytes out anyway (like a\n> be32) you can have a nice interface like:\n>\n>   if (read_be32(bitmap_git, &commit_idx_pos) < 0 ||\n>       read_u8(bitmap_git, &xor_offset) < 0 ||\n>       read_u8(bitmap_git, &flags) < 0)\n> \treturn error(\"truncated bitmap entry\");\n\nYeah, that kind of flow reads really well.\n\n> But given that there is only one spot that calls these, that kind of\n> refactoring might not be worth it (right now it just uses the magic\n> number \"6\" right before grabbing the data).\n\nYeah, it seems most of the callers with SEEK_SET are \"I find the\nnext offset from a table and jump there in preparation for doing\nsomething\".  I suspect callers with SEEK_CUR would fit in the\nread_X() pattern better?  From that angle, it smells that the two\nkinds of seek functions may want to be split into two different\nhelpers.\n\nThanks.\n\n\n"},{"id":"474121","messageId":"20230324203737.GA549549@coredump.intra.peff.net","threadId":"59424","inReplyTo":"xmqqr0tedjf1.fsf@gitster.g","subject":"Re: [PATCH 5/6] pack-bitmap.c: use `bitmap_index_seek()` where possible","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-24T20:37:37Z","receivedAt":"2023-03-24T20:37:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 24, 2023 at 12:43:46PM -0700, Junio C Hamano wrote:\n\n> > But given that there is only one spot that calls these, that kind of\n> > refactoring might not be worth it (right now it just uses the magic\n> > number \"6\" right before grabbing the data).\n> \n> Yeah, it seems most of the callers with SEEK_SET are \"I find the\n> next offset from a table and jump there in preparation for doing\n> something\".  I suspect callers with SEEK_CUR would fit in the\n> read_X() pattern better?  From that angle, it smells that the two\n> kinds of seek functions may want to be split into two different\n> helpers.\n\nYes, I think the SEEK_SET cases really do need to be doing more\nchecking. AFAICT they are blindly trusting the offsets in the file\n(which is locally generated, so it's more of a corruption problem than a\nsecurity one, but still). And this series improves that, which is good\n(but I still think it should be a die() and not a BUG()).\n\nThe SEEK_CUR cases in theory could all look like the nice read_be32() I\nshowed earlier, but I think in practice there are a lot of variants\n(skipping read of index_pos, advancing past size given by\newah_read_mmap(), and so on). And the current code, while ugly, does\ngive more specific error messages (e.g., telling on _which_ commit we\nfound the truncated data). So I dunno.\n\nCertainly there could be more consistency in the magic numbers. E.g., in\nthis code:\n\n                if (bitmap_git->map_size - bitmap_git->map_pos < bitmap_header_size) {\n                        error(_(\"corrupt ewah bitmap: truncated header for bitmap of commit \\\"%s\\\"\"),\n                                oid_to_hex(&xor_item->oid));\n                        goto corrupt;\n                }\n\n                bitmap_git->map_pos += sizeof(uint32_t) + sizeof(uint8_t);\n                xor_flags = read_u8(bitmap_git->map, &bitmap_git->map_pos);\n                bitmap = read_bitmap_1(bitmap_git);\n\nThere is an assumption that sizeof(uint32_t) + sizeof(uint8_t) is equal\nto bitmap_header_size - 1. That's not wrong, but it's hard to verify\nthat it's doing the right thing, and it's potentially fragile to changes\n(though such changes seem unlikely).\n\n-Peff\n"},{"id":"474133","messageId":"xmqqcz4xesom.fsf@gitster.g","threadId":"59424","inReplyTo":"20230324203737.GA549549@coredump.intra.peff.net","subject":"Re: [PATCH 5/6] pack-bitmap.c: use `bitmap_index_seek()` where possible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-24T21:38:17Z","receivedAt":"2023-03-24T21:38:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Yes, I think the SEEK_SET cases really do need to be doing more\n> checking. AFAICT they are blindly trusting the offsets in the file\n> (which is locally generated, so it's more of a corruption problem than a\n> security one, but still). And this series improves that, which is good\n> (but I still think it should be a die() and not a BUG()).\n\nYes, I think by mistake I merged the topic way too early than it has\nbeen discussed sufficiently.  I haven't reverted the merge into 'next'\nbut it may not be a bad idea if the concensus is that the seek-like\nwhence interface is too ugly to live.  BUG() that triggers on data\nerrors should be updated to die(), whether we do it as a follow-on\npatch or with a replacement iteration.\n\n> Certainly there could be more consistency in the magic numbers. E.g., in\n> this code:\n>\n>                 if (bitmap_git->map_size - bitmap_git->map_pos < bitmap_header_size) {\n>                         error(_(\"corrupt ewah bitmap: truncated header for bitmap of commit \\\"%s\\\"\"),\n>                                 oid_to_hex(&xor_item->oid));\n>                         goto corrupt;\n>                 }\n>\n>                 bitmap_git->map_pos += sizeof(uint32_t) + sizeof(uint8_t);\n>                 xor_flags = read_u8(bitmap_git->map, &bitmap_git->map_pos);\n>                 bitmap = read_bitmap_1(bitmap_git);\n>\n> There is an assumption that sizeof(uint32_t) + sizeof(uint8_t) is equal\n> to bitmap_header_size - 1. That's not wrong, but it's hard to verify\n> that it's doing the right thing, and it's potentially fragile to changes\n> (though such changes seem unlikely).\n\nYeah, that was the part of the code I was looking at when I wrote\nthe message you are responding to X-<.\n\nThanks.\n"},{"id":"474140","messageId":"ZB4qyqHCWIofXg6q@nand.local","threadId":"59424","inReplyTo":"xmqqcz4xesom.fsf@gitster.g","subject":"Re: [PATCH 5/6] pack-bitmap.c: use `bitmap_index_seek()` where possible","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-03-24T22:57:14Z","receivedAt":"2023-03-24T22:57:32Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Mar 24, 2023 at 02:38:17PM -0700, Junio C Hamano wrote:\n> Jeff King <peff@peff.net> writes:\n>\n> > Yes, I think the SEEK_SET cases really do need to be doing more\n> > checking. AFAICT they are blindly trusting the offsets in the file\n> > (which is locally generated, so it's more of a corruption problem than a\n> > security one, but still). And this series improves that, which is good\n> > (but I still think it should be a die() and not a BUG()).\n>\n> Yes, I think by mistake I merged the topic way too early than it has\n> been discussed sufficiently.  I haven't reverted the merge into 'next'\n> but it may not be a bad idea if the concensus is that the seek-like\n> whence interface is too ugly to live.  BUG() that triggers on data\n> errors should be updated to die(), whether we do it as a follow-on\n> patch or with a replacement iteration.\n\nYeah, I was a little surprised to see it merged down so quickly ;-).\n\nIt's fine with me if you want to hold it in 'next' while I send a\nreplacement. Otherwise, if you want to revert it out of 'next', that's\nfine with me too.\n\nI doubt it should take that long to reroll and address the concerns in\nthis thread.\n\nThanks,\nTaylor\n"},{"id":"474141","messageId":"ZB4tiLfNsD3LW/JS@nand.local","threadId":"59424","inReplyTo":"20230321175612.GF3119834@coredump.intra.peff.net","subject":"Re: [PATCH 4/6] pack-bitmap.c: factor out manual `map_pos` manipulation","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-03-24T23:08:56Z","receivedAt":"2023-03-24T23:09:02Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Mar 21, 2023 at 01:56:12PM -0400, Jeff King wrote:\n> I like the idea of centralizing the bounds checks here.\n>\n> I do think copying lseek() is a little awkward, though. As you note, we\n> only care about SEEK_SET and SEEK_CUR, and we have to BUG() on anything\n> else. Which means we have a run-time check, rather than a compile time\n> check. If we had two functions like:\n>\n>   void bitmap_index_seek_to(struct bitmap_index *bitmap_git, size_t pos)\n>   {\n> \tbitmap_git->map_pos = pos;\n> \tif (bitmap_git->map_pos >= bitmap_git->map_size)\n> \t   ...etc...\n>   }\n>\n>   void bitmap_index_incr(struct bitmap_index *bitmap_git, size_t offset)\n>   {\n> \tbitmap_index_seek_to(bitmap_git, st_add(bitmap_git->map_pos, offset));\n>   }\n>\n> then the compiler can see all cases directly. I dunno how much it\n> matters.\n\nYeah, I think that trying to copy the interface of lseek() was a\nmistake, since it ended up creating more awkwardness than anything else.\nI like the combination of bitmap_index_seek_to() and _incr(), though\nthough I called the latter bitmap_index_seek_set() instead.\n\nWe've got to be a little reminiscent of lseek() after all ;-).\n\n> The other thing that's interesting from a bounds-checking perspective is\n> that you're checking the seek _after_ you've read an item. Which has two\n> implications:\n>\n>   - I don't think we could ever overflow size_t here.\n\nYep, agreed. Let's drop the st_add() call there entirely, since it's not\ndoing anything useful and just serving to confuse the reader.\n\n>   - The more interesting case is that we are not overflowing size_t, but\n>     simply walking past bitmap_git->map_size. But again, we are reading\n>     first and then seeking. So if our seek does go past, then by\n>     definition we just read garbage bytes, which is undefined behavior.\n>\n>     For a bounds-check, wouldn't we want it the other way around? To\n>     make sure we have 4 bytes available, and then if so read them and\n>     increment the offset?\n\nI thought on first blush that this would be a much larger change, but\nactually it's quite small. The EWAH code already checks its reads ahead\nof time, so we don't have to worry about those. It's really that\nread_be32() and read_u8() do the read-then-seek.\n\nSo I think that the change here is limited to making sure that we can\nread enough bytes in those functions before actually executing the read.\n\n> > +\tif (bitmap_git->map_pos >= bitmap_git->map_size)\n> > +\t\tBUG(\"bitmap position exceeds size (%\"PRIuMAX\" >= %\"PRIuMAX\")\",\n> > +\t\t    (uintmax_t)bitmap_git->map_pos,\n> > +\t\t    (uintmax_t)bitmap_git->map_size);\n>\n> This uses \">=\", which is good, because it is valid to walk the pointer\n> to one past the end of the map, which is effectively EOF. But as above,\n> in that condition the callers should be checking for this EOF state\n> before reading the bytes.\n\nI think that having both is useful. Checking before you read avoids\nundefined behavior. Checking after you seek ensures that we never have a\nbitmap_index struct with a bogus map_pos value.\n\nThanks,\nTaylor\n"},{"id":"474142","messageId":"ZB4ujCTiRgAmZaQo@nand.local","threadId":"59424","inReplyTo":"61ae4ad5-4d4d-933c-a2cb-e7e2cd734401@github.com","subject":"Re: [PATCH 4/6] pack-bitmap.c: factor out manual `map_pos` manipulation","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-03-24T23:13:16Z","receivedAt":"2023-03-24T23:13:21Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Mar 24, 2023 at 02:04:05PM -0400, Derrick Stolee wrote:\n> The other alternative would be to use an enum instead of an arbitrary int.\n> The compiler can warn to some extent, but it's not as helpful as a method\n> name distinction.\n\nYeah, I think that another enum here just to distinguish between seeking\nto an absolute position versus a relative one is probably overkill in\nthis instance.\n\n> >> +\tif (bitmap_git->map_pos >= bitmap_git->map_size)\n> >> +\t\tBUG(\"bitmap position exceeds size (%\"PRIuMAX\" >= %\"PRIuMAX\")\",\n> >> +\t\t    (uintmax_t)bitmap_git->map_pos,\n> >> +\t\t    (uintmax_t)bitmap_git->map_size);\n> >\n> > This uses \">=\", which is good, because it is valid to walk the pointer\n> > to one past the end of the map, which is effectively EOF. But as above,\n> > in that condition the callers should be checking for this EOF state\n> > before reading the bytes.\n>\n> In other words, it would be too easy for a strange data shape to trigger\n> this BUG() statement, which should be reserved for the most-extreme cases\n> of developer mistake. Things like \"this is an unacceptable 'whence' value\"\n> or \"this should never be NULL\" make sense, but this is too likely to be\n> hit due to a runtime error.\n\n> Would it make sense to return an 'int' instead of the size_t of map_pos?\n> That way we could return in error if this is exceeded, and then all\n> callers can respond \"oh wait, that move would exceed the file size, so\n> I should fail in my own way...\"?\n\nWorks for me. I think bitmap_index_seek_to() would probably return the\nerror() itself, since I don't think it makes sense to require each\ncaller to come up with the same \"bitmap position exceeds size\" thing.\n\nWe want the message from that error() to appear regardless, but each\ncaller can decide what it wants to do in the presence of an error (e.g.,\ncontinue on, propagate the error, abort the program, etc).\n\nSomething like this:\n\n    static int bitmap_index_seek_to(struct bitmap_index *bitmap_git, size_t pos)\n    {\n      bitmap_git->map_pos = pos;\n\n      if (bitmap_git->map_pos >= bitmap_git->map_size)\n        return error(_(\"bitmap position exceeds size \"\n                       \"(%\"PRIuMAX\" >= %\"PRIuMAX\")\"),\n                     (uintmax_t)bitmap_git->map_pos,\n                     (uintmax_t)bitmap_git->map_size);\n      return 0;\n    }\n\nThanks,\nTaylor\n"},{"id":"474143","messageId":"ZB4w2gCo/qPCmWkz@nand.local","threadId":"59424","inReplyTo":"20230324182929.GA536252@coredump.intra.peff.net","subject":"Re: [PATCH 4/6] pack-bitmap.c: factor out manual `map_pos` manipulation","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-03-24T23:23:06Z","receivedAt":"2023-03-24T23:23:24Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Mar 24, 2023 at 02:29:29PM -0400, Jeff King wrote:\n> We know the advance will succeed because we checked ahead of time that\n> we had enough bytes. So it really is a BUG() if we don't, as it would\n> indicate somebody missed the earlier check. On the other hand, it is a\n> weird spot for an extra check, because by definition we'll have just\n> read off the array just before the seek.\n\nHere you claim that we want bitmap_index_seek_to() to call BUG() if we\nend up with map_pos >= map_size. But...\n\n> The case where we _do_ seek directly to a file-provided offset, rather\n> than incrementing, is an important check that this series adds, but that\n> one should be a die() and not a BUG().\n\n...here you say that it should be a die().\n\nI think it does depend on the context. When seeking directly to a\nposition before reading something, die()-ing is appropriate. The case\nwhere you seek to a relative position to reflect that you just read\nsomething, a BUG() is appropriate.\n\nSo really, I think you want something like this:\n\n    static void bitmap_index_seek_set(struct bitmap_index *bitmap_git, size_t pos)\n    {\n      if (pos >= bitmap_git->map_size)\n        die(_(\"bitmap position exceeds size (%\"PRIuMAX\" >= %\"PRIuMAX\")\"),\n            (uintmax_t)bitmap_git->map_pos,\n            (uintmax_t)bitmap_git->map_size);\n\n      bitmap_git->map_pos = pos;\n    }\n\n    static void bitmap_index_seek_ahead(struct bitmap_index *bitmap_git,\n                                        size_t offset)\n    {\n      if (bitmap_git->map_pos + offset >= bitmap_git->map_size)\n        BUG(\"cannot seek %\"PRIuMAX\" byte(s) ahead of %\"PRIuMAX\" \"\n            \"(%\"PRIuMAX\" >= %\"PRIuMAX\")\",\n            (uintmax_t)offset,\n            (uintmax_t)bitmap_git->map_pos,\n            (uintmax_t)(bitmap_git->map_pos + offset),\n            (uintmax_t)bitmap_git->map_size);\n\n      bitmap_git->map_pos += offset;\n    }\n\nDoes that match what you were thinking?\n\nThanks,\nTaylor\n"},{"id":"474144","messageId":"ZB4xEO82vTp6yTfx@nand.local","threadId":"59424","inReplyTo":"ZB4ujCTiRgAmZaQo@nand.local","subject":"Re: [PATCH 4/6] pack-bitmap.c: factor out manual `map_pos` manipulation","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-03-24T23:24:00Z","receivedAt":"2023-03-24T23:24:04Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Mar 24, 2023 at 07:13:16PM -0400, Taylor Blau wrote:\n> > Would it make sense to return an 'int' instead of the size_t of map_pos?\n> > That way we could return in error if this is exceeded, and then all\n> > callers can respond \"oh wait, that move would exceed the file size, so\n> > I should fail in my own way...\"?\n>\n> Works for me. I think bitmap_index_seek_to() would probably return the\n> error() itself, since I don't think it makes sense to require each\n> caller to come up with the same \"bitmap position exceeds size\" thing.\n\nActually, I take that back. See my response lower in this thread:\n\n    https://lore.kernel.org/git/ZB4w2gCo/qPCmWkz@nand.local/\n\nThanks,\nTaylor\n"},{"id":"474146","messageId":"20230325045708.GA555973@coredump.intra.peff.net","threadId":"59424","inReplyTo":"ZB4w2gCo/qPCmWkz@nand.local","subject":"Re: [PATCH 4/6] pack-bitmap.c: factor out manual `map_pos` manipulation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-25T04:57:08Z","receivedAt":"2023-03-25T04:57:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 24, 2023 at 07:23:06PM -0400, Taylor Blau wrote:\n\n> On Fri, Mar 24, 2023 at 02:29:29PM -0400, Jeff King wrote:\n> > We know the advance will succeed because we checked ahead of time that\n> > we had enough bytes. So it really is a BUG() if we don't, as it would\n> > indicate somebody missed the earlier check. On the other hand, it is a\n> > weird spot for an extra check, because by definition we'll have just\n> > read off the array just before the seek.\n> \n> Here you claim that we want bitmap_index_seek_to() to call BUG() if we\n> end up with map_pos >= map_size. But...\n\nI think the paragraph above doesn't have enough context. I meant\nincrementing the pos here (which is why \"we checked ahead of time that\nwe had enough bytes\"), in which case it is a BUG() (double-checking the\nearlier check).\n\nIn a seek_to(), there is no previous check. We have to make sure the\nrequested offset is within bounds.\n\n> > The case where we _do_ seek directly to a file-provided offset, rather\n> > than incrementing, is an important check that this series adds, but that\n> > one should be a die() and not a BUG().\n> \n> ...here you say that it should be a die().\n\nRight, so that one would be a die(). Or better still, an error().\n\n> I think it does depend on the context. When seeking directly to a\n> position before reading something, die()-ing is appropriate. The case\n> where you seek to a relative position to reflect that you just read\n> something, a BUG() is appropriate.\n\nRight, exactly. We are agreeing, I think.\n\n> So really, I think you want something like this:\n> \n>     static void bitmap_index_seek_set(struct bitmap_index *bitmap_git, size_t pos)\n>     {\n>       if (pos >= bitmap_git->map_size)\n>         die(_(\"bitmap position exceeds size (%\"PRIuMAX\" >= %\"PRIuMAX\")\"),\n>             (uintmax_t)bitmap_git->map_pos,\n>             (uintmax_t)bitmap_git->map_size);\n> \n>       bitmap_git->map_pos = pos;\n>     }\n> \n>     static void bitmap_index_seek_ahead(struct bitmap_index *bitmap_git,\n>                                         size_t offset)\n>     {\n>       if (bitmap_git->map_pos + offset >= bitmap_git->map_size)\n>         BUG(\"cannot seek %\"PRIuMAX\" byte(s) ahead of %\"PRIuMAX\" \"\n>             \"(%\"PRIuMAX\" >= %\"PRIuMAX\")\",\n>             (uintmax_t)offset,\n>             (uintmax_t)bitmap_git->map_pos,\n>             (uintmax_t)(bitmap_git->map_pos + offset),\n>             (uintmax_t)bitmap_git->map_size);\n> \n>       bitmap_git->map_pos += offset;\n>     }\n> \n> Does that match what you were thinking?\n\nYes, though I am of the opinion that the assertion in seek_ahead() is\nlargely pointless, simply because if it ever triggered we would already\nhave triggered undefined behavior. I'm not opposed to adding it if you\nfeel strongly, I just wouldn't bother myself (and instead would focus on\nmaking the \"do we have enough bytes to read\" checks more consistent and\nharder-to-get-wrong).\n\nSeeking to exactly map_size in the seek_set() case (i.e., the \"=\" in\n\">=\") is a little funny, but not illegal. Either way, you'd want to\ncheck \"and do we have N bytes to read from this offset\" immediately\nafterwards (and your series does), so that would catch any non-zero\nreads there.\n\n-Peff\n"}]}