that doesn't change the flaw of the patch, if there are scenarios where the paths indeed are different, then you need to answer the following questions:

1. What are those scenarios
2. And is the code correct in those scenarios

otherwise, I would change your patch to just fix the copy method, that when the destination and the source are the same, don't perform the copy, then you can safely guarantee it to work. but providing a patch that fixes your scenario, yet there is no proof that it fixes all, will be hard to squeeze in.

There are scenarios where CATALINA_BASE is different from CATALINA_HOME, and not different as in symlinked, different as in two different directories.

Fliip

Haroon Rafique wrote:
On Today at 9:29am, FHDL=>Filip Hanik - Dev Lists <[EMAIL PROTECTED]> wrote:

FHDL> Haroon Rafique wrote:
FHDL> >/www/apache-tomcat-5.5.16/webapps/sws.war
FHDL> >/www/tomcat/webapps/sws.war
FHDL> FHDL> this would turn my vote into -1, based on false information provided
FHDL> earlier.
FHDL> That should be explanation enough.
FHDL>
Hi Filip,

Seems like everyone is missing my point.

False information??? you guys are pretty paranoid. I explained clearly that /www/tomcat is a symlink to /www/apache-tomcat-5.5.16.

FHDL> Haroon, you'd need to provide a more solid test case, if the paths FHDL> indeed are different. FHDL> FHDL> Filip

To further quench your fears. I removed the symlink and actually named the directory /www/tomcat. Then I changed the code in question to as follows:

if (tag != null) {
    File localWarCopy = new File(deployed, basename + ".war");
    System.out.println("Copying " + localWar + " to " + localWarCopy);
    copy(localWar, localWarCopy);
    System.out.println("Length: " + localWarCopy.length());
    localWar = localWarCopy;
    Thread.sleep(20000);
    File needlessCopy = new File(getAppBase(), basename + ".war");
    System.out.println("Copying " + localWar + " to " + needlessCopy);
    copy(localWar, needlessCopy);
    System.out.println("Length: " + needlessCopy.length());
} Pardon me for my sarcasm while I use a variable called needlessCopy. Here's the output in catalina.out when I run a remote deploy with a tag using ant:

Copying /www/tomcat/work/Catalina/localhost/manager/some-tag-here/sws.war to /www/tomcat/webapps/sws.war
Length: 8047695
Copying /www/tomcat/webapps/sws.war to /www/tomcat/webapps/sws.war
Length: 0

You can draw your own conclusions. Are they identical???


FHDL> FHDL> FHDL> > On Today at 11:06am, RM=>Reinhard Moosauer <[EMAIL PROTECTED]> wrote:
FHDL> >
FHDL> > RM> Hi Filip, Haroon,
FHDL> > RM> FHDL> > RM> as far as I understand, the problem is a copy operation which copies
FHDL> > RM> one file on itself.
FHDL> > RM> Haroon showed that this is happing in his case.
FHDL> > RM> Unfortunately, it it not proved that this is happening in all cases.
FHDL> > RM> Furthermore, the removal of the second copy operation could still
FHDL> > RM> cause a regression in other cases.
FHDL> > RM> FHDL> >
FHDL> > Hi Reinhard,
FHDL> >
FHDL> > I'm obviously not very familiar with the tomcat code and by no means an
FHDL> > expert so I didn't realize that there was copy protection involved. In
FHDL> > any case, all of the involved code only works if tag is non-null. To my
FHDL> > knowledge, I'm the only one who uses remote war deployment while using
FHDL> > tags, and the empirical evidence that I have suggests that the 2nd copy
FHDL> > operation is not necessary (it could have even been a copy-and-paste
FHDL> > error).
FHDL> >
FHDL> > RM> FHDL> > RM> Maybe we could fix the bug in a more secure way:
FHDL> > RM> How about adding a check to the copy()-method, so that it can no
FHDL> > RM> more delete a file by copying on itself?
FHDL> > RM> We could:
FHDL> > RM> 1. just ignore the copy if the 2 paths are the same
FHDL> > RM> 2. throw an IllegalArgumentException if the 2 paths are the same
FHDL> > RM> FHDL> >
FHDL> > Gotta be careful here. When I said the 2 paths are same, I generalized a
FHDL> > little bit. The two paths are almost the same. They are:
FHDL> >
FHDL> > /www/apache-tomcat-5.5.16/webapps/sws.war
FHDL> > /www/tomcat/webapps/sws.war
FHDL> >
FHDL> > One of them involves a symlink. They are the same as long as you
FHDL> > consider the fact that /www/tomcat is a symbolic link to
FHDL> > apache-tomcat-5.5.16:
FHDL> >
FHDL> > lrwxrwxrwx 1 root root 20 Mar 21 14:33 /www/tomcat ->
FHDL> > apache-tomcat-5.5.16
FHDL> >
FHDL> > So, my (non-binding) vote :-), would be to:
FHDL> >
FHDL> > 1) Remove the 2nd copy() method all together
FHDL> > 2) Clean up copy() method to throw an IllegalArgumentException if the 2 FHDL> > paths are the same while *ALSO* taking into account any symbolic
FHDL> >    links.
FHDL> >
FHDL> > RM> FHDL> > RM> Personally, I would prefer the second solution, because it make
FHDL> > RM> errors like this one better to find in the future.
FHDL> > RM> The two code-lines removed by Haroon could have a try-catch-block to
FHDL> > RM> ignore the IllegArgExc here (with a bold FIXME-Message to the log!)
FHDL> > RM> FHDL> > RM> ***
FHDL> > RM> I suggest to be very careful in changing this part of tomcat. This
FHDL> > RM> case proves again, that the test-coverage in the manager code is not
FHDL> > RM> pretty good. Mainly because there are many configuration and
FHDL> > RM> deployment scenarios, which affect its behavior. (I suffered a
FHDL> > RM> similar case some days ago)
FHDL> > RM> ***
FHDL> > RM> FHDL> >
FHDL> > I agree about test-coverage.
FHDL> >
FHDL> > I fully appreciate and understand the need for caution. However, by
FHDL> > their own admission, most tomcat developers do not have the time to test
FHDL> > the remote deployment while tagging via ant. To my knowledge, I am the
FHDL> > only who uses this in production with tomcat 5.5.12+ (since I'm the only
FHDL> > one complaining about it for the last 6 months or so). Combined with the
FHDL> > fact that the whole operation is inside a if (tag != null) {} leads me
FHDL> > to believe that it is safe to take out the 2nd copy() method. Believe
FHDL> > me, with the 2nd copy() method in tact, remote deploying via ant while
FHDL> > tagging does *NOT* work.
FHDL> >
FHDL> > RM> FHDL> > RM> Please tell, if I can help FHDL> > RM> FHDL> > RM> Reinhard FHDL> > RM> FHDL> >
FHDL> > You have helped already by replying.
FHDL> >
FHDL> > Regards,
FHDL> > --
FHDL> > Haroon Rafique
FHDL> > <[EMAIL PROTECTED]>
FHDL> >
FHDL> > ---------------------------------------------------------------------
FHDL> > To unsubscribe, e-mail: [EMAIL PROTECTED]
FHDL> > For additional commands, e-mail: [EMAIL PROTECTED]
FHDL> >
FHDL> > FHDL> FHDL> FHDL> ---------------------------------------------------------------------
FHDL> To unsubscribe, e-mail: [EMAIL PROTECTED]
FHDL> For additional commands, e-mail: [EMAIL PROTECTED]
FHDL> FHDL>
--
Haroon Rafique
<[EMAIL PROTECTED]>


---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]



---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]

Reply via email to