195956 - filenames w/ 8 or more spaces truncated in ftp directory listing & not found for selection

Status: VERIFIED FIXED

(Core Graveyard :: Networking: FTP, defect)

Description

[Kenshi]

User-Agent:       Mozilla/5.0 (X11; U; FreeBSD i386; en-US; rv:1.2.1) Gecko/20030220
Build Identifier: Mozilla/5.0 (X11; U; FreeBSD i386; en-US; rv:1.2.1) Gecko/20030220

By FTP'ing to my own server, I realized that Mozilla doesn't seem to print
anything after the 8th space in a filename. After clicking on these files,
Mozilla says, "File not found" or something to that effect. Files with less than
8 spaces or underscores seem to be alright. I had someone else confirm that the
same thing happens on the Linux 1.2.1 version. The console ftp client works just
fine with the same files.

Reproducible: Always

Steps to Reproduce:
1. Create an FTP server with at least one file with at least 8 spaces and some
text after the 8th space.
2. Use Mozilla to FTP to it.

Actual Results:  
File: Cowboy Bebop - 01 - Asteroid Blues sub.avi  79862 KB 02/16/03 05:16:00
File: Cowboy Bebop - 02 - Stray Dog Strut         80944 KB 02/16/03 05:17:00
File: Cowboy Bebop - 03 - Honkey Tonk Women       74130 KB 02/16/03 05:18:00
File: Cowboy Bebop - 04 - Gateway Shuffle sub.avi 94044 KB 02/16/03 05:20:00
File: Cowboy Bebop - 05 - Ballad of Fallen        65634 KB 02/16/03 05:21:00

Expected Results:  
Printed the full filename for all files.

Comment 1

[Olivier Blin]

Same bug with Mozilla 1.3b on Linux 2.4.21pre5.
When mozilla displays the name of ftp files with more than seven spaces, their
name is truncated from the eighth space.
For example when a file is named "this is a file with more than seven spaces" in
a ftp directory, it writes in the html code :
<a href="this%20is%20a%20file%20with%20more%20than%20seven"><img
src="internal-gopher-unknown" alt="File: "/>this is a file with more than seven</a>
You can see that the words after the eighth space are not displayed.
This problem doesn't occur when mozilla displays the same file from my hard disk
using the file "protocol".

Comment 2

[benc]

+clean-report, allplats
Confirmed: post 1.0 (does not break in Camino 0.7)
Mozilla 1.3b, Mac OS X.

I created a file on my ftp server:

touch "spaces 1 2 3 4 5 6 7 8 9"

The problem is stranger than that:

touch "space        word"

however, the displayed text is:

"space word"
but the source shows the correct number of spaces for the HREF and the text:
 
<td><a href="space%20%20%20%20%20%20%20%20word"><img
src="internal-gopher-unknown" alt="File: "/>space        word</a></td>

 <td></td>

Comment 3

[benc]

-> major. 

There are several dupes lying around. This would be less of a problem, except it
seems everyone who reports this has really long filnames for their music
collections.

So, I found that truncation occurs after the 8th space, seemingly regardless of
length.

Looking at the first example I provide in #2: the filename displays up to the
"7", and the link is:

ftp://regress.mcom.com/testing/spaces%201%202%203%204%205%206%207

The second example "space       word", it turns out is actually a bug in the
html we produce, so I moved it to bug 198490.

This happens in Phoenix 0.4 too, so I don't know how far back this goes. I'm
going to have to quit all my browsers and back trace more later.

Comment 4

[benc]

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

Comment 5

[Felix Miata]

You guys drive me nuts duping older bugs to newer bugs. Bug 193829, filed two
weeks before this, was marked "clean-report", "regression", & "nsbeta1", 2/3 of
which (as of now) is functionally lost by duping it to this, which is
"clean-report" only.

Summary on this bug makes no sense. Are we downloading files and halting before
completion because of 8 spaces somewhere?

Comment 6

[benc]

I don't dupe old to new unless there is a good reason. In this case, Olivier
figured it out, after the previous bug had stumped Asa, dougt, me and others.

You are right, this should be marked nsbeta and regression. I typically do
"nsbeta" by looking across all bugs at once, and lately I've been trying to only
mark regression I know roughly where it regressed.

WORKSFORME: Camino 0.7, Mac OS X.
So this broke sometime after 1.0.

Comment 7

[Felix Miata]

Clarifying summary to indicate: 1-filenames (not files) are truncated,
2-how/when the truncation occurs, 3-why this is a problem.

Comment 8

[benc]

Happened sometime after 1.1, before 1.2a. 

Isn't that when we added the new parser?

Comment 9

[Doug Turner (:dougt)]

When parsing a file specifed in comment #3, the code LIST parsing code trips up
on calculating the lenght of the file name:

        result->fe_fname = tokens[tokmarker+4];
        result->fe_fnlen = (&(tokens[numtoks-1][toklen[numtoks-1]]))
                           - (result->fe_fname);

Cyrus, do you have any suggestions or time to take a look at this?

Comment 10

[Michael Dunn]

Updating KWs

Comment 11

[Michael Dunn]

"spaces 1 2 3 4 5 6 7 8 9" is my diplay in V7.02

Comment 12

[Doug Turner (:dougt)]

Attachment: patch v.0

a simple but non-optimal solution.  The list parsing code has a hard limit of
the number of tokens per line it can expect.  Currently that limit is 16.  For
a line that contain a file name with multiple spaces, this limit is fatally to
small.	This patch increase the number of tokes to 40 which should fix most but
not all files.	any file that contains ~30 (ten for other stuff on that line)
spaces will fail.

a more optimal, but potenially more involved fix would be to dynamically
allocate these token blocks.  Before I go and do that, i wanted to get some
feedback.

Comment 13

[Michael Dunn]

Benc -- Will you be able to run some FTP rtegression testing when this fix
lands? It might be appropriate to even do some testing with a dev build given
the risk.

Comment 14

[Darin Fisher]

Comment on attachment 119864 [details] [diff] [review]
patch v.0

sr=darin

Comment 15

[Cyrus Patel]

sorry, was on the road. 

Doug, you hit the nail on the head in #9 and your fix in #12 will do the trick
(As you point out, 40 may not be enough either, so I suggest an increase to 64
or so). 

Alternatively, one could avoid the necessity of increasing the token array size
by replacing all instances of "&(tokens[numtoks-1][toklen[numtoks-1]])" with
"&line[linelen_sans_whitespace]" 

where 'linelen_sans_whitespace' is a variable that holds the linelength
excluding trailing whitespace (the existing variable 'linelen' is the 
length _including_ whitespace).

Comment 16

[Cyrus Patel]

Doug, 

A modified parseLIST.c (as per my suggestion in the previous comment) is at
http://fb14.uni-mainz.de/~cyp/moz/ftpbugs/parseLIST.zip

I've modified some of the files in the test-suite to simulate listings with
filenames/symlinks that have lots of spaces in them. The old parser is included 
as parseLIST.old.

I haven't increased the size of the array. 

Comment 17

[Doug Turner (:dougt)]

Attachment: patch v.2

Cyrus,
Thank you very much for your suggestions and update to ParseFTP.  I will
incorporate your changes into mozilla as they do correctly fix the reported
problem.

Comment 18

[Doug Turner (:dougt)]

Attachment: a patch now...

Comment 19

[Darin Fisher]

Comment on attachment 120484 [details] [diff] [review]
a patch now...

>Index: ParseFTPList.cpp

>+    unsigned int toklen[(sizeof(tokens)/sizeof(tokens[0]))];

nit: use NS_ARRAY_LENGTH(tokens) instead (#include "nsMemory.h")


>+    unsigned int linelen_sans_wsp;

acronyms don't usually make such good variable names unless the acronym
is very very common.  in this case, i'd suggest at least a comment:

  unsigned int linelen_sans_wsp; // line length sans whitespace


>+    linelen_sans_wsp = &(tokens[numtoks-1][toklen[numtoks-1]]) - tokens[0];
>+    if (numtoks == (sizeof(tokens)/sizeof(tokens[0])) )

use NS_ARRAY_LENGTH here as well.

sr=darin with those changes.

Comment 20

[Doug Turner (:dougt)]

darin and I spoke and he will turn his head at not using NS_ARRAY_LENGTH.  The
reason is that I don't want the deltas between mozilla's and the official
version of ParseList to drift too far.

Checking in ParseFTPList.cpp;
/cvsroot/mozilla/netwerk/streamconv/converters/ParseFTPList.cpp,v  <-- 
ParseFTPList.cpp
new revision: 1.4; previous revision: 1.3
done


Again thank you for the fix.

Comment 21

[benc]

VERIFIED Mac OS X 2003-04-17-03, commercial.

Temporary problems w/ other systems today, other plats coming soon.

If anyone else has a daily build, verify away if you have your own ftp server...

touch "spaces 1 2 3 4 5 6 7 8 9"

will create a testcase.

Comment 22

[benc]

VERIFIED: Mozilla 1.4b Win98+Linux