You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Ben Reser <be...@reser.org> on 2015/01/13 19:59:53 UTC
Re: svn commit: r1631598 - in /subversion/trunk/subversion: libsvn_fs_fs/verify.c
tests/libsvn_fs_fs/fs-fs-fuzzy-test.c
On 10/13/14 3:54 PM, stefan2@apache.org wrote:
> Author: stefan2
> Date: Mon Oct 13 22:54:13 2014
> New Revision: 1631598
>
> URL: http://svn.apache.org/r1631598
> Log:
> Add FSFS index checksum verification code to 'svnadmin verify'.
>
> We don't verify the index data against the checksums on every
> access as there is plenty of cross-verification within the indexes,
> between them as well as between index and rev data. Only if some
> inconsistency has been detected and the user wants to trace down
> the source (using 'svnadmin verify'), will we verify that the index
> data has not been tampered with.
>
> * subversion/libsvn_fs_fs/verify.c
> (verify_index_checksum,
> verify_index_checksums): New per rev / pack file verification code.
> (verify_index_consistency): Call the new index checksum test before
> using any of the index contents.
>
> * subversion/tests/libsvn_fs_fs/fs-fs-fuzzy-test.c
> (fuzzing_1_byte_1_rev): Remove the code that would accept certain
> index modifications. Accept only case changes
> in index MD5 digests because they don't affect
> the validity.
[snip]
> @@ -144,14 +120,30 @@ fuzzing_1_byte_1_rev(const char *repo_na
> err = svn_repos_verify_fs3(repos, revision, revision, TRUE, FALSE, FALSE,
> NULL, NULL, NULL, NULL, iterpool);
>
> - /* Benign changes may or may not be detected. */
> - if (!is_legal)
> + /* Case-only changes in checksum digests are not an error.
> + * We allow upper case chars to be used in MD5 checksums in all other
> + * places, thus restricting them here would be inconsistent. */
> + if ( i >= filesize - footer_len /* Within footer */
> + && c_old >= 'a' && c_old <= 'f' /* 'a' to 'f', only appear
> + in checksum digests */
> + && c_new == c_old - 'a' + 'A') /* respective upper case */
> + {
> + if (err)
> + {
> + /* Let us know where we were too strict ... */
> + printf("Detected case change in checksum digest at offset 0x%"
> + APR_UINT64_T_HEX_FMT " (%" APR_OFF_T_FMT ") in r%ld: "
> + "%c -> %c\n", i, i, revision, c_old, c_new);
> +
> + SVN_ERR(err);
> + }
> + }
> + else if (!err)
> {
> /* Let us know where we miss changes ... */
> - if (!err)
> - printf("Undetected mod at offset %"APR_UINT64_T_HEX_FMT
> - " (%"APR_OFF_T_FMT") in r%ld: 0x%02x -> 0x%02x\n",
> - i, i, revision, c_old, c_new);
> + printf("Undetected mod at offset 0x%"APR_UINT64_T_HEX_FMT
> + " (%"APR_OFF_T_FMT") in r%ld: 0x%02x -> 0x%02x\n",
> + i, i, revision, c_old, c_new);
>
> SVN_TEST_ASSERT(err);
> }
>
>
Those printf formats aren't valid on all platforms/builds of APR. In this case
i is an apr_off_t and you're using APR_UINT64_T_HEX_FMT with it. Unfortunately
you can't guarantee that apr_off_t is a 64-bit int. If LFS support isn't
enabled and the platform has its own off_t then APR will use whatever the size
is for that, or just an int if off_t isn't defined. See aprenv.py in APR's
source code for the various scenarios.
Not sure what you want to do about this since it makes the hex output more
difficult to do. I'd probably just remove the hex output since it's a test and
we can probably deal with that bit of convenience.
Re: svn commit: r1631598 - in /subversion/trunk/subversion:
libsvn_fs_fs/verify.c tests/libsvn_fs_fs/fs-fs-fuzzy-test.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Tue, Jan 13, 2015 at 9:17 PM, Ben Reser <be...@reser.org> wrote:
> On 1/13/15 11:44 AM, Ben Reser wrote:
> > On 1/13/15 11:34 AM, Ben Reser wrote:
> >> On 1/13/15 11:13 AM, Branko Čibej wrote:
> >>> Since it is a test, what's wrong with just casting the first vararg to
> >>> (apr_uint64_t) instead, since we "know" (i.e., hope) that off_t won't
> >>> overflow 64 bits ...
> >>
> >> If you cast it to apr_uint64_t it can read into memory it shouldn't be
> (i.e.
> >> the test may crash on platforms without 64-bit off_t). Maybe we don't
> care
> >> about those platforms, but I'm not aware of us requiring LFS/64-bit
> off_t.
> >
> > I should be clearer here. It can read into memory it shouldn't
> already. The
> > cast just makes the warning go away and hides the problem.
>
> Nevermind I was thinking in terms of passing pointers and this is passing
> by
> value not reference so Branko is right you can just cast the value.
> Committed
> in r1651469.
>
> Thanks for fixing this, Ben! Casting to uint64 is the right thing to do.
-- Stefan^2.
Re: svn commit: r1631598 - in /subversion/trunk/subversion: libsvn_fs_fs/verify.c
tests/libsvn_fs_fs/fs-fs-fuzzy-test.c
Posted by Ben Reser <be...@reser.org>.
On 1/13/15 11:44 AM, Ben Reser wrote:
> On 1/13/15 11:34 AM, Ben Reser wrote:
>> On 1/13/15 11:13 AM, Branko Čibej wrote:
>>> Since it is a test, what's wrong with just casting the first vararg to
>>> (apr_uint64_t) instead, since we "know" (i.e., hope) that off_t won't
>>> overflow 64 bits ...
>>
>> If you cast it to apr_uint64_t it can read into memory it shouldn't be (i.e.
>> the test may crash on platforms without 64-bit off_t). Maybe we don't care
>> about those platforms, but I'm not aware of us requiring LFS/64-bit off_t.
>
> I should be clearer here. It can read into memory it shouldn't already. The
> cast just makes the warning go away and hides the problem.
Nevermind I was thinking in terms of passing pointers and this is passing by
value not reference so Branko is right you can just cast the value. Committed
in r1651469.
Re: svn commit: r1631598 - in /subversion/trunk/subversion: libsvn_fs_fs/verify.c
tests/libsvn_fs_fs/fs-fs-fuzzy-test.c
Posted by Ben Reser <be...@reser.org>.
On 1/13/15 11:34 AM, Ben Reser wrote:
> On 1/13/15 11:13 AM, Branko Čibej wrote:
>> Since it is a test, what's wrong with just casting the first vararg to
>> (apr_uint64_t) instead, since we "know" (i.e., hope) that off_t won't
>> overflow 64 bits ...
>
> If you cast it to apr_uint64_t it can read into memory it shouldn't be (i.e.
> the test may crash on platforms without 64-bit off_t). Maybe we don't care
> about those platforms, but I'm not aware of us requiring LFS/64-bit off_t.
I should be clearer here. It can read into memory it shouldn't already. The
cast just makes the warning go away and hides the problem.
Re: svn commit: r1631598 - in /subversion/trunk/subversion: libsvn_fs_fs/verify.c
tests/libsvn_fs_fs/fs-fs-fuzzy-test.c
Posted by Ben Reser <be...@reser.org>.
On 1/13/15 11:13 AM, Branko Čibej wrote:
> Since it is a test, what's wrong with just casting the first vararg to
> (apr_uint64_t) instead, since we "know" (i.e., hope) that off_t won't
> overflow 64 bits ...
If you cast it to apr_uint64_t it can read into memory it shouldn't be (i.e.
the test may crash on platforms without 64-bit off_t). Maybe we don't care
about those platforms, but I'm not aware of us requiring LFS/64-bit off_t.
Re: svn commit: r1631598 - in /subversion/trunk/subversion: libsvn_fs_fs/verify.c
tests/libsvn_fs_fs/fs-fs-fuzzy-test.c
Posted by Branko Čibej <br...@wandisco.com>.
On 13.01.2015 19:59, Ben Reser wrote:
> On 10/13/14 3:54 PM, stefan2@apache.org wrote:
>> Author: stefan2
>> Date: Mon Oct 13 22:54:13 2014
>> New Revision: 1631598
>>
>> URL: http://svn.apache.org/r1631598
>> Log:
>> Add FSFS index checksum verification code to 'svnadmin verify'.
>>
>> We don't verify the index data against the checksums on every
>> access as there is plenty of cross-verification within the indexes,
>> between them as well as between index and rev data. Only if some
>> inconsistency has been detected and the user wants to trace down
>> the source (using 'svnadmin verify'), will we verify that the index
>> data has not been tampered with.
>>
>> * subversion/libsvn_fs_fs/verify.c
>> (verify_index_checksum,
>> verify_index_checksums): New per rev / pack file verification code.
>> (verify_index_consistency): Call the new index checksum test before
>> using any of the index contents.
>>
>> * subversion/tests/libsvn_fs_fs/fs-fs-fuzzy-test.c
>> (fuzzing_1_byte_1_rev): Remove the code that would accept certain
>> index modifications. Accept only case changes
>> in index MD5 digests because they don't affect
>> the validity.
> [snip]
>
>> @@ -144,14 +120,30 @@ fuzzing_1_byte_1_rev(const char *repo_na
>> err = svn_repos_verify_fs3(repos, revision, revision, TRUE, FALSE, FALSE,
>> NULL, NULL, NULL, NULL, iterpool);
>>
>> - /* Benign changes may or may not be detected. */
>> - if (!is_legal)
>> + /* Case-only changes in checksum digests are not an error.
>> + * We allow upper case chars to be used in MD5 checksums in all other
>> + * places, thus restricting them here would be inconsistent. */
>> + if ( i >= filesize - footer_len /* Within footer */
>> + && c_old >= 'a' && c_old <= 'f' /* 'a' to 'f', only appear
>> + in checksum digests */
>> + && c_new == c_old - 'a' + 'A') /* respective upper case */
>> + {
>> + if (err)
>> + {
>> + /* Let us know where we were too strict ... */
>> + printf("Detected case change in checksum digest at offset 0x%"
>> + APR_UINT64_T_HEX_FMT " (%" APR_OFF_T_FMT ") in r%ld: "
>> + "%c -> %c\n", i, i, revision, c_old, c_new);
>> +
>> + SVN_ERR(err);
>> + }
>> + }
>> + else if (!err)
>> {
>> /* Let us know where we miss changes ... */
>> - if (!err)
>> - printf("Undetected mod at offset %"APR_UINT64_T_HEX_FMT
>> - " (%"APR_OFF_T_FMT") in r%ld: 0x%02x -> 0x%02x\n",
>> - i, i, revision, c_old, c_new);
>> + printf("Undetected mod at offset 0x%"APR_UINT64_T_HEX_FMT
>> + " (%"APR_OFF_T_FMT") in r%ld: 0x%02x -> 0x%02x\n",
>> + i, i, revision, c_old, c_new);
>>
>> SVN_TEST_ASSERT(err);
>> }
>>
>>
> Those printf formats aren't valid on all platforms/builds of APR. In this case
> i is an apr_off_t and you're using APR_UINT64_T_HEX_FMT with it. Unfortunately
> you can't guarantee that apr_off_t is a 64-bit int. If LFS support isn't
> enabled and the platform has its own off_t then APR will use whatever the size
> is for that, or just an int if off_t isn't defined. See aprenv.py in APR's
> source code for the various scenarios.
>
> Not sure what you want to do about this since it makes the hex output more
> difficult to do. I'd probably just remove the hex output since it's a test and
> we can probably deal with that bit of convenience.
Since it is a test, what's wrong with just casting the first vararg to
(apr_uint64_t) instead, since we "know" (i.e., hope) that off_t won't
overflow 64 bits ...
-- Brane