Closed Bug 257427 Opened 20 years ago Closed 16 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?
Comment on attachment 298511 [details] [diff] [review]
per comment 8

a1.9+=damons
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: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.