Pepper: Stop using SRPC for irt_open_resource().

components/nacl/loader/nacl_ipc_adapter.cc

Index: components/nacl/loader/nacl_ipc_adapter.cc
diff --git a/components/nacl/loader/nacl_ipc_adapter.cc b/components/nacl/loader/nacl_ipc_adapter.cc
index 076594eff91f9a59a54047ff6ac88af5c10bc588..4de0e04440580fa5c6ccd139ce8d81d5a14bfeb7 100644
--- a/components/nacl/loader/nacl_ipc_adapter.cc
+++ b/components/nacl/loader/nacl_ipc_adapter.cc
@@ -12,6 +12,7 @@
 #include "base/location.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/shared_memory.h"
+#include "base/task_runner_util.h"
 #include "build/build_config.h"
 #include "ipc/ipc_channel.h"
 #include "ipc/ipc_platform_file.h"
@@ -24,6 +25,7 @@
 #include "native_client/src/trusted/desc/nacl_desc_sync_socket.h"
 #include "native_client/src/trusted/desc/nacl_desc_wrapper.h"
 #include "native_client/src/trusted/service_runtime/include/sys/fcntl.h"
+#include "native_client/src/trusted/validator/rich_file_info.h"
 #include "ppapi/c/ppb_file_io.h"
 #include "ppapi/proxy/ppapi_messages.h"
 #include "ppapi/proxy/serialized_handle.h"
@@ -329,6 +331,7 @@ NaClIPCAdapter::NaClIPCAdapter(const IPC::ChannelHandle& handle,
       cond_var_(&lock_),
       task_runner_(runner),
       locked_data_() {
+  main_task_runner_ = base::MessageLoop::current()->task_runner();
   io_thread_data_.channel_ = IPC::Channel::CreateServer(handle, this);
   // Note, we can not PostTask for ConnectChannelOnIOThread here. If we did,
   // and that task ran before this constructor completes, the reference count
@@ -465,6 +468,7 @@ int NaClIPCAdapter::TakeClientFileDescriptor() {
 
 bool NaClIPCAdapter::OnMessageReceived(const IPC::Message& msg) {
   uint32_t type = msg.type();
+
   if (type == IPC_REPLY_ID) {
     int id = IPC::SyncMessage::GetMessageId(msg);
     IOThreadData::PendingSyncMsgMap::iterator it =
@@ -475,7 +479,39 @@ bool NaClIPCAdapter::OnMessageReceived(const IPC::Message& msg) {
       io_thread_data_.pending_sync_msgs_.erase(it);
     }
   }
+  // Handle PpapiHostMsg_OpenResource outside the lock as it requires sending
+  // IPC to handle properly.

[dmichael (off chromium), 2014/08/27 20:49:14]

I know Mark strongly preferred knowledge of proxy messages stayed in
ppapi/proxy. (Arguably this one is more of an IRT than proxy thing, I guess.)

Also FYI...  Bill had written something to send a message while handling another
message. He had added a new "ChannelHandle" thing to SerializedHandle....  looks
like Hidehiko removed it
https://codereview.chromium.org/150713003/diff/390001/components/nacl/loader/...
a while back. I think more manual work is probably appropriate here though.

Now that you're doing Async messages, do you still care about doing this without
the lock? Maybe it would be fine to move this in to the NaClMessageScanner?

[Mark Seaborn, 2014/08/28 21:33:49]

Yeah, open_resource() isn't part of PPAPI, so it's more appropriate to have
handling of open_resource messages in a */nacl/* file than in ppapi/proxy/.  So
this part seems OK as it is.

+  if (type == PpapiHostMsg_OpenResource::ID) {
+    PickleIterator iter = IPC::SyncMessage::GetDataIterator(&msg);
+    ppapi::proxy::SerializedHandle ignore_fd;
+    uint64_t token_lo;
+    uint64_t token_hi;
+    if (!IPC::ReadParam(&msg, &iter, &ignore_fd) ||
+        !IPC::ReadParam(&msg, &iter, &token_lo) ||
+        !IPC::ReadParam(&msg, &iter, &token_hi)) {
+      return false;
+    }
+
+    if (token_lo != 0 || token_hi != 0) {
+      // We've received a valid file token, but we have to validate it with the
+      // browser before it can be used. This is to prevent a compromised
+      // renderer from running arbitrary code in the plugin process.

[Mark Seaborn, 2014/08/28 23:40:22]

"plugin process" -> "NaCl process" to clarify

[teravest, 2014/09/04 22:13:30]

Done.

+      DCHECK(!resolve_file_token_cb_.is_null());
+
+      // resolve_file_token_cb_ must be invoked from the main thread.
+      resolve_file_token_cb_.Run(
+          token_lo,
+          token_hi,
+          base::Bind(&NaClIPCAdapter::OnFileTokenResolved,
+                     this,
+                     msg));
+      return true;

[Mark Seaborn, 2014/08/28 23:40:22]

Maybe comment: We don't release the message to NaCl untrusted code immediately. 
We defer it until we get an async message back from the browser process.

[teravest, 2014/09/04 22:13:30]

Done.

+    }
+  }
+  return RewriteMessage(msg, type);
+}
 
+bool NaClIPCAdapter::RewriteMessage(const IPC::Message& msg, uint32_t type) {
   {
     base::AutoLock lock(lock_);
     scoped_refptr<RewrittenMessage> rewritten_msg(new RewrittenMessage);
@@ -556,6 +592,80 @@ bool NaClIPCAdapter::OnMessageReceived(const IPC::Message& msg) {
   return true;
 }
 
+void NaClIPCAdapter::OnFileTokenResolved(const IPC::Message& orig_msg,
+                                         IPC::PlatformFileForTransit ipc_fd,
+                                         base::FilePath file_path) {
+  if (ipc_fd != IPC::InvalidPlatformFileForTransit()) {

[Mark Seaborn, 2014/08/28 23:40:22]

Maybe handle the error case first for readability:

if (ipc_fd == IPC::InvalidPlatformFileForTransit()) {
  RewriteMessage(orig_msg, PpapiHostMsg_OpenResource::ID);
  return;
}

[teravest, 2014/09/04 22:13:30]

Done.

+    // The file token was sucessfully resolved.
+    std::string file_path_str = file_path.AsUTF8Unsafe();
+    base::PlatformFile handle =
+        IPC::PlatformFileForTransitToPlatformFile(ipc_fd);
+    int32_t new_fd;
+#if defined(OS_WIN)
+    // On Windows, valid handles are 32 bit unsigned integers so this is
+    // safe.
+    new_fd = reinterpret_cast<uintptr_t>(handle);
+#else
+    new_fd = handle;
+#endif
+    // The file token was resolved successfully, so we populate the new
+    // NaClDesc with that information.
+    char* alloc_file_path = static_cast<char*>(
+        malloc(file_path_str.length() + 1));
+    strcpy(alloc_file_path, file_path_str.c_str());
+    scoped_ptr<NaClDescWrapper> desc_wrapper(new NaClDescWrapper(
+        NaClDescIoDescFromHandleAllocCtor(
+            (NaClHandle) new_fd, NACL_ABI_O_RDONLY)));
+
+    // Mark the desc as OK for mmapping.
+    NaClDescMarkSafeForMmap(desc_wrapper->desc());
+
+    // Provide metadata for validation.
+    struct NaClRichFileInfo info;
+    NaClRichFileInfoCtor(&info);
+    info.known_file = 1;
+    info.file_path = alloc_file_path;  // Takes ownership.
+    info.file_path_length =
+        static_cast<uint32_t>(file_path_str.length());
+    NaClSetFileOriginInfo(desc_wrapper->desc(), &info);
+    NaClRichFileInfoDtor(&info);
+    {
+      scoped_ptr<IPC::Message> new_msg(new IPC::Message(
+          orig_msg.routing_id(),
+          orig_msg.type(),
+          IPC::Message::PRIORITY_NORMAL));
+
+      scoped_refptr<RewrittenMessage> rewritten_msg(new RewrittenMessage);
+      // We have to rewrite the message a bit here to clear the file token
+      // data.
+      new_msg->set_reply();
+      new_msg->WriteInt(IPC::SyncMessage::GetMessageId(orig_msg));
+
+      // Write the SerializedHandle. There's only ever one in this message, so
+      // we just write an index of zero.
+      ppapi::proxy::SerializedHandle sh;
+      sh.set_file_handle(ipc_fd, PP_FILEOPENFLAG_READ, 0);
+      ppapi::proxy::SerializedHandle::WriteHeader(sh.header(),
+                                                  new_msg.get());
+      new_msg->WriteBool(true);
+      new_msg->WriteInt(0);
+
+      // Write empty file tokens.
+      new_msg->WriteUInt64(0);
+      new_msg->WriteUInt64(0);
+
+      rewritten_msg->AddDescriptor(desc_wrapper.release());
+      {
+        base::AutoLock lock(lock_);
+        SaveMessage(*new_msg, rewritten_msg.get());
+      }
+    }
+    cond_var_.Signal();
+  } else {
+    RewriteMessage(orig_msg, PpapiHostMsg_OpenResource::ID);
+  }
+}
+
 void NaClIPCAdapter::OnChannelConnected(int32 peer_pid) {
 }