attr: fix msan issue in read_attr_from_index

[spectral54]

cc: Jeff King peff@peff.net

[spectral54]

/submit

[gitgitgadget[bot]]

Submitted as pull.1747.git.1718654424683.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1747/spectral54/msan-attr-v1

To fetch this version to local tag pr-1747/spectral54/msan-attr-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1747/spectral54/msan-attr-v1

[gitgitgadget[bot]]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Kyle Lippincott via GitGitGadget" <gitgitgadget@gmail.com> writes:

> Make the call to `read_attr_from_buf` conditional on `buf` being
> non-NULL, ensuring that `size` is not read if it's never set.

Makes good sense.

> Signed-off-by: Kyle Lippincott <spectral@google.com>
> ---
>     attr: fix msan issue in read_attr_from_index
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1747%2Fspectral54%2Fmsan-attr-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1747/spectral54/msan-attr-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1747
>
>  attr.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/attr.c b/attr.c
> index 300f994ba6e..a2e0775f7e5 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -865,7 +865,8 @@ static struct attr_stack *read_attr_from_index(struct index_state *istate,
>       stack = read_attr_from_blob(istate, &istate->cache[sparse_dir_pos]->oid, relative_path, flags);
>   } else {
>       buf = read_blob_data_from_index(istate, path, &size);
> -     stack = read_attr_from_buf(buf, size, path, flags);
> +     if (buf)
> +         stack = read_attr_from_buf(buf, size, path, flags);
>   }
>   return stack;
>  }

Not directly related to the issue this patch addresses, but I notice
that both buf and size variables have unnecesarily wide scope.  As a
clean-up we may want to move their declaration into this "} else {"
block.  But that is totally outside the scope (no pun intended) of
this patch.

Will queue.
Thanks.  

[gitgitgadget[bot]]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Kyle Lippincott via GitGitGadget" <gitgitgadget@gmail.com> writes:

> The issue exists because `size` is an output parameter from
> `read_blob_data_from_index`, but it's only modified if
> `read_blob_data_from_index` returns non-NULL.

Correct.

> The read of `size` when
> calling `read_attr_from_buf` unconditionally may read from an
> uninitialized value. `read_attr_from_buf` checks that `buf` is non-NULL
> before reading from `size`, but by then it's already too late: the
> uninitialized read will have happened already.

Yes, but it is dubious that reading an uninitialized value that we
know will not be used is a problem, so I am inclined to say that
MSAN is giving a false positive here.

> Furthermore, there's no
> guarantee that the compiler won't reorder things so that it checks
> `size` before checking `!buf`.

This I do not understand.  Are you talking about buf vs length here
in the callee?

        static struct attr_stack *read_attr_from_buf(char *buf, size_t length,
                                                     const char *path, unsigned flags)
        {
                struct attr_stack *res;
                char *sp;
                int lineno = 0;

                if (!buf)
                        return NULL;
                if (length >= ATTR_MAX_FILE_SIZE) {
                        warning(_("ignoring overly large gitattributes blob '%s'"), path);
                        free(buf);
                        return NULL;
                }

At the machine level, a prefetch may happen from both buf and
length, but the program ought to behave the same way as the code is
executed serially as written.  If the compiler allows the outside
world to observe that resulting code checks length even when buf is
NULL, such a compiler is broken.  So I do not think that is what you
are referring to, but then I do not know what problem you are
describing.

Having said all that ...

> Make the call to `read_attr_from_buf` conditional on `buf` being
> non-NULL, ensuring that `size` is not read if it's never set.

... this makes the logic at the caller crystal clear, so even if
there are suboptimal checker that bothers us with false positives,
the change itself justifies itself, I would say.

>   } else {
>       buf = read_blob_data_from_index(istate, path, &size);
> -     stack = read_attr_from_buf(buf, size, path, flags);
> +     if (buf)
> +         stack = read_attr_from_buf(buf, size, path, flags);
>   }
>   return stack;

Thanks.

[gitgitgadget[bot]]

On the Git mailing list, Junio C Hamano wrote (reply to this):

Junio C Hamano <gitster@pobox.com> writes:

> Having said all that ...
>
>> Make the call to `read_attr_from_buf` conditional on `buf` being
>> non-NULL, ensuring that `size` is not read if it's never set.
>
> ... this makes the logic at the caller crystal clear, so even if
> there are suboptimal checker that bothers us with false positives,
> the change itself justifies itself, I would say.

Well, "even if there were *no* MSAN or other issues wrt usage of size"
was what I wanted to say.  Sorry for a noise.

>>      } else {
>>          buf = read_blob_data_from_index(istate, path, &size);
>> -        stack = read_attr_from_buf(buf, size, path, flags);
>> +        if (buf)
>> +            stack = read_attr_from_buf(buf, size, path, flags);
>>      }
>>      return stack;
>
> Thanks.

[gitgitgadget[bot]]

On the Git mailing list, Kyle Lippincott wrote (reply to this):

On Mon, Jun 17, 2024 at 1:30 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Kyle Lippincott via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > The issue exists because `size` is an output parameter from
> > `read_blob_data_from_index`, but it's only modified if
> > `read_blob_data_from_index` returns non-NULL.
>
> Correct.
>
> > The read of `size` when
> > calling `read_attr_from_buf` unconditionally may read from an
> > uninitialized value. `read_attr_from_buf` checks that `buf` is non-NULL
> > before reading from `size`, but by then it's already too late: the
> > uninitialized read will have happened already.
>
> Yes, but it is dubious that reading an uninitialized value that we
> know will not be used is a problem, so I am inclined to say that
> MSAN is giving a false positive here.
>
> > Furthermore, there's no
> > guarantee that the compiler won't reorder things so that it checks
> > `size` before checking `!buf`.
>
> This I do not understand.  Are you talking about buf vs length here
> in the callee?
>
>         static struct attr_stack *read_attr_from_buf(char *buf, size_t length,
>                                                      const char *path, unsigned flags)
>         {
>                 struct attr_stack *res;
>                 char *sp;
>                 int lineno = 0;
>
>                 if (!buf)
>                         return NULL;
>                 if (length >= ATTR_MAX_FILE_SIZE) {
>                         warning(_("ignoring overly large gitattributes blob '%s'"), path);
>                         free(buf);
>                         return NULL;
>                 }
>
> At the machine level, a prefetch may happen from both buf and
> length, but the program ought to behave the same way as the code is
> executed serially as written.  If the compiler allows the outside
> world to observe that resulting code checks length even when buf is
> NULL, such a compiler is broken.  So I do not think that is what you
> are referring to, but then I do not know what problem you are
> describing.

Once there's an uninitialized read, we're in undefined behavior
territory. There's no requirement that the compiler keep the code
operating the way we'd logically expect once there's undefined
behavior, especially with the optimizer involved.

I think that you're right though: when I wrote this I'd convinced
myself that this wasn't guaranteed to work, but taking a look now I
can't think of a way for this to go wrong, because afaik size_t is
such a simple type, conceptually. When compiling `read_attr_from_buf`,
it can't actually assume any relationship between `buf` and `length`.
Maybe I was thinking that with link-time optimizations (whole-program
optimizations) it can go wrong? I'm not remembering what I was
thinking about when I wrote that, sorry.

>
> Having said all that ...
>
> > Make the call to `read_attr_from_buf` conditional on `buf` being
> > non-NULL, ensuring that `size` is not read if it's never set.
>
> ... this makes the logic at the caller crystal clear, so even if
> there are suboptimal checker that bothers us with false positives,
> the change itself justifies itself, I would say.
>
> >       } else {
> >               buf = read_blob_data_from_index(istate, path, &size);
> > -             stack = read_attr_from_buf(buf, size, path, flags);
> > +             if (buf)
> > +                     stack = read_attr_from_buf(buf, size, path, flags);
> >       }
> >       return stack;
>
> Thanks.

[gitgitgadget[bot]]

This branch is now known as kl/attr-read-attr-fromindex-msan-workaround.

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/1ffe7c8bd74dc0e10a984a0b98a32b757e65a31a.

[gitgitgadget[bot]]

There was a status update in the "New Topics" section about the branch kl/attr-read-attr-fromindex-msan-workaround on the Git mailing list:

Code clarification to avoid an appearance of using an uninitialized
variable.

Will merge to 'next'?
source: <pull.1747.git.1718654424683.gitgitgadget@gmail.com>

[gitgitgadget[bot]]

On the Git mailing list, Jeff King wrote (reply to this):

On Mon, Jun 17, 2024 at 08:00:24PM +0000, Kyle Lippincott via GitGitGadget wrote:

> The issue exists because `size` is an output parameter from
> `read_blob_data_from_index`, but it's only modified if
> `read_blob_data_from_index` returns non-NULL. The read of `size` when
> calling `read_attr_from_buf` unconditionally may read from an
> uninitialized value. `read_attr_from_buf` checks that `buf` is non-NULL
> before reading from `size`, but by then it's already too late: the
> uninitialized read will have happened already. Furthermore, there's no
> guarantee that the compiler won't reorder things so that it checks
> `size` before checking `!buf`.
> 
> Make the call to `read_attr_from_buf` conditional on `buf` being
> non-NULL, ensuring that `size` is not read if it's never set.

Yeah, this is the same one I mentioned when bisecting in the other
thread[1]. But I got confused by applying my fixup patch at various
points in the bisection, and thought it _used_ to be a problem, and
isn't anymore. It's the other way around. It was introduced by
c793f9cb08, which moved the NULL check into the helper.

That patch is from Taylor, but I'm listed as a co-author, and I'm almost
certain moving that NULL check was my suggestion. So it's doubly bad
that I didn't figure out what was going on earlier. ;)

Possible UB aside, I doubt this can trigger bad behavior in practice.
But I also wouldn't call it a false positive in MSan. We really are
reading the uninitialized value and passing it. Your fix here is the
obviously correct thing to do.

-Peff

[1] https://lore.kernel.org/git/20240608081855.GA2390433@coredump.intra.peff.net/

[gitgitgadget[bot]]

User Jeff King <peff@peff.net> has been added to the cc: list.

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/5dd6567a387ee94974281a8f00f9ee297cbbafcc.

[gitgitgadget[bot]]

This patch series was integrated into next via https://github.com/git/git/commit/eebafb2d7142c956adbfc8ee3e56e4df3a172559.

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/549c31e40c753c4b0c135c922992fb6126b68f34.

[gitgitgadget[bot]]

There was a status update in the "Cooking" section about the branch kl/attr-read-attr-fromindex-msan-workaround on the Git mailing list:

Code clarification to avoid an appearance of using an uninitialized
variable.

Will merge to 'master'.
source: <pull.1747.git.1718654424683.gitgitgadget@gmail.com>

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/c546bcd81428eb3148250d323a8f9e1dbd6b5572.

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/532083fd1609b840cb4076440800ed7b4f2d9df0.

[gitgitgadget[bot]]

This patch series was integrated into master via https://github.com/git/git/commit/532083fd1609b840cb4076440800ed7b4f2d9df0.

[gitgitgadget[bot]]

This patch series was integrated into next via https://github.com/git/git/commit/532083fd1609b840cb4076440800ed7b4f2d9df0.

[gitgitgadget[bot]]

Closed via 532083fd1609b840cb4076440800ed7b4f2d9df0.