dom/filesystem/tests/test_basic.html ASSERT failure on OS X with level 3 due to /Volumes restriction

RESOLVED FIXED in Firefox 55

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: haik, Assigned: Alex_Gaynor)

Tracking

55 Branch
mozilla55
Unspecified
macOS
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: sbmc2)

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
On OS X, with a debug build, with security.sandbox.content.level=3, the test dom/filesystem/tests/test_basic.html triggers an ASSERT failure in the content process.

This appears to be triggered by the /Volumes restriction added in bug 1363179.

I tried running the test with '(allow file-read* (literal "/Volumes"))' added to the rules and that let the test pass. Whether or not that's the right fix needs more thought.

Process 44710 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
    frame #0 XUL`mozilla::dom::Directory::Create() at Directory.cpp:92
   89  	#ifdef DEBUG
   90  	  bool isDir;
   91  	  nsresult rv = aFile->IsDirectory(&isDir);
-> 92  	  MOZ_ASSERT(NS_SUCCEEDED(rv) && isDir);
   93  	#endif
   94  	
   95  	  RefPtr<Directory> directory = new Directory(aParent, aFile, aFileSystem);
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0 XUL`mozilla::dom::Directory::Create() at Directory.cpp:92
    frame #1 XUL`mozilla::dom::GetDirectoryListingTaskChild::SetSuccessRequestResult() at GetDirectoryListingTask.cpp:152
    frame #2 XUL`mozilla::dom::FileSystemTaskChildBase::SetRequestResult() at FileSystemTaskBase.cpp:209
    frame #3 XUL`mozilla::dom::FileSystemTaskChildBase::Recv__delete__() at FileSystemTaskBase.cpp:219
    frame #4 XUL`mozilla::dom::PFileSystemRequestChild::OnMessageReceived() at PFileSystemRequestChild.cpp:90
    frame #5 XUL`mozilla::ipc::PBackgroundChild::OnMessageReceived() at PBackgroundChild.cpp:1630
    frame #6 XUL`mozilla::ipc::MessageChannel::DispatchAsyncMessage() at MessageChannel.cpp:2040
    frame #7 XUL`mozilla::ipc::MessageChannel::DispatchMessage() at MessageChannel.cpp:1975
    frame #8 XUL`mozilla::ipc::MessageChannel::RunMessage() at MessageChannel.cpp:1844
    frame #9 XUL`mozilla::ipc::MessageChannel::MessageTask::Run() at MessageChannel.cpp:1877
    frame #10 XUL`mozilla::SchedulerGroup::Runnable::Run() at SchedulerGroup.cpp:370
    frame #11 XUL`nsThread::ProcessNextEvent() at nsThread.cpp:1273
    frame #12 XUL`NS_ProcessPendingEvents() at nsThreadUtils.cpp:335
    frame #13 XUL`nsBaseAppShell::NativeEventCallback() at nsBaseAppShell.cpp:97
    frame #14 XUL`nsAppShell::ProcessGeckoEvents() at nsAppShell.mm:399
    frame #15 CoreFoundation`__CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__() 
    frame #16 CoreFoundation`__CFRunLoopDoSources0() 
    frame #17 CoreFoundation`__CFRunLoopRun() 
    frame #18 CoreFoundation`CFRunLoopRunSpecific() 
    frame #19 HIToolbox`RunCurrentEventLoopInMode() 
    frame #20 HIToolbox`ReceiveNextEventCommon() 
    frame #21 HIToolbox`_BlockUntilNextEventMatchingListInModeWithFilter() 
    frame #22 AppKit`_DPSNextEvent() 
    frame #23 AppKit`-[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:]() 
    frame #24 XUL`::-[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:](NSEventMask, NSDate *, NSString *, BOOL)() at nsAppShell.mm:130
    frame #25 AppKit`-[NSApplication run]() 
    frame #26 XUL`nsAppShell::Run() at nsAppShell.mm:673
    frame #27 XUL`XRE_RunAppShell() at nsEmbedFunctions.cpp:893
    frame #28 XUL`mozilla::ipc::MessagePumpForChildProcess::Run() at MessagePump.cpp:269
    frame #29 XUL`MessageLoop::RunInternal() at message_loop.cc:238
    frame #30 XUL`MessageLoop::RunHandler() at message_loop.cc:231
    frame #31 XUL`MessageLoop::Run() at message_loop.cc:211
    frame #32 XUL`XRE_InitChildProcess() at nsEmbedFunctions.cpp:709
    frame #33 XUL`mozilla::BootstrapImpl::XRE_InitChildProcess() at Bootstrap.cpp:65
    frame #34 plugin-container`content_process_main() at plugin-container.cpp:64
    frame #35 plugin-container`main() at MozillaRuntimeMain.cpp:25
    frame #36 plugin-container`start()
(Reporter)

Updated

2 years ago
See Also: → 1363179
Whiteboard: sbmc2
(Reporter)

Comment 1

2 years ago
The problem is that Directory::Create() can't be called in DEBUG mode to create a directory object for a directory the content process doesn't have access to. This is due to the following code extra DEBUG check.

http://searchfox.org/mozilla-central/rev/35b37316149108d53a02fb8498e250ea8a22ab5d/dom/filesystem/Directory.cpp#89
  
  Directory::Create(nsISupports* aParent, nsIFile* aFile,
                    FileSystemBase* aFileSystem)
  ...
    #ifdef DEBUG
      bool isDir;
      nsresult rv = aFile->IsDirectory(&isDir);
      MOZ_ASSERT(NS_SUCCEEDED(rv) && isDir);
    #endif
  ...

With sandboxing, we're not going to be able to rely on being able to access the directory so this code should go.

That's a better fix than adding a sandbox '(allow file-read* (literal "/Volumes"))' rule which would allow the test to pass, but wouldn't fix the problem for directory listing operations on other directories.
(Assignee)

Updated

2 years ago
Assignee: nobody → agaynor
(Assignee)

Comment 2

2 years ago
Ok, I took a look at the rest of the |dom/| directory, and I couldn't find anywhere else that triggers this assert, so I think, even though that DEBUG block is clearly wrong in a sandboxed process, I'm going to see how hard it would be to move the Directory object to the parent, since it seems unfortunate to remove a debugging assertion for one callsite. If I've missed any tests and really a ton more our broken we should revisit.
(Assignee)

Comment 3

2 years ago
I spent some more time thinking about this, and the potential for tests (or application code) that break as we tighten up the sandbox only increases.

More importantly, if this code is really going to be accessible from content, in an e10s sandboxed world, it doesn't make sense for Directory methods to be touching the filesystem, it just can't work as a general rule.

Given this, I think the originally proposed solution of just deleting that |#if DEBUG| branch makes the most sense. Before I send a patch for that, I'd like some feedback from the DOM folks who own this code. :baku, do you have any objections to this change?
Flags: needinfo?(amarchesini)
Comment hidden (mozreview-request)

Comment 5

2 years ago
mozreview-review
Comment on attachment 8874843 [details]
Bug 1367560 - Remove an assertion from the Directory constructor that the directory exists on disk from DEBUG builds;

https://reviewboard.mozilla.org/r/146234/#review150206
Attachment #8874843 - Flags: review?(amarchesini) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 6

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6a8333a5941e
Remove an assertion from the Directory constructor that the directory exists on disk from DEBUG builds; r=baku
Keywords: checkin-needed

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6a8333a5941e
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Flags: needinfo?(amarchesini)
You need to log in before you can comment on or make changes to this bug.