{"thread":{"id":"58491","subject":"t9210-scalar.sh fails with SANITIZE=undefined","startedAt":"2022-09-22T10:04:48Z","lastAt":"2022-09-22T22:56:30Z","messageCount":6,"participants":["Jeff King","Derrick Stolee","Victoria Dye","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"463431","messageId":"YywzNTzd72tox8Z+@coredump.intra.peff.net","threadId":"58491","inReplyTo":null,"subject":"t9210-scalar.sh fails with SANITIZE=undefined","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-22T10:04:37Z","receivedAt":"2022-09-22T10:04:48Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Running t9210 with the tip of master triggers a problem with UBSan:\n\n  $ make SANITIZE=undefined\n  [...]\n  $ cd t\n  $ ./t9210-scalar.sh -v -i\n  [...]\n  read-cache.c:1886:46: runtime error: member access within misaligned address 0x7f7c09bf7055 for type 'struct ondisk_cache_entry', which requires 4 byte alignment\n  0x7f7c09bf7055: note: pointer points here\n  33 2e 74 00 63 2c 31  42 17 3f 49 72 63 2c 31  42 17 3f 49 72 00 00 fe  01 02 60 06 4d 00 00 81  a4\n              ^\n\nNow here's the interesting part. We do carefully read most of the data\nout of the struct with get_be16(), which should handle alignment (we\nhave to do so because that on_disk_cache_entry is literally just a cast\nfrom an mmap'd buffer). But the line in question is just:\n\n  const uint16_t *flagsp = (const uint16_t *)(ondisk->data + hashsz);\n\nIt's not even reading anything, but just computing an offset within the\nstruct. I don't think that line in particular is to blame. If I use an\nolder version of Git that predates it on the same repo generated by\nt9210, I get a similar error for a different line.\n\nAnother thing to note is that the command which fails isn't scalar\nitself! It's just vanilla \"git add -- loose.t\". But it's curious that we\nnever saw this alignment problem before. I wonder if the scalar-cloned\nrepository has some index options turned on that trigger the issue.\n\nI didn't dig further. It's obviously new in v2.38.0-rc1, but I'm not\nsure it's a show-stopper. It _might_ have been there all along, and is\njust now surfacing. Or it might be in an existing experimental feature\nthat just wasn't exercised enough in the tests. Or if it really is new\nin scalar, then it will only hurt people using scalar, which didn't\nexist before. So I don't think it's a regression in the strictest sense,\nbut it might be nice to get a more accurate understanding of the problem\nbefore the release.\n\n-Peff\n"},{"id":"463438","messageId":"ff921c34-139a-9e2b-ca1f-d6f9f7213d1b@github.com","threadId":"58491","inReplyTo":"YywzNTzd72tox8Z+@coredump.intra.peff.net","subject":"Re: t9210-scalar.sh fails with SANITIZE=undefined","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-09-22T12:35:22Z","receivedAt":"2022-09-22T12:35:40Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/22/2022 6:04 AM, Jeff King wrote:\n> Running t9210 with the tip of master triggers a problem with UBSan:\n> \n>   $ make SANITIZE=undefined\n>   [...]\n>   $ cd t\n>   $ ./t9210-scalar.sh -v -i\n>   [...]\n>   read-cache.c:1886:46: runtime error: member access within misaligned address 0x7f7c09bf7055 for type 'struct ondisk_cache_entry', which requires 4 byte alignment\n>   0x7f7c09bf7055: note: pointer points here\n>   33 2e 74 00 63 2c 31  42 17 3f 49 72 63 2c 31  42 17 3f 49 72 00 00 fe  01 02 60 06 4d 00 00 81  a4\n>               ^\n> \n> Now here's the interesting part. We do carefully read most of the data\n> out of the struct with get_be16(), which should handle alignment (we\n> have to do so because that on_disk_cache_entry is literally just a cast\n> from an mmap'd buffer). But the line in question is just:\n> \n>   const uint16_t *flagsp = (const uint16_t *)(ondisk->data + hashsz);\n> \n> It's not even reading anything, but just computing an offset within the\n> struct. I don't think that line in particular is to blame. If I use an\n> older version of Git that predates it on the same repo generated by\n> t9210, I get a similar error for a different line.\n> \n> Another thing to note is that the command which fails isn't scalar\n> itself! It's just vanilla \"git add -- loose.t\". But it's curious that we\n> never saw this alignment problem before. I wonder if the scalar-cloned\n> repository has some index options turned on that trigger the issue.\n> \n> I didn't dig further. It's obviously new in v2.38.0-rc1, but I'm not\n> sure it's a show-stopper. It _might_ have been there all along, and is\n> just now surfacing. Or it might be in an existing experimental feature\n> that just wasn't exercised enough in the tests. Or if it really is new\n> in scalar, then it will only hurt people using scalar, which didn't\n> exist before. So I don't think it's a regression in the strictest sense,\n> but it might be nice to get a more accurate understanding of the problem\n> before the release.\n\nInteresting find!\n\nHere are the index-related settings that Scalar sets as of -rc1:\n\n* core.preloadIndex=true\n* index.threads=true\n* index.version=4\n\nMy gut feeling is that index.version=4 might be the culprit. I thought\nwe tested GIT_TEST_INDEX_VERSION=4 in some CI modes, but apparently we\ndo not. Do you get the same error in other tests with that environment\nvariable?\n\nThanks,\n-Stolee\n"},{"id":"463465","messageId":"Yyyzk3FVjmms7dkO@coredump.intra.peff.net","threadId":"58491","inReplyTo":"ff921c34-139a-9e2b-ca1f-d6f9f7213d1b@github.com","subject":"Re: t9210-scalar.sh fails with SANITIZE=undefined","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-22T19:12:19Z","receivedAt":"2022-09-22T19:12:24Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 22, 2022 at 08:35:22AM -0400, Derrick Stolee wrote:\n\n> > I didn't dig further. It's obviously new in v2.38.0-rc1, but I'm not\n> > sure it's a show-stopper. It _might_ have been there all along, and is\n> > just now surfacing. Or it might be in an existing experimental feature\n> > that just wasn't exercised enough in the tests. Or if it really is new\n> > in scalar, then it will only hurt people using scalar, which didn't\n> > exist before. So I don't think it's a regression in the strictest sense,\n> > but it might be nice to get a more accurate understanding of the problem\n> > before the release.\n> \n> Interesting find!\n> \n> Here are the index-related settings that Scalar sets as of -rc1:\n> \n> * core.preloadIndex=true\n> * index.threads=true\n> * index.version=4\n> \n> My gut feeling is that index.version=4 might be the culprit. I thought\n> we tested GIT_TEST_INDEX_VERSION=4 in some CI modes, but apparently we\n> do not. Do you get the same error in other tests with that environment\n> variable?\n\nYeah, that seems by far the most likely of those three. And indeed,\nrunning with GIT_TEST_INDEX_VERSION=4 causes even t0000 to fail with the\nsame problem. A minimal reproduction in git.git is just:\n\n  make SANITIZE=undefined\n  git clone . tmp\n  cd tmp\n  rm .git/index\n  export GIT_INDEX_VERSION=4\n  ../git reset --hard ;# ok, writes v4 index\n  ../git reset --hard ;# fails reading unaligned v4 index\n\nSo it seems like a problem with the v4 format in general. Which...yikes.\n\n-Peff\n"},{"id":"463478","messageId":"50c57a60-8346-6952-93d9-432a70ef74c5@github.com","threadId":"58491","inReplyTo":"Yyyzk3FVjmms7dkO@coredump.intra.peff.net","subject":"Re: t9210-scalar.sh fails with SANITIZE=undefined","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2022-09-22T22:09:52Z","receivedAt":"2022-09-22T22:10:06Z","isPatch":false,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Jeff King wrote:\n> On Thu, Sep 22, 2022 at 08:35:22AM -0400, Derrick Stolee wrote:\n> \n>>> I didn't dig further. It's obviously new in v2.38.0-rc1, but I'm not\n>>> sure it's a show-stopper. It _might_ have been there all along, and is\n>>> just now surfacing. Or it might be in an existing experimental feature\n>>> that just wasn't exercised enough in the tests. Or if it really is new\n>>> in scalar, then it will only hurt people using scalar, which didn't\n>>> exist before. So I don't think it's a regression in the strictest sense,\n>>> but it might be nice to get a more accurate understanding of the problem\n>>> before the release.\n>>\n>> Interesting find!\n>>\n>> Here are the index-related settings that Scalar sets as of -rc1:\n>>\n>> * core.preloadIndex=true\n>> * index.threads=true\n>> * index.version=4\n>>\n>> My gut feeling is that index.version=4 might be the culprit. I thought\n>> we tested GIT_TEST_INDEX_VERSION=4 in some CI modes, but apparently we\n>> do not. Do you get the same error in other tests with that environment\n>> variable?\n> \n> Yeah, that seems by far the most likely of those three. And indeed,\n> running with GIT_TEST_INDEX_VERSION=4 causes even t0000 to fail with the\n> same problem. A minimal reproduction in git.git is just:\n> \n>   make SANITIZE=undefined\n>   git clone . tmp\n>   cd tmp\n>   rm .git/index\n>   export GIT_INDEX_VERSION=4\n>   ../git reset --hard ;# ok, writes v4 index\n>   ../git reset --hard ;# fails reading unaligned v4 index\n> \n> So it seems like a problem with the v4 format in general. Which...yikes.\n\nOther than allowing us to use a (non-packed) 'struct ondisk_cache_entry' to\nparse the index entries, is there any reason why the on-disk index entries\nshould (or need to be) 4-byte aligned? If not, we could update how we read\nthe 'ondisk' index entry in 'create_from_disk()' to avoid the misalignment.\nE.g.:\n\n------------------8<------------------8<------------------8<------------------\ndiff --git a/read-cache.c b/read-cache.c\nindex b09128b188..f132a3f256 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------------------>8------------------>8------------------>8------------------\n\nthe do the same sort of conversion with the rest of the function. It's\ncertainly uglier than just using the 'struct ondisk_index_entry *' pointer,\nbut it should avoid the misaligned addressing error.\n\n> \n> -Peff\n\n"},{"id":"463481","messageId":"YyzhR8CGu2CNQMfJ@coredump.intra.peff.net","threadId":"58491","inReplyTo":"50c57a60-8346-6952-93d9-432a70ef74c5@github.com","subject":"Re: t9210-scalar.sh fails with SANITIZE=undefined","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-22T22:27:19Z","receivedAt":"2022-09-22T22:27:24Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 22, 2022 at 03:09:52PM -0700, Victoria Dye wrote:\n\n> Other than allowing us to use a (non-packed) 'struct ondisk_cache_entry' to\n> parse the index entries, is there any reason why the on-disk index entries\n> should (or need to be) 4-byte aligned? If not, we could update how we read\n> the 'ondisk' index entry in 'create_from_disk()' to avoid the misalignment.\n\nI don't think so. And indeed, we already use get_be16(), etc, for most\nof the access (which is mostly there for endian-fixing, but also\nresolves alignment problems).\n\n> ------------------8<------------------8<------------------8<------------------\n> diff --git a/read-cache.c b/read-cache.c\n> index b09128b188..f132a3f256 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> ------------------>8------------------>8------------------>8------------------\n> \n> the do the same sort of conversion with the rest of the function. It's\n> certainly uglier than just using the 'struct ondisk_index_entry *' pointer,\n> but it should avoid the misaligned addressing error.\n\nYeah, I think that's probably the only reasonable solution. I thought\nditching ondisk_cache_entry entirely (which is basically what this is\ndoing) would be a tough sell, but a quick \"grep\" shows it really isn't\nused in all that many spots.\n\nI also wondered why other versions do not have a similar problem. After\nall, cache entries contain pathnames which are going to be of varying\nlengths. But this seems telling:\n\n  $ git grep -m1 -B1 -A2 align_padding_size\n  read-cache.c-/* These are only used for v3 or lower */\n  read-cache.c:#define align_padding_size(size, len) ((size + (len) + 8) & ~7) - (size + len)\n  read-cache.c-#define align_flex_name(STRUCT,len) ((offsetof(struct STRUCT,data) + (len) + 8) & ~7)\n  read-cache.c-#define ondisk_cache_entry_size(len) align_flex_name(ondisk_cache_entry,len)\n\nSo we actually pad the entries in earlier versions to align them, but\ndon't in v4. I'm not sure if that was a conscious choice to save space,\nor an unintended consequence (though it is mentioned in the docs, I\nthink that came after the code).\n\nThat's probably all obvious to people who work with the index a lot.\nIt's the one part of Git I've mostly managed to remain oblivious to. :)\n\n-Peff\n"},{"id":"463483","messageId":"xmqq7d1vxbl6.fsf@gitster.g","threadId":"58491","inReplyTo":"YyzhR8CGu2CNQMfJ@coredump.intra.peff.net","subject":"Re: t9210-scalar.sh fails with SANITIZE=undefined","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-22T22:56:21Z","receivedAt":"2022-09-22T22:56:30Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I also wondered why other versions do not have a similar problem. After\n> all, cache entries contain pathnames which are going to be of varying\n> lengths. But this seems telling:\n>\n>   $ git grep -m1 -B1 -A2 align_padding_size\n>   read-cache.c-/* These are only used for v3 or lower */\n>   read-cache.c:#define align_padding_size(size, len) ((size + (len) + 8) & ~7) - (size + len)\n>   read-cache.c-#define align_flex_name(STRUCT,len) ((offsetof(struct STRUCT,data) + (len) + 8) & ~7)\n>   read-cache.c-#define ondisk_cache_entry_size(len) align_flex_name(ondisk_cache_entry,len)\n>\n> So we actually pad the entries in earlier versions to align them, but\n> don't in v4. I'm not sure if that was a conscious choice to save space,\n> or an unintended consequence (though it is mentioned in the docs, I\n> think that came after the code).\n\nI think we didn't even have on-disk vs in-core distinction in the\nearly index code.  The active_cache[] array used to be an array of\npointers into the (read-only) mmapped memory, peeking into on-disk\nindex we just \"read\".  Back when v4 was introduced, that arrangement\nwas (thought to be) long gone---we iterated over the mmapped memory\nand used create_from_disk() to munge the on-disk representation into\na machine native form.  At that point, there was no point in having\nthe padding---we are supposed to be using get_beXX() and stuff\nwithout having to worry about alignment requirements.\n\n"}]}