129811 - PASV data connection issues

Status: VERIFIED FIXED

(Core Graveyard :: Networking: FTP, defect)

Description

[Bradley Baetz (:bbaetz)]

The mozilla ftp state machine used to send PASV, LIST, then RETR. Some servers
closed the data connection after the LIST failed for a file, and so a while back
dougt added code to add an extra PASV command after a failing LIST, starting a
new connection. (bug 92582) Servers not working without this included
ftp.asus.com and ftp.microsoft.com, among others

This has caused several problems relating to when connections are closed, which
connection the server expects us to use and so on. (bug 101128 and
dependancies/dupes) Some require us to use them in order of PASV, regardless of
whether LIST succeeded, if the data connection open, others reuse the same
connection, meaning that we can't close it, and so on. See bugs 103514, 127003,
101128, and several dupes. This may also be the cause of bug 112250, bug 128237
and several other comments in various bugs (where that issue isn't the one for
that bug, but the commenter doens't open a new bug)

I'll use a testcase from a bug akkana filed, which was a dupe of the crasher
127003, but her test case doesnt' work even with that fixed:

To reproduce, go to ftp://ftp.mutt.org/mutt/, then try to download a file. It
will hang, because its sending data on the same data conneciton used for LIST,
whilst we connected to the same dest port from a different source port.

The problem was lived with because ie/ns4 has the same issues if you disabled
PORT mode (Which we don't support), however I think I have come up with a fix.
The fix is to only open a new one if the ports are different.

I'll attach a patch which works with ftp.mutt.org, and doesn't appear to break
anything else.

mkaply, can you please test on os2? I removed the os2 workarround which
hopefully should no longer be needed.

comments on the patch, everyone else?

Comment 1

[Bradley Baetz (:bbaetz)]

Attachment: patch

Comment 2

[tenthumbs]

Have you considered using the ABOR command? It would seem to be the
thing to do to cancel a pending data connection.

Comment 3

[Bradley Baetz (:bbaetz)]

ABOR only works if we're actully trasnferring something - this case is where we
try to download a drirectory (which fails) - theres no transfer to abort.

Comment 4

[Bradley Baetz (:bbaetz)]

tever/benc: Can I get you to run regression tests on this patch please, see if
we can find one where it breaks?

Comment 5

[Doug Turner (:dougt)]

Comment on attachment 73379 [details] [diff] [review]
patch

Maybe we should use some other status?

+
+    if (mUploading)
+	 request->Cancel(NS_OK);

Comment 6

[tenthumbs]

> ABOR only works if we're actually transferring something - this case 
> is where we try to download a directory (which fails) - there's no
> transfer to abort.

But there is. Mozilla does a PASV _before_ the RETR so a data connection
exists. The server may well have its side of that connection already
open. Mozilla probably does as well. All the servers with problems all
produce error messages that refer to data. I'm just suggesting that ABOR
exists for the explicit purpose of canceling a data connection. It
would seem to be a good idea to try to use documented commands before
trying assorted kludges.

Comment 7

[Darin Fisher]

Comment on attachment 73379 [details] [diff] [review]
patch

>Index: nsFtpConnectionThread.cpp

>+        if (newDataConn) {
>+            if (st)
>+                st->SetReuseConnection(PR_FALSE);
>+            mDPipe = 0;

maybe you want to release |st| as well here?

sr=darin

Comment 8

[Bradley Baetz (:bbaetz)]

Hmm. Let me play with this, see where it gets me.

The mUploading bit is part of another patch, oops.
st is just mDPipe after a QI, so its released at the end of the block.

Comment 9

[Tom Everingham]

Hey bbaetz, the URL posted doesn't give me a file listing with todays build.  I
get a 'Finished FTP transaction' while the page continuously tries to load. 
Eventually, a blank index page comes up.    This is without the patch.

Comment 10

[Bradley Baetz (:bbaetz)]

Right; thats what this patch is meant to (hopefully) fix.

I'll look at the ABOR issue when I get home. That would be a cleaner issue. You
may want to hold off testing until I check that out.

Comment 11

[Bradley Baetz (:bbaetz)]

*** Bug 131163 has been marked as a duplicate of this bug. ***

Comment 12

[Bradley Baetz (:bbaetz)]

OK, I tried out ABOR. It doesn't seem to work on any servers which didn't work.
Some servers still try to write to the closed and ABOR'd socket even though
they've opened a new one (ftp.caudium.net, bug 131163), and others try to write
to the old socket, with pasv returning a new port, which is what this patch
fixes (ftp.3com.com).

I suspect that servers won't handle this unless its between the two responses
from the LIST/RETR commands. Most srevers I tried accept ABOR with a scucess
return code after just USER/PASS, so the fact that it succeeds is an issue

Combining the two options isn't really going to work, so I'd like to stick with
the patch I have.

tever: can you test this, please?

Comment 13

[tenthumbs]

> Most srevers I tried accept ABOR with a scucess return code after
> just USER/PASS, so the fact that it succeeds is an issue.

I do believe it's supposed to return 226 on success and 225 on failure.

Comment 14

[Bradley Baetz (:bbaetz)]

The problem is that the server returned 226, but still tried to use teh old PASV
connection.

Comment 15

[Tom Everingham]

bbaetz, ok I applied the patch and went through a list of known working servers
of different types.  The original url now works. (I checked it before adding the
patch and it did not work, as expected) 

I am seeing a problem when trying to access ftp://ftp.mozilla.org.  It spins for
a little while and eventually comes up with an index page but no files listed. 
I'm seeing the same problem hitting our internal daily build server.  They are 
running wu-2.6.1 and wu-2.6.0 respectively but I have other test servers with
these versions that work properly. 

ftp://ftp.3com.com doesn't work.  Status bar says "beginning FTP transaction"
which doesn't go away and then the busy indicator just spins.

Comment 16

[Bradley Baetz (:bbaetz)]

ftp.3com.com won't work - thats bug 128927, and isn't affected by this patch.

I've been seeing problems with ftp.m.o even with ns4 and ncftp, and one of the
irc bots complains about it every so often - I think its just the load, and the
lack of error reporting in some cases. Although that wouldn't explain the
problems with the ns server.

Just to confirm, you're only seeing those problems are only with this patch?

Comment 17

[Tom Everingham]

ok, i misunderstood your comment on ftp.3com.com 

yes, I compared the patched version to build 2002031908. With the patched
version I get the "Index of ftp://ftp.mozilla.org/" and two bars with no files
listed. After a minute (I didn't notice this yesterday) I get an Alert dialog
which says "Can't open data connection".  Same with our internal server.

With build 2002031908 I get the "Index of ftp://ftp.mozilla.org/" with a list of
files between the two bars.

Comment 18

[Tom Everingham]

sorry, actually I'm comparing to a trunk build.  I'll have to get a mozilla
build to compare this to I guess.

Comment 19

[Tom Everingham]

ok, compared it to the nightly mozilla build and I don't have a problem hitting
either of those sites.  I'm only seeing it on the patched version.

Comment 20

[Bradley Baetz (:bbaetz)]

*** Bug 133146 has been marked as a duplicate of this bug. ***

Comment 21

[Bradley Baetz (:bbaetz)]

Attachment: v2

OK, got it. Don't try reusing the connection if the srever has closed it.
Checked that this fixes with with ethereal + an assertion if the port is the
same, but isAlive is false.

(This also includes the one liner for bug 120473)

Comment 22

[Tom Everingham]

checked the new patched version againt my list of different servers and a few
dozen random sites.  They were all able to connect and I could download off the
ones that I tried.  ftp.m.o and our build server now display a list of files. 
The site in bug 120473 also works with the patch.

looks good to me

Comment 23

[Darin Fisher]

Comment on attachment 75874 [details] [diff] [review]
v2

>Index: nsFtpConnectionThread.cpp

how about some comments explaining why this is the right time to cancel
the request?  it's not obvious.

>+    if (mUploading)
>+        request->Cancel(NS_OK);

>+            ( mResponseMsg.Find("MACOS Peter's Server") > -1))

heh :-)


>-    if ((mResponseCode/100 == 5) && (mServerType != FTP_OS2_TYPE)) {
>+    if (mResponseCode/100 == 5) {

FTP_OS2_TYPE is now ok?


>+        rv = mDPipe->AsyncRead(mDRequestForwarder, nsnull, 0, PRUint32(-1), 0, getter_AddRefs(mDPipeRequest));    
>+        if (NS_FAILED(rv)){
>+            PR_LOG(gFTPLog, PR_LOG_DEBUG, ("(%x) forwarder->AsyncRead failed (rv=%x)\n", this, rv));
>+            return FTP_ERROR;
>+        }

all or any of the network events could already be in your event queue by the
time
AsyncRead returns.

>+        
>+        // Suspend the read
>+        // If we don't do this, then the remote server could close the
>+        // connection before we get the error message, and then we process the
>+        // onstop as if it was from the real data connection
>+        rv = mDPipeRequest->Suspend();

this call is not guaranteed to block any or all events (see bug 93055).

despite all of the testing, there really is no guarantee here.	if it causes
problems to receive nsIStreamListener events after calling Suspend, then this
bug is not fixed because you will in some rare cases context switch and have
events posted before reaching this call to Suspend.

at any rate, this at least appears to work, so sr=darin.

Comment 24

[Bradley Baetz (:bbaetz)]

The only thing we want to Suspend() from is the OnStop notification when the
server closes teh connection on us.

Yes, this isn't guarnteed to work, but it works in all the cases I can find, and
we're no worse off if it doesn't.

Comment 25

[Gagan]

Comment on attachment 75874 [details] [diff] [review]
v2

r=gagan looks all ok to me but make sure we have enough servers tested for this
change. thanks bradley!

Comment 26

[Asa Dotzler [:asa]]

Comment on attachment 75874 [details] [diff] [review]
v2

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 27

[Bradley Baetz (:bbaetz)]

Checked in

Comment 28

[benc]

VERIFIED:
this has worked on several ftp runs, including mozilla 1.1 and netscape 7.