rep_read_contents() edge case checksum error, FSFS f7

[Julian Foad <ju...@apache.org>]

Stefan, is this an edge case bug?

rep_read_contents() in libsvn_fs_fs/cached_data.c@1780363 is quoted below.

rep_read_contents() potentially reads one or more times from the 
fulltext cache and then finds the fulltext cache is no longer available. 
I suppose this could happen because the entry could be evicted because 
other entries are being more recently/frequently accessed even though we 
haven't finished calling rep_read_contents() for this particular rep.

In this case, rep_read_contents() calls skip_contents() to skip-read 
from the window stream, to catch up that stream to the point we'd 
already reached in the fulltext cache before it became unavailable.

Then we read the requested LEN bytes from the window stream, and update 
the calculated checksum with those LEN bytes ... but not with the 
skipped bytes.

Won't that lead to a checksum mismatch when we reach the end of the rep?

- Julian


[[[
/* 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. */
static svn_error_t *
rep_read_contents(void *baton,
                   char *buf,
                   apr_size_t *len)
{
   struct rep_read_baton *rb = baton;

   /* Get data from the fulltext cache for as long as we can. */
   if (rb->fulltext_cache)
     {
       svn_boolean_t cached;
       SVN_ERR(get_contents_from_fulltext(&cached, rb, buf, len));
       if (cached)
         return SVN_NO_ERROR;

       /* Cache miss.  From now on, we will never read from the fulltext
        * cache for this representation anymore. */
       rb->fulltext_cache = NULL;
     }

   /* No fulltext cache to help us.  We must read from the window stream. */
   if (!rb->rs_list)
     {
       /* Window stream not initialized, yet.  Do it now. */
       rb->len = rb->rep.expanded_size;
       SVN_ERR(build_rep_list(&rb->rs_list, &rb->base_window,
                              &rb->src_state, rb->fs, &rb->rep,
                              rb->filehandle_pool));

       /* In case we did read from the fulltext cache before, make the
        * window stream catch up.  Also, initialize the fulltext buffer
        * if we want to cache the fulltext at the end. */
       SVN_ERR(skip_contents(rb, rb->fulltext_delivered));
     }

   /* Get the next block of data. */
   SVN_ERR(get_contents_from_windows(rb, buf, len));

   if (rb->current_fulltext)
     svn_stringbuf_appendbytes(rb->current_fulltext, buf, *len);

   /* 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
      twice. */
   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;
           svn_checksum_t expected;
           expected.kind = svn_checksum_md5;
           expected.digest = rb->md5_digest;

           rb->checksum_finalized = TRUE;
           SVN_ERR(svn_checksum_final(&md5_checksum, rb->md5_checksum_ctx,
                                      rb->pool));
           if (!svn_checksum_match(md5_checksum, &expected))
             return svn_error_create(SVN_ERR_FS_CORRUPT,
                     svn_checksum_mismatch_err(&expected, md5_checksum,
                         rb->pool,
                         _("Checksum mismatch while reading 
representation")),
                     NULL);
         }
     }

   if (rb->off == rb->len && rb->current_fulltext)
     {
       fs_fs_data_t *ffd = rb->fs->fsap_data;
       SVN_ERR(svn_cache__set(ffd->fulltext_cache, &rb->fulltext_cache_key,
                              rb->current_fulltext, rb->pool));
       rb->current_fulltext = NULL;
     }

   return SVN_NO_ERROR;
}
]]]

Re: rep_read_contents() edge case checksum error, FSFS f7

[Stefan Fuhrmann <st...@alice-dsl.de>]

On 30.01.2017 20:09, Stefan Fuhrmann wrote:
> On 29.01.2017 22:27, Julian Foad wrote:
>> Stefan, is this an edge case bug?
>>
>> rep_read_contents() in libsvn_fs_fs/cached_data.c@1780363 is quoted 
>> below.
>>
>> rep_read_contents() potentially reads one or more times from the 
>> fulltext cache
>> and then finds the fulltext cache is no longer available. I suppose 
>> this could
>> happen because the entry could be evicted because other entries are 
>> being more
>> recently/frequently accessed even though we haven't finished calling
>> rep_read_contents() for this particular rep.
>>
>> In this case, rep_read_contents() calls skip_contents() to skip-read 
>> from the
>> window stream, to catch up that stream to the point we'd already 
>> reached in the
>> fulltext cache before it became unavailable.
>>
>> Then we read the requested LEN bytes from the window stream, and 
>> update the
>> calculated checksum with those LEN bytes ... but not with the skipped 
>> bytes.
>>
>> Won't that lead to a checksum mismatch when we reach the end of the rep?
>
> Yes, that looks like a bug.  I'll fix it next weekend.
>
> The problem is not f7-specific and will only be transient
> b/c the next reader will miss the cache and reconstruct
> the content from the first window on.
>
> Also, using bulk updates should reduce the incident rate
> of this issue.

Turns out that there is indeed a minor bug but that
one prevents the checksum from ever being checked
in the case that you described. So, this should not
be the source for the checksum mismatch that you
are seeing.

In r1781650, I added a regression test reproducing
the above conditions and it does not fail.

The thing is that while we don't run the checksum
over the "catch-up" part of the data, we also don't
advance the OFF element either. Hence, it will never
match the representation length to trigger the checksum
completion and comparison.

I'll fix this today (probably).

-- Stefan^2.

Re: rep_read_contents() edge case checksum error, FSFS f7

[Stefan Fuhrmann <st...@apache.org>]

On 29.01.2017 22:27, Julian Foad wrote:
> Stefan, is this an edge case bug?
>
> rep_read_contents() in libsvn_fs_fs/cached_data.c@1780363 is quoted below.
>
> rep_read_contents() potentially reads one or more times from the fulltext cache
> and then finds the fulltext cache is no longer available. I suppose this could
> happen because the entry could be evicted because other entries are being more
> recently/frequently accessed even though we haven't finished calling
> rep_read_contents() for this particular rep.
>
> In this case, rep_read_contents() calls skip_contents() to skip-read from the
> window stream, to catch up that stream to the point we'd already reached in the
> fulltext cache before it became unavailable.
>
> Then we read the requested LEN bytes from the window stream, and update the
> calculated checksum with those LEN bytes ... but not with the skipped bytes.
>
> Won't that lead to a checksum mismatch when we reach the end of the rep?

Yes, that looks like a bug.  I'll fix it next weekend.

The problem is not f7-specific and will only be transient
b/c the next reader will miss the cache and reconstruct
the content from the first window on.

Also, using bulk updates should reduce the incident rate
of this issue.

-- Stefan^2.