Closed
Bug 257427
Opened 20 years ago
Closed 17 years ago
opening filepicker without first appending a filter crashes
Categories
(Core :: XUL, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: ajschult784, Assigned: timeless)
References
()
Details
(Keywords: crash)
Attachments
(2 files, 1 obsolete file)
1.21 KB,
patch
|
Details | Diff | Splinter Review | |
3.00 KB,
patch
|
jag+mozilla
:
superreview+
beltzner
:
approval1.9b3-
damons
:
approval1.9+
|
Details | Diff | Splinter Review |
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?
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...)
?
Comment 4•17 years ago
|
||
OS=ALL?
Reporter | ||
Comment 5•17 years ago
|
||
I suppose. In default builds, the xul filepicker is only used on linux, IIRC.
OS: Linux → All
QA Contact: jrgmorrison → xptoolkit.widgets
Comment 8•17 years ago
|
||
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
}
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 10•17 years ago
|
||
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?
Comment 11•17 years ago
|
||
Attachment #298511 -
Flags: approval1.9? → approval1.9+
Attachment #298511 -
Flags: approval1.9b3?
Comment 12•17 years ago
|
||
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-
Assignee | ||
Comment 13•17 years ago
|
||
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.
Description
•