Closed Bug 257427 Opened 20 years ago Closed 17 years ago

opening filepicker without first appending a filter crashes

Categories

(Core :: XUL, defect)

x86
All
defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: ajschult784, Assigned: timeless)

References

()

Details

(Keywords: crash)

Attachments

(2 files, 1 obsolete file)

as described in bug 256927, the filepicker can crash if opened without first appending any filters (via appendFilter(s)). The stacktrace from that bug is attachment 157380 [details]
(In reply to comment #0) > attachment 157380 [details] What's preferable in such a case, failing SetFilter() or defaulting to filter all files?
all files
Append all files' filter if we have none... This fixes the cz scenario, but nsFileView::SetFilter() makes the assumption that it's getting a decent |aFilterString|, and we're still hosed if garbage is passed in. Revised testcase from bug 256927 comment 1, also crashes/hangs the browser: picker = futils.getPicker (null,";",{}); picker.init ( window, futils.MSG_OPEN, Components.interfaces.nsIFilePicker.modeOpen); picker.show(); http://lxr.mozilla.org/seamonkey/source/xpfe/components/filepicker/src/nsFileView.cpp#305 305 const PRUnichar* chr, *aPos = aFilterString; 306 for (chr = aFilterString; *chr; ++chr) { 307 if (*chr == ';') { 308 PRUnichar* aNewString = nsCRT::strndup(aPos, (chr - aPos)); 309 mCurrentFilters.AppendElement(aNewString); 310 311 // ; will be followed by a space, and then the next filter 312 chr += 2; 313 aPos = chr; 314 } 315 } This block could probably do some length checking (or further validate the filter string:| (However, the above example is intentional misuse...) ?
OS=ALL?
I suppose. In default builds, the xul filepicker is only used on linux, IIRC.
OS: Linux → All
QA Contact: jrgmorrison → xptoolkit.widgets
Attached patch per bug 305879 comment 6 (obsolete) — Splinter Review
Assignee: jag → timeless
Status: NEW → ASSIGNED
Attachment #297571 - Flags: review?(cbiesinger)
Comment on attachment 297571 [details] [diff] [review] per bug 305879 comment 6 I would consider doing more than the bare minimum here. |aPos| should be |pos|, and |aNewString| can be inlined in both cases. Also, if someone decides to pass in "filter1;filter2" things don't work right, and "filter1;filter2;" will crash you since you'll move the iterator past the end of the string. Oh, but I guess const nsAString& lets you pass in a non-null-terminated string, so you're already asking for trouble. Something like this should do the trick: nsAString::const_iterator iter = aFilterString.BeginReading(), end = aFilterString.EndReading(), start; while (PR_TRUE) { // skip over delimiters while (iter != end && (*iter == ';' || *iter == ' ')) ++iter; if (iter == end) break; start = iter; // start of a filter ++iter; // we know this is neither ';' nor ' ', skip to next char // find next delimiter or end of string while (iter != end && (*iter != ';' && *iter != ' ')) ++iter; mCurrentFilters.AppendElement(nsCRT::strndup(start, iter - start)); if (iter == end) break; ++iter; // we know this is either ';' or ' ', skip to next char }
Attached patch per comment 8Splinter Review
this also tries to use the same allocator for deallocation :o, and handle oom :o
Attachment #297571 - Attachment is obsolete: true
Attachment #298511 - Flags: review?(jag)
Attachment #297571 - Flags: review?(cbiesinger)
Comment on attachment 298511 [details] [diff] [review] per comment 8 timeless, you have r=jag on the changes you made (handling OOM, strndup -> ToNewUnicode) to the code I put in comment 8, and sr=jag on the whole thing, so if you feel ok giving r=timeless (you did the testing, and I take it you wouldn't have submitted this patch if the code didn't pass your review), let's try and get this baby checked in.
Attachment #298511 - Flags: review?(jag) → superreview+
Attachment #298511 - Flags: approval1.9?
Attachment #298511 - Flags: approval1.9? → approval1.9+
Attachment #298511 - Flags: approval1.9b3?
Comment on attachment 298511 [details] [diff] [review] per comment 8 Sorry this missed code freeze, not sure it needs to be in this beta, feel free to land when the tree unfreezes.
Attachment #298511 - Flags: approval1.9b3? → approval1.9b3-
Comment on attachment 298511 [details] [diff] [review] per comment 8 mozilla/xpfe/components/filepicker/public/nsIFileView.idl 1.4 mozilla/xpfe/components/filepicker/src/nsFileView.cpp 1.33
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: