On Mon, Jul 27, 2020 at 3:28 AM Daniel Shahaf <d.s_at_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
Received on 2020-07-28 16:42:31 CEST