t9210-scalar.sh fails with SANITIZE=undefined

t9210-scalar.sh fails with SANITIZE=undefined

[Jeff King]

Running t9210 with the tip of master triggers a problem with UBSan:

  $ make SANITIZE=undefined
  [...]
  $ cd t
  $ ./t9210-scalar.sh -v -i
  [...]
  read-cache.c:1886:46: runtime error: member access within misaligned address 0x7f7c09bf7055 for type 'struct ondisk_cache_entry', which requires 4 byte alignment
  0x7f7c09bf7055: note: pointer points here
  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
              ^

Now here's the interesting part. We do carefully read most of the data
out of the struct with get_be16(), which should handle alignment (we
have to do so because that on_disk_cache_entry is literally just a cast
from an mmap'd buffer). But the line in question is just:

  const uint16_t *flagsp = (const uint16_t *)(ondisk->data + hashsz);

It's not even reading anything, but just computing an offset within the
struct. I don't think that line in particular is to blame. If I use an
older version of Git that predates it on the same repo generated by
t9210, I get a similar error for a different line.

Another thing to note is that the command which fails isn't scalar
itself! It's just vanilla "git add -- loose.t". But it's curious that we
never saw this alignment problem before. I wonder if the scalar-cloned
repository has some index options turned on that trigger the issue.

I didn't dig further. It's obviously new in v2.38.0-rc1, but I'm not
sure it's a show-stopper. It _might_ have been there all along, and is
just now surfacing. Or it might be in an existing experimental feature
that just wasn't exercised enough in the tests. Or if it really is new
in scalar, then it will only hurt people using scalar, which didn't
exist before. So I don't think it's a regression in the strictest sense,
but it might be nice to get a more accurate understanding of the problem
before the release.

-Peff

Re: t9210-scalar.sh fails with SANITIZE=undefined

[Derrick Stolee]

On 9/22/2022 6:04 AM, Jeff King wrote:
> Running t9210 with the tip of master triggers a problem with UBSan:
> 
>   $ make SANITIZE=undefined
>   [...]
>   $ cd t
>   $ ./t9210-scalar.sh -v -i
>   [...]
>   read-cache.c:1886:46: runtime error: member access within misaligned address 0x7f7c09bf7055 for type 'struct ondisk_cache_entry', which requires 4 byte alignment
>   0x7f7c09bf7055: note: pointer points here
>   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
>               ^
> 
> Now here's the interesting part. We do carefully read most of the data
> out of the struct with get_be16(), which should handle alignment (we
> have to do so because that on_disk_cache_entry is literally just a cast
> from an mmap'd buffer). But the line in question is just:
> 
>   const uint16_t *flagsp = (const uint16_t *)(ondisk->data + hashsz);
> 
> It's not even reading anything, but just computing an offset within the
> struct. I don't think that line in particular is to blame. If I use an
> older version of Git that predates it on the same repo generated by
> t9210, I get a similar error for a different line.
> 
> Another thing to note is that the command which fails isn't scalar
> itself! It's just vanilla "git add -- loose.t". But it's curious that we
> never saw this alignment problem before. I wonder if the scalar-cloned
> repository has some index options turned on that trigger the issue.
> 
> I didn't dig further. It's obviously new in v2.38.0-rc1, but I'm not
> sure it's a show-stopper. It _might_ have been there all along, and is
> just now surfacing. Or it might be in an existing experimental feature
> that just wasn't exercised enough in the tests. Or if it really is new
> in scalar, then it will only hurt people using scalar, which didn't
> exist before. So I don't think it's a regression in the strictest sense,
> but it might be nice to get a more accurate understanding of the problem
> before the release.

Interesting find!

Here are the index-related settings that Scalar sets as of -rc1:

* core.preloadIndex=true
* index.threads=true
* index.version=4

My gut feeling is that index.version=4 might be the culprit. I thought
we tested GIT_TEST_INDEX_VERSION=4 in some CI modes, but apparently we
do not. Do you get the same error in other tests with that environment
variable?

Thanks,
-Stolee

Re: t9210-scalar.sh fails with SANITIZE=undefined

[Jeff King]

On Thu, Sep 22, 2022 at 08:35:22AM -0400, Derrick Stolee wrote:

> > I didn't dig further. It's obviously new in v2.38.0-rc1, but I'm not
> > sure it's a show-stopper. It _might_ have been there all along, and is
> > just now surfacing. Or it might be in an existing experimental feature
> > that just wasn't exercised enough in the tests. Or if it really is new
> > in scalar, then it will only hurt people using scalar, which didn't
> > exist before. So I don't think it's a regression in the strictest sense,
> > but it might be nice to get a more accurate understanding of the problem
> > before the release.
> 
> Interesting find!
> 
> Here are the index-related settings that Scalar sets as of -rc1:
> 
> * core.preloadIndex=true
> * index.threads=true
> * index.version=4
> 
> My gut feeling is that index.version=4 might be the culprit. I thought
> we tested GIT_TEST_INDEX_VERSION=4 in some CI modes, but apparently we
> do not. Do you get the same error in other tests with that environment
> variable?

Yeah, that seems by far the most likely of those three. And indeed,
running with GIT_TEST_INDEX_VERSION=4 causes even t0000 to fail with the
same problem. A minimal reproduction in git.git is just:

  make SANITIZE=undefined
  git clone . tmp
  cd tmp
  rm .git/index
  export GIT_INDEX_VERSION=4
  ../git reset --hard ;# ok, writes v4 index
  ../git reset --hard ;# fails reading unaligned v4 index

So it seems like a problem with the v4 format in general. Which...yikes.

-Peff

Re: t9210-scalar.sh fails with SANITIZE=undefined

[Victoria Dye]

Jeff King wrote:
> On Thu, Sep 22, 2022 at 08:35:22AM -0400, Derrick Stolee wrote:
> 
>>> I didn't dig further. It's obviously new in v2.38.0-rc1, but I'm not
>>> sure it's a show-stopper. It _might_ have been there all along, and is
>>> just now surfacing. Or it might be in an existing experimental feature
>>> that just wasn't exercised enough in the tests. Or if it really is new
>>> in scalar, then it will only hurt people using scalar, which didn't
>>> exist before. So I don't think it's a regression in the strictest sense,
>>> but it might be nice to get a more accurate understanding of the problem
>>> before the release.
>>
>> Interesting find!
>>
>> Here are the index-related settings that Scalar sets as of -rc1:
>>
>> * core.preloadIndex=true
>> * index.threads=true
>> * index.version=4
>>
>> My gut feeling is that index.version=4 might be the culprit. I thought
>> we tested GIT_TEST_INDEX_VERSION=4 in some CI modes, but apparently we
>> do not. Do you get the same error in other tests with that environment
>> variable?
> 
> Yeah, that seems by far the most likely of those three. And indeed,
> running with GIT_TEST_INDEX_VERSION=4 causes even t0000 to fail with the
> same problem. A minimal reproduction in git.git is just:
> 
>   make SANITIZE=undefined
>   git clone . tmp
>   cd tmp
>   rm .git/index
>   export GIT_INDEX_VERSION=4
>   ../git reset --hard ;# ok, writes v4 index
>   ../git reset --hard ;# fails reading unaligned v4 index
> 
> So it seems like a problem with the v4 format in general. Which...yikes.

Other than allowing us to use a (non-packed) 'struct ondisk_cache_entry' to
parse the index entries, is there any reason why the on-disk index entries
should (or need to be) 4-byte aligned? If not, we could update how we read
the 'ondisk' index entry in 'create_from_disk()' to avoid the misalignment.
E.g.:

------------------8<------------------8<------------------8<------------------
diff --git a/read-cache.c b/read-cache.c
index b09128b188..f132a3f256 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1875,7 +1875,7 @@ static int read_index_extension(struct index_state *istate,
 
 static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,
 					    unsigned int version,
-					    struct ondisk_cache_entry *ondisk,
+					    const char *ondisk,
 					    unsigned long *ent_size,
 					    const struct cache_entry *previous_ce)
 {
@@ -1883,7 +1883,7 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,
 	size_t len;
 	const char *name;
 	const unsigned hashsz = the_hash_algo->rawsz;
-	const uint16_t *flagsp = (const uint16_t *)(ondisk->data + hashsz);
+	const char *flagsp = ondisk + offsetof(struct ondisk_cache_entry, data) + hashsz;
 	unsigned int flags;
 	size_t copy_len = 0;
 	/*
------------------>8------------------>8------------------>8------------------

the do the same sort of conversion with the rest of the function. It's
certainly uglier than just using the 'struct ondisk_index_entry *' pointer,
but it should avoid the misaligned addressing error.

> 
> -Peff

Re: t9210-scalar.sh fails with SANITIZE=undefined

[Jeff King]

On Thu, Sep 22, 2022 at 03:09:52PM -0700, Victoria Dye wrote:

> Other than allowing us to use a (non-packed) 'struct ondisk_cache_entry' to
> parse the index entries, is there any reason why the on-disk index entries
> should (or need to be) 4-byte aligned? If not, we could update how we read
> the 'ondisk' index entry in 'create_from_disk()' to avoid the misalignment.

I don't think so. And indeed, we already use get_be16(), etc, for most
of the access (which is mostly there for endian-fixing, but also
resolves alignment problems).

> ------------------8<------------------8<------------------8<------------------
> diff --git a/read-cache.c b/read-cache.c
> index b09128b188..f132a3f256 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -1875,7 +1875,7 @@ static int read_index_extension(struct index_state *istate,
>  
>  static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,
>  					    unsigned int version,
> -					    struct ondisk_cache_entry *ondisk,
> +					    const char *ondisk,
>  					    unsigned long *ent_size,
>  					    const struct cache_entry *previous_ce)
>  {
> @@ -1883,7 +1883,7 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool,
>  	size_t len;
>  	const char *name;
>  	const unsigned hashsz = the_hash_algo->rawsz;
> -	const uint16_t *flagsp = (const uint16_t *)(ondisk->data + hashsz);
> +	const char *flagsp = ondisk + offsetof(struct ondisk_cache_entry, data) + hashsz;
>  	unsigned int flags;
>  	size_t copy_len = 0;
>  	/*
> ------------------>8------------------>8------------------>8------------------
> 
> the do the same sort of conversion with the rest of the function. It's
> certainly uglier than just using the 'struct ondisk_index_entry *' pointer,
> but it should avoid the misaligned addressing error.

Yeah, I think that's probably the only reasonable solution. I thought
ditching ondisk_cache_entry entirely (which is basically what this is
doing) would be a tough sell, but a quick "grep" shows it really isn't
used in all that many spots.

I also wondered why other versions do not have a similar problem. After
all, cache entries contain pathnames which are going to be of varying
lengths. But this seems telling:

  $ git grep -m1 -B1 -A2 align_padding_size
  read-cache.c-/* These are only used for v3 or lower */
  read-cache.c:#define align_padding_size(size, len) ((size + (len) + 8) & ~7) - (size + len)
  read-cache.c-#define align_flex_name(STRUCT,len) ((offsetof(struct STRUCT,data) + (len) + 8) & ~7)
  read-cache.c-#define ondisk_cache_entry_size(len) align_flex_name(ondisk_cache_entry,len)

So we actually pad the entries in earlier versions to align them, but
don't in v4. I'm not sure if that was a conscious choice to save space,
or an unintended consequence (though it is mentioned in the docs, I
think that came after the code).

That's probably all obvious to people who work with the index a lot.
It's the one part of Git I've mostly managed to remain oblivious to. :)

-Peff

Re: t9210-scalar.sh fails with SANITIZE=undefined

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I also wondered why other versions do not have a similar problem. After
> all, cache entries contain pathnames which are going to be of varying
> lengths. But this seems telling:
>
>   $ git grep -m1 -B1 -A2 align_padding_size
>   read-cache.c-/* These are only used for v3 or lower */
>   read-cache.c:#define align_padding_size(size, len) ((size + (len) + 8) & ~7) - (size + len)
>   read-cache.c-#define align_flex_name(STRUCT,len) ((offsetof(struct STRUCT,data) + (len) + 8) & ~7)
>   read-cache.c-#define ondisk_cache_entry_size(len) align_flex_name(ondisk_cache_entry,len)
>
> So we actually pad the entries in earlier versions to align them, but
> don't in v4. I'm not sure if that was a conscious choice to save space,
> or an unintended consequence (though it is mentioned in the docs, I
> think that came after the code).

I think we didn't even have on-disk vs in-core distinction in the
early index code.  The active_cache[] array used to be an array of
pointers into the (read-only) mmapped memory, peeking into on-disk
index we just "read".  Back when v4 was introduced, that arrangement
was (thought to be) long gone---we iterated over the mmapped memory
and used create_from_disk() to munge the on-disk representation into
a machine native form.  At that point, there was no point in having
the padding---we are supposed to be using get_beXX() and stuff
without having to worry about alignment requirements.