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: <[email protected]> * 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);
