You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Orivej Desh <or...@gmx.fr> on 2020/07/25 23:27:22 UTC
[PATCH] Fix harmless uninitialized read in
svn_fs_fs__l2p_index_append
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.
Re: [PATCH] Fix harmless uninitialized read in svn_fs_fs__l2p_index_append
Posted by Nathan Hartman <ha...@gmail.com>.
On Thu, Jul 30, 2020 at 9:56 PM Daniel Shahaf <d....@daniel.shahaf.name>
wrote:
> Nathan Hartman wrote on Thu, 30 Jul 2020 19:17 -0400:
> > On Wed, Jul 29, 2020 at 6:28 PM Daniel Shahaf <d....@daniel.shahaf.name>
> > wrote:
> > > Nathan Hartman wrote on Tue, 28 Jul 2020 10:42 -0400:
> > > > 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.)
> >
> > For FSFS, +1.
> >
>
> Nominated for 1.14.x with your vote.
Cool! Thank you.
Thanks also for fixing the conflict/merge bot error.
Nathan
Re: [PATCH] Fix harmless uninitialized read in
svn_fs_fs__l2p_index_append
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Nathan Hartman wrote on Thu, 30 Jul 2020 19:17 -0400:
> On Wed, Jul 29, 2020 at 6:28 PM Daniel Shahaf <d....@daniel.shahaf.name>
> wrote:
> > Nathan Hartman wrote on Tue, 28 Jul 2020 10:42 -0400:
> > > 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.)
>
> For FSFS, +1.
>
Nominated for 1.14.x with your vote.
Cheers,
Daniel
Re: [PATCH] Fix harmless uninitialized read in svn_fs_fs__l2p_index_append
Posted by Nathan Hartman <ha...@gmail.com>.
On Wed, Jul 29, 2020 at 6:28 PM Daniel Shahaf <d....@daniel.shahaf.name>
wrote:
> Nathan Hartman wrote on Tue, 28 Jul 2020 10:42 -0400:
> > 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.)
For FSFS, +1.
For FSX, I'd like to experiment some more first.
Before I committed the patch, I ran the test suite on trunk with the
patch applied on [fsfs, fsx] x [local, svn, serf] and all tests
passed.
Now, before nominating for backport, I wanted to re-test on the 1.14.x
branch + the changes in r1880374. All tests pass for fsfs. But the fsx
tests are giving slightly different results on each run, with or
without the changes in r1880374: About 1 or 2 fails per run, different
tests each time.) I don't think this is related to the changes in
r1880374. I'll go into more detail in a new thread...
Again, all tests pass for fsfs.
Regarding backports, we actually have quite a few bug fixes in trunk.
It's on my TODO list to go through these and perhaps nominate some for
backport. Also there's a patch in our mailing list archive that IIRC
fixes some testsuite issues on Windows that needs to be committed.
It's on my TODO list to find it and commit it, unless someone does it
first.
Nathan
Re: [PATCH] Fix harmless uninitialized read in
svn_fs_fs__l2p_index_append
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
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
Re: [PATCH] Fix harmless uninitialized read in svn_fs_fs__l2p_index_append
Posted by Nathan Hartman <ha...@gmail.com>.
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.
Orivej, thanks again for your patches!
Nathan
Re: [PATCH] Fix harmless uninitialized read in svn_fs_fs__l2p_index_append
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Nathan Hartman wrote on Mon, 27 Jul 2020 03:29 +00:00:
> On Sat, Jul 25, 2020 at 7:27 PM Orivej Desh <or...@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
>
Re: [PATCH] Fix harmless uninitialized read in svn_fs_fs__l2p_index_append
Posted by Nathan Hartman <ha...@gmail.com>.
On Sat, Jul 25, 2020 at 7:27 PM Orivej Desh <or...@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
Re: [PATCH] Fix harmless uninitialized read in
svn_fs_fs__l2p_index_append
Posted by Orivej Desh <or...@gmx.fr>.
* Daniel Shahaf <d....@daniel.shahaf.name> [2020-07-27]
> Orivej Desh wrote on Sat, 25 Jul 2020 23:27 +0000:
> > 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.
>
> Orivej Desh wrote on Sat, 25 Jul 2020 23:27 +0000:
> > [[[
> > Fix harmless uninitialized read in svn_fs_fs__l2p_index_append
> >
> > * subversion/libsvn_fs_fs/index.c
> > (svn_fs_fs__l2p_index_append): Do not access proto_entry.offset when
> > it is unset due to reaching eof.
>
> Well written.
>
> > ]]]
> > +++ subversion/libsvn_fs_fs/index.c (working copy)
> > @@ -827,7 +827,7 @@ svn_fs_fs__l2p_index_append(svn_checksum_t **check
> > /* handle new revision */
> > - if ((entry > 0 && proto_entry.offset == 0) || eof)
> > + if (eof || (entry > 0 && proto_entry.offset == 0))
>
> Looks good to me, +1.
>
> Does libsvn_fs_x need the same change?
Indeed, I have fixed memory-sanitized "svnadmin create --fs-type fsx"
with the attached patch.
> Thanks for the patch,
>
> Daniel
Re: [PATCH] Fix harmless uninitialized read in
svn_fs_fs__l2p_index_append
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Orivej Desh wrote on Sat, 25 Jul 2020 23:27 +0000:
> 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.
Orivej Desh wrote on Sat, 25 Jul 2020 23:27 +0000:
> [[[
> Fix harmless uninitialized read in svn_fs_fs__l2p_index_append
>
> * subversion/libsvn_fs_fs/index.c
> (svn_fs_fs__l2p_index_append): Do not access proto_entry.offset when
> it is unset due to reaching eof.
Well written.
> ]]]
> +++ subversion/libsvn_fs_fs/index.c (working copy)
> @@ -827,7 +827,7 @@ svn_fs_fs__l2p_index_append(svn_checksum_t **check
> /* handle new revision */
> - if ((entry > 0 && proto_entry.offset == 0) || eof)
> + if (eof || (entry > 0 && proto_entry.offset == 0))
Looks good to me, +1.
Does libsvn_fs_x need the same change?
Thanks for the patch,
Daniel