On 09.05.2017 15:25, Stefan Sperling wrote:
> On Tue, May 09, 2017 at 01:39:49PM +0200, Stefan Sperling wrote:
>> On Tue, May 09, 2017 at 11:38:51AM +0200, Stefan Sperling wrote:
>>> On Tue, Apr 18, 2017 at 12:54:20AM +0000, Daniel Shahaf wrote:
>>>> % svnadmin load r2 < dump
>>>> <<< Started new transaction, based on original revision 1
>>>> * editing path : shattered-1.pdf ... done.
>>>> * editing path : shattered-2.pdf ...svnadmin: E200014: Checksum mismatch for '/shattered-2.pdf':
>>>> expected: 5bd9d8cabc46041579a311230539b8d1
>>>> actual: ee4aa52b139d925f8d8884402b0a750c
>>>>
>>>> zsh: exit 1 svnadmin load r2 < dump
>>>> %
>>>> ]]]
>>>>
>>>> That's with 1.9.5. Is it fixed on trunk now? I'm not sure whether
>>>> r1785754 addresses that.
>>> Yes, this is fixed on trunk :-)
>> I have now voted for and backported whatever I could to 1.9.x.
>>
>> Please add your votes to the STATUS file as well so we can finally
>> release these fixes (or improve them, if there are still concerns).
> On IRC, Branko and Johan raised concerns about the proposed backport.
>
> The proposed backport allows files with SHA1 collisions into the repository
> and avoids de-duplication of such content by the rep-cache. It fixes the
> integrity problem with the rep-cache but other problems remain.
>
> Clients will still suffer from the consequences of having such files in
> the repository. The RA layer cannot de-duplicate content, the working
> copy's pristine store is broken by such content, etc.
>
> So the question is whether we want to even store such content before all
> parts of the system are ready to handle it. Some users will likely keep
> asking for a way to reject this content because of the ripple effects
> it can cause. Many users may perceive a risk even if "only" working copies
> can be rendered unusable by committing such content, because dealing with
> fallout can be highly disruptive to the actual work people have to get done.
>
> We can block such content from entering the repository with a patch such
> as the one below. This check will only be done if rep-sharing is active.
> But since rep-sharing is active by default this patch will work for most
> of our users.
>
> This could be further extended by the config knob to give users a choice.
> I don't see a good way of adding such a knob in a patch release, though.
> Perhaps that could be done for 1.10.
[Hmpf. After answering on two other threads, I now found
this one and I can toss away those other posts ...]
Yeah, I think the patch gives us the most consistent behavior
with the least added effort. And it keeps the door open for
allowing checksum collisions in the future.
So, +1 from my side. And then let's continue with the out-
standing releases.
> Index: subversion/libsvn_fs_fs/transaction.c
> ===================================================================
> --- subversion/libsvn_fs_fs/transaction.c (revision 1794541)
> +++ subversion/libsvn_fs_fs/transaction.c (working copy)
> @@ -2430,12 +2430,10 @@ get_shared_rep(representation_t **old_rep,
> scratch_pool);
>
> /* A mismatch should be extremely rare.
> - * If it does happen, log the event and don't share the rep. */
> + * If it does happen, reject the commit. */
> if (!same || err)
> {
> - /* SHA1 collision or worse.
> - We can continue without rep-sharing, but warn.
> - */
> + /* SHA1 collision or worse. */
> svn_stringbuf_t *old_rep_str
> = svn_fs_fs__unparse_representation(*old_rep,
> ffd->format, FALSE,
> @@ -2449,15 +2447,11 @@ get_shared_rep(representation_t **old_rep,
> const char *checksum__str
> = svn_checksum_to_cstring_display(&checksum, scratch_pool);
>
> - err = svn_error_createf(SVN_ERR_FS_AMBIUGOUS_CHECKSUM_REP,
> - err, "SHA1 of reps '%s' and '%s' "
> - "matches (%s) but contents differ",
> - old_rep_str->data, rep_str->data,
> - checksum__str);
> -
> - (fs->warning)(fs->warning_baton, err);
> - svn_error_clear(err);
> - *old_rep = NULL;
> + return svn_error_createf(SVN_ERR_FS_AMBIUGOUS_CHECKSUM_REP,
> + err, "SHA1 of reps '%s' and '%s' "
> + "matches (%s) but contents differ",
> + old_rep_str->data, rep_str->data,
> + checksum__str);
> }
>
> /* Restore FILE's read / write position. */
>
I have not tested it, but this looks fine,
-- Stefan^2.
[on my way to ApacheCON]
Received on 2017-05-13 22:38:21 CEST