You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2014/10/14 00:54:13 UTC
svn commit: r1631598 - in /subversion/trunk/subversion:
libsvn_fs_fs/verify.c tests/libsvn_fs_fs/fs-fs-fuzzy-test.c
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.
Modified:
subversion/trunk/subversion/libsvn_fs_fs/verify.c
subversion/trunk/subversion/tests/libsvn_fs_fs/fs-fs-fuzzy-test.c
Modified: subversion/trunk/subversion/libsvn_fs_fs/verify.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/verify.c?rev=1631598&r1=1631597&r2=1631598&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/verify.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/verify.c Mon Oct 13 22:54:13 2014
@@ -159,6 +159,93 @@ verify_rep_cache(svn_fs_t *fs,
return SVN_NO_ERROR;
}
+/* Verify that the MD5 checksum of the data between offsets START and END
+ * in FILE matches the EXPECTED checksum. If there is a mismatch use the
+ * indedx NAME in the error message. Supports cancellation with CANCEL_FUNC
+ * and CANCEL_BATON. SCRATCH_POOL is for temporary allocations. */
+static svn_error_t *
+verify_index_checksum(apr_file_t *file,
+ const char *name,
+ apr_off_t start,
+ apr_off_t end,
+ svn_checksum_t *expected,
+ svn_cancel_func_t cancel_func,
+ void *cancel_baton,
+ apr_pool_t *scratch_pool)
+{
+ unsigned char buffer[SVN__STREAM_CHUNK_SIZE];
+ apr_off_t size = end - start;
+ svn_checksum_t *actual;
+ svn_checksum_ctx_t *context
+ = svn_checksum_ctx_create(svn_checksum_md5, scratch_pool);
+
+ /* Calculate the index checksum. */
+ SVN_ERR(svn_io_file_seek(file, APR_SET, &start, scratch_pool));
+ while (size > 0)
+ {
+ apr_size_t to_read = size > sizeof(buffer)
+ ? sizeof(buffer)
+ : (apr_size_t)size;
+ SVN_ERR(svn_io_file_read_full2(file, buffer, to_read, NULL, NULL,
+ scratch_pool));
+ SVN_ERR(svn_checksum_update(context, buffer, to_read));
+ size -= to_read;
+
+ if (cancel_func)
+ SVN_ERR(cancel_func(cancel_baton));
+ }
+
+ SVN_ERR(svn_checksum_final(&actual, context, scratch_pool));
+
+ /* Verify that it matches the expected checksum. */
+ if (!svn_checksum_match(expected, actual))
+ {
+ const char *file_name;
+
+ SVN_ERR(svn_io_file_name_get(&file_name, file, scratch_pool));
+ SVN_ERR(svn_checksum_mismatch_err(expected, actual, scratch_pool,
+ _("%s checksum mismatch in file %s"),
+ name, file_name));
+ }
+
+ return SVN_NO_ERROR;
+}
+
+/* Verify the MD5 checksums of the index data in the rev / pack file
+ * containing revision START in FS. If given, invoke CANCEL_FUNC with
+ * CANCEL_BATON at regular intervals. Use SCRATCH_POOL for temporary
+ * allocations.
+ */
+static svn_error_t *
+verify_index_checksums(svn_fs_t *fs,
+ svn_revnum_t start,
+ svn_cancel_func_t cancel_func,
+ void *cancel_baton,
+ apr_pool_t *scratch_pool)
+{
+ svn_fs_fs__revision_file_t *rev_file;
+
+ /* Open the rev / pack file and read the footer */
+ SVN_ERR(svn_fs_fs__open_pack_or_rev_file(&rev_file, fs, start,
+ scratch_pool, scratch_pool));
+ SVN_ERR(svn_fs_fs__auto_read_footer(rev_file));
+
+ /* Verify the index contents against the checksum from the footer. */
+ SVN_ERR(verify_index_checksum(rev_file->file, "L2P index",
+ rev_file->l2p_offset, rev_file->p2l_offset,
+ rev_file->l2p_checksum,
+ cancel_func, cancel_baton, scratch_pool));
+ SVN_ERR(verify_index_checksum(rev_file->file, "P2L index",
+ rev_file->p2l_offset, rev_file->footer_offset,
+ rev_file->p2l_checksum,
+ cancel_func, cancel_baton, scratch_pool));
+
+ /* Done. */
+ SVN_ERR(svn_fs_fs__close_revision_file(rev_file));
+
+ return SVN_NO_ERROR;
+}
+
/* Verify that for all log-to-phys index entries for revisions START to
* START + COUNT-1 in FS there is a consistent entry in the phys-to-log
* index. If given, invoke CANCEL_FUNC with CANCEL_BATON at regular
@@ -657,9 +744,14 @@ verify_index_consistency(svn_fs_t *fs,
if (notify_func && (pack_start % ffd->max_files_per_dir == 0))
notify_func(pack_start, notify_baton, iterpool);
+ /* Check for external corruption to the indexes. */
+ err = verify_index_checksums(fs, pack_start, cancel_func,
+ cancel_baton, iterpool);
+
/* two-way index check */
- err = compare_l2p_to_p2l_index(fs, pack_start, pack_end - pack_start,
- cancel_func, cancel_baton, iterpool);
+ if (!err)
+ err = compare_l2p_to_p2l_index(fs, pack_start, pack_end - pack_start,
+ cancel_func, cancel_baton, iterpool);
if (!err)
err = compare_p2l_to_l2p_index(fs, pack_start, pack_end - pack_start,
cancel_func, cancel_baton, iterpool);
Modified: subversion/trunk/subversion/tests/libsvn_fs_fs/fs-fs-fuzzy-test.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_fs_fs/fs-fs-fuzzy-test.c?rev=1631598&r1=1631597&r2=1631598&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/libsvn_fs_fs/fs-fs-fuzzy-test.c (original)
+++ subversion/trunk/subversion/tests/libsvn_fs_fs/fs-fs-fuzzy-test.c Mon Oct 13 22:54:13 2014
@@ -93,7 +93,6 @@ fuzzing_1_byte_1_rev(const char *repo_na
for (i = 0; i < filesize; ++i)
{
svn_error_t *err = SVN_NO_ERROR;
- svn_boolean_t is_legal = FALSE;
/* Read byte */
unsigned char c_old, c_new;
@@ -110,29 +109,6 @@ fuzzing_1_byte_1_rev(const char *repo_na
SVN_ERR(svn_io_file_putc((char)c_new, rev_file->file, iterpool));
SVN_ERR(svn_io_file_flush(rev_file->file, iterpool));
- /* Certain modifications will not be detected because the result is
- legal and does not change the interpretation of any data. */
-
- /* L2P index page sizes are just an upper limit to how many entries
- a page may have our revs are too short to notice shorter page sizes
- as long as it is a power of two and uses the same number of bytes. */
- if ( i == rev_file->l2p_offset + 2 /* At the position of the
- L2P index page length */
- && c_new && !(c_new & (c_new - 1)) /* Power of 2 */
- && (c_new ^ c_old) < 0x80) /* Don't modify the continuation bit. */
- is_legal = TRUE;
-
- /* The last P2L entry is a filler and defaults to rev 1 in our case.
- However, a SVN_INVALID_REVNUM is also considered valid.
- The following check catches this condition (values are correct as
- given due to deltified storage of revision numbers). */
- if ( i == filesize - footer_len - 3 /* At the position of the
- deltified rev number
- of last P2L entry */
- && c_old == 0 /* Expected content */
- && (c_new == 1 + 2 * revision)) /* delta REVISION -> -1 */
- is_legal = TRUE;
-
/* Make sure we use a different namespace for the caches during
this iteration. */
svn_hash_sets(fs_config, SVN_FS_CONFIG_FSFS_CACHE_NS,
@@ -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);
}
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
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 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.