FTP should accept CR line endings (xitami server)

VERIFIED FIXED in mozilla1.1alpha

Status

()

P3
minor
VERIFIED FIXED
17 years ago
16 years ago

People

(Reporter: mkaply, Assigned: neeti)

Tracking

Trunk
mozilla1.1alpha
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [broken server], URL)

Attachments

(6 attachments)

(Reporter)

Description

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

Try to FTP.

You get a data connection error.
(Reporter)

Comment 1

17 years ago
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.
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)
Created attachment 67793 [details]
Xitami Log with the patch from bug 110241. I hit Stop after 20sec

Comment 4

17 years ago
Matti: i don't think you meant bug 11024. Did you mean patch in bug 110241?
Attachment #67793 - Attachment description: Xitami Log with the patch from bug 11024. I hit Stop after 20sec → Xitami Log with the patch from bug 110241. I hit Stop after 20sec
sure, i mean bug 110241 (bbaetz knows which bug i mean :-)
Can I get a packet trace? I think they'Re sending cr instead of crlf. Yuck.
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>.
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.
Status: NEW → ASSIGNED
Keywords: 4xp, mozilla1.0
Summary: Mozilla can't FTP to a xitami server → FTP should accept CR line endings
Whiteboard: [broken server]
Target Milestone: --- → mozilla0.9.9
Can someone point me to a public server where I can test this?
Severity: normal → minor
Priority: -- → P3
Target Milestone: mozilla0.9.9 → mozilla1.0
Ask me on IRC and i start my server...

broken server, no time => 1.1alpha
Target Milestone: mozilla1.0 → mozilla1.1alpha
*** Bug 152661 has been marked as a duplicate of this bug. ***

Comment 13

17 years ago
tapped summary to make cause more searchable.
Summary: FTP should accept CR line endings → FTP should accept CR line endings (xitami server)
I have no time to work on mozilla at the moment, so dougt is taking over FTP

open ftp bugs -> him
Assignee: bbaetz → dougt
Status: ASSIGNED → NEW
(Assignee)

Comment 15

16 years ago
Could anyone post a testcase to reproduce this bug?
Assignee: dougt → neeti
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...
(Assignee)

Comment 17

16 years ago
Created attachment 100437 [details] [diff] [review]
proposed patch
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

16 years ago
>IS this function portable?

yeah, it should be.
(Assignee)

Comment 20

16 years ago
Created attachment 100606 [details] [diff] [review]
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.
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?
(Assignee)

Comment 22

16 years ago
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

16 years ago
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.
Attachment #100606 - Flags: needs-work+
(Assignee)

Comment 24

16 years ago
+
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

 







(Assignee)

Comment 25

16 years ago
Created attachment 100901 [details] [diff] [review]
revised patch

Comment 26

16 years ago
Comment on attachment 100901 [details] [diff] [review]
revised patch

r=dougt
Attachment #100901 - Flags: review+

Comment 27

16 years ago
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.
Attachment #100901 - Flags: needs-work+
(Assignee)

Comment 28

16 years ago
Created attachment 100914 [details] [diff] [review]
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.
(Assignee)

Comment 29

16 years ago
Attached revised patch

Comment 30

16 years ago
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
Attachment #100914 - Flags: superreview+
(Assignee)

Comment 31

16 years ago
Created attachment 100918 [details] [diff] [review]
patch removing the nits

Comment 32

16 years ago
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!
Attachment #100918 - Flags: superreview+
(Assignee)

Comment 33

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

r=dougt from aim
Attachment #100918 - Flags: review+
(Assignee)

Comment 34

16 years ago
fixed
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 35

16 years ago
Build: 2002-10-22-08-trunk

Installed v2.4d9.  I am able to open FTP sites.  Marking Verified!
Status: RESOLVED → VERIFIED
QA Contact: benc → jimmylee
You need to log in before you can comment on or make changes to this bug.