On Mon, Jul 27, 2020 at 3:28 AM Daniel Shahaf <d...@daniel.shahaf.name> wrote: > Nathan Hartman wrote on Mon, 27 Jul 2020 03:29 +00:00: > > 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)
All the more reason to fix it. The second patch looks good. It includes the first patch + two similar changes for fsx. Committed in r1880374. Orivej, thanks again for your patches! Nathan
