123388 - FTP should accept CR line endings (xitami server)

Status: VERIFIED FIXED

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

Description

[Mike Kaply [:mkaply]]

Download a xitami web server (I used Windows) and install it.

Try to FTP.

You get a data connection error.

Comment 1

[Mike Kaply [:mkaply]]

You can add bulletproof FTP server to this list:

www.bpftpserver.com

I can't find a Windows FTP server to test with because none of them work with 
Mozilla.

Comment 2

[Matthias Versen [:Matti]]

Bradley: I have Xitami installed. If you need a tescase and I'm on IRC...
(You can also try: FTP-Serv-U and TYPSOFT FTP server 0.95. All this servers are
working with NS4.7x but not with Mozilla)

Comment 3

[Matthias Versen [:Matti]]

Attachment: Xitami Log with the patch from bug 110241. I hit Stop after 20sec

Comment 4

[R.K.Aa.]

Matti: i don't think you meant bug 11024. Did you mean patch in bug 110241?

Comment 5

[Matthias Versen [:Matti]]

sure, i mean bug 110241 (bbaetz knows which bug i mean :-)

Comment 6

[Bradley Baetz (:bbaetz)]

Can I get a packet trace? I think they'Re sending cr instead of crlf. Yuck.

Comment 7

[Matthias Versen [:Matti]]

Is this enough ?
T 193.159.70.191:21 -> 192.168.0.10:1112 [AP]
  32 32 30 2d 20 57 65 6c    63 6f 6d 65 20 74 6f 20    220- Welcome to
  74 68 69 73 20 58 69 74    61 6d 69 20 46 54 50 20    this Xitami FTP
  73 65 72 76 65 72 2c 20    72 75 6e 6e 69 6e 67 20    server, running
  76 65 72 73 69 6f 6e 20    32 2e 35 62 34 20 6f 66    version 2.5b4 of
  20 58 69 74 61 6d 69 2e    20 0a 20 59 6f 75 20 61     Xitami. . You a
  72 65 20 75 73 65 72 20    6e 75 6d 62 65 72 20 31    re user number 1
  20 6f 66 20 61 20 70 65    72 6d 69 74 74 65 64 20     of a permitted
  32 35 20 75 73 65 72 73    2e 20 0a 20 20 0a 32 32    25 users. .  .22
  30 20 58 69 74 61 6d 69    20 46 54 50 20 32 2e 35    0 Xitami FTP 2.5
  62 34 20 28 63 29 20 31    39 39 31 2d 32 30 30 30    b4 (c) 1991-2000
  20 69 4d 61 74 69 78 20    3c 68 74 74 70 3a 2f 2f     iMatix <http://
  77 77 77 2e 69 6d 61 74    69 78 2e 63 6f 6d 3e 0d    www.imatix.com>.

Comment 8

[Bradley Baetz (:bbaetz)]

RFC959:

 End-of-Line

         The end-of-line sequence defines the separation of printing
         lines.  The sequence is Carriage Return, followed by Line Feed.

This just uses CR (0x0a instead of 0x0a 0x0d), and so the server is broken. Much
as I'd love to INVALID this, ns4 coped, and it seems a large number of windows
servers are broken in this respect.

The ftp directory listing stuff seems to have code to handle this, so it
shouldn't be too much effort. I'll cook up a patch for someone to test. It would
be great if someone could point me to a publically available server which has
this problem.

Please don't test without my second patch on bug 110241, which I'll checkin when
the tree opens.

Comment 9

[Bradley Baetz (:bbaetz)]

Can someone point me to a public server where I can test this?

Comment 10

[Matthias Versen [:Matti]]

Ask me on IRC and i start my server...

Comment 11

[Bradley Baetz (:bbaetz)]

broken server, no time => 1.1alpha

Comment 12

[Matthias Versen [:Matti]]

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

Comment 13

[benc]

tapped summary to make cause more searchable.

Comment 14

[Bradley Baetz (:bbaetz)]

I have no time to work on mozilla at the moment, so dougt is taking over FTP

open ftp bugs -> him

Comment 15

[neeti]

Could anyone post a testcase to reproduce this bug?

Comment 16

[Bradley Baetz (:bbaetz)]

neeti - your patch for bug 141714 probably didn't help this - you need to make
CRLF _OR_ CR _OR_ LF, not just one (and matching for LF instead of CRLF will
leave extra blank lines in the response for conforming servers, which is wrong)

For a testcase, download one of the servers mentioned in comment 0 or comment 1...

Comment 17

[neeti]

Attachment: proposed patch

Comment 18

[Bradley Baetz (:bbaetz)]

Comment on attachment 100437 [details] [diff] [review]
proposed patch

>+
>+       eolLength = strcspn(currLine, CRLF);

IS this function portable?

>+
>+        if ((currLine[eolLength] == nsCRT::CR) && (currLine[eolLength+1] == nsCRT::LF))

You need to make sure that you don't run of the end of the string, here


>@@ -691,8 +700,8 @@

>+              if ((mTryingCachedControl && mResponseCode == 530 &&
>+                  mInternalError == NS_ERROR_FTP_PASV) || (mResponseCode == 425 && mInternalError == NS_ERROR_FTP_PASV)) {

Whats this part for?

Comment 19

[Doug Turner (:dougt)]

>IS this function portable?

yeah, it should be.

Comment 20

[neeti]

Attachment: revised patch

Checked strcspn patch on windows and linux, works ok. Confirmed with sfraser
that is available on Mac.

>+if ((currLine[eolLength] == nsCRT::CR) && (currLine[eolLength+1] ==
nsCRT::LF))

new patch checks for end of string

>+ if ((mTryingCachedControl && mResponseCode == 530 &&
>+  mInternalError == NS_ERROR_FTP_PASV) || (mResponseCode == 425 &&
mInternalError == NS_ERROR_FTP_PASV)) {

We are getting a 425 - can't open data connection error. Without this patch we
just stop processing and throw an error dialog, instead of reconnecting.

Comment 21

[Bradley Baetz (:bbaetz)]

Yes, but without this patch for the newline characters, throwing an error is the
correct thing to do. ie that error code means we shouldn't bother to retry.
Doesn't it?

Comment 22

[neeti]

We do not get the 425 data connection error with Netscape4.7x and IE and they
both are able to display the page properly.

Comment 23

[Doug Turner (:dougt)]

Comment on attachment 100606 [details] [diff] [review]
revised patch

+	PRInt32 eolLength = 0;
+	PRInt32 crlfLength = 0;
+	PRInt32 currLineLength = 0;
+
+	eolLength = strcspn(currLine, CRLF);
+	currLineLength = strlen(currLine);

there is no need to assign twice here.	


+	if ((!eolLength) && (currLine[eolLength] != nsCRT::CR)
+	    && (currLine[eolLength] != nsCRT::LF)) {

How does this logic work?  Maybe I need another cup of coffee, but if eolLength
is null, how can anything in currLine ever be either nsCRT:LF or nsCRT:CR.  


+
+	 if ((currLineLength >= (eolLength + 1)) && (currLine[eolLength] ==
nsCRT::CR)
+	     && (currLine[eolLength+1] == nsCRT::LF))
+	     crlfLength = 2; // CR +LF 
+	 else 
+	     crlfLength = 1; // + LF or CR
+
+	 line.Assign(currLine, eolLength + crlfLength);

cant you just do something like:

crlfLength = currLineLength - eolLength 


+		   mInternalError == NS_ERROR_FTP_PASV) || (mResponseCode ==
425 && mInternalError == NS_ERROR_FTP_PASV)) {

80 chars to a line please.

Comment 24

[neeti]

+
PRInt32 eolLength = 0;
+
PRInt32 crlfLength = 0;
+
PRInt32 currLineLength = 0;
+
+
eolLength = strcspn(currLine, CRLF);
+
currLineLength = strlen(currLine);

Changed the assignment happening twice.

+
if ((!eolLength) && (currLine[eolLength] != nsCRT::CR)
+
    && (currLine[eolLength] != nsCRT::LF)) {

stcspn return the index of the first occurrence of CR or LF. CurrLine could have
multiple CR's and LF's all over it. If we have a CR or LF as the first character
in the line, then eolLength would be 0. In that case we need to check for 

currLine[eolLength] != nsCRT::CR) && (currLine[eolLength] != nsCRT::LF)

+
+
 if ((currLineLength >= (eolLength + 1)) && (currLine[eolLength] ==
nsCRT::CR)
+
     && (currLine[eolLength+1] == nsCRT::LF))
+
     crlfLength = 2; // CR +LF 
+
 else 
+
     crlfLength = 1; // + LF or CR
+
+
 line.Assign(currLine, eolLength + crlfLength);

>cant you just do something like:

>crlfLength = currLineLength - eolLength 

We cannot do crlfLength = currLineLength - eolLength, because currLine is not
delimited by just one CRLF, but could have multiple text lines.

+
	   mInternalError == NS_ERROR_FTP_PASV) || (mResponseCode ==
425 && mInternalError == NS_ERROR_FTP_PASV)) {

>80 chars to a line please. 
done

 

Comment 25

[neeti]

Attachment: revised patch

Comment 26

[Doug Turner (:dougt)]

Comment on attachment 100901 [details] [diff] [review]
revised patch

r=dougt

Comment 27

[Darin Fisher]

Comment on attachment 100901 [details] [diff] [review]
revised patch

>Index: nsFtpConnectionThread.cpp

>     const char* currLine = lines.get();
>     while (*currLine) {
>+           
>+       PRInt32 eolLength = strcspn(currLine, CRLF);
>+       PRInt32 currLineLength = strlen(currLine);
>+
>+       if ((!eolLength) && (currLine[eolLength] != nsCRT::CR) &&
>+           (currLine[eolLength] != nsCRT::LF)) {
>+           mControlReadCarryOverBuf.Assign(currLine);
>+           break;
>+       }

is strcspn really available cross-platform?  even on the mac?

i think you could simplify this to:

      while (*currLine) {
	  PRInt32 eolLength = strcspn(currLine, CRLF);

	  if (eolLength == 0 && currLine[0] == '\0')
	      break;

	  PRInt32 currLineLength = strlen(currLine);

	  ...
      }

strcspn will only return zero if the first byte in the string CR, LF, or \0.
this means that assigning currLine to mControlReadCarryOverBuf is a no-op.
BTW: is that what you intended?  i think this patch still needs some work.

Comment 28

[neeti]

Attachment: revised patch

stcspn() is available on the Mac (checked with sfraser).

We should only be assigning currLine to mControlReadCarryOverBuf only when
currLine contains no CRLF(eolLength == currLineLength)

thanks for catching this.

Comment 29

[neeti]

Attached revised patch

Comment 30

[Darin Fisher]

Comment on attachment 100914 [details] [diff] [review]
revised patch

>Index: nsFtpConnectionThread.cpp

>     const char* currLine = lines.get();
>     while (*currLine) {
>+       PRInt32 eolLength = strcspn(currLine, CRLF);
>+
>+       if (eolLength == 0 && currLine[0] == '\0')
>+	      break;

minor nit: please eliminate tab in front of "break"


>+        if ((currLineLength >= (eolLength + 1)) &&

minor nit: doesn't that line of code simplify to:

	  if ((currLineLength > eolLength) &&

sr=darin

Comment 31

[neeti]

Attachment: patch removing the nits

Comment 32

[Darin Fisher]

Comment on attachment 100918 [details] [diff] [review]
patch removing the nits

sr=darin

minor minor nit:

if ((currLineLength > (eolLength)) &&

no need for extra ( )'s around eolLength.  no need for a new patch :-) 

thx neeti!

Comment 33

[neeti]

Comment on attachment 100918 [details] [diff] [review]
patch removing the nits

r=dougt from aim

Comment 34

[neeti]

fixed

Comment 35

[Jimmy Lee]

Build: 2002-10-22-08-trunk

Installed v2.4d9.  I am able to open FTP sites.  Marking Verified!