On Tue, Dec 3, 2013 at 4:59 PM, Evgeny Kotkov
> I've noticed that merge of the FSFS format 7 branch in r1546842 
> a regression: 'svnadmin recover' now crashes on the --pre-1.4-compatible
> repositories. If you create a repository using the 'svnadmin create
> --compatible-version=1.3 / 1.2 / 1.1' or take any existing repository with
> 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"),
> should have failed upon this regression, however, the test is seriously
> and always reports false positive (PASS). I propose a series of 3 patches
> 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 . 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'
> equivalently, running 'svnadmin create --compatible-version=1.0') is
> prohibited since r1494223  and should error out like this:
> svnadmin: E205000: Repositories compatible with 1.0.x must use
> So, the corresponding test should fail during the repository creation
> but it doesn't. This happens due to a mistake in the 'create_repos'
> which incorrectly uses the "if not" statement to handle the absence of
> MINOR_VERSION. This check effectively is the "if MINOR_VERSION is None
> MINOR_VERSION == 0" and we end up with creating a 1.9-compatible
> in a "--pre-1.4-compatible" regression test. I've changed that
> statement to
> a "if MINOR_VERSION is None" (following PEP8 ), see the attached
> patch #1
> 2) Now, applying patch #1 leaves us with two other problems (minor and
> - Minor: we cannot create a 1.0-compatible FSFS repository, but still
> to have a regression test for issue 4213 . So, I've bumped the
> MINOR_REVISION to 3, thus making the test initialization equivalent
> the 'svnadmin create --pre-1.4-compatible' call (as noted in the test
> - Major: currently the test *checks nothing*, because simply calling
> 'run_svnadmin' method does not check EXIT_CODE, STDOUT or STDERR.
> So, even with patched svnadmin that errors out on any 'recover'
> the test still passes (and it still passes with crashing svnadmin!).
> order to fix this, I've replaced the 'run_svnadmin' call with
> (These changes are combined in my patch #2)
> With patches #1 and #2 applied, we finally have the expected FAIL in the
> ("svnadmin recover terminated by signal 8") and can proceed to fixing it.
> crash happens due to the way 'svn_fs_fs__find_max_ids' function uses the
> svn_fs_fs__revision_file_t structure since r1532029 . 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 , I've
> checked that undoing the r1367683 changeset  fails the test with the
> expected error (as described in the issue):
> svnadmin: E000002: Can't open file
>  http://svn.apache.org/viewvc?view=revision&revision=r1546842
>  http://subversion.tigris.org/issues/show_bug.cgi?id=4213
>  http://svn.apache.org/viewvc?view=revision&revision=r1494223
>  http://www.python.org/dev/peps/pep-0008/#programming-recommendations
>  http://svn.apache.org/viewvc?view=revision&revision=r1532029
>  http://svn.apache.org/viewvc?view=revision&revision=r1367683
Thanks, Evgeny, for those patches.
Committed as r1547488, -9 and -6.
Received on 2013-12-03 18:27:40 CET