130299 - confused by , in PASV response message

Status: RESOLVED FIXED

(Core Graveyard :: Networking: FTP, defect, P3)

Description

[Andreas Schoenberg]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9.9) Gecko/20020311
BuildID:    2002031104

mozilla does not except server response codes that are different from:
"227 Entering Passive Mode"
But as described in rfc 1123 the client should only trigger on the three digits
response code and not on the text behind it, because the text is not well
standardized.

Reproducible: Always
Steps to Reproduce:
1. open ftp://ftp.ftpproxy.org
2.
3.

Actual Results:  mozilla is unable to connect to the server.

Comment 1

[Matthias Versen [:Matti]]

I get an error : "ok, entering passive mode (....) " but i don't get the
directory list 

-> New

Comment 2

[Bradley Baetz (:bbaetz)]

No, thats not the issue here. Possibly similar to bug 128927. I'll check if that
patch helps me, in a bit.

Comment 3

[Bradley Baetz (:bbaetz)]

OK, its not the code, or the CRLF in a separate packet. The problem is the comma
in the message.

For PASV, we have to parse the returned value, and there appears to be code in
there to look for both (xxx,xxx,xxx,xxx,ppp,ppp) form, as well as the form
without brackets. So when we see the , in "ok, entering passive mode", we die
because "ok" isn't a valid number.

We should probably just try with brackets, then without. As with much of ftp,
theres no standard for this, unfortunately.

Comment 4

[Bradley Baetz (:bbaetz)]

Well... I've fixed this probelm, and will attach a patch. This still doesn't fix
the problem with this server though.

The problem is that RETR <directory> returns a 0 length file. ns4 works because
it tries to LIST names ending with a / first. ftp://ftp.ftpproxy.org/beta
(without the trailing /) fails. This is a server bug, I think.

Anyway, patch for this issue coming up.

Comment 5

[Bradley Baetz (:bbaetz)]

Attachment: patch

We first look for numbers in brackets, then try without.

To verify that this fixes the problem, observe that with the patch this doesn't
hang, or try to download ftp://ftp.ftpproxy.org/ftpproxy-1.1.5.tgz directly (or
any other file, not directory.

Tested on localhost, ftp.m.o, and ftp.ftpproxy.org

Comment 6

[Darin Fisher]

it seems like you shouldn't have to call PR_sscanf in two different places.  if
you can somehow determine the location of the first number, then you can just
call PR_sscanf from there.  how about searching backwards instead of forwards?

i.e., search backwards for the 5th comma, and then search backward until you're
no longer seeing ASCII digits, then call PR_sscanf.  wouldn't that be simpler? 
or is  it likely that servers will send random text after the "ww,xx,yy,zz,aa,bb"?

Comment 7

[Bradley Baetz (:bbaetz)]

I don't want to be tricked by:

2xx PASV channel number 1, go ahead (a,b,c,d,x,y)

or something. Robustness ;)

Comment 8

[Doug Turner (:dougt)]

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

+	 if (*ptr) {
+	     ++ptr;
+	     fields = PR_sscanf(ptr, 

You should probably check for null before calling PR_sscanf. (in both cases)

Add an assertion if fields is <6 in the second case.  Maybe it will help us
track down another problem.

fix that and r=dougt

Comment 9

[Bradley Baetz (:bbaetz)]

ptr can't be null - we get it from a string just above.

*ptr could be null, but that means we have an empty string, which then won't
match in scanf, so thats ok

Added:

NS_ASSERTION(fields == 6, "Can't parse PASV response"); just before the if at
the end of the patch.

Comment 10

[Bradley Baetz (:bbaetz)]

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

Comment 11

[Bradley Baetz (:bbaetz)]

Attachment: v2

added the assertion in

Comment 12

[Darin Fisher]

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

sr=darin

Comment 13

[Darin Fisher]

i sr='d the patch because it looks solid, but in response to comment #7,

  "2xx PASV channel number 1, go ahead (a,b,c,d,x,y)"

would not get fouled up if you searched backwards for the 5th comma, and then
searched backward until you're no longer on an ASCII digit, and then called
PR_sscanf.  it just seems like a much simpler algorithm... but whatever :-/

Comment 14

[Bradley Baetz (:bbaetz)]

but then I need to do isnum... scanf is designed for this, and I prefer clarity,
given the non-perf critical nature of this code.

Comment 15

[Gagan]

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

r=gagan
make sure this gets enough testing done too!

Comment 16

[Asa Dotzler [:asa]]

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

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

Comment 17

[Bradley Baetz (:bbaetz)]

Checked in.

Comment 18

[benc]

reporter: is this working for you? Try RC1.

Comment 19

[Andreas Schoenberg]

Yes. is working fine.

Thanks

Comment 20

[benc]

VERIFIED: per reporter.

Comment 21

[benc]

In the past, this site had stopped working, it would connect, then display a
blank page. I did not file a bug, because when I tested their server by hand, it
would close the connection immediately after connecting.

However, the site is now working via ftp from DOS and Solaris line command
clients, and is not working via Mozilla 1.3a nor from a recent, patch-test daily
build that Darin gave me.

I'm not sure what to do, so I'm starting by reopening and keyword +regression.
If this is a new problem w/ a new bug, I'll move.

Comment 22

[Andreas Schoenberg]

We found the problem.


It was yet not mozilla bug, but a ftp.server bug.


The ftp.server did not reply correctly on a 'GET <DIRECTORY>' request.


The problem is fixed and I think we can re-close the ticket.




Thanks


Andreas

Comment 23

[benc]

RESOLVED, WFM: Chimera 0.6 + Mozilla 1.3b/Mach-O.