257427 - opening filepicker without first appending a filter crashes

Status: RESOLVED FIXED

Description

[Andrew Schultz]

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]

Comment 1

[jon rekai]

(In reply to comment #0)
> attachment 157380 [details]

What's preferable in such a case, failing SetFilter() or defaulting to filter
all files?

Comment 2

[timeless]

all files

Comment 3

[jon rekai]

Created attachment 164021 [details] [diff] [review]
sanity check in JS/nsFilePicker::show()

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

[Wayne Mery (:wsmwk)]

OS=ALL?

Comment 5

[Andrew Schultz]

I suppose.  In default builds, the xul filepicker is only used on linux, IIRC.

Comment 7

[timeless]

Created attachment 297571 [details] [diff] [review]
per bug 305879 comment 6

Comment 8

[jag (Peter Annema)]

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
}

Comment 9

[timeless]

Created attachment 298511 [details] [diff] [review]
per comment 8

this also tries to use the same allocator for deallocation :o, and handle oom :o

Comment 10

[jag (Peter Annema)]

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.

Comment 11

[Damon Sicore (:damons)]

Comment on attachment 298511 [details] [diff] [review]
per comment 8

a1.9+=damons

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

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.

Comment 13

[timeless]

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