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@xxxxxxxxxxxxxxxxxxxxxxx/
