Closed
Bug 1367560
Opened 7 years ago
Closed 7 years ago
dom/filesystem/tests/test_basic.html ASSERT failure on OS X with level 3 due to /Volumes restriction
Categories
(Core :: Security: Process Sandboxing, defect)
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: haik, Assigned: Alex_Gaynor)
References
Details
(Whiteboard: sbmc2)
Attachments
(1 file)
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 | ||
Comment 1•7 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•7 years ago
|
Assignee: nobody → agaynor
Assignee | ||
Comment 2•7 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•7 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•7 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•7 years ago
|
Keywords: checkin-needed
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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6a8333a5941e
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•7 years ago
|
Flags: needinfo?(amarchesini)
You need to log in
before you can comment on or make changes to this bug.
Description
•