philip_at_apache.org writes:
> Author: philip
> Date: Wed Mar 14 14:24:36 2018
> New Revision: 1826720
>
> URL: http://svn.apache.org/viewvc?rev=1826720&view=rev
> Log:
> Add a checksum length verification to the FSFS checksum code. This
> may have prevented the issue 4722 checksum bug by making the problem
> much more visible: it would have failed lots of successful commits
> that only succeeded by ignoring the length error.
>
> * subversion/libsvn_fs_fs/cached_data.c
> (rep_read_contents): This is now a READ_FULL_FN for svn_stream_t
> and a short read is EOF at which point the length can be checked.
> When originally written the code was a READ_FN and a short read
> was not necessarily EOF.
>
> Modified:
> subversion/trunk/subversion/libsvn_fs_fs/cached_data.c
>
> Modified: subversion/trunk/subversion/libsvn_fs_fs/cached_data.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/cached_data.c?rev=1826720&r1=1826719&r2=1826720&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_fs_fs/cached_data.c (original)
> +++ subversion/trunk/subversion/libsvn_fs_fs/cached_data.c Wed Mar 14 14:24:36 2018
> @@ -2103,13 +2103,14 @@ skip_contents(struct rep_read_baton *bat
>
> /* BATON is of type `rep_read_baton'; read the next *LEN bytes of the
> representation and store them in *BUF. Sum as we read and verify
> - the MD5 sum at the end. */
> + the MD5 sum at the end. This is a READ_FULL_FN for svn_stream_t. */
> static svn_error_t *
> rep_read_contents(void *baton,
> char *buf,
> apr_size_t *len)
> {
> struct rep_read_baton *rb = baton;
> + apr_size_t len_requested = *len;
>
> /* Get data from the fulltext cache for as long as we can. */
> if (rb->fulltext_cache)
> @@ -2150,6 +2151,12 @@ rep_read_contents(void *baton,
> if (rb->current_fulltext)
> svn_stringbuf_appendbytes(rb->current_fulltext, buf, *len);
>
> + /* This is a FULL_READ_FN so a short read implies EOF. */
> + rb->off += *len;
> + if (*len < len_requested && rb->off != rb->len)
> + return svn_error_create(SVN_ERR_FS_CORRUPT, NULL,
> + _("Length mismatch while reading representation"));
> +
I'm wondering about this change now. In 1.9.7 we were passing the wrong
length in rb->len and this caused the occasional commit to fail with a
checksum error. It doesn't happen very often, probably less than 1 in
16,000 of those commits that trigger deduplication. This extra check
makes nearly every commit that triggers deduplication fail when the
length is wrong, which would have made the bug much more obvious and we
may have caught it before it was released.
The reason I'm having second thoughts is a report on the user list of
something that looks like issue 4554: the expanded length incorrectly
stored as zero. If I manually corrupt a repo to have the same problem
this new check causes "svnadmin dump/verify" to fail with the above
error. Removing the new check allows dump/verify run without producing
an error.
It seems wrong for our checksum code to allow the wrong length, but
preventing dump when the data could otherwise be extracted is also
wrong. Perhaps we should simply allow the checksum code to read any
length when rb->len is zero? That doesn't seem right. Perhaps the
error should just be a warning as below?
Index: subversion/libsvn_fs_fs/cached_data.c
===================================================================
--- subversion/libsvn_fs_fs/cached_data.c (revision 1827133)
+++ subversion/libsvn_fs_fs/cached_data.c (working copy)
@@ -2154,8 +2154,13 @@
/* This is a FULL_READ_FN so a short read implies EOF. */
rb->off += *len;
if (*len < len_requested && rb->off != rb->len)
- return svn_error_create(SVN_ERR_FS_CORRUPT, NULL,
+ if (rb->fs->warning)
+ {
+ svn_error_t *err = svn_error_create(SVN_ERR_FS_CORRUPT, NULL,
_("Length mismatch while reading representation"));
+ rb->fs->warning(rb->fs->warning_baton, err);
+ svn_error_clear(err);
+ }
/* Perform checksumming. We want to check the checksum as soon as
the last byte of data is read, in case the caller never performs
--
Philip
Received on 2018-03-19 03:04:38 CET