A possible fix for Jenkins-14551 - subversion-plugin duplicating file contents

[Kenny Ayers]

Hi folks,

Short Version:

  I may have a fix for Jenkins-14551 (https://issues.jenkins-ci.org/browse/JENKINS-14551).  I've submitted this potential resolution to SVNKit as well as their 1.7.6 SVN branch has the same issue (http://issues.tmatesoft.com/issue/SVNKIT-368).  I've compiled the change into the subversion-plugin on my test server, and the fix appears to work.

  Can a contributor peer review this change?

  How do I go about submitting this fix to the Jenkins SVNKit repo?  Do I need a unit test before I can do a pull request?  The bug is obvious when you look at the code, and the unit test setup and execution seems like it would be complicated.  I've forked the Jenkins SVNKit repo and committed the modification here: https://github.com/theotherwhitemeat/svnkit-1/commit/27decb28216ee4fd15b8fcbdb769bf41d81978eb

Longer Version:

  In org.tmatesoft.svn.core.internal.wc.SVNUpdateEditor15.java, in function addFileWithHistory (line 867), there's a code block that calls myFileFetcher.fetchFile() twice.  Each time this is called, baseTextOS is written to.  Upon the second write, the file contents are duplicated. Here's the code:

baseTextOS = SVNFileUtil.openFileForWriting(info.copiedBaseText);

myFileFetcher.fetchFile(copyFromPath, copyFromRevision, baseTextOS, baseProperties);

SVNChecksumOutputStream checksumBaseTextOS = new SVNChecksumOutputStream(baseTextOS, 

SVNChecksumOutputStream.MD5_ALGORITHM, true);

baseTextOS = checksumBaseTextOS;

myFileFetcher.fetchFile(copyFromPath, copyFromRevision, baseTextOS, baseProperties);

info.copiedBaseChecksum = checksumBaseTextOS.getDigest();

  I was able to find this by stepping through the code using NetBeans IDE 7.3 attached to a remote debugging session on Jenkins.  I've compiled and tested this change inside the context of the subversion-plugin and the file contents are no longer duplicated.

  I've forked the svnkit repo used in Jenkins here, and committed this change if anyone would like to download the fix and do some testing:

  https://github.com/theotherwhitemeat/svnkit-1/commit/27decb28216ee4fd15b8fcbdb769bf41d81978eb

  Here's my patch:

Index: SVNUpdateEditor15.java

===================================================================

--- SVNUpdateEditor15.java (revision 9722)

+++ SVNUpdateEditor15.java (working copy)

@@ -864,7 +864,6 @@

             OutputStream baseTextOS = null;

             try {

                 baseTextOS = SVNFileUtil.openFileForWriting(info.copiedBaseText);

-                myFileFetcher.fetchFile(copyFromPath, copyFromRevision, baseTextOS, baseProperties);

                 SVNChecksumOutputStream checksumBaseTextOS = new SVNChecksumOutputStream(baseTextOS, 

                         SVNChecksumOutputStream.MD5_ALGORITHM, true);

                 baseTextOS = checksumBaseTextOS;

Thank you,

Kenny Ayers

[Christoph Kutzinski]

Nice research. Good job.

However, I'd like to wait whta the SVNkit developers say about it:

The bug is not so obvious to me, as the value of baseTextOS, which seems to be the ouptut file for fetchFile, changes between the 2 calls to myFileFetcher.fetchFile()

So the question is, why in your case both values seem to point to the same file

 

Gesendet: Freitag, 26. April 2013 um 04:22 Uhr
Von: "Kenny Ayers" <theother...@gmail.com>
An: jenkin...@googlegroups.com
Betreff: A possible fix for Jenkins-14551 - subversion-plugin duplicating file contents

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
 
 

[Stephen Connolly]

Traditionally, the jenkins fork is maintaining a (hopefully) smaller set of patches on top of the upstream version.

The aim is to get the set of patches to zero and then drop the fork.

With reference to the above aim, my preference would be to get it in upstream rather than add to our current patch set.

It is a real pain trying to update the code from upstream, at least every time I have tried I have had to give up and get KK to do it (he has some set of magic workspaces or something) so I would just love if we can get the need for this fork to disappear completely

-Stephen

--

[Kenny Ayers]

Hey Kutzi,

  It's helpful to step through the code to see the interaction.

  SVNUpdateEditor15 sees that a file is being added that contains file history.  baseTextOS is a bufferedOutputStream that is written to with the content of the previous version of the file during the first call to fetchFile(), here:

                baseTextOS = SVNFileUtil.openFileForWriting(info.copiedBaseText);

                myFileFetcher.fetchFile(copyFromPath, copyFromRevision, baseTextOS, baseProperties);

  At this point baseTextOS.buf contains "test line 1\n" (the contents of the original file).  Now a SVNChecksumOutputStream is instantiated with baseTextOS.  So checksumBaseTextOS.myTarget.buf contains "test line 1\n".

                SVNChecksumOutputStream checksumBaseTextOS = new SVNChecksumOutputStream(baseTextOS, 

                        SVNChecksumOutputStream.MD5_ALGORITHM, true);

  Now baseTextOS is set to checksumBaseTextOS, and then fetchFile() is called once again.  After this call, checksumBaseTextOS.myTarget.buf contains "test line 1\n test line 1"

                baseTextOS = checksumBaseTextOS;

                myFileFetcher.fetchFile(copyFromPath, copyFromRevision, baseTextOS, baseProperties);

  The buffer contents are duplicated, and eventually written out to the file on disk.  Screenshots below of the step through.

  I hope this demonstrates the bug well enough.  Thank you again for your helping getting subversion-plugin to compile and for pointing me towards the Jenkins fork of SVNKit.  That was very helpful.

  It looks like the SVNKit folks were able to reproduce the issue and confirmed the modification I submitted appears to resolve the issue (please see the last comment here: http://issues.tmatesoft.com/issue/SVNKIT-368).

-Kenny

[Kenny Ayers]

Hey Stephen,

  Alexander Kitaev from SVNKit has peer-reviewed this change and has rolled it into the upstream libraries, and the new binaries are available here: http://teamcity.tmatesoft.com/viewLog.html?buildId=6105&tab=artifacts&buildTypeId=bt43 (http://issues.tmatesoft.com/issue/SVNKIT-368#comment=60-4930).  I'm not sure what the process is for getting this updated in the plugin, please let me know if there is anything else I can do to help.

Thank you,

Kenny

[Stephen Connolly]

The process I usually follow is to beg KK to do the merge pleading that it blew up in my face and there is no way he could do it during his lunchbreak...

Though I may be using the "no way you could do that in your lunchbreak" dare a bit too often... he may have wised up to my tricks... perhaps I need to find a new one ;-)

[Kenny Ayers]

Ah, I see... perhaps I can bribe him with mail-order cookies or beer or something...

I'll do a pull request on my change, and see if that gets the ball rolling.

Thanks for the replies,

Kenny

[Kenny Ayers]

KK - any word on integrating this pull request?

[linards...@gmail.com]

meeging is good, but NOT SCREWING UP with the consequences this change can cause I would suggest to do extensive testing. Some bussinesses rely on this pligins stability...

 

--

 

Sent from my Nokia N9

 

[Kenny Ayers]

HX_unbanned,

I demonstrated the bug reproduction steps and testing methodology above, and the fix was submitted and accepted by SVNKit here: http://issues.tmatesoft.com/issue/SVNKIT-368

You can see the commit was made to trunk here: http://svn.svnkit.com/repos/svnkit/trunk/svnkit/src/main/java/org/tmatesoft/svn/core/internal/wc/SVNUpdateEditor15.java

And to 1.7.x branch, here: http://svn.svnkit.com/repos/svnkit/branches/1.7.x/svnkit/src/main/java/org/tmatesoft/svn/core/internal/wc/SVNUpdateEditor15.java

If you have reservations on the fix, please take a look at this material and let me know which part of it you'd like to discuss.  

Thank you,

Kenny

[Christoph Kutzinski]

Linards, if your business is critically depending on this plugins
stability, you should do testing on your side before updating the plugin
anyway.
Besides that, Jenkins developers definitely do not intend to "screw up"
plugins.

thanks
Christoph

> <http://teamcity.tmatesoft.com/viewLog.html?buildId=6105&tab=artifacts&buildTypeId=bt43>
> (http://issues.tmatesoft.com/issue/SVNKIT-368#comment=60-4930 <http://issues.tmatesoft.com/issue/SVNKIT-368#comment=60-4930>).

> I'm not sure what the process is for getting this updated
> in the plugin, please let me know if there is anything else
> I can do to help.
>
> Thank you,
>
> Kenny
>
>
> On Friday, April 26, 2013 3:04:03 AM UTC-7, Stephen Connolly
> wrote:
>
> Traditionally, the jenkins fork is maintaining a
> (hopefully) smaller set of patches on top of the
> upstream version.
>
> The aim is to get the set of patches to zero and then
> drop the fork.
>
> With reference to the above aim, my preference would be
> to get it in upstream rather than add to our current
> patch set.
>
> It is a real pain trying to update the code from
> upstream, at least every time I have tried I have had to
> give up and get KK to do it (he has some set of magic
> workspaces or something) so I would just love if we can
> get the need for this fork to disappear completely
>
> -Stephen
>
>
> On 26 April 2013 03:22, Kenny Ayers
> <theother...@gmail.com> wrote:
>
> Hi folks,
>

> *Short Version:*
> *
> *

> I may have a fix for Jenkins-14551

> (https://issues.jenkins-ci.__org/browse/JENKINS-14551 <https://issues.jenkins-ci.org/browse/JENKINS-14551>).

> I've submitted this potential resolution to SVNKit
> as well as their 1.7.6 SVN branch has the same issue

> (http://issues.tmatesoft.com/__issue/SVNKIT-368
> <http://issues.tmatesoft.com/issue/SVNKIT-368>).

> I've compiled the change into the
> subversion-plugin on my test server, and the fix
> appears to work.
>
> Can a contributor peer review this change?
>
> How do I go about submitting this fix to the
> Jenkins SVNKit repo? Do I need a unit test before I
> can do a pull request? The bug is obvious when you
> look at the code, and the unit test setup and
> execution seems like it would be complicated. I've
> forked the Jenkins SVNKit repo and committed the
> modification here:

> https://github.com/__theotherwhitemeat/svnkit-1/__commit/__27decb28216ee4fd15b8fcbdb769bf__41d81978eb
> <https://github.com/theotherwhitemeat/svnkit-1/commit/27decb28216ee4fd15b8fcbdb769bf41d81978eb>
>
> *Longer Version:*
> *
> *
> In
> org.tmatesoft.svn.core.__internal.wc.SVNUpdateEditor15.__java,

> in function addFileWithHistory (line 867), there's a
> code block that calls myFileFetcher.fetchFile()
> twice. Each time this is called, baseTextOS is
> written to. Upon the second write, the file
> contents are duplicated. Here's the code:
>
>
> baseTextOS =

> SVNFileUtil.__openFileForWriting(info.__copiedBaseText);
> myFileFetcher.fetchFile(__copyFromPath,

> copyFromRevision, baseTextOS, baseProperties);
> SVNChecksumOutputStream checksumBaseTextOS = new

> SVNChecksumOutputStream(__baseTextOS,
> SVNChecksumOutputStream.MD5___ALGORITHM, true);
> baseTextOS = checksumBaseTextOS;
> myFileFetcher.fetchFile(__copyFromPath,

> copyFromRevision, baseTextOS, baseProperties);
> info.copiedBaseChecksum =

> checksumBaseTextOS.getDigest()__;

>
>
>
> I was able to find this by stepping through the
> code using NetBeans IDE 7.3 attached to a remote
> debugging session on Jenkins. I've compiled and
> tested this change inside the context of the
> subversion-plugin and the file contents are no
> longer duplicated.
>
> I've forked the svnkit repo used in Jenkins here,
> and committed this change if anyone would like to
> download the fix and do some testing:
>

> https://github.com/__theotherwhitemeat/svnkit-1/__commit/__27decb28216ee4fd15b8fcbdb769bf__41d81978eb

> <https://github.com/theotherwhitemeat/svnkit-1/commit/27decb28216ee4fd15b8fcbdb769bf41d81978eb>
>
> Here's my patch:
>
> Index: SVNUpdateEditor15.java

> ==============================__==============================__=======
> --- SVNUpdateEditor15.java(revision 9722)
> +++ SVNUpdateEditor15.java(working copy)

> @@ -864,7 +864,6 @@
> OutputStream baseTextOS = null;
> try {
> baseTextOS =

> SVNFileUtil.__openFileForWriting(info.__copiedBaseText);
> -
> myFileFetcher.fetchFile(__copyFromPath,

> copyFromRevision, baseTextOS, baseProperties);
> SVNChecksumOutputStream
> checksumBaseTextOS = new

> SVNChecksumOutputStream(__baseTextOS,
>
> SVNChecksumOutputStream.MD5___ALGORITHM, true);

> baseTextOS = checksumBaseTextOS;
>
> Thank you,
>
> Kenny Ayers
>
> --
> You received this message because you are subscribed
> to the Google Groups "Jenkins Developers" group.
> To unsubscribe from this group and stop receiving
> emails from it, send an email to

> jenkinsci-de...@__googlegroups.com.
>
> For more options, visit
> https://groups.google.com/__groups/opt_out
> <https://groups.google.com/groups/opt_out>.

>
>
>
> --
> You received this message because you are subscribed to the
> Google Groups "Jenkins Developers" group.
> To unsubscribe from this group and stop receiving emails
> from it, send an email to jenkinsci-de...@googlegroups.com.
> For more options, visit
> https://groups.google.com/groups/opt_out

> <https://groups.google.com/groups/opt_out>.

[Wino Tu]

Hi,

If I could link in the svnkit correction matter I found that in file svnkit\src\main\java\org\tmatesoft\svn\core\wc\SVNEvent.java there is something like this:

line 419:

    public SVNExternal getExternalInfo() {

        return myExternalInfo;

    }

    public SVNExternal getPreviousExternalInfo() {

        return myExternalInfo;

    }

    public SVNEvent setExternalInfo(SVNExternal prev, SVNExternal _new) {

        this.myPreviousExternalInfo = prev;

        this.myExternalInfo = _new;

        return this;

    }

I think is typical c&p error. Could you also add correction for this ?

Thanks,

Chris Z.

an email to jenkinsci-dev+unsubscribe@googlegroups.com.

For more options, visit https://groups.google.com/groups/opt_out.



--
You received this message because you are subscribed to the Google
Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send

an email to jenkinsci-dev+unsubscribe@googlegroups.com.

For more options, visit https://groups.google.com/groups/opt_out.

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.

To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-dev+unsubscribe@googlegroups.com.