Would this be safe?
In subversion/libsvn_fs_fs/transaction.c, just before commit_body()
bumps the revision number in the "current" file, it calls
verify_as_revision_before_current_plus_plus(), quoted below.
verify_as_revision_before_current_plus_plus() is currently compiled in
to debug builds but not to release builds. We can say therefore it gets
reasonable coverage in test suite runs but has had little or no
real-world testing.
In a customer's production system, two recent and very similar commits
each resulted in a corruption ("Checksum mismatch while reading
representation...") being reported in post-commit processing in
dav_svn__merge_response().
I want to recommend that the customer rebuilds their Subversion (1.9.4)
release build with this "verify" code enabled, and runs it in their
production server, in order to try to prevent further corruptions from
entering the repository. It seems very possible that this may block such
a broken commit if and when it happens again.
Please could anybody cast a second pair of eyes over this code and say
whether it looks safe to enable in production. It looks low risk to me,
but it is a little "tricky": in particular, it opens a second FS
instance and fakes the "youngest revision seen" field and then relies on
this FS instance never reading the "current" file nor using anything
that would have been done post-commit. It also doesn't pass any of the
configured FS options to this second instance, which looks OK for the
time being but not future-proof.
Thanks...
- Julian
[[[
/* Open a new svn_fs_t handle to FS, set that handle's concept of
"current youngest revision" to NEW_REV, and call
svn_fs_fs__verify_root() on NEW_REV's revision root.
Intended to be called as the very last step in a commit before
'current' is bumped. This implies that we are holding the write
lock. */
static svn_error_t *
verify_as_revision_before_current_plus_plus(svn_fs_t *fs,
svn_revnum_t new_rev,
apr_pool_t *pool)
{
#ifdef SVN_DEBUG
fs_fs_data_t *ffd = fs->fsap_data;
svn_fs_t *ft; /* fs++ == ft */
svn_fs_root_t *root;
fs_fs_data_t *ft_ffd;
apr_hash_t *fs_config;
SVN_ERR_ASSERT(ffd->svn_fs_open_);
/* make sure FT does not simply return data cached by other instances
* but actually retrieves it from disk at least once.
*/
fs_config = apr_hash_make(pool);
svn_hash_sets(fs_config, SVN_FS_CONFIG_FSFS_CACHE_NS,
svn_uuid_generate(pool));
SVN_ERR(ffd->svn_fs_open_(&ft, fs->path,
fs_config,
pool,
pool));
ft_ffd = ft->fsap_data;
/* Don't let FT consult rep-cache.db, either. */
ft_ffd->rep_sharing_allowed = FALSE;
/* Time travel! */
ft_ffd->youngest_rev_cache = new_rev;
SVN_ERR(svn_fs_fs__revision_root(&root, ft, new_rev, pool));
SVN_ERR_ASSERT(root->is_txn_root == FALSE && root->rev == new_rev);
SVN_ERR_ASSERT(ft_ffd->youngest_rev_cache == new_rev);
SVN_ERR(svn_fs_fs__verify_root(root, pool));
#endif /* SVN_DEBUG */
return SVN_NO_ERROR;
}
[...]
commit_body(void *baton, apr_pool_t *pool)
{
[...]
[... Move the finished rev file into place]
[... Write final revprops file]
/* Update the 'current' file. */
SVN_ERR(verify_as_revision_before_current_plus_plus(cb->fs, new_rev,
pool));
SVN_ERR(write_final_current(cb->fs, txn_id, new_rev, start_node_id,
start_copy_id, pool));
[...]
}
]]]
Received on 2017-01-30 22:40:04 CET