Nathan Hartman wrote on Mon, 27 Jul 2020 03:29 +00:00: > 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. >
Agree with your analysis. Regarding the label "harmless", though, I think there's a further hair to split. The value of an uninitialized local variable may be either indeterminate or a trap representation. If the value of PROTO_ENTRY.OFFSET is a plain indeterminate value, then the uninitialized read would be harmless (under a conforming C implementation), for the reasons Nathan explained. However, if the value of PROTO_ENTRY.OFFSET is a trap representation, the uninitialized read would be undefined behaviour — which might well still be harmless in practice, of course, but would not be guaranteed to be harmless under a conforming C implementation. Cheers, Daniel (who's reminded of CVE-2008-0166, https://www.schneier.com/blog/archives/2008/05/random_number_b.html) > I ran the test suite on latest trunk with this patch applied (fsfs x > local/svn/serf); all tests pass. :-) > > Thanks again! > Nathan >
