That makes sense, I've committed r1464413. Ben Reser <b...@reser.org> writes:
> You should apply the later one. The check for the kind is redundant. > > Just before this check we call svn_fs_base__dag_file_checksum() and > specify that we want tb->base_checksum->kind. Ultimately, this calls > svn_fs_base__rep_contents_checksums() which just does a > svn_checksum_dup on the checksum in the representation of the kind we > asked for. So ultimately, that kind test in the if statement ends up > testing that our functions are following their contract. > > On Wed, Apr 3, 2013 at 2:04 PM, Philip Martin > <philip.mar...@wandisco.com> wrote: >> As part of http://subversion.tigris.org/issues/show_bug.cgi?id=4344 I >> was playing with a BDB repository created by driving the svn_fs API >> directly in repos-test.c:node_locations2. This creates a revision where >> a file has no checksum. A subsequent commit that modifies the file can >> SEGV: >> >> Program received signal SIGSEGV, Segmentation fault. >> [Switching to Thread 0x7ff8d49ab700 (LWP 4957)] >> 0x00007ff8d6430320 in txn_body_apply_textdelta (baton=0x7ff8da368dc0, >> trail=0x7ff8da368e60) at ../src/subversion/libsvn_fs_base/tree.c:3733 >> 3733 if (tb->base_checksum->kind == checksum->kind >> >> because svn_fs_base__dag_file_checksum returns a NULL checksum as the >> documentation allows (repos-test itself doesn't SEGV because it >> doesn't pass a base checksum, but if you interrupt the test after r2 and >> then checkout/commit using a standard client the SEGV occurs). >> >> I can fix the SEGV using this patch: >> >> Index: ../src/subversion/libsvn_fs_base/tree.c >> =================================================================== >> --- ../src/subversion/libsvn_fs_base/tree.c (revision 1464080) >> +++ ../src/subversion/libsvn_fs_base/tree.c (working copy) >> @@ -3730,7 +3730,7 @@ >> we're calculating both SHA1 and MD5 checksums somewhere in >> reps-strings.c. Could we keep them both around somehow so this >> check could be more comprehensive? */ >> - if (tb->base_checksum->kind == checksum->kind >> + if (checksum && tb->base_checksum->kind == checksum->kind >> && !svn_checksum_match(tb->base_checksum, checksum)) >> return svn_checksum_mismatch_err(tb->base_checksum, checksum, >> trail->pool, >> >> If I look at the FSFS code it doesn't do the kind comparison, it was >> removed in r874326 but that's a revert which may have reverted more than >> intended. The FSFS code does: >> >> if (!svn_checksum_match(tb->base_checksum, checksum)) >> return svn_checksum_mismatch_err(tb->base_checksum, checksum, pool, >> _("Base checksum mismatch on '%s'"), >> tb->path); >> >> so I can also fix the SEGV with: >> >> Index: ../src/subversion/libsvn_fs_base/tree.c >> =================================================================== >> --- ../src/subversion/libsvn_fs_base/tree.c (revision 1464080) >> +++ ../src/subversion/libsvn_fs_base/tree.c (working copy) >> @@ -3730,8 +3730,7 @@ >> we're calculating both SHA1 and MD5 checksums somewhere in >> reps-strings.c. Could we keep them both around somehow so this >> check could be more comprehensive? */ >> - if (tb->base_checksum->kind == checksum->kind >> - && !svn_checksum_match(tb->base_checksum, checksum)) >> + if (!svn_checksum_match(tb->base_checksum, checksum)) >> return svn_checksum_mismatch_err(tb->base_checksum, checksum, >> trail->pool, >> _("Base checksum mismatch on '%s'"), >> >> Which patch should I apply? Should BDB and FSFS be the same? >> >> -- >> Certified & Supported Apache Subversion Downloads: >> http://www.wandisco.com/subversion/download > -- Certified & Supported Apache Subversion Downloads: http://www.wandisco.com/subversion/download
