Nathan Hartman wrote on Mon, 27 Jul 2020 03:29 +00:00:
> On Sat, Jul 25, 2020 at 7:27 PM Orivej Desh <orivej_at_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
>
Received on 2020-07-27 09:31:48 CEST