Nathan Hartman wrote on Tue, 28 Jul 2020 10:42 -0400: > 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.
Thanks. Nominate these for backport? My +1 on the FSFS part for 1.14.x. (ENOTIME to review the FSX part currently.) Cheers, Daniel > Orivej, thanks again for your patches! > > Nathan
