phi...@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