On Tue, Dec 3, 2013 at 4:59 PM, Evgeny Kotkov
<[email protected]>wrote:
> Hi,
>
> I've noticed that merge of the FSFS format 7 branch in r1546842 [1]
> introduced
> a regression: 'svnadmin recover' now crashes on the --pre-1.4-compatible
> FSFS
> repositories. If you create a repository using the 'svnadmin create
> --compatible-version=1.3 / 1.2 / 1.1' or take any existing repository with
> FSFS
> format <= 3, attempt to recover it will coredump:
> [[[
> Repository lock acquired.
> Please wait; recovering the repository may take some time...
> Floating point exception (core dumped)
> ]]]
>
> More interesting, we have a test (namely "recover --pre-1.4-compatible"),
> which
> should have failed upon this regression, however, the test is seriously
> broken
> and always reports false positive (PASS). I propose a series of 3 patches
> that
> sequentially fix the test (in order to get the expected FAIL) and then the
> crash itself.
>
> So, "recover --pre-1.4-compatible" test is a regression test for the issue
> 4213 [2]. It looks pretty simple, ...
> [[[
> svntest.main.safe_rmtree(sbox.repo_dir, 1)
> svntest.main.create_repos(sbox.repo_dir, minor_version=0)
> svntest.main.run_svnadmin("recover", sbox.repo_dir)
> ]]]
>
> ...but has several problems:
>
> 1) First of all, creating an FSFS repository with MINOR_VERSION set to '0'
> (or
> equivalently, running 'svnadmin create --compatible-version=1.0') is
> prohibited since r1494223 [3] and should error out like this:
> [[[
> svnadmin: E205000: Repositories compatible with 1.0.x must use
> --fs-type=bdb
> ]]]
>
> So, the corresponding test should fail during the repository creation
> phase,
> but it doesn't. This happens due to a mistake in the 'create_repos'
> method,
> which incorrectly uses the "if not" statement to handle the absence of
> MINOR_VERSION. This check effectively is the "if MINOR_VERSION is None
> or
> MINOR_VERSION == 0" and we end up with creating a 1.9-compatible
> repository
> in a "--pre-1.4-compatible" regression test. I've changed that
> statement to
> a "if MINOR_VERSION is None" (following PEP8 [4]), see the attached
> patch #1
>
> 2) Now, applying patch #1 leaves us with two other problems (minor and
> major):
>
> - Minor: we cannot create a 1.0-compatible FSFS repository, but still
> want
> to have a regression test for issue 4213 [2]. So, I've bumped the
> MINOR_REVISION to 3, thus making the test initialization equivalent
> to
> the 'svnadmin create --pre-1.4-compatible' call (as noted in the test
> docstring).
>
> - Major: currently the test *checks nothing*, because simply calling
> the
> 'run_svnadmin' method does not check EXIT_CODE, STDOUT or STDERR.
> So, even with patched svnadmin that errors out on any 'recover'
> command,
> the test still passes (and it still passes with crashing svnadmin!).
> In
> order to fix this, I've replaced the 'run_svnadmin' call with
> 'run_and_verify_svnadmin'.
>
> (These changes are combined in my patch #2)
>
> With patches #1 and #2 applied, we finally have the expected FAIL in the
> test
> ("svnadmin recover terminated by signal 8") and can proceed to fixing it.
> The
> crash happens due to the way 'svn_fs_fs__find_max_ids' function uses the
> new
> svn_fs_fs__revision_file_t structure since r1532029 [5]. It calls the
> 'svn_fs_fs__item_offset' function with uninitialized REV_FILE and this
> leads to a coredump for old repositories. REV_FILE is initialized (via
> 'svn_fs_fs__open_pack_or_rev_file') right after the erroneous
> 'svn_fs_fs__item_offset' call, so swapping the initialization and offset
> evaluation should do the trick. I've attached the valgrind output and the
> patch #3, which fixes this issue.
>
> All tests (including the fixed one) pass.
>
> NOTE: In order to prove that after all these manipulations the "recover
> --pre-1.4-compatible" is a correct regression test for issue 4213 [2], I've
> checked that undoing the r1367683 changeset [6] fails the test with the
> expected error (as described in the issue):
> [[[
> svnadmin: E000002: Can't open file
> 'svn-test-work/repositories/svnadmin_tests-30/db/revs/1'
> ]]]
>
> [1] http://svn.apache.org/viewvc?view=revision&revision=r1546842
> [2] http://subversion.tigris.org/issues/show_bug.cgi?id=4213
> [3] http://svn.apache.org/viewvc?view=revision&revision=r1494223
> [4] http://www.python.org/dev/peps/pep-0008/#programming-recommendations
> [5] http://svn.apache.org/viewvc?view=revision&revision=r1532029
> [6] http://svn.apache.org/viewvc?view=revision&revision=r1367683
>
>
Thanks, Evgeny, for those patches.
Committed as r1547488, -9 and -6.
-- Stefan^2.
