subversion-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From s...@apache.org
Subject svn commit: r1794611 - in /subversion/trunk/subversion: libsvn_fs_fs/transaction.c libsvn_fs_x/transaction.c tests/libsvn_fs/fs-test.c
Date Tue, 09 May 2017 16:49:24 GMT
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: <20170509132523.GN55397@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);
 



Mime
View raw message