333505 - nsLocalFileOSX crash

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Shane Caraveo]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1

nsDirectoryEnumerator can crash if you call HasMoreElements after a call to HasMoreElements has returned false.  This is because when there are no more entries, Close() is called, which sets mIterator and mFSRefsArray to null, but no checks against that are done in HasMoreElements before calling FSGetCatalogInfoBulk.

patch will be attached

Reproducible: Always




I discovered this while working on a pyxpcom registration problem I am having on the 1.8 branch.  a simple example of a repro in python is:

while dirEntries.HasMoreElements():
    entry = dirEntries.GetNext(components.interfaces.nsIFile)
    dirEntries.HasMoreElements()

The extra HasMoreElements() call at the end will result in Close() being called, thus setting mIterator to null, then the regular check in while statement crashes.

Comment 1

[Shane Caraveo]

Attachment: nullcheck.patch

Comment 2

[Mark Mentovai]

Comment on attachment 217950 [details] [diff] [review]
nullcheck.patch

>Index: nsLocalFileOSX.cpp
>         NS_IMETHOD HasMoreElements(PRBool *result) 
>         {
>+          if (mIterator == nsnull || mFSRefsArray == nsnull)
>+            return NS_ERROR_FAILURE;

I think we should set *result = PR_FALSE and return NS_OK.  Assuming your sample code and a properly-initialized dirEntries:

|while dirEntries.HasMoreElements():
|    entry = dirEntries.GetNext(components.interfaces.nsIFile)
|    dirEntries.HasMoreElements()

When the second call to HasMoreElements crashes, it's not really because HasMoreElements has failed, it's because there are no more elements.  The Unix and Win32 implementatins of nsIDirectoryEnumerator agree with this reasoning.

Comment 3

[jag (Peter Annema)]

Attachment: Check for nulls and return PR_FALSE/NS_OK

Comment 4

[Benjamin Smedberg]

Comment on attachment 308855 [details] [diff] [review]
Check for nulls and return PR_FALSE/NS_OK

Unit-test, please

Comment 5

[jag (Peter Annema)]

Attachment: Check for nulls and return PR_FALSE/NS_OK. Now with unit test written by Callek.

Comment 6

[Damon Sicore (:damons)]

Comment on attachment 309747 [details] [diff] [review]
Check for nulls and return PR_FALSE/NS_OK. Now with unit test written by Callek.

a1.9+=damons

Thanks for the tests!

Comment 7

[jag (Peter Annema)]

Checking in io/nsLocalFileOSX.mm;
/cvsroot/mozilla/xpcom/io/nsLocalFileOSX.mm,v  <--  nsLocalFileOSX.mm
new revision: 1.3; previous revision: 1.2
done
RCS file: /cvsroot/mozilla/xpcom/tests/unit/test_bug333505.js,v
done
Checking in tests/unit/test_bug333505.js;
/cvsroot/mozilla/xpcom/tests/unit/test_bug333505.js,v  <--  test_bug333505.js
initial revision: 1.1
done