You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by ph...@apache.org on 2018/03/14 14:24:37 UTC
svn commit: r1826720 -
/subversion/trunk/subversion/libsvn_fs_fs/cached_data.c
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"));
+
/* Perform checksumming. We want to check the checksum as soon as
the last byte of data is read, in case the caller never performs
a short read, but we don't want to finalize the MD5 context
@@ -2157,7 +2164,6 @@ rep_read_contents(void *baton,
if (!rb->checksum_finalized)
{
SVN_ERR(svn_checksum_update(rb->md5_checksum_ctx, buf, *len));
- rb->off += *len;
if (rb->off == rb->len)
{
svn_checksum_t *md5_checksum;
Re: svn commit: r1826720 - /subversion/trunk/subversion/libsvn_fs_fs/cached_data.c
Posted by Philip Martin <ph...@codematters.co.uk>.
Philip Martin <ph...@codematters.co.uk> writes:
> 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?
I've decided the error should be an FS warning.
Issue 4554 only occurs when the representation is PLAIN rather than
DELTA and the rest of the code now recognises that and fixes up the zero
length to have the correct value. So issue 4554 will not trigger this
new error, that's good.
The new error will trigger if the length is zero for a DELTA
representation, as the code cannot fix that length, and also for a
non-zero but incorrect length. Since this is repository corruption the
ideal repsonse would be to indicate the problem but to allow the data to
be retreived if possible.
Consider
svnadmin create repo --compatible-version 1.8
svnmucc -mm -U file://`pwd`/repo put ../src/INSTALL f
chmod +w repo/db/revs/0/1
Now edit the r1 file and change the expanded length:
text: 1 0 17791 55700 df5d0e251e4b811f0ed63694e7a2cd00 fb07bf4d765ccf4e18afe74d55
being carefull to keep the file the same length, e.g. change 55700 to
45700. Using 1.10
svnadmin verify repo
will not report an error, but
svnadmin dump repo | svnadmin load repo2
will fail because the dump file contains the wrong length. The dump
file does contain the full text, so by editing the dump file the data
could be retreived. With the new warning on trunk I get:
$ subversion/svnadmin/svnadmin verify repo
* Verifying repository metadata ...
* Verified revision 0.
svnadmin: warning: apr_err=SVN_ERR_FS_CORRUPT
svnadmin: warning: W160004: Length mismatch while reading representation expected 45700 got 55700
* Verified revision 1.
which also gives me the information I need to correct the dump file and
also allows the dump to run.
--
Philip
Re: svn commit: r1826720 - /subversion/trunk/subversion/libsvn_fs_fs/cached_data.c
Posted by Philip Martin <ph...@codematters.co.uk>.
philip@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