Implement open_resource in non-SFI mode.

components/nacl/renderer/manifest_service_channel.cc

Index: components/nacl/renderer/manifest_service_channel.cc
diff --git a/components/nacl/renderer/manifest_service_channel.cc b/components/nacl/renderer/manifest_service_channel.cc
index f0f4adb8f940a261a4116c1f6fe9b1f292e0e4e2..26d7ae455e93dc767937a8335f0f46dfedc1678f 100644
--- a/components/nacl/renderer/manifest_service_channel.cc
+++ b/components/nacl/renderer/manifest_service_channel.cc
@@ -4,6 +4,7 @@
 
 #include "components/nacl/renderer/manifest_service_channel.h"
 
+#include "base/bind.h"
 #include "base/callback.h"
 #include "base/callback_helpers.h"
 #include "content/public/renderer/render_thread.h"
@@ -24,7 +25,8 @@ ManifestServiceChannel::ManifestServiceChannel(
       channel_(new IPC::SyncChannel(
           handle, IPC::Channel::MODE_CLIENT, this,
           content::RenderThread::Get()->GetIOMessageLoopProxy(),
-          true, waitable_event)) {
+          true, waitable_event)),
+      weak_ptr_factory_(this) {
 }
 
 ManifestServiceChannel::~ManifestServiceChannel() {
@@ -32,12 +34,17 @@ ManifestServiceChannel::~ManifestServiceChannel() {
     base::ResetAndReturn(&connected_callback_).Run(PP_ERROR_FAILED);
 }
 
+void ManifestServiceChannel::Send(IPC::Message* message) {
+  channel_->Send(message);
+}
+
 bool ManifestServiceChannel::OnMessageReceived(const IPC::Message& message) {
-  // TODO(hidehiko): Implement OpenResource.
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(ManifestServiceChannel, message)
       IPC_MESSAGE_HANDLER(PpapiHostMsg_StartupInitializationComplete,
                           OnStartupInitializationComplete)
+      IPC_MESSAGE_HANDLER_DELAY_REPLY(PpapiHostMsg_OpenResource,
+                                      OnOpenResource)
       IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   return handled;
@@ -57,4 +64,28 @@ void ManifestServiceChannel::OnStartupInitializationComplete() {
   delegate_->StartupInitializationComplete();
 }
 
+void ManifestServiceChannel::OnOpenResource(
+    const std::string& key, IPC::Message* reply) {
+  delegate_->OpenResource(key,

[Mark Seaborn, 2014/04/30 21:20:58]

On Windows, it would be better for this to be a no-op that replies with an error
message.  (See comments below.)

As security hardening, could you possibly make this code path return a failure
under PNaCl too?  (i.e. On the open web, outside of the web store.) 
open_resource() is not exposed to PNaCl.

The old code path is currently exposed to untrusted code on the open web.  This
isn't ideal, and I'd like to disable it under PNaCl, but the old code path has
been around for a while and is fairly stable.  It would be nice if the new code
path is hidden from PNaCl from the start.

In ppb_nacl_private_impl.cc, you can assume that "!enable_dyncode_syscalls" is
synonymous with PNaCl mode (as loader/nacl_listener.cc does -- see the comment
there).

[hidehiko, 2014/05/01 05:20:31]

Done.

+                          base::Bind(&ManifestServiceChannel::DidOpenResource,
+                                     weak_ptr_factory_.GetWeakPtr(), reply));
+}
+
+void ManifestServiceChannel::DidOpenResource(
+    IPC::Message* reply, const base::PlatformFile& platform_file) {
+#if defined(OS_WIN)
+  // Currently this is used only for non-SFI mode, which is not supported on
+  // windows.
+  NOTREACHED();

[Mark Seaborn, 2014/04/30 21:20:58]

This would allow the NaCl process to crash the renderer on debug builds, which
is a little undesirable.  On release builds this is a no-op, I think, so
Send(reply) gets called...

Would this leak a file handle if called on Windows?

[hidehiko, 2014/05/01 05:20:31]

IMHO, crashing on Debug build is fine. Actually, the code path should never be
called for now. So in new code, I kept NOTREACHED(), but added the code to
return an error to the peer process for Release build.
Yes, but again this shouldn't be called on Windows. Actually, to support windows
we need to change some code in ppapi/native_client/src/trusted/plugin/... as
well, and currently it always returns -1 on Win even if it is accidentally
invoked (again, the code shouldn't be called).

[Mark Seaborn, 2014/05/01 06:32:35]

For what meaning of "should"? :-)  Untrusted code can send this IPC if it
bypasses the IRT.

In general, untrusted code shouldn't be able to crash the renderer process.

I wouldn't like to assume here that NOTREACHED() only has an effect in debug
builds.  I could imagine it changing to abort in release builds too -- it would
be cheap to compile it to a single "HLT" instruction.

[hidehiko, 2014/05/01 13:05:40]

I'm a bit confused. I thought nacl_helper is a part of trusted code. In
nacl_listener.cc, we create the IPC channel for this class only for non-SFI
mode.
So, this class won't be instantiated and this code path won't be executed on
Win.
I thought the same thing for NaCl in SFI mode.

This is a half of the meaning of "should never" in my previous reply (sorry for
my poor explanation). # The remaining half is, trivially, we do not expect for
this function to be invoked on Win :-).

If we cannot trust nacl_helper itself (i.e., we need to assume that some bad
plugin can create an IPC channel somehow and pass it to the renderer process),
my thought is wrong, though.

[Mark Seaborn, 2014/05/01 23:28:33]

Ah, sorry, I hadn't realised that the new IPC channel was only set up by
nacl_listener.cc for Non-SFI Mode.  I had assumed it was set up unconditionally.

In that case, I needn't have asked you to add these extra checks.  However, I
think it's OK to have the extra checks.  It doesn't hurt to have the renderer
check without relying on nacl_listener.cc.

You're right, nacl_helper is trusted before it loads the nexe (and after, for
SFI NaCl).

[hidehiko, 2014/05/02 01:32:27]

Thanks. So let's keep the current error handling.

+#else
+  // Here, PlatformFileForTransit is alias of base::FileDescriptor.
+  PpapiHostMsg_OpenResource::WriteReplyParams(
+      reply,
+      ppapi::proxy::SerializedHandle(
+          ppapi::proxy::SerializedHandle::FILE,
+          base::FileDescriptor(platform_file, true)));

[Mark Seaborn, 2014/04/30 21:20:58]

On error, platform_file will be PP_kInvalidFileHandle (-1).  It's not clear to
me whether that's valid to pass into the IPC layer.

Having test coverage for the error case would answer this. :-)

[hidehiko, 2014/05/01 05:20:31]

I think it's fine. Anyway, let's add a test case in a later CL.

[Mark Seaborn, 2014/05/01 06:32:35]

Hmm, as I read it, that means you're not sure whether the error path works? :-)

Note that in the current version of the change, this is exposed to untrusted
code in SFI NaCl.  In that situation, I'd want to be sure this works OK.  It
wouldn't be good for this to be broken temporarily, even if it got fixed later
-- the broken version might get included in a stable branch of Chrome.

However, see my other comment -- if we can disable this code path for SFI NaCl
as well as for PNaCl, I'm less worried about this being temporarily not fully
tested.

If you're going to extend the test coverage in a follow-on change, can you at
least test the error path manually, please, and say whether open_resource()
returns EINVAL OK?

[hidehiko, 2014/05/01 13:05:40]

I've already confirmed manually, and invoking open_resource() with non-existing
key returns ENOENT as expected. (note: not EINVAL).

+#endif
+  Send(reply);

[Mark Seaborn, 2014/04/30 21:20:58]

What are the implications of calling Send(Reply) without doing
WriteReplyParams() first, on Windows?

[hidehiko, 2014/05/01 05:20:31]

acknowledged.

+}
+
 }  // namespace nacl