Closed Bug 333505 Opened 14 years ago Closed 12 years ago

nsLocalFileOSX crash

Categories

(Core Graveyard :: File Handling, defect, critical)

PowerPC
macOS
defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: shanec, Assigned: jag+mozilla)

Details

(Keywords: crash)

Attachments

(1 file, 2 obsolete files)

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.
Attached patch nullcheck.patch (obsolete) — Splinter Review
Keywords: crash
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: file-handling → jag
Attachment #217950 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #308855 - Flags: superreview?(benjamin)
Attachment #308855 - Flags: review?(benjamin)
Comment on attachment 308855 [details] [diff] [review]
Check for nulls and return PR_FALSE/NS_OK

Unit-test, please
Attachment #308855 - Flags: superreview?(benjamin)
Attachment #308855 - Flags: review?(benjamin)
Attachment #308855 - Attachment is obsolete: true
Attachment #309747 - Flags: superreview?(benjamin)
Attachment #309747 - Flags: review?(benjamin)
Attachment #309747 - Flags: superreview?(benjamin)
Attachment #309747 - Flags: superreview+
Attachment #309747 - Flags: review?(benjamin)
Attachment #309747 - Flags: review+
Attachment #309747 - Flags: approval1.9?
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!
Attachment #309747 - Flags: approval1.9? → approval1.9+
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
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.