Re: [PATCH] Fix harmless uninitialized read in svn_fs_fs__l2p_index_append

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.

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 05:29:59 CEST