Closed Bug 116170 Opened 24 years ago Closed 24 years ago

crash on ftp://ftp.dict.org/ [FIX]

Categories

(Core Graveyard :: Networking: FTP, defect)

Product:
Core Graveyard
Component:
Networking: FTP
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.8

People

(Reporter: jmd, Assigned: pete)

References

(
URL
)

Details

(Keywords: crash, testcase)

Attachments

(1 file, 12 obsolete files)

damn, i new i should have used Adopt ;-)
5.68 KB, patch
darin.moz
: superreview+
Details | Diff | Splinter Review
This site doesn't seem to require pass for USER anonymous. Dunno if that's a protocol violation, be we sure the hell shouldn't crash over it. LIST output is also nonstandard. NS4.7 handles it fine. 097 branch. Login trace: ---- Connecting to ftp.dict.org (209.58.150.153) port 21 <--- 220-You are user number (1,1) of (5,2) allowed. <--- 220-Setting memory limit to 1024+1024kbytes <--- 220-Local time is now 00:59 and the load is 0.06. <--- 220 You will be disconnected after 1800 seconds of inactivity. ---> USER anonymous <--- 230- <--- Welcome to the miranda.org anonymous FTP server. <--- 230- <--- 230- Local Sites <--- 230- ----------- <--- 230- /pub/dict/ The DICT Development Group <--- 230- /pub/bogus/ The BOGUS Development Group (historic) <--- 230- <--- 230- Mirrors <--- 230- ------- <--- 230- /pub/Light/ The Light IRC Script <--- 230- /pub/qt/ Troll Tech's QT GUI Toolkit <--- 230- /pub/rfc/ RFCs <--- 230- <--- 230- <--- 230 Anonymous user logged in. ---> PWD <--- 257 "/"
Keywords: crash
confirming with win2k build 20011220.. -> OS:ALL PR_FormatTimeUSEnglish(char * 0x0012fa44, unsigned int 234, const char * 0x019dd8b0, const PRExplodedTime * 0x040b72a8) line 1798 + 6 bytes nsFTPDirListingConv::DigestBufferLines(char * 0x03d3f690, nsCString & {...}) line 1029 + 30 bytes nsFTPDirListingConv::OnDataAvailable(nsFTPDirListingConv * const 0x03f86530, nsIRequest * 0x03f81db8, nsISupports * 0x00000000, nsIInputStream * 0x04106428, unsigned int 0, unsigned int 316) line 298 + 16 bytes nsStreamListenerTee::OnDataAvailable(nsStreamListenerTee * const 0x04168098, nsIRequest * 0x03f81db8, nsISupports * 0x00000000, nsIInputStream * 0x03f825d0, unsigned int 0, unsigned int 316) line 56 + 51 bytes DataRequestForwarder::OnDataAvailable(DataRequestForwarder * const 0x03f81dbc, nsIRequest * 0x03f82800, nsISupports * 0x00000000, nsIInputStream * 0x03f825d0, unsigned int 0, unsigned int 316) line 313 + 52 bytes nsOnDataAvailableEvent::HandleEvent() line 193 + 70 bytes nsARequestObserverEvent::HandlePLEvent(PLEvent * 0x03f8603c) line 116 PL_HandleEvent(PLEvent * 0x03f8603c) line 590 + 10 bytes PL_ProcessPendingEvents(PLEventQueue * 0x00e2e500) line 520 + 9 bytes _md_EventReceiverProc(HWND__ * 0x0002014e, unsigned int 49378, unsigned int 0, long 14869760) line 1071 + 9 bytes USER32! 77e02e98() USER32! 77e030e0() USER32! 77e05824() nsAppShellService::Run(nsAppShellService * const 0x00e20da8) line 303 main1(int 2, char * * 0x003c6ae8, nsISupports * 0x00000000) line 1264 + 32 bytes main(int 2, char * * 0x003c6ae8) line 1594 + 37 bytes mainCRTStartup() line 338 + 17 bytes KERNEL32! 77e87d08()
OS: Linux → All
Doug, can you have a quick look and maybe help? bbaetz is off for a while. Thanks, /be
Assignee: bbaetz → dougt
Keywords: mozilla0.9.8
We handle not requiring a password, not as optimal as I would like, but it works. And no, it is not a protocol violation. As for the list output... your right it is not standard.. there isnt a 'standard' list output. RFC959 does not state that it has to be, in fact it says that it is going to be hard to parse. This is clearly the case. The problme is that the server claims that it is a UNIX service, yet its list output is not the common "ls -l" output. I have a simple patch with prevents the crash. I to think some more about how to handle this case. considering I have a cold and am going on vacation in a few hours, this a "real" fix for this servers is not going to happen until next year. This next fix should have a full regression test performed. tever or benc can help on this.
I problem still exists if: a. a server returns that it is a UNIX service from SYST b. its data response to the LIST started with -, d, l c. the remainder of the output is not valid ls -l At somepoint, I really want to rewrite all of this LIST parsing code!!!
NS4.7 parses it fine... can't you steal its code, or is it worse? :) Just showing the directory as plain text would be an acceptable 1.0 fix here. Least you could navigate it by manually changing the URL based on what you see. Hrm... just figured something out. ---> PASV <--- 227 Passive mode OK (209,58,150,153,4,208) ---- Connecting data socket to (209.58.150.153) port 1232 ---> LIST -l <--- 150 Accepted data connection from 206.135.164.221:1363 total 1 drwxr-xr-x 2 520 520 4096 Jul 22 04:2 Light drwxr-xr-x 3 501 501 4096 Feb 2 200 alephnull drwxr-xr-x 12 501 501 4096 Nov 2 200 bogus drwxrwxr-x 8 501 404 4096 Feb 20 200 dict drwxr-xr-x 20 500 500 4096 Nov 8 03:1 qt drwxr-xr-x 17 500 500 135168 Dec 21 03:0 rfc drwxr-xr-x 2 552 552 4096 Oct 31 14:5 teamnob <--- 226-Options: -l <--- 226 7 matches total LIST -l! :) But I don't suppose we can send this to all servers id'ing as 'unix', can we? That would be too easy. Boy I hate FTP. > I problem still exists if: A "we can't hyperlink the directory names all pretty" problem, or a "crash, possibly exploitable" problem. One of the above I'm not too concerned about.
I applied dougt's patch and it still core dumps. It lasts a little longer though if thats any consolidation before going down . . . --pete
Isn't this a potential security hole? Remote data, Mozilla processes it, then crashes... sounds like every buffer overflow exploit I've seen.
dir parsing sucks. Seriously. There are bugs on rewriting it complelty, but the spec doesn't specify a standard format, so this is hard
Running command line ftp it works fine. ftp> ls -l 150 Accepted data connection from 24.47.114.241:1535 total 1 -r--r--r-- 1 501 404 2557 Feb 22 199 ANNOUNCE -r--r--r-- 1 501 404 1034 Mar 16 199 INITSCRIPT -r--r--r-- 1 501 404 5271 Mar 16 199 INSTALL -r--r--r-- 1 501 404 844 Mar 1 199 README -r--r--r-- 1 501 404 960 Mar 16 199 README.NEW drwxrwxr-x 2 501 404 4096 Feb 20 200 brewer1898 drwxrwxr-x 2 501 404 4096 Apr 23 200 contrib -r--r--r-- 1 501 404 2857457 Jul 6 199 dict-gazetteer-1.3.tar.gz -rw-r--r-- 1 501 404 646270 Apr 6 200 dict-jargon-4.2.0.tar.gz -r--r--r-- 1 501 404 4204499 Jul 6 199 dict-misc-1.5.tar.gz -r--r--r-- 1 501 404 293649 Feb 22 199 dict-web1913-1.4.tar.gz -r--r--r-- 1 501 404 6363650 Feb 16 199 dict-wn-1.5.tar.gz -rw-r--r-- 1 501 404 531110 Jan 12 200 dictd-1.5.5.tar.gz -rw-r--r-- 1 501 404 47469 Jan 25 200 dictfmt-src-20000129.tar.gz -r--r--r-- 1 501 404 6962 Jul 12 199 javadict-0.99q.tar.gz drwxrwxr-x 2 501 404 4096 Jan 29 200 old drwxrwxr-x 4 501 404 4096 Apr 3 200 pre drwxrwxr-x 4 501 404 4096 Jan 29 200 raw -r--r--r-- 1 501 404 13795530 Feb 22 199 web1913-0.46-a.tar.gz drwxrwxr-x 2 501 404 4096 Sep 3 22:4 www 226-Options: -l 226 20 matches total ftp> --pete
I understand it "sucks" and is "hard". My question is - is this exploitable? Mozilla's security scares the hell out of me, frankly. We have no audit process I'm aware of, so a problem is innevitable. The only reason he haven't been hit yet is we're such a moving target. I fear for users of 1.0. I don't care if we display the site. We just need to NOT CRASH.
Known crashes are *never* acceptable, totally agree. Who is saying that this crasher is ok?? --pete
> Who is saying that this crasher is ok?? Well, the patch was designed to try and get this certain site to not trigger a crash, not to close the many other ways FTP list data could crash Moz. Which could potentially be a remote hole.
Not all crashes are caused by exploitable buffer overruns. How about some specific diagnosis, instead of generalized fear? /be
I said potentially. "Specific diagnosis" is exactly what I'm asking for. My point was, and is, no one really is on top of all the crashers doing such checks (and proactively checking other areas for security issues). And I can't tell you how many times I've seen FreeBSD, or wuftpd, or ProFTPD, (and others) make a release with known crashes they didn't think were exploitable, and, turns out a month later, after the new version is heavily deployed, oops, it is. Is any netscape employee in charge of such things, or is it (as usual, for commercial software companies) not a priority? It's a huge task so I don't see a volunteer steping up full time. See also: Netscape 4.71, 4.72, 4.73, 4.74, 4.75, 4.76, 4.77, 4.78... I think only one or two didn't contain a severe security fix. Has someone AT LEAST audited us for all the same problems 4.7x had? It'd be pretty damned embarassing to have the rewrite get hit by the same problem. GIF comment overflows? Brown orifice? All the malacious javascript/cache interactions? "Cache Cow" and "Son of Cache Cow"? The history exposing in 4.07, the MIME overflow in 3.x, 4.0x and 4.5. nocache being ignored on https sites. The Acros-Suencksen SSL Vulnerability in 4.73. The JavaScript Cookie Exploit in 4.72. The javascript mail that intercepts forwards in Netscape 6 and 6.01. Shall I go on? Lets face it, the NSCP programmers have a horrible track record. But it's not exactly their fault. I doubt Netscape had a proactive security team (or even one guy) checking the software for such problems before shipping it to millions. As soon as Mozilla hits 1.0 and isn't a moving target, it's going to be bad. The only reason it won't be as bad is we don't have the users 4.x used to. (which explains all the IE problems in the last 2 years)
Ok, you're orating now -- do that in a newsgroup, preferably m.security. mstoltz@netscape.com, ben.bucksch@beonex.com, and others are in fact working on security and auditing for security bugs. Netscape pays at least one person full time to develop attacks and exploits. Inform yourself as to the particulars (again, particulars) in the Mozilla security, and please stop spamming this bug. /be
Attached patch Don't bitch, *fix* . . . ;-) (obsolete) — — Splinter Review
I'll take some review on this pach. Thanks --pete
Assignee: dougt → petejc
Target Milestone: --- → mozilla0.9.8
Status: NEW → ASSIGNED
Ok, try to get it to crash now . . . --pete
Attached patch cleaned up, took out debug code (obsolete) — — Splinter Review
Keywords: review
Summary: crash on ftp://ftp.dict.org/ → crash on ftp://ftp.dict.org/ [FIX]
Fixing crashes is clearly the most important issue here but the parsing issue should be moved up in importance. One idea is in the attachment to (the rather poorly named) bug 95590. FWIW, it is instructive to view this URL through a squid proxy, noting what it does and what options it offers to the user to handle a rather specific but complex situation.
tenthumbs, that's what this quick crasher patch does. It checks the directory bits for some sanity. In addition to the date numbers we are sending to the nsPRTime struct. There was no testing there at all for bad or null pointers. Very bad and unsafe. I'd like to get this crasher fix checked in. Then when someone has time, the real issue of cleaning up this parsing code can be addressed. But I agree, while I only breifly looked at the code here, I do see to many assumptions being made and not near enough error checking for sanity. Now if I could only get some review love. Time to start spamming again. --pete
Comment on attachment 62629 [details] [diff] [review] cleaned up, took out debug code This looks fine. My tree is non-existant at the moment, but assuming that it fixes it, r=bbaetz. Can someone please attach the LIST results which are sent, just so I can see what it looks like?
Attachment #62629 - Flags: review+
Here's a LIST. Note LIST -l I already included above (comment #5). ---> LIST <--- 150 Accepted data connection from 216.133.165.95:1091 total 1 Light alephnull bogus dict qt rfc teamnob ---- Closing data socket <--- 226 7 matches total
Yea, everything in mozilla is displayed w/ a file icon. But other than that, everything works fine. You can cd into dirs etc w/out any crashes, actually save the tarballs displayed, etc. It actually works better than the 4.x implementation, because 4.x takes any file begining with a 'd' and treats it as a dir. So in 4.x, you have this: dict-gazetteer-1.3.tar.gz/ dict-jargon-4.2.0.tar.gz/ dict-misc-1.5.tar.gz/ dict-web1913-1.4.tar.gz/ dict-wn-1.5.tar.gz/ dictd-1.5.5.tar.gz/ dictfmt-src-20000129.tar.gz/ Then when you click on the tarball, you get an alert saying that it is unable to find the dir . This patche fixes *that* problem as well. sr= anyone? --pete
Attachment #62529 - Attachment is obsolete: true
Attachment #62606 - Attachment is obsolete: true
Pete: note that a file type + permission field is also a valid file name so you have to ensure you're looking at the right thing. It must be the first non-whitespace token, it must be exactly 10 characters in length, and it must be followed by at least one other non-whitespace token. It must also Bmatch a regular expression like: [-dlbcps][r-][w-][sSx-][r-][w-][sSx-][r-][w-][tTx-] possibly case-insensitive. Your code would miss "drwsr-xr-x".
If I read the patch right, then "drws-dec01" would be treated as a directory. I also wonder if case-insensitive testing is really the right thing to do. Perhaps, it would be best to split off the parsing stuff into another bug and just fix the crashing here.
Yes, "drws-dec01" would read as a dir *ONLY* on sites sending back horked listings. This insures a reasonable amount of safety and sanity to check for valid directories under this particular circumstance. The way it is now and in 4.X, *ANY* word beginning w/ 'd' is rendered as a dir. This patch will prevent crashes while yeilding the most desirable behavior when encountering horked listings. I haven't moved beyond the scope of this bug. If you want to open up another bug against listing parsing, go right ahead. --pete
If by "horked" you mean that a server returns a NLST type response to a LIST command then I believe you have to prove that this is really a protocol violation.
Attachment #62629 - Attachment is obsolete: true
Attachment #62686 - Attachment is obsolete: true
Dude, I don't care if it is a protocol violation or not. The facts are: It *was* BROKEN It *is* now FIXED s/BROKEN/FIXED/g I just want to get this patch reviewed and move on . . . --pete
Comment on attachment 62747 [details] [diff] [review] created a more wholesome, less "quick fix" patch >+ // insure that we have enough data to parse here >+ NS_ASSERTION(PL_strlen(aLine) > 46, "ls -l is incorrectly formatted"); >+ if (PL_strlen(aLine) < 46) { Shouldn't the assertion be testing >= 46? Anyway, the assertion condition should be the complement of the if condition. >+ aEntry->mName = nsEscape(aLine, url_Path); >+ // initialize the time struct to 0 >+ aEntry->mMDTM.tm_month = 0; >+ aEntry->mMDTM.tm_mday = 00; >+ aEntry->mMDTM.tm_year = 0000; Fun octal 0 constant spellings! Why? > // check first character of ls -l output > // For example: "dr-x--x--x" is what we're starting with. >- if (line[0] == 'D' || line[0] == 'd') { >+ // Sanity check for dir permission bits >+ if (line[0] == 'D' || line[0] == 'd' && PL_strlen(line) >= 10) { >+ /** >+ * test for these valid permission bits >+ * r, w, x, - not going to worry about s t, X, u, g, o >+ */ >+ PRInt16 i; How about just int here? Don't use sized int typedefs for locals. PRIntn if you must for name consonance (it's int). >+ PRBool isDir = PR_FALSE; >+ // check only the first set of bits >+ for (i = 1; i < 4; ++i) { >+ switch(line[i]) { >+ // most common bits >+ case 'r': case 'w': case 'x': >+ case '-': case 's': >+ isDir = PR_TRUE; >+ break; >+ default: >+ isDir = PR_FALSE; >+ break; Nit: don't these case statements seem overindented? The case 'r': etc. labels are not themselves statements, so I don't think they deserve a full statement's worth of indentation. I use half-tabs for such labels, indenting them by 2 and their labeled statements by 4 in this case. Darin, dougt: you guys have dibs on r=, but I think if this patch stops the crash, then (with any easy cleanups reviewers request) it ought to go in soon. /be
I'm sorry but I don't think a patch that treats "dxxx-12345" as a permission string is a good idea. Who knows what bugs it might tickle. FWIW, here's an idea off the top of my head. /* Is this string a file type + permission string? Assume leading whitespace has been trimmed. There are a numebr of macros to make the logic, such as it is, easier to see. */ #define IS_LWS(c) (strchr(" \t\r\n", c) != 0) #define IS_FTYPE(c) (strchr("-dlbcsp",c) != 0) #define IS_RPERM(c) (strchr("r-",c) != 0) #define IS_WPERM(c) (strchr("w-",c) != 0) #define IS_SPERM(c) (strchr("sSx-",c) != 0) #define IS_TPERM(c) (strchr("tTx-",c) != 0) Boolean IsPermString(const char *foo) { const char *cp; if (strlen(foo)< 10) return FALSE; if (!IS_FTYPE(foo[0])) return FALSE; /* shorter tests first */ if (!IS_RPERM(foo[1]) return FALSE; if (!IS_WPERM(foo[2]) return FALSE; if (!IS_RPERM(foo[4]) return FALSE; if (!IS_WPERM(foo[5]) return FALSE; if (!IS_RPERM(foo[7]) return FALSE; if (!IS_WPERM(foo[8]) return FALSE; if (!IS_SPERM(foo[3]) return FALSE; if (!IS_SPERM(foo[6]) return FALSE; /* this next part is necessary only if the first token is part of a longer string */ #if 1 for (cp = &foo[10]; *cp; +=cp) if (!IS_LWS(*cp)) return TRUE; return FALSE; #else return TRUE; #endif } Then one can do a simple test like: const char *foo = strip_leading_whitespace(aLine); if (!IsPermString(foo)) { // handle the NLST style result goto NSLT_handler; } /* now we can test for directories, symlinks, etc. safely */ ... Feel free to use it, ignore it, whatever.
Attached patch clean-up as to Brendans comments (obsolete) — — Splinter Review
tenthumbs, open another bug on the parser issues in this code. There is a LOT of things that need to be fixed, not just the permission bit parsing. This patch cleanly fixes the root of any NLIST or *non* LIST output so we don't crash. Please review (id=62946) Thanks --pete
Attachment #62747 - Attachment is obsolete: true
Pete's right, it's better not to crash, and a separate bug for a separate problem is usually best, so long as the problem reported in this bug's summary is fixed. I don't know ftp, and don't have the RFCs handy -- is it really legit to get an NLST response to a LIST request? Of course, we should handle anything, at least not crash. /be
Pete has one solution to the crashes. I have another. The directory issue just crops up because the underlying problem is mozilla's preconception about the form of the LIST response. It's not really a separate problem. As for LIST, rfc959 says: LIST (LIST) This command causes a list to be sent from the server to the passive DTP. If the pathname specifies a directory or other group of files, the server should transfer a list of files in the specified directory. If the pathname specifies a file then the server should send current information on the file. A null argument implies the user's current working or default directory. The data transfer is over the data connection in type ASCII or type EBCDIC. (The user must ensure that the TYPE is appropriately ASCII or EBCDIC). Since the information on a file may vary widely from system to system, this information may be hard to use automatically in a program, but may be quite useful to a human user. and nothing else.
Network Working Group A. Bhushan Request for Comments: 114 MIT Project MAC NIC: 5823 16 April 1971 A FILE TRANSFER PROTOCOL Wow... no wonder FTP sucks so bad. 1971? On of the objectives stated in the current version (1985!) is "to shield a user from variations in file storage systems among hosts". That sure worked out well. Anyway, the question at hand: > is it really legit to get an NLST response to a LIST request? It appears so. It mentions LIST is probably only useful to human eyes, and NLST could be used for automatic processing. How about this. (Assuming we really want to bother with the extended info (owner, permissions, mod date, size). If the system is of type unix, we try to match to a (very well written) regexp that matches normal: drwxr-xr-x 2 root wheel 512 Nov 30 05:44 pub form lines. Now that I look, FWIW, ftp.mozilla.org DOESN'T MATCH that line: drwxrwxr-x 16 22 8192 Apr 30 2001 pub No group info. So match maybe these two, or if there's another major format used. If LIST doesn't match these very specific characteristics, "blacklist" the site, ignore whatever was returned, and use NLST from then forward. Unfortunatly I just realized the problem with NLST. It doesn't indicate whether a file is a directory or not. I vote Mozilla drop support for FTP listings. Blah. What an awful protocol.
tenthumbs, the cause for the crash here is the code tries to access an uninitialized PRtime struct. The code was assuming that there would always be data available to parse and populate the time struct. In the case of an NLST, there is not enough data so we have undefined values populating the struct causing the crash. I added the sanity insurance for directories because again, in the case of an NLST, ALL names starting w/ a 'd' were being listed as a directory as in the case of 4.x as well. I put in the perm bit sanity check to help out with this problem. As to my last patch, the only way you can spoof the code to think it is a dir is with a file name like this: drwxrwxraaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa This will appear as a dir. So what . . . It's not going to cause a crash. The worst thing is you will have what has been implemented in 4.x for ages. The file will be listed as: drwxrwxraaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/ When a user clicks on it, they will get an alert that says 'Can't cd to ...' Is this really unacceptable here? The bottom line is the fpt code needs a real douching which is beyond the scope of this particular bug. I'd recommend someone file a bug to rewrite the mozilla ftp code when there is time. I understand your point here. I just think so much fucking time is wasted debating and not enough time coding. If you feel so strongly about it, file a new bug and rewrite the ftp code. As a volunteer, I am only inspired enough to fix this crahser. ;-) --pete
tenthumbs: petejc has a compiling patch, you have a good-looking code sketch. The patch wins as a "solution" in the here-and-now -- the sketch is not really a solution we can check in -- unless someone is motivated to improve the patch per the sketch. I'm hoping you guys will come to an agreement on what should go in, when. Otherwise I'll have to jump in and be heavy-handed, which I hate doing. petejc: you willing to whip up a patch based on the sketch in comment #33? /be
> petejc: you willing to whip up a patch based on the sketch in comment #33? Yes, but not in this bug. tenthumbs sketch is gearing towards a reimplementation of this code. Which I agree with. Again, i would like to check in the crasher fix now, then when either I or tenthumbs have the time, work on a more in-depth solution. To me, crahsers fixes are important to get in asap. --pete
Pete: we're in basic agreement about the fundamental issue, namely, mozilla receives a line of text in an unexpected format and botches the processing. I am suggesting that a better and safer approach is to attack the problem aggressively by detecting such lines and not processing them. Ideally, mozilla should parse the line into tokens, etc, but that's a major rewrite. Luckily, the first token of a proper ls-l style listing line is a permission string which has a specific format. I am using using the rather rigorous permission format test as a filter. If the first token fails, then mozilla should abort further processing. In the future, mozilla could do something clever but, right now, anything up to and including dropping the line on the floor is acceptable. As it happens, this eliminates some other consistency tests but that's just a bonus. I also don't like irritating users. If a user knows that a link should be a file but mozilla pops up a warning about a directory, then the user can rightly think mozilla is broken. There is also the question of how servers will react. We know that some servers become seriously wedged if you talk to them in ways they don't expect, e.g. trying to retrieve a directory. We have no data on how servers will react if you try to cd to a regular file. I would think it is better not to provoke the sleeping daemons. Brendan: "Win a few, lose most of them." That's my motto. If you feel you must overrule me, that's fine with me. It happens all the time. However, I will reserve the right to gloat when I am eventually proved correct. :-)
Comment on attachment 62946 [details] [diff] [review] clean-up as to Brendans comments Something's off here, where indentation of parent statement level to child seems to go by 6 or 5 spaces: + for (i = 1; i < 4; ++i) { + switch(line[i]) { + // most common bits + case 'r': case 'w': case 'x': + case '-': case 's': + isDir = PR_TRUE; + break; + default: + isDir = PR_FALSE; + break; + } + } The for and the switch are good, and the case and default labels are "half-indented", but the case statements go in by 6 from the switch, not 4, and then the closing switch brace exdents only by 5. Nit o' the day, fix it and get r=. I'm giving sr= to goose this crashfix along. Please file that cleanup sequel bug and record its number here. Thanks, /be
Attachment #62946 - Flags: superreview+
Attached patch white space love (obsolete) — — Splinter Review
tenthumbs, how about after I get this crahser fix checked in, we rename this bug to "bad ftp permission parsing" or something and then get in the code for dealing properly with the permission bits. In sometime soon. I just want to see crahser fixes get in ASAP. --pete
Pete: Why is 46 magic? What about real tab characters in the line? Shouldn't the time struct be initialized by nsFTPDirListingConv::InitPRExplodedTime? Shouldn't it also happen in some constructor somewhere? Since I don't think you're actually addressing the crash, leaving this bug open is fine with me. :-)
Attached patch patch (obsolete) — — Splinter Review
For the record, this is all I propose doing for this bug.
tenthumbs, I added your ls_lCandidate() method to my patch. So I should not get any more lip from you, and maybe we can get this sucker reviewed and checked in. ;-) I applied your patch and it wouldn't compile because of sysnax errors. I fixed those errors and then tested. You code pretty much horked ftp altogether. No ftp sites would display for me. It is a good idea to test you patches before posting them. Anyway, everyone should be happy here . . . On a side note, links are just not dealt with properly. But that is another bug. PLEASE, PLEASE can we get some review now. --pete
You eliminated the explanation of what I did and why. Now no one in the future will understand it. Why don't you just ignore my stuff if you dislike it so much.
That last comment was nasty. Sorry. Let me re-phrase it. Documentation is as much a part of programming as coding. Without the proper documentation, code becomes meaningless. I object to the removal of my documentation.
Comment on attachment 63061 [details] [diff] [review] patch Actually, this probably won't work. Withdrawn.
Attachment #63061 - Attachment is obsolete: true
Attached patch w/ tenthumbs comment (obsolete) — — Splinter Review
I think dougt and darin should r/sr= here. /be
Comment on attachment 63092 [details] [diff] [review] w/ tenthumbs comment + // insure that we have enough data to parse here Please add something to this comment for the reasons behind using 46. nit: if (line[0] == 'D' || line[0] == 'd' && ls_lCandidate(line)) I would have used an extra () to make this more readable. Thanks for fixing this. Looks good. r=dougt.
Attachment #63092 - Flags: review+
Pete, after you land this, please contact benc@netscape.com. He can run a regression test. Benc, Please make sure you add this ftp site to your testing.
its on the todo list...
Attached patch nit fix as per dougt's comments (obsolete) — — Splinter Review
Need an sr from Darin or Brendan here. Man this is a bad crasher. I jsut tried to load the page from a branch build and it took down the house. Benc, this shouldn't cause any regressions. I did test this fix pretty thoroughly. If you find other ftp sites that cause a crash or don't work properly let me know. --pete
Comment on attachment 63837 [details] [diff] [review] nit fix as per dougt's comments >Index: nsFTPDirListingConv.cpp >+#define IS_LWS(c) (PL_strchr(" \t\r\n", c) != 0) >+#define IS_FTYPE(c) (PL_strchr("-dlbcsp",c) != 0) extra space between |,| and |c| in first #define >+ // insure that we have enough data to parse here >+ // using 27 for a minimal LIST line >+ if (PL_strlen(aLine) <= 27) { >+ NS_ASSERTION(PL_strlen(aLine) >= 27, "ls -l is incorrectly formatted"); i'm not sure i understand this assertion... you're only asserting that aLine is 27 chars long... it will never be greater than 27 chars long when this NS_ASSERTION is reached. perhaps == would be better? >+ aEntry->mName = nsEscape(aLine, url_Path); since mName is a nsCString, this assignment will leak the return value of nsEscape. > // the application/http-index-format specs > // viewers of such a format can then reformat this into the > // current locale (or anything else they choose) >- PR_FormatTimeUSEnglish(buffer, sizeof(buffer), >+ >+ // make sure we don't have a null time struct >+ if ((thisEntry->mMDTM.tm_month + thisEntry->mMDTM.tm_mday + >+ thisEntry->mMDTM.tm_year + thisEntry->mMDTM.tm_hour + >+ thisEntry->mMDTM.tm_min) != nsnull) { >+ PR_FormatTimeUSEnglish(buffer, sizeof(buffer), > "%a, %d %b %Y %H:%M:%S", &thisEntry->mMDTM ); >+ } otherwise, what's in the buffer? is it okay to use it as is?
Attachment #63837 - Flags: needs-work+
Attached patch As per Darins review (obsolete) — — Splinter Review
> extra space between |,| and |c| in first #define got it. > i'm not sure i understand this assertion... This assertion is actually annoying me. Changed to NS_WARNING. > since mName is a nsCString, this assignment will leak the return value of nsEscape. Using .Assign(). What about the 7 or so other '=' assignments in this file. They will leak also no? > otherwise, what's in the buffer? is it okay to use it as is? NO. The buffer is zero'd out. We need this test to prevent the crash. If you lift it, we crash. --pete
pete: Assign will leak too. Don't you want Adopt? /be
Attachment #62946 - Attachment is obsolete: true
Attachment #63026 - Attachment is obsolete: true
Attachment #63070 - Attachment is obsolete: true
Attachment #63092 - Attachment is obsolete: true
Attachment #63837 - Attachment is obsolete: true
Attachment #63856 - Attachment is obsolete: true
Comment on attachment 63862 [details] [diff] [review] damn, i new i should have used Adopt ;-) sr=darin would be good to file another bug to cleanup string usage in this file... could use Adopt in a number of other places... certainly many that call nsEscape.
Attachment #63862 - Flags: superreview+
Yup. Filed: http://bugzilla.mozilla.org/show_bug.cgi?id=118639 Will check in the the tree reopens. Thanks --pete
Original reporter here... Using Linux 2002010808, no longer seeing a crash. Do see some behavior that could behaps be made a little nicer. When I click on "Light": PASV.. 227 Passive mode OK (...).. SIZE /pub/Light.. 550 Not a regular file.. RETR /pub/Light.. 450 Not a regular file.. CWD /pub/Light.. 250 Changed to /pub/Light.. LIST.. 150 Accepted data connection from 209.101.118.160:1520.. 226-Binary mode requested, but A (ASCII) used... 226 11 matches total.. My knowledge of FTP might be a little rusty, but I'd imagine the 550 return from SIZE is a good indication not to attempt a RETR. Though, the server probably shouldn't be returning a permanent error (550) to SIZE and a transient one (450) to RETR. Aside from that, the only other thing I took note of is the last response, 226-, it sent the LIST in ASCII though we were in binary mode. I would guess this is now Mozilla does all LISTs, but just in case it needs to be handled differantly.
Marking FIXED. At least this specific bug. ;-) --pete
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
VERIFIED: rc1 w98 + reporter.
Status: RESOLVED → VERIFIED
Keywords: testcase
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: