On Sat, Jul 25, 2020 at 7:27 PM Orivej Desh <ori...@gmx.fr> wrote: > > Clang 10 memory sanitizer reports an uninitialized read of .offset in > if ((entry > 0 && proto_entry.offset == 0) || eof) > when read_l2p_entry_from_proto_index set eof and left the proto_entry unset.
Thanks for your patches! +1 to commit the first patch... I'll review the second patch tomorrow (unless someone beats me to it). I've reviewed the first one so far. In reviewing, my concern was whether changing the order of the expressions could alter behavior. I studied the code paths through svn_fs_fs__l2p_index_append(), read_l2p_entry_from_proto_index(), and read_uint64_from_proto_index(). By my reading, when the 'if' in question runs, 'proto_entry.offset' could be uninitialized if and only if 'eof' is true: * If, by chance, it is non-zero, 'eof' prevails and the block is entered. * If, by chance, it is zero, the block is entered without testing 'eof', but as 'eof' must be true, that would happen anyway. In this scenario, the correct block is executed for the wrong reason. As you say, it's harmless. Nevertheless, I think the patch should be applied because (1) we should not read uninitialized data, (2) testing 'eof' first expresses the intent more accurately. I ran the test suite on latest trunk with this patch applied (fsfs x local/svn/serf); all tests pass. :-) Thanks again! Nathan
