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..913253f3adca2b2972783ac44162a541a8e546a1 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,43 @@ 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.
+  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 NaCl process.
+      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));
+
+      // In this case, 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.
+      return true;
+    }
+  }
+  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 +596,81 @@ 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()) {
+    RewriteMessage(orig_msg, PpapiHostMsg_OpenResource::ID);

[teravest, 2014/09/04 22:22:41]

The failure on Windows is that we can't just rewrite the original message in
this case; the file tokens have to be cleared to zero.

After fixing this, I can add some DCHECKs() in manifest_service.cc to check that
we always return zero values for file tokens.

[Mark Seaborn, 2014/09/05 15:43:46]

OK, I'll look again after you've done that.

BTW, a CHECK might be better than a DCHECK.  It's good to minimise differences
between release/debug builds, since the bots usually run release builds.

I think Chromium might enable DCHECKs on the _rel trybots, but I don't know if
that works for NaCl untrusted code.

+    return;
+  }
+
+  // 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();
+}
+
 void NaClIPCAdapter::OnChannelConnected(int32 peer_pid) {
 }