Nathan Hartman wrote on Tue, 28 Jul 2020 10:42 -0400:
> 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.
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
Received on 2020-07-30 00:28:24 CEST