{"thread":{"id":"58506","subject":"[PATCH] read-cache: avoid misaligned reads in index v4","startedAt":"2022-09-23T19:44:04Z","lastAt":"2022-09-28T17:34:42Z","messageCount":12,"participants":["Victoria Dye via GitGitGadget","Jeff King","Junio C Hamano","Phillip Wood","Victoria Dye"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"463547","messageId":"pull.1366.git.1663962236069.gitgitgadget@gmail.com","threadId":"58506","inReplyTo":null,"subject":"[PATCH] read-cache: avoid misaligned reads in index v4","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-23T19:43:55Z","receivedAt":"2022-09-23T19:44:04Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye <vdye@github.com>\n\nThe process for reading the index into memory from disk is to first read its\ncontents into a single memory-mapped file buffer (type 'char *'), then\nsequentially convert each on-disk index entry into a corresponding incore\n'cache_entry'. To access the contents of the on-disk entry for processing, a\nmoving pointer within the memory-mapped file is cast to type 'struct\nondisk_cache_entry *'.\n\nIn index v4, the entries in the on-disk index file are written *without*\naligning their first byte to a 4-byte boundary; entries are a variable\nlength (depending on the entry name and whether or not extended flags are\nused). As a result, casting the 'char *' buffer pointer to 'struct\nondisk_cache_entry *' then accessing its contents in a 'SANITIZE=undefined'\nbuild can trigger the following error:\n\n  read-cache.c:1886:46: runtime error: member access within misaligned\n  address <address> for type 'struct ondisk_cache_entry', which requires 4\n  byte alignment\n\nAvoid this error by reading fields directly from the 'char *' buffer, using\nthe 'offsetof' individual fields in 'struct ondisk_cache_entry'.\n\nReported-by: Jeff King <peff@peff.net>\nSigned-off-by: Victoria Dye <vdye@github.com>\n---\n    read-cache: avoid misaligned reads in index v4\n    \n    This fixes the bug reported in [1], where unaligned index entries in the\n    memory-mapped index file triggered a 'SANITIZE=undefined' error due to\n    casting to & accessing unaligned data a 4 byte-aligned 'struct\n    ondisk_cache_entry *' type.\n    \n    In addition to the originally-reported 't9210-scalar.sh' now passing in\n    a 'SANITIZE=undefined' build, I did some light testing by first writing\n    a v4 index with a released version of Git (v2.37), then running some\n    index-modifying operations ('git status', 'git add') with this patch's\n    changes, then again running 'git status' with the stable version. I\n    didn't see anything out of the ordinary but, considering how critical\n    \"reading the index\" is, I'd very much appreciate some extra-thorough\n    reviews on this patch. :)\n    \n    Thanks!\n    \n     * Victoria\n    \n    [1]\n    https://lore.kernel.org/git/YywzNTzd72tox8Z+@coredump.intra.peff.net/\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1366%2Fvdye%2Fbugfix%2Findex-v4-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1366/vdye/bugfix/index-v4-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1366\n\n read-cache.c | 42 +++++++++++++++++++++++-------------------\n 1 file changed, 23 insertions(+), 19 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex b09128b1884..d16eb979060 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1875,7 +1875,7 @@ static int read_index_extension(struct index_state *istate,\n \n static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,\n \t\t\t\t\t    unsigned int version,\n-\t\t\t\t\t    struct ondisk_cache_entry *ondisk,\n+\t\t\t\t\t    const char *ondisk,\n \t\t\t\t\t    unsigned long *ent_size,\n \t\t\t\t\t    const struct cache_entry *previous_ce)\n {\n@@ -1883,7 +1883,7 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,\n \tsize_t len;\n \tconst char *name;\n \tconst unsigned hashsz = the_hash_algo->rawsz;\n-\tconst uint16_t *flagsp = (const uint16_t *)(ondisk->data + hashsz);\n+\tconst char *flagsp = ondisk + offsetof(struct ondisk_cache_entry, data) + hashsz;\n \tunsigned int flags;\n \tsize_t copy_len = 0;\n \t/*\n@@ -1901,15 +1901,15 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,\n \n \tif (flags & CE_EXTENDED) {\n \t\tint extended_flags;\n-\t\textended_flags = get_be16(flagsp + 1) << 16;\n+\t\textended_flags = get_be16(flagsp + sizeof(uint16_t)) << 16;\n \t\t/* We do not yet understand any bit out of CE_EXTENDED_FLAGS */\n \t\tif (extended_flags & ~CE_EXTENDED_FLAGS)\n \t\t\tdie(_(\"unknown index entry format 0x%08x\"), extended_flags);\n \t\tflags |= extended_flags;\n-\t\tname = (const char *)(flagsp + 2);\n+\t\tname = (const char *)(flagsp + 2 * sizeof(uint16_t));\n \t}\n \telse\n-\t\tname = (const char *)(flagsp + 1);\n+\t\tname = (const char *)(flagsp + sizeof(uint16_t));\n \n \tif (expand_name_field) {\n \t\tconst unsigned char *cp = (const unsigned char *)name;\n@@ -1935,20 +1935,24 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,\n \n \tce = mem_pool__ce_alloc(ce_mem_pool, len);\n \n-\tce->ce_stat_data.sd_ctime.sec = get_be32(&ondisk->ctime.sec);\n-\tce->ce_stat_data.sd_mtime.sec = get_be32(&ondisk->mtime.sec);\n-\tce->ce_stat_data.sd_ctime.nsec = get_be32(&ondisk->ctime.nsec);\n-\tce->ce_stat_data.sd_mtime.nsec = get_be32(&ondisk->mtime.nsec);\n-\tce->ce_stat_data.sd_dev   = get_be32(&ondisk->dev);\n-\tce->ce_stat_data.sd_ino   = get_be32(&ondisk->ino);\n-\tce->ce_mode  = get_be32(&ondisk->mode);\n-\tce->ce_stat_data.sd_uid   = get_be32(&ondisk->uid);\n-\tce->ce_stat_data.sd_gid   = get_be32(&ondisk->gid);\n-\tce->ce_stat_data.sd_size  = get_be32(&ondisk->size);\n+\tce->ce_stat_data.sd_ctime.sec = get_be32(ondisk + offsetof(struct ondisk_cache_entry, ctime)\n+\t\t\t\t\t\t\t+ offsetof(struct cache_time, sec));\n+\tce->ce_stat_data.sd_mtime.sec = get_be32(ondisk + offsetof(struct ondisk_cache_entry, mtime)\n+\t\t\t\t\t\t\t+ offsetof(struct cache_time, sec));\n+\tce->ce_stat_data.sd_ctime.nsec = get_be32(ondisk + offsetof(struct ondisk_cache_entry, ctime)\n+\t\t\t\t\t\t\t + offsetof(struct cache_time, nsec));\n+\tce->ce_stat_data.sd_mtime.nsec = get_be32(ondisk + offsetof(struct ondisk_cache_entry, mtime)\n+\t\t\t\t\t\t\t + offsetof(struct cache_time, nsec));\n+\tce->ce_stat_data.sd_dev   = get_be32(ondisk + offsetof(struct ondisk_cache_entry, dev));\n+\tce->ce_stat_data.sd_ino   = get_be32(ondisk + offsetof(struct ondisk_cache_entry, ino));\n+\tce->ce_mode  = get_be32(ondisk + offsetof(struct ondisk_cache_entry, mode));\n+\tce->ce_stat_data.sd_uid   = get_be32(ondisk + offsetof(struct ondisk_cache_entry, uid));\n+\tce->ce_stat_data.sd_gid   = get_be32(ondisk + offsetof(struct ondisk_cache_entry, gid));\n+\tce->ce_stat_data.sd_size  = get_be32(ondisk + offsetof(struct ondisk_cache_entry, size));\n \tce->ce_flags = flags & ~CE_NAMEMASK;\n \tce->ce_namelen = len;\n \tce->index = 0;\n-\toidread(&ce->oid, ondisk->data);\n+\toidread(&ce->oid, (const unsigned char *)ondisk + offsetof(struct ondisk_cache_entry, data));\n \n \tif (expand_name_field) {\n \t\tif (copy_len)\n@@ -2117,12 +2121,12 @@ static unsigned long load_cache_entry_block(struct index_state *istate,\n \tunsigned long src_offset = start_offset;\n \n \tfor (i = offset; i < offset + nr; i++) {\n-\t\tstruct ondisk_cache_entry *disk_ce;\n \t\tstruct cache_entry *ce;\n \t\tunsigned long consumed;\n \n-\t\tdisk_ce = (struct ondisk_cache_entry *)(mmap + src_offset);\n-\t\tce = create_from_disk(ce_mem_pool, istate->version, disk_ce, &consumed, previous_ce);\n+\t\tce = create_from_disk(ce_mem_pool, istate->version,\n+\t\t\t\t      mmap + src_offset,\n+\t\t\t\t      &consumed, previous_ce);\n \t\tset_index_entry(istate, i, ce);\n \n \t\tsrc_offset += consumed;\n\nbase-commit: 1b3d6e17fe83eb6f79ffbac2f2c61bbf1eaef5f8\n-- \ngitgitgadget\n"},{"id":"463554","messageId":"Yy4nkEnhuzt2iH+R@coredump.intra.peff.net","threadId":"58506","inReplyTo":"pull.1366.git.1663962236069.gitgitgadget@gmail.com","subject":"Re: [PATCH] read-cache: avoid misaligned reads in index v4","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-23T21:39:28Z","receivedAt":"2022-09-23T21:39:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 23, 2022 at 07:43:55PM +0000, Victoria Dye via GitGitGadget wrote:\n\n> Avoid this error by reading fields directly from the 'char *' buffer, using\n> the 'offsetof' individual fields in 'struct ondisk_cache_entry'.\n\nThanks for moving this forward. I agree this should fix the alignment\nproblems, and I didn't see anything in the patch that would do the wrong\nthing. I do have some style/technique suggestions, though.\n\n> @@ -1883,7 +1883,7 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,\n>  \tsize_t len;\n>  \tconst char *name;\n>  \tconst unsigned hashsz = the_hash_algo->rawsz;\n> -\tconst uint16_t *flagsp = (const uint16_t *)(ondisk->data + hashsz);\n> +\tconst char *flagsp = ondisk + offsetof(struct ondisk_cache_entry, data) + hashsz;\n\nNow we use the \"const char *\" pointer instead of the cast to the\nondisk_cache_entry struct, which is good, and is what fixes the\nalignment question.\n\nBut we also convert flagsp from being a uint16_t into a byte pointer.\nI'm not sure if that's strictly necessary from an alignment perspective,\nas we'd dereference it only via get_be16(), which handles alignment and\ntype conversion itself.\n\nI'd imagine the standard probably says that even forming such a pointer\nis illegal, so in that sense, it probably is undefined behavior. But I\nthink it's one of those things that's OK in practice.\n\nThat might be splitting hairs, but if you kept it as a uint16_t pointer,\nthen code like this:\n\n> @@ -1901,15 +1901,15 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,\n>  \n>  \tif (flags & CE_EXTENDED) {\n>  \t\tint extended_flags;\n> -\t\textended_flags = get_be16(flagsp + 1) << 16;\n> +\t\textended_flags = get_be16(flagsp + sizeof(uint16_t)) << 16;\n\ndoesn't need to be changed. I don't know if it's that big a deal either\nway, though.\n\n> @@ -1935,20 +1935,24 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,\n>  \n>  \tce = mem_pool__ce_alloc(ce_mem_pool, len);\n>  \n> -\tce->ce_stat_data.sd_ctime.sec = get_be32(&ondisk->ctime.sec);\n> [...]\n> +\tce->ce_stat_data.sd_ctime.sec = get_be32(ondisk + offsetof(struct ondisk_cache_entry, ctime)\n> +\t\t\t\t\t\t\t+ offsetof(struct cache_time, sec));\n\nI had figured we'd be able to drop ondisk_cache_entry entirely. But here\nyou're using it essentially as a template for a set of constants\nretrieved via offsetof().\n\nThat's OK from an alignment perspective. It does mean we'd be in trouble\nif a compiler ever decided to introduce padding into the struct. That's\nprobably unlikely. We don't use __attribute__((packed)) because it's not\nportable, and our existing uses have generally been OK, because our\ndata structures are organized around 8-byte alignment. We might have\nproblems on a theoretical 128-bit processor or something.\n\nSo I don't think this is a problem now, and unlikely to be in the near\nfuture. But another way to do it would just be an actual set of offsets\n(either #define or an enum). That maybe makes the intended use more\nobvious, and also prevents people from accidentally misusing the struct.\nI'm not sure if it's worth it for not.\n\nIt is a bit of a pain to write. Either you have magic numbers, or you\nhave to reference the offset and size of the previous entry:\n\n  #define ONDISK_CACHE_CTIME 0\n  #define ONDISK_CACHE_MTIME (ONDISK_CACHE_CTIME + sizeof(struct cache_time))\n  #define ONDISK_CACHE_DEV (ONDISK_CACHE_MTIME + sizeof(struct cache_time))\n\nAnother strategy is to just parse left-to-right, advancing the byte\npointer. Like:\n\n  ce->ce_state_data.sd_ctime.sec = get_be32(ondisk);\n  ondisk += sizeof(uint32_t);\n  ce->ce_state_data.sd_mtime.sec = get_be32(ondisk);\n  ondisk += sizeof(uint32_t);\n  ...etc...\n\nYou can even stick that in a helper function that does the get_b32() and\nadvances, so you know they're always done in sync. See pack-bitmap.c's\nread_be32(), etc. IMHO this produces a nice result because the reading\ncode itself becomes the source of truth for the format.\n\nBut one tricky thing there is if you want to parse out of order. And it\ndoes seem that we read the struct out of order in this case. But I don't\nthink there's any reason we need to do so. Of course reordering the\nfunction would make the change much more invasive.\n\nSo all that said, I'm OK with this approach as the minimal fix, and then\nwe can think about further refactoring or cleanup on top.\n\nOne final note, though:\n\n> +\tce->ce_stat_data.sd_mtime.sec = get_be32(ondisk + offsetof(struct ondisk_cache_entry, mtime)\n> +\t\t\t\t\t\t\t+ offsetof(struct cache_time, sec));\n\nHere (and elsewhere), you can assume that the offsetof() \"sec\" in\ncache_time is 0, for two reasons:\n\n  - I didn't look up chapter and verse, but I'm pretty sure the standard\n    does guarantee that the first field of a struct is at the beginning.\n\n  - If there's any padding, this whole scheme is hosed anyway, because\n    it means sizeof(cache_time) is bigger than we expect, which messes\n    up the offsetof() the entry after us (in this case sd_dev).\n\nSo this can just be:\n\n  ce->ce_stat_data.sd_mtime.sec = get_be32(ondisk + offsetof(struct ondisk_cache_entry, mtime));\n\nwhich is mercifully shorter.\n\nAssuming we dismiss the rest of what I said as not worth it for a\nminimal fix, I do think that simplification is worth rolling a v2.\n\n-Peff\n\nPS BTW, I mentioned earlier \"can we just get rid of ondisk_cache_entry\".\n   We also use it for the writing side, of course. That doesn't have\n   alignment issues, but it does have the same \"I hope there's never any\n   padding\" question. In an ideal world, it would be using the\n   equivalent put_be32(), but again, that's getting out of the \"minimal\n   fix\" territory.\n"},{"id":"463557","messageId":"xmqqtu4xsq79.fsf@gitster.g","threadId":"58506","inReplyTo":"Yy4nkEnhuzt2iH+R@coredump.intra.peff.net","subject":"Re: [PATCH] read-cache: avoid misaligned reads in index v4","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-23T22:04:10Z","receivedAt":"2022-09-23T22:04:17Z","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> Here (and elsewhere), you can assume that the offsetof() \"sec\" in\n> cache_time is 0, for two reasons:\n>\n>   - I didn't look up chapter and verse, but I'm pretty sure the standard\n>     does guarantee that the first field of a struct is at the beginning.\n\nhttps://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf\n\n6.7.2.1 #13 (page 103)\n\n    Within a structure object, the non-bit-field members and the\n    units in which bit-fields reside have addresses that increase in\n    the order in which they are declared. A pointer to a structure\n    object, suitably converted, points to its initial member (or if\n    that member is a bit-field, then to the unit in which it\n    resides), and vice versa. There may be unnamed padding within a\n    structure object, but not at its beginning.\n\nAs an initial padding is forbidden, the first member's offset is zero.\n\n"},{"id":"463595","messageId":"bb3a2470-7ff5-e4a6-040a-96e0e3833978@gmail.com","threadId":"58506","inReplyTo":"pull.1366.git.1663962236069.gitgitgadget@gmail.com","subject":"Re: [PATCH] read-cache: avoid misaligned reads in index v4","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-09-25T08:25:08Z","receivedAt":"2022-09-25T08:25:45Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Victoria\n\nOn 23/09/2022 20:43, Victoria Dye via GitGitGadget wrote:\n> From: Victoria Dye <vdye@github.com>\n> \n> The process for reading the index into memory from disk is to first read its\n> contents into a single memory-mapped file buffer (type 'char *'), then\n> sequentially convert each on-disk index entry into a corresponding incore\n> 'cache_entry'. To access the contents of the on-disk entry for processing, a\n> moving pointer within the memory-mapped file is cast to type 'struct\n> ondisk_cache_entry *'.\n> \n> In index v4, the entries in the on-disk index file are written *without*\n> aligning their first byte to a 4-byte boundary; entries are a variable\n> length (depending on the entry name and whether or not extended flags are\n> used). As a result, casting the 'char *' buffer pointer to 'struct\n> ondisk_cache_entry *' then accessing its contents in a 'SANITIZE=undefined'\n> build can trigger the following error:\n> \n>    read-cache.c:1886:46: runtime error: member access within misaligned\n>    address <address> for type 'struct ondisk_cache_entry', which requires 4\n>    byte alignment\n> \n> Avoid this error by reading fields directly from the 'char *' buffer, using\n> the 'offsetof' individual fields in 'struct ondisk_cache_entry'.\n\nI was confused as to why this was safe as it means we're still reading \nthe individual fields from unaligned addresses. The reason it is safe is \nthat we use get_bexx() to read the fields and those functions handle \nunaligned access. I wonder if it is worth adding a note to clarify that \nif you re-roll.\n\nBest Wishes\n\nPhillip\n> \n> Reported-by: Jeff King <peff@peff.net>\n> Signed-off-by: Victoria Dye <vdye@github.com>\n> ---\n>      read-cache: avoid misaligned reads in index v4\n>      \n>      This fixes the bug reported in [1], where unaligned index entries in the\n>      memory-mapped index file triggered a 'SANITIZE=undefined' error due to\n>      casting to & accessing unaligned data a 4 byte-aligned 'struct\n>      ondisk_cache_entry *' type.\n>      \n>      In addition to the originally-reported 't9210-scalar.sh' now passing in\n>      a 'SANITIZE=undefined' build, I did some light testing by first writing\n>      a v4 index with a released version of Git (v2.37), then running some\n>      index-modifying operations ('git status', 'git add') with this patch's\n>      changes, then again running 'git status' with the stable version. I\n>      didn't see anything out of the ordinary but, considering how critical\n>      \"reading the index\" is, I'd very much appreciate some extra-thorough\n>      reviews on this patch. :)\n>      \n>      Thanks!\n>      \n>       * Victoria\n>      \n>      [1]\n>      https://lore.kernel.org/git/YywzNTzd72tox8Z+@coredump.intra.peff.net/\n> \n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1366%2Fvdye%2Fbugfix%2Findex-v4-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1366/vdye/bugfix/index-v4-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1366\n> \n>   read-cache.c | 42 +++++++++++++++++++++++-------------------\n>   1 file changed, 23 insertions(+), 19 deletions(-)\n> \n> diff --git a/read-cache.c b/read-cache.c\n> index b09128b1884..d16eb979060 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -1875,7 +1875,7 @@ static int read_index_extension(struct index_state *istate,\n>   \n>   static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,\n>   \t\t\t\t\t    unsigned int version,\n> -\t\t\t\t\t    struct ondisk_cache_entry *ondisk,\n> +\t\t\t\t\t    const char *ondisk,\n>   \t\t\t\t\t    unsigned long *ent_size,\n>   \t\t\t\t\t    const struct cache_entry *previous_ce)\n>   {\n> @@ -1883,7 +1883,7 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,\n>   \tsize_t len;\n>   \tconst char *name;\n>   \tconst unsigned hashsz = the_hash_algo->rawsz;\n> -\tconst uint16_t *flagsp = (const uint16_t *)(ondisk->data + hashsz);\n> +\tconst char *flagsp = ondisk + offsetof(struct ondisk_cache_entry, data) + hashsz;\n>   \tunsigned int flags;\n>   \tsize_t copy_len = 0;\n>   \t/*\n> @@ -1901,15 +1901,15 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,\n>   \n>   \tif (flags & CE_EXTENDED) {\n>   \t\tint extended_flags;\n> -\t\textended_flags = get_be16(flagsp + 1) << 16;\n> +\t\textended_flags = get_be16(flagsp + sizeof(uint16_t)) << 16;\n>   \t\t/* We do not yet understand any bit out of CE_EXTENDED_FLAGS */\n>   \t\tif (extended_flags & ~CE_EXTENDED_FLAGS)\n>   \t\t\tdie(_(\"unknown index entry format 0x%08x\"), extended_flags);\n>   \t\tflags |= extended_flags;\n> -\t\tname = (const char *)(flagsp + 2);\n> +\t\tname = (const char *)(flagsp + 2 * sizeof(uint16_t));\n>   \t}\n>   \telse\n> -\t\tname = (const char *)(flagsp + 1);\n> +\t\tname = (const char *)(flagsp + sizeof(uint16_t));\n>   \n>   \tif (expand_name_field) {\n>   \t\tconst unsigned char *cp = (const unsigned char *)name;\n> @@ -1935,20 +1935,24 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,\n>   \n>   \tce = mem_pool__ce_alloc(ce_mem_pool, len);\n>   \n> -\tce->ce_stat_data.sd_ctime.sec = get_be32(&ondisk->ctime.sec);\n> -\tce->ce_stat_data.sd_mtime.sec = get_be32(&ondisk->mtime.sec);\n> -\tce->ce_stat_data.sd_ctime.nsec = get_be32(&ondisk->ctime.nsec);\n> -\tce->ce_stat_data.sd_mtime.nsec = get_be32(&ondisk->mtime.nsec);\n> -\tce->ce_stat_data.sd_dev   = get_be32(&ondisk->dev);\n> -\tce->ce_stat_data.sd_ino   = get_be32(&ondisk->ino);\n> -\tce->ce_mode  = get_be32(&ondisk->mode);\n> -\tce->ce_stat_data.sd_uid   = get_be32(&ondisk->uid);\n> -\tce->ce_stat_data.sd_gid   = get_be32(&ondisk->gid);\n> -\tce->ce_stat_data.sd_size  = get_be32(&ondisk->size);\n> +\tce->ce_stat_data.sd_ctime.sec = get_be32(ondisk + offsetof(struct ondisk_cache_entry, ctime)\n> +\t\t\t\t\t\t\t+ offsetof(struct cache_time, sec));\n> +\tce->ce_stat_data.sd_mtime.sec = get_be32(ondisk + offsetof(struct ondisk_cache_entry, mtime)\n> +\t\t\t\t\t\t\t+ offsetof(struct cache_time, sec));\n> +\tce->ce_stat_data.sd_ctime.nsec = get_be32(ondisk + offsetof(struct ondisk_cache_entry, ctime)\n> +\t\t\t\t\t\t\t + offsetof(struct cache_time, nsec));\n> +\tce->ce_stat_data.sd_mtime.nsec = get_be32(ondisk + offsetof(struct ondisk_cache_entry, mtime)\n> +\t\t\t\t\t\t\t + offsetof(struct cache_time, nsec));\n> +\tce->ce_stat_data.sd_dev   = get_be32(ondisk + offsetof(struct ondisk_cache_entry, dev));\n> +\tce->ce_stat_data.sd_ino   = get_be32(ondisk + offsetof(struct ondisk_cache_entry, ino));\n> +\tce->ce_mode  = get_be32(ondisk + offsetof(struct ondisk_cache_entry, mode));\n> +\tce->ce_stat_data.sd_uid   = get_be32(ondisk + offsetof(struct ondisk_cache_entry, uid));\n> +\tce->ce_stat_data.sd_gid   = get_be32(ondisk + offsetof(struct ondisk_cache_entry, gid));\n> +\tce->ce_stat_data.sd_size  = get_be32(ondisk + offsetof(struct ondisk_cache_entry, size));\n>   \tce->ce_flags = flags & ~CE_NAMEMASK;\n>   \tce->ce_namelen = len;\n>   \tce->index = 0;\n> -\toidread(&ce->oid, ondisk->data);\n> +\toidread(&ce->oid, (const unsigned char *)ondisk + offsetof(struct ondisk_cache_entry, data));\n>   \n>   \tif (expand_name_field) {\n>   \t\tif (copy_len)\n> @@ -2117,12 +2121,12 @@ static unsigned long load_cache_entry_block(struct index_state *istate,\n>   \tunsigned long src_offset = start_offset;\n>   \n>   \tfor (i = offset; i < offset + nr; i++) {\n> -\t\tstruct ondisk_cache_entry *disk_ce;\n>   \t\tstruct cache_entry *ce;\n>   \t\tunsigned long consumed;\n>   \n> -\t\tdisk_ce = (struct ondisk_cache_entry *)(mmap + src_offset);\n> -\t\tce = create_from_disk(ce_mem_pool, istate->version, disk_ce, &consumed, previous_ce);\n> +\t\tce = create_from_disk(ce_mem_pool, istate->version,\n> +\t\t\t\t      mmap + src_offset,\n> +\t\t\t\t      &consumed, previous_ce);\n>   \t\tset_index_entry(istate, i, ce);\n>   \n>   \t\tsrc_offset += consumed;\n> \n> base-commit: 1b3d6e17fe83eb6f79ffbac2f2c61bbf1eaef5f8\n"},{"id":"463638","messageId":"e5954e90-6b5c-46a6-0842-b3d7d1e06b33@github.com","threadId":"58506","inReplyTo":"Yy4nkEnhuzt2iH+R@coredump.intra.peff.net","subject":"Re: [PATCH] read-cache: avoid misaligned reads in index v4","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2022-09-26T15:39:10Z","receivedAt":"2022-09-26T16:47:53Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Jeff King wrote:\n> On Fri, Sep 23, 2022 at 07:43:55PM +0000, Victoria Dye via GitGitGadget wrote:\n>> @@ -1883,7 +1883,7 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,\n>>  \tsize_t len;\n>>  \tconst char *name;\n>>  \tconst unsigned hashsz = the_hash_algo->rawsz;\n>> -\tconst uint16_t *flagsp = (const uint16_t *)(ondisk->data + hashsz);\n>> +\tconst char *flagsp = ondisk + offsetof(struct ondisk_cache_entry, data) + hashsz;\n> \n> Now we use the \"const char *\" pointer instead of the cast to the\n> ondisk_cache_entry struct, which is good, and is what fixes the\n> alignment question.\n> \n> But we also convert flagsp from being a uint16_t into a byte pointer.\n> I'm not sure if that's strictly necessary from an alignment perspective,\n> as we'd dereference it only via get_be16(), which handles alignment and\n> type conversion itself.\n> \n> I'd imagine the standard probably says that even forming such a pointer\n> is illegal, so in that sense, it probably is undefined behavior. But I\n> think it's one of those things that's OK in practice.\n\nYep, per the C standard §6.3.2.3 #7 [1]:\n\n  A pointer to an object or incomplete type may be converted to a pointer to\n  a different object or incomplete type. If the resulting pointer is not\n  correctly aligned for the pointed-to type, the behavior is undefined.\n\nTo your point, it is probably fine in practice, but I'd lean towards\nsticking with a 'char *' to play it safe.\n\n[1] https://www.open-std.org/JTC1/SC22/WG14/www/docs/n1256.pdf\n\n>> @@ -1935,20 +1935,24 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,\n>>  \n>>  \tce = mem_pool__ce_alloc(ce_mem_pool, len);\n>>  \n>> -\tce->ce_stat_data.sd_ctime.sec = get_be32(&ondisk->ctime.sec);\n>> [...]\n>> +\tce->ce_stat_data.sd_ctime.sec = get_be32(ondisk + offsetof(struct ondisk_cache_entry, ctime)\n>> +\t\t\t\t\t\t\t+ offsetof(struct cache_time, sec));\n> \n> I had figured we'd be able to drop ondisk_cache_entry entirely. But here\n> you're using it essentially as a template for a set of constants\n> retrieved via offsetof().\n> \n> That's OK from an alignment perspective. It does mean we'd be in trouble\n> if a compiler ever decided to introduce padding into the struct. That's\n> probably unlikely. We don't use __attribute__((packed)) because it's not\n> portable, and our existing uses have generally been OK, because our\n> data structures are organized around 8-byte alignment. We might have\n> problems on a theoretical 128-bit processor or something.\n\nIn addition to portability, using '__attribute__((packed))' could hurt\nperformance (and, in a large index, that might have a noticeable effect).\n\nAs for dropping 'ondisk_cache_entry()', I didn't want to drop it only from\nthe \"read\" operation (and use something like the \"parse left-to-right\"\nstrategy below) while leaving it in \"write.\" And, as you mentioned later,\nchanging 'ce_write_entry()' is a lot more invasive than what's already in\nthis patch and possibly out-of-scope.\n\n> Another strategy is to just parse left-to-right, advancing the byte\n> pointer. Like:\n> \n>   ce->ce_state_data.sd_ctime.sec = get_be32(ondisk);\n>   ondisk += sizeof(uint32_t);\n>   ce->ce_state_data.sd_mtime.sec = get_be32(ondisk);\n>   ondisk += sizeof(uint32_t);\n>   ...etc...\n> \n> You can even stick that in a helper function that does the get_b32() and\n> advances, so you know they're always done in sync. See pack-bitmap.c's\n> read_be32(), etc. IMHO this produces a nice result because the reading\n> code itself becomes the source of truth for the format.\n> \n\n...\n\n> One final note, though:\n> \n>> +\tce->ce_stat_data.sd_mtime.sec = get_be32(ondisk + offsetof(struct ondisk_cache_entry, mtime)\n>> +\t\t\t\t\t\t\t+ offsetof(struct cache_time, sec));\n> \n> Here (and elsewhere), you can assume that the offsetof() \"sec\" in\n> cache_time is 0, for two reasons:\n> \n>   - I didn't look up chapter and verse, but I'm pretty sure the standard\n>     does guarantee that the first field of a struct is at the beginning.\n> \n>   - If there's any padding, this whole scheme is hosed anyway, because\n>     it means sizeof(cache_time) is bigger than we expect, which messes\n>     up the offsetof() the entry after us (in this case sd_dev).\n> \n> So this can just be:\n> \n>   ce->ce_stat_data.sd_mtime.sec = get_be32(ondisk + offsetof(struct ondisk_cache_entry, mtime));\n> \n> which is mercifully shorter.\n> \n> Assuming we dismiss the rest of what I said as not worth it for a\n> minimal fix, I do think that simplification is worth rolling a v2.\n\nThat makes sense from a technical perspective, but I included the starting\nentry offset for readability reasons. It might be confusing to someone\nunfamiliar with C struct memory alignment to see every other 'get_be32'\nrefer to the exact entry it's reading via the 'offsetof()', but have that\ninformation absent only for a few entries. And, the double 'offsetof()'\nwould still be used by the 'mtime.nsec'/'ctime.nsec' fields anyway.\n\nIn any case, if this patch is intended to be a short-lived change on the way\nto a more complete refactor and/or I'm being overzealous on the readability,\nI'd be happy to change it. :) \n\nThanks!\n\n> \n> -Peff\n> \n> PS BTW, I mentioned earlier \"can we just get rid of ondisk_cache_entry\".\n>    We also use it for the writing side, of course. That doesn't have\n>    alignment issues, but it does have the same \"I hope there's never any\n>    padding\" question. In an ideal world, it would be using the\n>    equivalent put_be32(), but again, that's getting out of the \"minimal\n>    fix\" territory.\n\n"},{"id":"463650","messageId":"YzHiyPkCgVHym3H4@coredump.intra.peff.net","threadId":"58506","inReplyTo":"e5954e90-6b5c-46a6-0842-b3d7d1e06b33@github.com","subject":"Re: [PATCH] read-cache: avoid misaligned reads in index v4","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-26T17:35:04Z","receivedAt":"2022-09-26T17:57:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 26, 2022 at 08:39:10AM -0700, Victoria Dye wrote:\n\n> > So this can just be:\n> > \n> >   ce->ce_stat_data.sd_mtime.sec = get_be32(ondisk + offsetof(struct ondisk_cache_entry, mtime));\n> > \n> > which is mercifully shorter.\n> > \n> > Assuming we dismiss the rest of what I said as not worth it for a\n> > minimal fix, I do think that simplification is worth rolling a v2.\n> \n> That makes sense from a technical perspective, but I included the starting\n> entry offset for readability reasons. It might be confusing to someone\n> unfamiliar with C struct memory alignment to see every other 'get_be32'\n> refer to the exact entry it's reading via the 'offsetof()', but have that\n> information absent only for a few entries. And, the double 'offsetof()'\n> would still be used by the 'mtime.nsec'/'ctime.nsec' fields anyway.\n\nAh, right, I wasn't looking close enough. I was thinking that you were\nreading the whole struct via a single function call, but of course that\nis not true with get_be32(), and the nsec loads just below make that\nobvious.\n\n> In any case, if this patch is intended to be a short-lived change on the way\n> to a more complete refactor and/or I'm being overzealous on the readability,\n> I'd be happy to change it. :) \n\nNo, I was just mis-reading it. I think what you've got here is a good\nstopping point to fix the immediate problem.\n\n-Peff\n"},{"id":"463655","messageId":"xmqqpmfiowb0.fsf@gitster.g","threadId":"58506","inReplyTo":"bb3a2470-7ff5-e4a6-040a-96e0e3833978@gmail.com","subject":"Re: [PATCH] read-cache: avoid misaligned reads in index v4","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-26T17:54:59Z","receivedAt":"2022-09-26T18:09:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> I was confused as to why this was safe as it means we're still reading\n> the individual fields from unaligned addresses. The reason it is safe\n> is that we use get_bexx() to read the fields and those functions\n> handle unaligned access. I wonder if it is worth adding a note to\n> clarify that if you re-roll.\n\nI've got so used to ours that gives unaligned accesses, but I guess\nin some other circles, get_XeYY() means reading YY bit-wide integer\nin X byte-order from a preperly aligned address, and if that is the\ncase, I do not mind such a comment somewhere.  It would help those\ncoming from such a background very much.\n\nThanks.\n\n\n"},{"id":"463664","messageId":"YzH4rDpHXdeLURSN@coredump.intra.peff.net","threadId":"58506","inReplyTo":"Yy4nkEnhuzt2iH+R@coredump.intra.peff.net","subject":"Re: [PATCH] read-cache: avoid misaligned reads in index v4","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-26T19:08:28Z","receivedAt":"2022-09-26T19:08:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 23, 2022 at 05:39:28PM -0400, Jeff King wrote:\n\n> Another strategy is to just parse left-to-right, advancing the byte\n> pointer. Like:\n> \n>   ce->ce_state_data.sd_ctime.sec = get_be32(ondisk);\n>   ondisk += sizeof(uint32_t);\n>   ce->ce_state_data.sd_mtime.sec = get_be32(ondisk);\n>   ondisk += sizeof(uint32_t);\n>   ...etc...\n> \n> You can even stick that in a helper function that does the get_b32() and\n> advances, so you know they're always done in sync. See pack-bitmap.c's\n> read_be32(), etc. IMHO this produces a nice result because the reading\n> code itself becomes the source of truth for the format.\n> \n> But one tricky thing there is if you want to parse out of order. And it\n> does seem that we read the struct out of order in this case. But I don't\n> think there's any reason we need to do so. Of course reordering the\n> function would make the change much more invasive.\n\nBy the way, this last paragraph turns out not to be true. We do rely on\nthe order because we need to know the length of the name (retrieved from\nthe flags field) in order to allocate the internal ce_entry, which has a\nFLEX_ARRAY. And we must allocate the struct before populating its fields\nfrom the earlier bytes of the ondisk entry.\n\nSo we either need to go out of order, or parse into a dummy ce_entry and\nthen memcpy the results into the heap-allocated one.\n\nYou can still do a partial conversion as below, which I do think\nimproves readability, but without getting rid of the match for the\nflagsp pointer, it feels like it may not be accomplishing enough to be\nworth it.\n\nNote also that there is some confusion with signed vs unsigned pointers.\nIt doesn't really matter in practice because get_be* is casting under\nthe hood, but the compiler is picky here. Arguably read_be32() should\ntake a void (just like get_be32() does). But I do find it a bit odd that\nall of the index code uses a signed pointer for the mmap. Most of our\nother code uses \"const unsigned char *\" to indicate that we expect\nbinary data. We could switch over, but it's a rather invasive patch. And\nwhile we get rid of some casts (e.g., when we call oidread()), we'd gain\nsome new ones (some code uses strtol() to parse ascii numbers).\n\nIn the patch below I hacked around it by passing through a local void\npointer. ;)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex d16eb97906..8668ded8f5 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1875,17 +1875,20 @@ static int read_index_extension(struct index_state *istate,\n \n static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,\n \t\t\t\t\t    unsigned int version,\n-\t\t\t\t\t    const char *ondisk,\n+\t\t\t\t\t    const void *ondisk_map,\n \t\t\t\t\t    unsigned long *ent_size,\n \t\t\t\t\t    const struct cache_entry *previous_ce)\n {\n+\tconst unsigned char *ondisk = ondisk_map;\n \tstruct cache_entry *ce;\n \tsize_t len;\n \tconst char *name;\n \tconst unsigned hashsz = the_hash_algo->rawsz;\n-\tconst char *flagsp = ondisk + offsetof(struct ondisk_cache_entry, data) + hashsz;\n+\tconst unsigned char *flagsp = ondisk + offsetof(struct ondisk_cache_entry, data) + hashsz;\n \tunsigned int flags;\n \tsize_t copy_len = 0;\n+\tsize_t pos;\n+\n \t/*\n \t * Adjacent cache entries tend to share the leading paths, so it makes\n \t * sense to only store the differences in later entries.  In the v4\n@@ -1935,24 +1938,21 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,\n \n \tce = mem_pool__ce_alloc(ce_mem_pool, len);\n \n-\tce->ce_stat_data.sd_ctime.sec = get_be32(ondisk + offsetof(struct ondisk_cache_entry, ctime)\n-\t\t\t\t\t\t\t+ offsetof(struct cache_time, sec));\n-\tce->ce_stat_data.sd_mtime.sec = get_be32(ondisk + offsetof(struct ondisk_cache_entry, mtime)\n-\t\t\t\t\t\t\t+ offsetof(struct cache_time, sec));\n-\tce->ce_stat_data.sd_ctime.nsec = get_be32(ondisk + offsetof(struct ondisk_cache_entry, ctime)\n-\t\t\t\t\t\t\t + offsetof(struct cache_time, nsec));\n-\tce->ce_stat_data.sd_mtime.nsec = get_be32(ondisk + offsetof(struct ondisk_cache_entry, mtime)\n-\t\t\t\t\t\t\t + offsetof(struct cache_time, nsec));\n-\tce->ce_stat_data.sd_dev   = get_be32(ondisk + offsetof(struct ondisk_cache_entry, dev));\n-\tce->ce_stat_data.sd_ino   = get_be32(ondisk + offsetof(struct ondisk_cache_entry, ino));\n-\tce->ce_mode  = get_be32(ondisk + offsetof(struct ondisk_cache_entry, mode));\n-\tce->ce_stat_data.sd_uid   = get_be32(ondisk + offsetof(struct ondisk_cache_entry, uid));\n-\tce->ce_stat_data.sd_gid   = get_be32(ondisk + offsetof(struct ondisk_cache_entry, gid));\n-\tce->ce_stat_data.sd_size  = get_be32(ondisk + offsetof(struct ondisk_cache_entry, size));\n+\tpos = 0;\n+\tce->ce_stat_data.sd_ctime.sec = read_be32(ondisk, &pos);\n+\tce->ce_stat_data.sd_ctime.nsec = read_be32(ondisk, &pos);\n+\tce->ce_stat_data.sd_mtime.sec = read_be32(ondisk, &pos);\n+\tce->ce_stat_data.sd_mtime.nsec = read_be32(ondisk, &pos);\n+\tce->ce_stat_data.sd_dev   = read_be32(ondisk, &pos);\n+\tce->ce_stat_data.sd_ino   = read_be32(ondisk, &pos);\n+\tce->ce_mode  = read_be32(ondisk, &pos);\n+\tce->ce_stat_data.sd_uid   = read_be32(ondisk, &pos);\n+\tce->ce_stat_data.sd_gid   = read_be32(ondisk, &pos);\n+\tce->ce_stat_data.sd_size  = read_be32(ondisk, &pos);\n \tce->ce_flags = flags & ~CE_NAMEMASK;\n \tce->ce_namelen = len;\n \tce->index = 0;\n-\toidread(&ce->oid, (const unsigned char *)ondisk + offsetof(struct ondisk_cache_entry, data));\n+\toidread(&ce->oid, ondisk + pos);\n \n \tif (expand_name_field) {\n \t\tif (copy_len)\n"},{"id":"463673","messageId":"YzH+IPFBGleIsAUe@coredump.intra.peff.net","threadId":"58506","inReplyTo":"YzH4rDpHXdeLURSN@coredump.intra.peff.net","subject":"Re: [PATCH] read-cache: avoid misaligned reads in index v4","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-26T19:31:44Z","receivedAt":"2022-09-26T19:31:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 26, 2022 at 03:08:28PM -0400, Jeff King wrote:\n\n> So we either need to go out of order, or parse into a dummy ce_entry and\n> then memcpy the results into the heap-allocated one.\n\nHere's the \"dummy\" version. I did this mostly to satisfy my own\ncuriosity, and am sharing so the effort isn't lost. If you are ready to\nmove on from the topic, don't feel compelled to read or respond. :)\n\nI do find it a bit more straight-forward, but the extra copy is ugly,\nplus the \"yuck\" comment.\n\ndiff --git a/read-cache.c b/read-cache.c\nindex d16eb97906..8773f833bb 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1875,17 +1875,19 @@ static int read_index_extension(struct index_state *istate,\n \n static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,\n \t\t\t\t\t    unsigned int version,\n-\t\t\t\t\t    const char *ondisk,\n+\t\t\t\t\t    const void *ondisk_map,\n \t\t\t\t\t    unsigned long *ent_size,\n \t\t\t\t\t    const struct cache_entry *previous_ce)\n {\n+\tconst unsigned char *ondisk = ondisk_map;\n+\tstruct cache_entry pe; /* parsed entry; not sized to hold name */\n \tstruct cache_entry *ce;\n \tsize_t len;\n-\tconst char *name;\n \tconst unsigned hashsz = the_hash_algo->rawsz;\n-\tconst char *flagsp = ondisk + offsetof(struct ondisk_cache_entry, data) + hashsz;\n \tunsigned int flags;\n \tsize_t copy_len = 0;\n+\tsize_t pos = 0;\n+\n \t/*\n \t * Adjacent cache entries tend to share the leading paths, so it makes\n \t * sense to only store the differences in later entries.  In the v4\n@@ -1895,24 +1897,35 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,\n \t */\n \tint expand_name_field = version == 4;\n \n-\t/* On-disk flags are just 16 bits */\n-\tflags = get_be16(flagsp);\n+\tpe.ce_stat_data.sd_ctime.sec = read_be32(ondisk, &pos);\n+\tpe.ce_stat_data.sd_ctime.nsec = read_be32(ondisk, &pos);\n+\tpe.ce_stat_data.sd_mtime.sec = read_be32(ondisk, &pos);\n+\tpe.ce_stat_data.sd_mtime.nsec = read_be32(ondisk, &pos);\n+\tpe.ce_stat_data.sd_dev   = read_be32(ondisk, &pos);\n+\tpe.ce_stat_data.sd_ino   = read_be32(ondisk, &pos);\n+\tpe.ce_mode  = read_be32(ondisk, &pos);\n+\tpe.ce_stat_data.sd_uid   = read_be32(ondisk, &pos);\n+\tpe.ce_stat_data.sd_gid   = read_be32(ondisk, &pos);\n+\tpe.ce_stat_data.sd_size  = read_be32(ondisk, &pos);\n+\n+\toidread(&pe.oid, ondisk + pos);\n+\tpos += hashsz;\n+\n+\tflags = read_be16(ondisk, &pos);\n \tlen = flags & CE_NAMEMASK;\n \n \tif (flags & CE_EXTENDED) {\n \t\tint extended_flags;\n-\t\textended_flags = get_be16(flagsp + sizeof(uint16_t)) << 16;\n+\t\textended_flags = read_be16(ondisk, &pos) << 16;\n \t\t/* We do not yet understand any bit out of CE_EXTENDED_FLAGS */\n \t\tif (extended_flags & ~CE_EXTENDED_FLAGS)\n \t\t\tdie(_(\"unknown index entry format 0x%08x\"), extended_flags);\n \t\tflags |= extended_flags;\n-\t\tname = (const char *)(flagsp + 2 * sizeof(uint16_t));\n \t}\n-\telse\n-\t\tname = (const char *)(flagsp + sizeof(uint16_t));\n+\tpe.ce_flags = flags & ~CE_NAMEMASK;\n \n \tif (expand_name_field) {\n-\t\tconst unsigned char *cp = (const unsigned char *)name;\n+\t\tconst unsigned char *cp = ondisk + pos;\n \t\tsize_t strip_len, previous_len;\n \n \t\t/* If we're at the beginning of a block, ignore the previous name */\n@@ -1924,43 +1937,29 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,\n \t\t\t\t\tprevious_ce->name);\n \t\t\tcopy_len = previous_len - strip_len;\n \t\t}\n-\t\tname = (const char *)cp;\n+\t\tpos = cp - ondisk;\n \t}\n \n \tif (len == CE_NAMEMASK) {\n-\t\tlen = strlen(name);\n+\t\tlen = strlen((const char *)ondisk + pos);\n \t\tif (expand_name_field)\n \t\t\tlen += copy_len;\n \t}\n \n \tce = mem_pool__ce_alloc(ce_mem_pool, len);\n-\n-\tce->ce_stat_data.sd_ctime.sec = get_be32(ondisk + offsetof(struct ondisk_cache_entry, ctime)\n-\t\t\t\t\t\t\t+ offsetof(struct cache_time, sec));\n-\tce->ce_stat_data.sd_mtime.sec = get_be32(ondisk + offsetof(struct ondisk_cache_entry, mtime)\n-\t\t\t\t\t\t\t+ offsetof(struct cache_time, sec));\n-\tce->ce_stat_data.sd_ctime.nsec = get_be32(ondisk + offsetof(struct ondisk_cache_entry, ctime)\n-\t\t\t\t\t\t\t + offsetof(struct cache_time, nsec));\n-\tce->ce_stat_data.sd_mtime.nsec = get_be32(ondisk + offsetof(struct ondisk_cache_entry, mtime)\n-\t\t\t\t\t\t\t + offsetof(struct cache_time, nsec));\n-\tce->ce_stat_data.sd_dev   = get_be32(ondisk + offsetof(struct ondisk_cache_entry, dev));\n-\tce->ce_stat_data.sd_ino   = get_be32(ondisk + offsetof(struct ondisk_cache_entry, ino));\n-\tce->ce_mode  = get_be32(ondisk + offsetof(struct ondisk_cache_entry, mode));\n-\tce->ce_stat_data.sd_uid   = get_be32(ondisk + offsetof(struct ondisk_cache_entry, uid));\n-\tce->ce_stat_data.sd_gid   = get_be32(ondisk + offsetof(struct ondisk_cache_entry, gid));\n-\tce->ce_stat_data.sd_size  = get_be32(ondisk + offsetof(struct ondisk_cache_entry, size));\n-\tce->ce_flags = flags & ~CE_NAMEMASK;\n-\tce->ce_namelen = len;\n+\tmemcpy(ce, &pe, sizeof(pe));\n \tce->index = 0;\n-\toidread(&ce->oid, (const unsigned char *)ondisk + offsetof(struct ondisk_cache_entry, data));\n+\tce->ce_namelen = len;\n+\t/* yuck, our memcpy overwrote this */\n+\tce->mem_pool_allocated = 1;\n \n \tif (expand_name_field) {\n \t\tif (copy_len)\n \t\t\tmemcpy(ce->name, previous_ce->name, copy_len);\n-\t\tmemcpy(ce->name + copy_len, name, len + 1 - copy_len);\n-\t\t*ent_size = (name - ((char *)ondisk)) + len + 1 - copy_len;\n+\t\tmemcpy(ce->name + copy_len, ondisk + pos, len + 1 - copy_len);\n+\t\t*ent_size = pos + len + 1 - copy_len;\n \t} else {\n-\t\tmemcpy(ce->name, name, len + 1);\n+\t\tmemcpy(ce->name, ondisk + pos, len + 1);\n \t\t*ent_size = ondisk_ce_size(ce);\n \t}\n \treturn ce;\n"},{"id":"463684","messageId":"xmqqmtalagdi.fsf@gitster.g","threadId":"58506","inReplyTo":"YzH+IPFBGleIsAUe@coredump.intra.peff.net","subject":"Re: [PATCH] read-cache: avoid misaligned reads in index v4","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-26T23:02:49Z","receivedAt":"2022-09-26T23:03:02Z","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> I do find it a bit more straight-forward, but the extra copy is ugly,\n> plus the \"yuck\" comment.\n\nYeah, the \"yuck\" memcpy() is, eh, unfortunate.  Consistently using a\nsingle \"pointer\" (pos) to keep track of where we are does make the\nstructure of the code appear uniform and easier to follow, though.\n\n"},{"id":"463858","messageId":"pull.1366.v2.git.1664385541084.gitgitgadget@gmail.com","threadId":"58506","inReplyTo":"pull.1366.git.1663962236069.gitgitgadget@gmail.com","subject":"[PATCH v2] read-cache: avoid misaligned reads in index v4","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-28T17:19:00Z","receivedAt":"2022-09-28T17:19:11Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye <vdye@github.com>\n\nThe process for reading the index into memory from disk is to first read its\ncontents into a single memory-mapped file buffer (type 'char *'), then\nsequentially convert each on-disk index entry into a corresponding incore\n'cache_entry'. To access the contents of the on-disk entry for processing, a\nmoving pointer within the memory-mapped file is cast to type 'struct\nondisk_cache_entry *'.\n\nIn index v4, the entries in the on-disk index file are written *without*\naligning their first byte to a 4-byte boundary; entries are a variable\nlength (depending on the entry name and whether or not extended flags are\nused). As a result, casting the 'char *' buffer pointer to 'struct\nondisk_cache_entry *' then accessing its contents in a 'SANITIZE=undefined'\nbuild can trigger the following error:\n\n  read-cache.c:1886:46: runtime error: member access within misaligned\n  address <address> for type 'struct ondisk_cache_entry', which requires 4\n  byte alignment\n\nAvoid this error by reading fields directly from the 'char *' buffer, using\nthe 'offsetof' individual fields in 'struct ondisk_cache_entry'.\nAdditionally, add documentation describing why the new approach avoids the\nmisaligned address error, as well as advice on how to improve the\nimplementation in the future.\n\nReported-by: Jeff King <peff@peff.net>\nSigned-off-by: Victoria Dye <vdye@github.com>\n---\n    read-cache: avoid misaligned reads in index v4\n    \n    This fixes the bug reported in [1], where unaligned index entries in the\n    memory-mapped index file triggered a 'SANITIZE=undefined' error due to\n    casting to & accessing unaligned data a 4 byte-aligned 'struct\n    ondisk_cache_entry *' type.\n    \n    In addition to the originally-reported 't9210-scalar.sh' now passing in\n    a 'SANITIZE=undefined' build, I did some light testing by first writing\n    a v4 index with a released version of Git (v2.37), then running some\n    index-modifying operations ('git status', 'git add') with this patch's\n    changes, then again running 'git status' with the stable version. I\n    didn't see anything out of the ordinary but, considering how critical\n    \"reading the index\" is, I'd very much appreciate some extra-thorough\n    reviews on this patch. :)\n    \n    \n    Changes since V1\n    ================\n    \n     * Added a comment explaining the somewhat unintuitive use of the\n       'ondisk' buffer in 'create_from_disk()', including why the 'get_be*'\n       functions do not run into the same misaligned address error this\n       patch is fixing.\n     * Added a 'NEEDSWORK' comment recommending a future refactor to avoid\n       the need for 'struct ondisk_cache_entry' entirely.\n    \n    Thanks!\n    \n     * Victoria\n    \n    [1]\n    https://lore.kernel.org/git/YywzNTzd72tox8Z+@coredump.intra.peff.net/\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1366%2Fvdye%2Fbugfix%2Findex-v4-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1366/vdye/bugfix/index-v4-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1366\n\nRange-diff vs v1:\n\n 1:  be9039c810b ! 1:  0f0fe7cabfd read-cache: avoid misaligned reads in index v4\n     @@ Commit message\n      \n          Avoid this error by reading fields directly from the 'char *' buffer, using\n          the 'offsetof' individual fields in 'struct ondisk_cache_entry'.\n     +    Additionally, add documentation describing why the new approach avoids the\n     +    misaligned address error, as well as advice on how to improve the\n     +    implementation in the future.\n      \n          Reported-by: Jeff King <peff@peff.net>\n          Signed-off-by: Victoria Dye <vdye@github.com>\n      \n       ## read-cache.c ##\n      @@ read-cache.c: static int read_index_extension(struct index_state *istate,\n     + \treturn 0;\n     + }\n       \n     ++/*\n     ++ * Parses the contents of the cache entry contained within the 'ondisk' buffer\n     ++ * into a new incore 'cache_entry'.\n     ++ *\n     ++ * Note that 'char *ondisk' may not be aligned to a 4-byte address interval in\n     ++ * index v4, so we cannot cast it to 'struct ondisk_cache_entry *' and access\n     ++ * its members. Instead, we use the byte offsets of members within the struct to\n     ++ * identify where 'get_be16()', 'get_be32()', and 'oidread()' (which can all\n     ++ * read from an unaligned memory buffer) should read from the 'ondisk' buffer\n     ++ * into the corresponding incore 'cache_entry' members.\n     ++ */\n       static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,\n       \t\t\t\t\t    unsigned int version,\n      -\t\t\t\t\t    struct ondisk_cache_entry *ondisk,\n     @@ read-cache.c: static struct cache_entry *create_from_disk(struct mem_pool *ce_me\n      -\tce->ce_stat_data.sd_uid   = get_be32(&ondisk->uid);\n      -\tce->ce_stat_data.sd_gid   = get_be32(&ondisk->gid);\n      -\tce->ce_stat_data.sd_size  = get_be32(&ondisk->size);\n     ++\t/*\n     ++\t * NEEDSWORK: using 'offsetof()' is cumbersome and should be replaced\n     ++\t * with something more akin to 'load_bitmap_entries_v1()'s use of\n     ++\t * 'read_be16'/'read_be32'. For consistency with the corresponding\n     ++\t * ondisk entry write function ('copy_cache_entry_to_ondisk()'), this\n     ++\t * should be done at the same time as removing references to\n     ++\t * 'ondisk_cache_entry' there.\n     ++\t */\n      +\tce->ce_stat_data.sd_ctime.sec = get_be32(ondisk + offsetof(struct ondisk_cache_entry, ctime)\n      +\t\t\t\t\t\t\t+ offsetof(struct cache_time, sec));\n      +\tce->ce_stat_data.sd_mtime.sec = get_be32(ondisk + offsetof(struct ondisk_cache_entry, mtime)\n\n\n read-cache.c | 61 ++++++++++++++++++++++++++++++++++++----------------\n 1 file changed, 42 insertions(+), 19 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex b09128b1884..32024029274 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1873,9 +1873,20 @@ static int read_index_extension(struct index_state *istate,\n \treturn 0;\n }\n \n+/*\n+ * Parses the contents of the cache entry contained within the 'ondisk' buffer\n+ * into a new incore 'cache_entry'.\n+ *\n+ * Note that 'char *ondisk' may not be aligned to a 4-byte address interval in\n+ * index v4, so we cannot cast it to 'struct ondisk_cache_entry *' and access\n+ * its members. Instead, we use the byte offsets of members within the struct to\n+ * identify where 'get_be16()', 'get_be32()', and 'oidread()' (which can all\n+ * read from an unaligned memory buffer) should read from the 'ondisk' buffer\n+ * into the corresponding incore 'cache_entry' members.\n+ */\n static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,\n \t\t\t\t\t    unsigned int version,\n-\t\t\t\t\t    struct ondisk_cache_entry *ondisk,\n+\t\t\t\t\t    const char *ondisk,\n \t\t\t\t\t    unsigned long *ent_size,\n \t\t\t\t\t    const struct cache_entry *previous_ce)\n {\n@@ -1883,7 +1894,7 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,\n \tsize_t len;\n \tconst char *name;\n \tconst unsigned hashsz = the_hash_algo->rawsz;\n-\tconst uint16_t *flagsp = (const uint16_t *)(ondisk->data + hashsz);\n+\tconst char *flagsp = ondisk + offsetof(struct ondisk_cache_entry, data) + hashsz;\n \tunsigned int flags;\n \tsize_t copy_len = 0;\n \t/*\n@@ -1901,15 +1912,15 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,\n \n \tif (flags & CE_EXTENDED) {\n \t\tint extended_flags;\n-\t\textended_flags = get_be16(flagsp + 1) << 16;\n+\t\textended_flags = get_be16(flagsp + sizeof(uint16_t)) << 16;\n \t\t/* We do not yet understand any bit out of CE_EXTENDED_FLAGS */\n \t\tif (extended_flags & ~CE_EXTENDED_FLAGS)\n \t\t\tdie(_(\"unknown index entry format 0x%08x\"), extended_flags);\n \t\tflags |= extended_flags;\n-\t\tname = (const char *)(flagsp + 2);\n+\t\tname = (const char *)(flagsp + 2 * sizeof(uint16_t));\n \t}\n \telse\n-\t\tname = (const char *)(flagsp + 1);\n+\t\tname = (const char *)(flagsp + sizeof(uint16_t));\n \n \tif (expand_name_field) {\n \t\tconst unsigned char *cp = (const unsigned char *)name;\n@@ -1935,20 +1946,32 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,\n \n \tce = mem_pool__ce_alloc(ce_mem_pool, len);\n \n-\tce->ce_stat_data.sd_ctime.sec = get_be32(&ondisk->ctime.sec);\n-\tce->ce_stat_data.sd_mtime.sec = get_be32(&ondisk->mtime.sec);\n-\tce->ce_stat_data.sd_ctime.nsec = get_be32(&ondisk->ctime.nsec);\n-\tce->ce_stat_data.sd_mtime.nsec = get_be32(&ondisk->mtime.nsec);\n-\tce->ce_stat_data.sd_dev   = get_be32(&ondisk->dev);\n-\tce->ce_stat_data.sd_ino   = get_be32(&ondisk->ino);\n-\tce->ce_mode  = get_be32(&ondisk->mode);\n-\tce->ce_stat_data.sd_uid   = get_be32(&ondisk->uid);\n-\tce->ce_stat_data.sd_gid   = get_be32(&ondisk->gid);\n-\tce->ce_stat_data.sd_size  = get_be32(&ondisk->size);\n+\t/*\n+\t * NEEDSWORK: using 'offsetof()' is cumbersome and should be replaced\n+\t * with something more akin to 'load_bitmap_entries_v1()'s use of\n+\t * 'read_be16'/'read_be32'. For consistency with the corresponding\n+\t * ondisk entry write function ('copy_cache_entry_to_ondisk()'), this\n+\t * should be done at the same time as removing references to\n+\t * 'ondisk_cache_entry' there.\n+\t */\n+\tce->ce_stat_data.sd_ctime.sec = get_be32(ondisk + offsetof(struct ondisk_cache_entry, ctime)\n+\t\t\t\t\t\t\t+ offsetof(struct cache_time, sec));\n+\tce->ce_stat_data.sd_mtime.sec = get_be32(ondisk + offsetof(struct ondisk_cache_entry, mtime)\n+\t\t\t\t\t\t\t+ offsetof(struct cache_time, sec));\n+\tce->ce_stat_data.sd_ctime.nsec = get_be32(ondisk + offsetof(struct ondisk_cache_entry, ctime)\n+\t\t\t\t\t\t\t + offsetof(struct cache_time, nsec));\n+\tce->ce_stat_data.sd_mtime.nsec = get_be32(ondisk + offsetof(struct ondisk_cache_entry, mtime)\n+\t\t\t\t\t\t\t + offsetof(struct cache_time, nsec));\n+\tce->ce_stat_data.sd_dev   = get_be32(ondisk + offsetof(struct ondisk_cache_entry, dev));\n+\tce->ce_stat_data.sd_ino   = get_be32(ondisk + offsetof(struct ondisk_cache_entry, ino));\n+\tce->ce_mode  = get_be32(ondisk + offsetof(struct ondisk_cache_entry, mode));\n+\tce->ce_stat_data.sd_uid   = get_be32(ondisk + offsetof(struct ondisk_cache_entry, uid));\n+\tce->ce_stat_data.sd_gid   = get_be32(ondisk + offsetof(struct ondisk_cache_entry, gid));\n+\tce->ce_stat_data.sd_size  = get_be32(ondisk + offsetof(struct ondisk_cache_entry, size));\n \tce->ce_flags = flags & ~CE_NAMEMASK;\n \tce->ce_namelen = len;\n \tce->index = 0;\n-\toidread(&ce->oid, ondisk->data);\n+\toidread(&ce->oid, (const unsigned char *)ondisk + offsetof(struct ondisk_cache_entry, data));\n \n \tif (expand_name_field) {\n \t\tif (copy_len)\n@@ -2117,12 +2140,12 @@ static unsigned long load_cache_entry_block(struct index_state *istate,\n \tunsigned long src_offset = start_offset;\n \n \tfor (i = offset; i < offset + nr; i++) {\n-\t\tstruct ondisk_cache_entry *disk_ce;\n \t\tstruct cache_entry *ce;\n \t\tunsigned long consumed;\n \n-\t\tdisk_ce = (struct ondisk_cache_entry *)(mmap + src_offset);\n-\t\tce = create_from_disk(ce_mem_pool, istate->version, disk_ce, &consumed, previous_ce);\n+\t\tce = create_from_disk(ce_mem_pool, istate->version,\n+\t\t\t\t      mmap + src_offset,\n+\t\t\t\t      &consumed, previous_ce);\n \t\tset_index_entry(istate, i, ce);\n \n \t\tsrc_offset += consumed;\n\nbase-commit: 1b3d6e17fe83eb6f79ffbac2f2c61bbf1eaef5f8\n-- \ngitgitgadget\n"},{"id":"463860","messageId":"xmqqleq3v1w7.fsf@gitster.g","threadId":"58506","inReplyTo":"pull.1366.v2.git.1664385541084.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] read-cache: avoid misaligned reads in index v4","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-28T17:34:32Z","receivedAt":"2022-09-28T17:34:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Victoria Dye via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> +\t/*\n> +\t * NEEDSWORK: using 'offsetof()' is cumbersome and should be replaced\n> +\t * with something more akin to 'load_bitmap_entries_v1()'s use of\n> +\t * 'read_be16'/'read_be32'. For consistency with the corresponding\n> +\t * ondisk entry write function ('copy_cache_entry_to_ondisk()'), this\n> +\t * should be done at the same time as removing references to\n> +\t * 'ondisk_cache_entry' there.\n> +\t */\n\nSounds sensible.  Will replace and merge down to 'next'--- it will\nmost likely be part of the first batch in the next cycle.\n\nThanks, all.\n\n"}]}