Status

Core Graveyard
File Handling
--
critical
RESOLVED FIXED
12 years ago
2 years ago

People

(Reporter: Shane Caraveo, Assigned: jag (Peter Annema))

Tracking

({crash})

Trunk
PowerPC
Mac OS X
crash

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

12 years ago
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.
(Reporter)

Comment 1

12 years ago
Created attachment 217950 [details] [diff] [review]
nullcheck.patch

Updated

12 years ago
Keywords: crash

Comment 2

12 years ago
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.

Updated

12 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 3

10 years ago
Created attachment 308855 [details] [diff] [review]
Check for nulls and return PR_FALSE/NS_OK
Assignee: file-handling → jag
Attachment #217950 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #308855 - Flags: superreview?(benjamin)
Attachment #308855 - Flags: review?(benjamin)

Comment 4

10 years ago
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)
(Assignee)

Comment 5

10 years ago
Created attachment 309747 [details] [diff] [review]
Check for nulls and return PR_FALSE/NS_OK. Now with unit test written by Callek.
Attachment #308855 - Attachment is obsolete: true
Attachment #309747 - Flags: superreview?(benjamin)
Attachment #309747 - Flags: review?(benjamin)

Updated

10 years ago
Attachment #309747 - Flags: superreview?(benjamin)
Attachment #309747 - Flags: superreview+
Attachment #309747 - Flags: review?(benjamin)
Attachment #309747 - Flags: review+
(Assignee)

Updated

10 years ago
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+
(Assignee)

Comment 7

10 years ago
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
Last Resolved: 10 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.