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/ (local)