Closed
Bug 123388
Opened 23 years ago
Closed 22 years ago
FTP should accept CR line endings (xitami server)
Categories
(Core Graveyard :: Networking: FTP, defect, P3)
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla1.1alpha
People
(Reporter: mkaply, Assigned: neeti)
References
()
Details
(Whiteboard: [broken server])
Attachments
(6 files)
1.39 KB,
text/plain
|
Details | |
2.23 KB,
patch
|
Details | Diff | Splinter Review | |
2.46 KB,
patch
|
Details | Diff | Splinter Review | |
2.47 KB,
patch
|
dougt
:
review+
|
Details | Diff | Splinter Review |
2.46 KB,
patch
|
darin.moz
:
superreview+
|
Details | Diff | Splinter Review |
2.46 KB,
patch
|
neeti
:
review+
darin.moz
:
superreview+
|
Details | Diff | Splinter Review |
Download a xitami web server (I used Windows) and install it.
Try to FTP.
You get a data connection error.
Reporter | ||
Comment 1•23 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.
Comment 2•23 years ago
|
||
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•23 years ago
|
||
Matti: i don't think you meant bug 11024. Did you mean patch in bug 110241?
Updated•23 years ago
|
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
Comment 5•23 years ago
|
||
sure, i mean bug 110241 (bbaetz knows which bug i mean :-)
Comment 6•23 years ago
|
||
Can I get a packet trace? I think they'Re sending cr instead of crlf. Yuck.
Comment 7•23 years ago
|
||
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•23 years ago
|
||
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
Comment 9•23 years ago
|
||
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
Comment 10•23 years ago
|
||
Ask me on IRC and i start my server...
Comment 11•23 years ago
|
||
broken server, no time => 1.1alpha
Target Milestone: mozilla1.0 → mozilla1.1alpha
Comment 12•22 years ago
|
||
*** Bug 152661 has been marked as a duplicate of this bug. ***
Comment 13•22 years ago
|
||
tapped summary to make cause more searchable.
Summary: FTP should accept CR line endings → FTP should accept CR line endings (xitami server)
Comment 14•22 years ago
|
||
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•22 years ago
|
||
Could anyone post a testcase to reproduce this bug?
Assignee: dougt → neeti
Comment 16•22 years ago
|
||
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•22 years ago
|
||
Comment 18•22 years ago
|
||
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•22 years ago
|
||
>IS this function portable?
yeah, it should be.
Assignee | ||
Comment 20•22 years ago
|
||
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•22 years ago
|
||
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•22 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•22 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•22 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•22 years ago
|
||
Comment 26•22 years ago
|
||
Comment on attachment 100901 [details] [diff] [review]
revised patch
r=dougt
Attachment #100901 -
Flags: review+
Comment 27•22 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•22 years ago
|
||
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•22 years ago
|
||
Attached revised patch
Comment 30•22 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•22 years ago
|
||
Comment 32•22 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•22 years ago
|
||
Comment on attachment 100918 [details] [diff] [review]
patch removing the nits
r=dougt from aim
Attachment #100918 -
Flags: review+
Assignee | ||
Comment 34•22 years ago
|
||
fixed
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 35•22 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
Updated•10 months ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•