[e10s] Null deref crash when running test_pluginstream_newstream.html

RESOLVED FIXED in Firefox 47

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: mccr8, Assigned: haik)

Tracking

(Blocks 1 bug)

Trunk
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox46 affected, firefox47 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

4 years ago
When I run dom/plugins/test/mochitest/test_pluginstream_newstream.html with e10s, I get a crash:

calling NPN_NewStream...return value 0
Assertion failure: mRawPtr != 0 (You can't dereference a NULL nsCOMPtr with operator->().), at /Users/amccreight/mc/obj-dbg.noindex/dist/include/nsCOMPtr.h:734
#01: mozilla::plugins::PluginStreamParent::AnswerNPN_Write(nsCString const&, int*) (PluginStreamParent.cpp:43, in XUL)
#02: mozilla::plugins::PPluginStreamParent::OnCallReceived(IPC::Message const&, IPC::Message*&) (PPluginStreamParent.cpp:230, in XUL)
#03: mozilla::plugins::PPluginModuleParent::OnCallReceived(IPC::Message const&, IPC::Message*&) (PPluginModuleParent.cpp:1410, in XUL)
#04: mozilla::ipc::MessageChannel::DispatchInterruptMessage(IPC::Message const&, unsigned long) (MessageChannel.cpp:1453, in XUL)
#05: mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message const&) (MessageChannel.cpp:1302, in XUL)
#06: mozilla::ipc::MessageChannel::OnMaybeDequeueOne() (MessageChannel.cpp:1275, in XUL)
#07: MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) (message_loop.cc:365, in XUL)
#08: MessageLoop::DoWork() (message_loop.cc:459, in XUL)
#09: mozilla::ipc::DoWorkRunnable::Run() (MessagePump.cpp:221, in XUL)
#10: nsThread::ProcessNextEvent(bool, bool*) (nsCOMPtr.h:403, in XUL)

The crash is in PluginStreamParent, but oddly enough the crash looks to be in the content process, because I see the "oops your tab crashed!" message in the browser running the test.

The crash looks to be on this line:

  *written = mInstance->mNPNIface->write(mInstance->mNPP, mStream,
                                         data.Length(),
                                         const_cast<char*>(data.get()));
Reporter

Comment 1

4 years ago
Another odd thing: this test is only disabled in e10s in debug builds, so maybe it doesn't crash in opt builds?

Comment 2

4 years ago
PluginStreamParent runs in the content process, because that is the parent side of the content<->plugin bridge.

Can you debug and see which thing is null? mInstance isn't a nsCOMPtr so I'm confused about which thing is asserting.

I cannot explain the debug-only bit.
Reporter

Comment 3

4 years ago
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #2)
> PluginStreamParent runs in the content process, because that is the parent
> side of the content<->plugin bridge.
Yeah, Bill explained that to me. I didn't realize that was how bridges are set up, but it makes sense.

> Can you debug and see which thing is null? mInstance isn't a nsCOMPtr so I'm
> confused about which thing is asserting.

I'll do that. My best guess is that there's an assertion somewhere in there that does a dereference and we're just getting an odd stack due to inlining.
Reporter

Comment 4

4 years ago
I got a stack in the debugger. It looks like the null-deref is on nsPluginStreamToFile::mOutputStream, as the deref is on this line:
  mOutputStream->Write(aBuf, aCount, aWriteCount);

* thread #1: tid = 0x27775, 0x0000000102bab009 XUL`nsPluginStreamToFile::Write(char const*, unsigned int, unsigned int*) [inlined] nsCOMPtr<nsIOutputStream>::operator->() const + 72 at nsCOMPtr.h:733, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x0000000102bab009 XUL`nsPluginStreamToFile::Write(char const*, unsigned int, unsigned int*) [inlined] nsCOMPtr<nsIOutputStream>::operator->() const + 72 at nsCOMPtr.h:733
    frame #1: 0x0000000102baafc1 XUL`nsPluginStreamToFile::Write(this=<unavailable>, aBuf=<unavailable>, aCount=<unavailable>, aWriteCount=<unavailable>) + 81 at nsNPAPIPluginStreamListener.cpp:91
    frame #2: 0x0000000102b8a966 XUL`mozilla::plugins::parent::_write(npp=<unavailable>, pstream=<unavailable>, len=1024, buffer=0x000000010bb1a808) + 182 at nsNPAPIPlugin.cpp:888
    frame #3: 0x0000000102bf2def XUL`mozilla::plugins::PluginStreamParent::AnswerNPN_Write(this=0x0000000112a43240, data=<unavailable>, written=0x00007fff5fbfbd74) + 63 at PluginStreamParent.cpp:43
    frame #4: 0x0000000100ba8ef5 XUL`mozilla::plugins::PPluginStreamParent::OnCallReceived(this=0x0000000112a43240, msg__=<unavailable>, reply__=0x00007fff5fbfbe80) + 1253 at PPluginStreamParent.cpp:230
    frame #5: 0x0000000100b96eff XUL`mozilla::plugins::PPluginModuleParent::OnCallReceived(this=<unavailable>, msg__=0x00007fff5fbfbfc8, reply__=0x00007fff5fbfbe80) + 415 at PPluginModuleParent.cpp:1410
    frame #6: 0x00000001009e007c XUL`mozilla::ipc::MessageChannel::DispatchInterruptMessage(this=0x000000010bb1c868, aMsg=0x00007fff5fbfbfc8, stackDepth=<unavailable>) + 716 at MessageChannel.cpp:1452
    frame #7: 0x00000001009dfb42 XUL`mozilla::ipc::MessageChannel::DispatchMessage(this=0x000000010bb1c868, aMsg=<unavailable>) + 530 at MessageChannel.cpp:1302
    frame #8: 0x00000001009da064 XUL`mozilla::ipc::MessageChannel::OnMaybeDequeueOne(this=0x000000010bb1c868) + 244 at MessageChannel.cpp:1275
Reporter

Comment 7

4 years ago
RemoteOpenFileChild::CreateUnique() isn't implemented, and I guess that's what this would be using?
> RemoteOpenFileChild

Are you sure? I thought that was only used for JAR files.

What platform are you testing on? I could imagine the Mac sandbox could affect this.
Reporter

Comment 9

4 years ago
(In reply to Bill McCloskey (:billm) from comment #8)
> Are you sure? I thought that was only used for JAR files.
Ah, you are right. This is nsLocalFile.
 
> What platform are you testing on? I could imagine the Mac sandbox could
> affect this.

I'm on OSX.
OK, that must be it then. Not sure who to redirect this to. Brad, who do you think should look at this Mac sandboxing issue?
Flags: needinfo?(blassey.bugs)
Assignee

Comment 11

4 years ago
(In reply to Bill McCloskey (:billm) from comment #10)
> OK, that must be it then. Not sure who to redirect this to. Brad, who do you
> think should look at this Mac sandboxing issue?

I could take a crack at this if it's not needed urgently.
Assignee: nobody → haftandilian
Flags: needinfo?(blassey.bugs)
Blocks: e10s-tests
tracking-e10s: --- → +
Assignee

Comment 12

4 years ago
I think the file we're trying to create (on OS X 10.11.2 (15C50)) with the CreateUnique call in the nsPluginStreamToFile constructor is 

  ~/Library/Caches/TemporaryItems/testframe

When this fails, mOutputStream is never initialized resulting in the nsPluginStreamToFile::Write crash. 

I haven't got to lowest level file creation call that fails yet. I'm wondering if the current OS X sandbox permits the child creating such a file.

Creating a file in the nsPluginStreamToFile constructor seems like a bad idea.
(In reply to Haik Aftandilian [:haik] from comment #12)
> I think the file we're trying to create (on OS X 10.11.2 (15C50)) with the
> CreateUnique call in the nsPluginStreamToFile constructor is 
> 
>   ~/Library/Caches/TemporaryItems/testframe
> 
> When this fails, mOutputStream is never initialized resulting in the
> nsPluginStreamToFile::Write crash. 
> 
> I haven't got to lowest level file creation call that fails yet. I'm
> wondering if the current OS X sandbox permits the child creating such a file.

There seems to have been a similar problem a while back in bug 1190032. The file that couldn't be created due to sandbox rules was ~/Library/Caches/TemporaryItems/plugtmp. The fix appears to have been a bug-specific (i.e. file-specific) workaround in the sandbox ruleset. Thought I'd throw this out there in case it helps.
Assignee

Comment 14

4 years ago
(In reply to Stephen A Pohl [:spohl] from comment #13)
> There seems to have been a similar problem a while back in bug 1190032.

Thanks. Changing the sandbox ruleset in this case would require allowing files with any name to be created in ~/Library/Caches/TemporaryItems/. "testframe" comes from the iframe name in the test. I was surprised to see the iframe name being used in the Cache filename.

Here's the relevant test HTML.

https://dxr.mozilla.org/mozilla-central/rev/531d1f6d1cde1182e9f7f9dff81a4fc5abc0a601/dom/plugins/test/mochitest/test_pluginstream_newstream.html#18

And the open failure is coming from the system's open call libsystem_kernel.dylib`__open. On a Mac, running "sudo opensnoop -x -n plugin-container" displays the path to files for which the open system call fails for the process named plugin-container.
Assignee

Comment 15

4 years ago
I tested on Ubuntu and found that the test passes and the testframe file is put in /tmp.
Assignee

Comment 16

4 years ago
Discussed with Bob and JCP. With the Windows low integrity sandbox, we have a per-profile temp directory and we plan to take the same approach here. i.e., create a per-profile temporary directory in the parent and pass the path to the child via a preference. That will become the child's temporary directory and it will be given permission to create and write to files there. It would be better to be more restrictive and go through the parent for creation of any temporary files, but considering this approach because it minimizes changes needed to support the older nsNPAPI plugins using this stream functionality.
Assignee

Updated

4 years ago
See Also: → 1162327
Assignee

Comment 17

3 years ago
Modify the Mac sandbox to allow temporary files to be created in a parent-specified temporary directory. This replaces the NS_OS_TEMP_DIR directory.

Review commit: https://reviewboard.mozilla.org/r/34023/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34023/
Attachment #8717045 - Flags: review?(gpascutto)
Attachment #8717045 - Flags: review?(bobowen.code)
Assignee

Comment 18

3 years ago
The patch I just posted in comment is 17 is incomplete, but I wanted to make sure I'm going in the right direction and ask a question.

With the changes, the content process is reading a full path from the preferences and attempting to replace NS_OS_TEMP_DIR with that directory. I think I need to create an nsIFile in order to do that, but how does one create an nsIFile from an arbitrary local path. I found nsILocalFile, but it is marked as deprecated.
Comment on attachment 8717045 [details]
MozReview Request: Bug 1237847 - [e10s] Null deref crash when running test_pluginstream_newstream.html; r=bobowen

https://reviewboard.mozilla.org/r/34023/#review30671

::: dom/ipc/ContentProcess.cpp:21
(Diff revision 1)
> +#include "nsDirectoryServiceDefs.h"

These are common with the XP_WIN things, so move them up.

::: dom/ipc/ContentProcess.cpp:47
(Diff revision 1)
> +  // XXX how to create an nsIFile from a string path?

Example code from another component: (note the NS_ENSURE_SUCCESS thing is deprecated)

// Directory providers must also be accessed on the main thread.
  nsCOMPtr<nsIFile> cacheDir;
  rv = NS_GetSpecialDirectory(NS_APP_USER_PROFILE_LOCAL_50_DIR,
                              getter_AddRefs(cacheDir));
  if (NS_FAILED(rv)) {
    rv = NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR,
                                getter_AddRefs(cacheDir));
    if (NS_FAILED(rv)) {
      return rv;
    }
  }
// Get the root directory where to store all the databases.
  nsresult rv = cacheDir->Clone(getter_AddRefs(mStoreDirectory));
  NS_ENSURE_SUCCESS(rv, rv);

  rv = mStoreDirectory->AppendNative(STORE_DIRECTORY);
  NS_ENSURE_SUCCESS(rv, rv);
Attachment #8717045 - Flags: review?(gpascutto)
Assignee

Comment 20

3 years ago
I tried using the above NS_GetSpecialDirectory calls, but they fail due to nsDirectoryService::Get returning NS_ERROR_FAILURE. I don't know why yet, but could it be the child is not expected to need them or at this stage the nsDirectoryService isn't fully setup? Could not find where this is all setup in the code base.

The following did work though.

  nsCOMPtr<nsIFile> tempDir;
  tempDir = do_CreateInstance(NS_LOCAL_FILE_CONTRACTID);
  tempDir->InitWithPath(tempDirPath);
You can't get the profile directory from the content process: the content process doesn't run with a profile and doesn't know where the profile is located. That's why a "parent-specified" file/directory is required.
Assignee

Comment 22

3 years ago
Comment on attachment 8717045 [details]
MozReview Request: Bug 1237847 - [e10s] Null deref crash when running test_pluginstream_newstream.html; r=bobowen

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34023/diff/1-2/
Attachment #8717045 - Attachment description: MozReview Request: Bug 1237847 - [e10s] Null deref crash when running test_pluginstream_newstream.html; r?bobowen, gcp → MozReview Request: Bug 1237847 - [e10s] Null deref crash when running test_pluginstream_newstream.html; r?gcp
Attachment #8717045 - Flags: review?(bobowen.code) → review?(gpascutto)
Assignee

Comment 23

3 years ago
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #21)
> You can't get the profile directory from the content process: the content
> process doesn't run with a profile and doesn't know where the profile is
> located. That's why a "parent-specified" file/directory is required.

Thanks.

The new patch posted no longer passes the full path to a profile sub-directory to the child, instead does what the Windows version does by passing just a UUID which is used to build a sub-directory name. But the temp sub-directory is created in NS_OS_TEMP_DIR unlike on Windows.

Passing the full path to a profile sub-dir seemed like a bad idea because it exposes the profile dir to the child and I was worried that if the preference was changed, we might delete important user files when cleaning up the temp dir.

I also noticed on Mac we have $TMPDIR which is the same as 

  ~ $ getconf DARWIN_USER_TEMP_DIR
  /var/folders/46/188kdiwu49d3zkkfgrgsnx2m0000gn/T/
  ~ $ echo $TMPDIR
  /var/folders/46/188kdiwu49d3zkkfgrgsnx2m0000gn/T/

And documented in CONFSTR(3) as

     _CS_DARWIN_USER_TEMP_DIR
             Provides the path to a user's temporary items directory. The
             directory will be created it if does not already exist. This
             directory is created with access permissions of 0700 and
             restricted by the umask(2) of the calling process and is a good
             location for temporary files.

             By default, files in this location may be cleaned (removed) by
             the system if they are not accessed in 3 days.

Given that it can be automatically cleaned up by the OS, it might be a better place for this.
Assignee

Comment 24

3 years ago
https://reviewboard.mozilla.org/r/34023/#review30671

> These are common with the XP_WIN things, so move them up.

Cleaned up in new version.
Attachment #8717045 - Flags: review?(gpascutto)
Attachment #8717045 - Flags: review?(bobowen.code)
Attachment #8717045 - Flags: feedback+
Assignee

Updated

3 years ago
Attachment #8717045 - Attachment description: MozReview Request: Bug 1237847 - [e10s] Null deref crash when running test_pluginstream_newstream.html; r?gcp → MozReview Request: Bug 1237847 - [e10s] Null deref crash when running test_pluginstream_newstream.html; r?bobowen feedback+gcp
Attachment #8717045 - Flags: feedback+
Assignee

Comment 25

3 years ago
Comment on attachment 8717045 [details]
MozReview Request: Bug 1237847 - [e10s] Null deref crash when running test_pluginstream_newstream.html; r=bobowen

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34023/diff/2-3/
Comment on attachment 8717045 [details]
MozReview Request: Bug 1237847 - [e10s] Null deref crash when running test_pluginstream_newstream.html; r=bobowen

https://reviewboard.mozilla.org/r/34023/#review32447

Few nits, questions and thoughts.

::: browser/app/profile/firefox.js:1210
(Diff revision 3)
> +// ID (a UUID when set by gecko) that is used as a per profile suffix to a low

It would be a pain, but I wonder if where we have the Mac and Windows code in common we shouldn't be using a term like sandbox writable instead of low integrity, as that only really means something on Windows.

::: dom/ipc/ContentChild.cpp:1391
(Diff revision 3)
> +  // sandboxed temporary directory.

nit: I think this should say something like "sandbox writable temporary directory.".

::: dom/ipc/ContentChild.cpp:1401
(Diff revision 3)
> +  char *tempDirPathCString = ToNewCString(tempDirPath);

nit: Why not just do tempDirPath.get() like the others?

::: dom/ipc/ContentProcess.cpp:40
(Diff revision 3)
>    // sandbox pref level >= 1.

nit: This comment needs updating, in fact it might be best to keeps the checks for each OS separate, for clarity.
It's only partially by design that level 1 for Mac and Windows means we need this special temp directory.
They could be different.

::: dom/ipc/ContentProcess.cpp:56
(Diff revision 3)
> +      NS_WIN_LOW_INTEGRITY_TEMP_BASE,

nit: indentation changed ... if it were me I think I'd set a variable in #ifdefs before the call.

::: security/sandbox/mac/Sandbox.mm:428
(Diff revision 3)
> +  "        (home-subpath appTempDir))\n"

First time I've looked at the Mac rules in detail.
So this new dir lives under the home dir if I'm reading this correctly?

::: toolkit/xre/nsAppRunner.cpp:609
(Diff revision 3)
> +      NS_WIN_LOW_INTEGRITY_TEMP_BASE,

nit: As before.

::: toolkit/xre/nsAppRunner.cpp:643
(Diff revision 3)
>    // and sandbox pref level >= 1.

As before.

::: toolkit/xre/nsAppRunner.cpp:704
(Diff revision 3)
>    // We can't have created a low integrity temp before Vista.

nit: inside #ifdef
Attachment #8717045 - Flags: review?(bobowen.code) → review+
Assignee

Comment 27

3 years ago
Comment on attachment 8717045 [details]
MozReview Request: Bug 1237847 - [e10s] Null deref crash when running test_pluginstream_newstream.html; r=bobowen

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34023/diff/3-4/
Attachment #8717045 - Attachment description: MozReview Request: Bug 1237847 - [e10s] Null deref crash when running test_pluginstream_newstream.html; r?bobowen feedback+gcp → MozReview Request: Bug 1237847 - [e10s] Null deref crash when running test_pluginstream_newstream.html; r+bobowen
Assignee

Comment 28

3 years ago
https://reviewboard.mozilla.org/r/34023/#review32447

> It would be a pain, but I wonder if where we have the Mac and Windows code in common we shouldn't be using a term like sandbox writable instead of low integrity, as that only really means something on Windows.

Sounds like a good idea. Changed the comments and some variable names in these files to refer to a sandbox-writable temporary directory. Moved the Suffix pref down after the sandbox level because the comments reference the level.

> nit: I think this should say something like "sandbox writable temporary directory.".

Done.

> nit: Why not just do tempDirPath.get() like the others?

Fixed. I think at the time I was confused about string types.

> nit: This comment needs updating, in fact it might be best to keeps the checks for each OS separate, for clarity.
> It's only partially by design that level 1 for Mac and Windows means we need this special temp directory.
> They could be different.

I see. Moved the checks to Mac-specific and Windows-specific functions.

> nit: indentation changed ... if it were me I think I'd set a variable in #ifdefs before the call.

Fixed.

> nit: As before.

Fixed.

> As before.

Fixed.
Assignee

Comment 29

3 years ago
https://reviewboard.mozilla.org/r/34023/#review32447

> Sounds like a good idea. Changed the comments and some variable names in these files to refer to a sandbox-writable temporary directory. Moved the Suffix pref down after the sandbox level because the comments reference the level.

The above "Moved the Suffix pref down after the sandbox level because the comments reference the level" refers to the firefox.js file.
Assignee

Comment 30

3 years ago
https://reviewboard.mozilla.org/r/34023/#review32447

> First time I've looked at the Mac rules in detail.
> So this new dir lives under the home dir if I'm reading this correctly?

Yes, it's within the home directory as a result of NS_OS_TEMP_DIR being within the home directory. The "home-subpath" usage in the read and write rules here could be considered redundant because it acts a filter and only allows reads/writes to appTempDir that are also within the home directory. But I think it's also a good failsafe measure preventing the sandboxed-writable temp dir from ever being outside of the home directory. I gleaned that from http://reverse.put.as/wp-content/uploads/2011/09/Apple-Sandbox-Guide-v1.0.pdf
Assignee

Comment 31

3 years ago
Comment on attachment 8717045 [details]
MozReview Request: Bug 1237847 - [e10s] Null deref crash when running test_pluginstream_newstream.html; r=bobowen

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34023/diff/4-5/
Attachment #8717045 - Attachment description: MozReview Request: Bug 1237847 - [e10s] Null deref crash when running test_pluginstream_newstream.html; r+bobowen → MozReview Request: Bug 1237847 - [e10s] Null deref crash when running test_pluginstream_newstream.html; r=bobowen
Assignee

Updated

3 years ago
Keywords: checkin-needed

Comment 34

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5b20181a5b50
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Depends on: 1254774

Updated

3 years ago
Depends on: 1261751
You need to log in before you can comment on or make changes to this bug.