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 2017/05/09 16:49:24 UTC
svn commit: r1794611 - in /subversion/trunk/subversion:
libsvn_fs_fs/transaction.c libsvn_fs_x/transaction.c
tests/libsvn_fs/fs-test.c
Author: stsp
Date: Tue May 9 16:49:23 2017
New Revision: 1794611
URL: http://svn.apache.org/viewvc?rev=1794611&view=rev
Log:
In FSFS and FSX, reject commits which cause MD5 or SHA1 collisions.
Since r1785754 the rep-cache no longer depends on MD5/SHA1 hash algorithms
alone to index content. So we can store colliding content without causing
repository corruption if the rep-cache is active, which is great.
However, similar problems still exist in (at least) the RA layer and the
working copy. Until those are fixed, rejecting content which causes a hash
collision is the safest approach and avoids the undesired consequences of
storing such content.
Rejecting such content only works if rep-sharing is enabled (the default).
Content with hash collisions can still be stored if rep-sharing is disabled
but please beware of annoying consequences before doing so.
This commit is a follow-up to r1785754 and transforms a warning message
added in that revision into a fatal error.
Related dev@ discussion starts at:
https://svn.haxx.se/dev/archive-2017-05/0053.shtml
Subject: [PATCH] reject SHA1 collisions
Message-ID: <20...@ted.stsp.name>
* subversion/libsvn_fs_fs/transaction.c,
subversion/libsvn_fs_x/transaction.c
(get_shared_rep): Treat a checksum collision with different content
as a fatal error.
* subversion/tests/libsvn_fs/fs-test.c
(flag_fs_warnings): Remove, not needed anymore.
(test_rep_sharing_strict_content_check): Adjust test expectations.
Modified:
subversion/trunk/subversion/libsvn_fs_fs/transaction.c
subversion/trunk/subversion/libsvn_fs_x/transaction.c
subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
Modified: subversion/trunk/subversion/libsvn_fs_fs/transaction.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/transaction.c?rev=1794611&r1=1794610&r2=1794611&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/transaction.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/transaction.c Tue May 9 16:49:23 2017
@@ -2430,12 +2430,10 @@ get_shared_rep(representation_t **old_re
scratch_pool);
/* A mismatch should be extremely rare.
- * If it does happen, log the event and don't share the rep. */
+ * If it does happen, reject the commit. */
if (!same || err)
{
- /* SHA1 collision or worse.
- We can continue without rep-sharing, but warn.
- */
+ /* SHA1 collision or worse. */
svn_stringbuf_t *old_rep_str
= svn_fs_fs__unparse_representation(*old_rep,
ffd->format, FALSE,
@@ -2449,15 +2447,11 @@ get_shared_rep(representation_t **old_re
const char *checksum__str
= svn_checksum_to_cstring_display(&checksum, scratch_pool);
- err = svn_error_createf(SVN_ERR_FS_AMBIGUOUS_CHECKSUM_REP,
- err, "SHA1 of reps '%s' and '%s' "
- "matches (%s) but contents differ",
- old_rep_str->data, rep_str->data,
- checksum__str);
-
- (fs->warning)(fs->warning_baton, err);
- svn_error_clear(err);
- *old_rep = NULL;
+ return svn_error_createf(SVN_ERR_FS_AMBIGUOUS_CHECKSUM_REP,
+ err, "SHA1 of reps '%s' and '%s' "
+ "matches (%s) but contents differ",
+ old_rep_str->data, rep_str->data,
+ checksum__str);
}
/* Restore FILE's read / write position. */
Modified: subversion/trunk/subversion/libsvn_fs_x/transaction.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_x/transaction.c?rev=1794611&r1=1794610&r2=1794611&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_x/transaction.c (original)
+++ subversion/trunk/subversion/libsvn_fs_x/transaction.c Tue May 9 16:49:23 2017
@@ -2522,12 +2522,10 @@ get_shared_rep(svn_fs_x__representation_
scratch_pool);
/* A mismatch should be extremely rare.
- * If it does happen, log the event and don't share the rep. */
+ * If it does happen, reject the commit. */
if (!same || err)
{
- /* SHA1 collision or worse.
- We can continue without rep-sharing, but warn.
- */
+ /* SHA1 collision or worse. */
svn_stringbuf_t *old_rep_str
= svn_fs_x__unparse_representation(*old_rep, FALSE,
scratch_pool,
@@ -2539,15 +2537,11 @@ get_shared_rep(svn_fs_x__representation_
const char *checksum__str
= svn_checksum_to_cstring_display(&checksum, scratch_pool);
- err = svn_error_createf(SVN_ERR_FS_AMBIGUOUS_CHECKSUM_REP,
- err, "SHA1 of reps '%s' and '%s' "
- "matches (%s) but contents differ",
- old_rep_str->data, rep_str->data,
- checksum__str);
-
- (fs->warning)(fs->warning_baton, err);
- svn_error_clear(err);
- *old_rep = NULL;
+ return svn_error_createf(SVN_ERR_FS_AMBIGUOUS_CHECKSUM_REP,
+ err, "SHA1 of reps '%s' and '%s' "
+ "matches (%s) but contents differ",
+ old_rep_str->data, rep_str->data,
+ checksum__str);
}
/* Restore FILE's read / write position. */
Modified: subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_fs/fs-test.c?rev=1794611&r1=1794610&r2=1794611&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/libsvn_fs/fs-test.c (original)
+++ subversion/trunk/subversion/tests/libsvn_fs/fs-test.c Tue May 9 16:49:23 2017
@@ -7256,29 +7256,17 @@ test_cache_clear_during_stream(const svn
return SVN_NO_ERROR;
}
-/* Implements svn_fs_warning_callback_t by setting the svn_boolean_t at
- * *BATON to TRUE, indicating a warning has been logged. */
-static void
-flag_fs_warnings(void *baton, svn_error_t *err)
-{
- svn_boolean_t *flag = baton;
- *flag = TRUE;
-
- return;
-}
-
static svn_error_t *
test_rep_sharing_strict_content_check(const svn_test_opts_t *opts,
apr_pool_t *pool)
{
svn_fs_t *fs;
svn_fs_txn_t *txn;
- svn_fs_root_t *txn_root, *rev_root;
+ svn_fs_root_t *txn_root;
svn_revnum_t new_rev;
const char *fs_path, *fs_path2;
apr_pool_t *subpool = svn_pool_create(pool);
- svn_stringbuf_t *contents;
- svn_boolean_t had_warning = FALSE;
+ svn_error_t *err;
/* Bail (with success) on known-untestable scenarios */
if (strcmp(opts->fs_type, SVN_FS_TYPE_BDB) == 0)
@@ -7318,24 +7306,11 @@ test_rep_sharing_strict_content_check(co
/* Changing the file contents such that rep-sharing would kick in if
the file contents was not properly compared. */
SVN_ERR(svn_fs_open2(&fs, fs_path, NULL, subpool, subpool));
- svn_fs_set_warning_func(fs, flag_fs_warnings, &had_warning);
SVN_ERR(svn_fs_begin_txn2(&txn, fs, 1, 0, subpool));
SVN_ERR(svn_fs_txn_root(&txn_root, txn, subpool));
- SVN_ERR(svn_test__set_file_contents(txn_root, "foo", "very good", subpool));
- SVN_ERR(test_commit_txn(&new_rev, txn, NULL, subpool));
- SVN_TEST_INT_ASSERT(new_rev, 2);
-
- svn_pool_clear(subpool);
-
- SVN_TEST_ASSERT(had_warning);
-
- /* Reading the new contents must produce the new contents. */
- SVN_ERR(svn_fs_open2(&fs, fs_path, NULL, subpool, subpool));
- SVN_ERR(svn_fs_revision_root(&rev_root, fs, 2, pool));
- SVN_ERR(svn_test__get_file_contents(rev_root, "foo", &contents, pool));
-
- SVN_TEST_STRING_ASSERT(contents->data, "very good");
+ err = svn_test__set_file_contents(txn_root, "foo", "very good", subpool);
+ SVN_TEST_ASSERT_ERROR(err, SVN_ERR_FS_AMBIGUOUS_CHECKSUM_REP);
svn_pool_destroy(subpool);