Rename HandleConverter to NaClMessageScanner.

ppapi/proxy/nacl_message_scanner.cc

Index: ppapi/proxy/nacl_message_scanner.cc
diff --git a/ppapi/proxy/handle_converter.cc b/ppapi/proxy/nacl_message_scanner.cc
similarity index 45%
rename from ppapi/proxy/handle_converter.cc
rename to ppapi/proxy/nacl_message_scanner.cc
index 587585b53b6e449311721e83f120031beabd7395..fb1716f14fb5cdd1996c340fdbef5e98c10dd2f8 100644
--- a/ppapi/proxy/handle_converter.cc
+++ b/ppapi/proxy/nacl_message_scanner.cc
@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "ppapi/proxy/handle_converter.h"
+#include "ppapi/proxy/nacl_message_scanner.h"
 
 #include <vector>
 #include "base/bind.h"
@@ -21,28 +21,35 @@ class Message;
 
 namespace {
 
+typedef std::vector<ppapi::proxy::SerializedHandle> Handles;
+
+struct ScanOutput {

[dmichael (off chromium), 2013/10/31 18:21:33]

nit: This name is ambiguous; it also sounds like an action ("Scan the output"),
when it's really the result of the scan. Maybe ResultOfScan, ScanningResult, or
something would be clearer?

[bbudge, 2013/11/01 00:34:45]

I thought about ScanningContext or even just Context, but since this is mostly
for output, I think ScanningResults is best for now. Done.

+  ScanOutput() : handle_index(0) {}
+
+  Handles handles;  // Vector to hold handles found in the message.
+  int handle_index;  // Current handle index in the output message.
+  scoped_ptr<IPC::Message> msg;  // Output message, possibly NULL.

[dmichael (off chromium), 2013/10/31 18:21:33]

I think it would be nice to be super-duper clear about the NULL thing. I.e.,
// Output message. This could be NULL, so all ScanParam overloads must
// check for NULL before writing to |msg|. In some cases, a ScanParam
// overload may set |msg| to NULL to indicate that there are no
// parameters that need conversion. (See ResourceMessageReplyParams
// overload).

Or something. It's pretty tricky, so may be worthy of a long comment.

[bbudge, 2013/11/01 00:34:45]

Done. Also renamed the field to make it a little clearer.

+};
+
 void WriteHandle(int handle_index,
                  const ppapi::proxy::SerializedHandle& handle,
-                 IPC::Message* message) {
-  ppapi::proxy::SerializedHandle::WriteHeader(handle.header(), message);
+                 IPC::Message* msg) {
+  ppapi::proxy::SerializedHandle::WriteHeader(handle.header(), msg);
 
   // Now write the handle itself in POSIX style.
-  message->WriteBool(true);  // valid == true
-  message->WriteInt(handle_index);
+  msg->WriteBool(true);  // valid == true
+  msg->WriteInt(handle_index);
 }
 
-typedef std::vector<ppapi::proxy::SerializedHandle> Handles;
+// Define overloads for each kind of message parameter that requires special
+// handling. See ScanTuple for how these get used.
 
-// We define overloads for catching SerializedHandles so that we can share
-// them correctly to the untrusted side.
-// See ConvertHandlesImpl for how these get used.
-void ConvertHandlesInParam(const ppapi::proxy::SerializedHandle& handle,
-                           Handles* handles,
-                           IPC::Message* msg,
-                           int* handle_index) {
-  handles->push_back(handle);
-  if (msg)
-    WriteHandle((*handle_index)++, handle, msg);
+// Overload to match SerializedHandle.
+void ScanParam(const ppapi::proxy::SerializedHandle& handle,
+               ScanOutput* output) {
+  output->handles.push_back(handle);
+  if (output->msg)
+    WriteHandle(output->handle_index++, handle, output->msg.get());
 }
 
 void HandleWriter(int* handle_index,
@@ -51,129 +58,116 @@ void HandleWriter(int* handle_index,
   WriteHandle((*handle_index)++, handle, m);
 }
 
-void ConvertHandlesInParam(const ppapi::proxy::SerializedVar& var,
-                           Handles* handles,
-                           IPC::Message* msg,
-                           int* handle_index) {
+// Overload to match SerializedVar, which can contain handles.
+void ScanParam(const ppapi::proxy::SerializedVar& var,
+               ScanOutput* output) {
   std::vector<ppapi::proxy::SerializedHandle*> var_handles = var.GetHandles();
   if (var_handles.empty())
     return;

[bbudge, 2013/11/01 00:34:45]

I removed the early out. If another SerializedVar param does contain handles,
the rewritten message will be missing data.

[dmichael (off chromium), 2013/11/01 19:35:16]

Agreed. I missed that on my last review. I think we'd be okay in practice right
now, but it will bite us eventually if we break early here.

 
   for (size_t i = 0; i < var_handles.size(); ++i)
-    handles->push_back(*var_handles[i]);
-  if (msg)
-    var.WriteDataToMessage(msg, base::Bind(&HandleWriter, handle_index));
+    output->handles.push_back(*var_handles[i]);
+  if (output->msg)
+    var.WriteDataToMessage(output->msg.get(),
+                           base::Bind(&HandleWriter, &output->handle_index));
 }
 
 // For PpapiMsg_ResourceReply and the reply to PpapiHostMsg_ResourceSyncCall,
 // the handles are carried inside the ResourceMessageReplyParams.
-// NOTE: We only translate handles from host->NaCl. The only kind of
+// NOTE: We only intercept handles from host->NaCl. The only kind of
 //       ResourceMessageParams that travels this direction is
 //       ResourceMessageReplyParams, so that's the only one we need to handle.
-void ConvertHandlesInParam(
-    const ppapi::proxy::ResourceMessageReplyParams& params,
-    Handles* handles,
-    IPC::Message* msg,
-    int* handle_index) {
+void ScanParam(const ppapi::proxy::ResourceMessageReplyParams& params,
+               ScanOutput* output) {
+  // If the resource reply doesn't contain any handles, it doesn't have to be
+  // rewritten.

[dmichael (off chromium), 2013/10/31 18:21:33]

Maybe note that this only works because ResourceMessageReply *only* carries
handles inside ResourceMessageReplyParams. Otherwise, we might have to collect
handles from other params.

Also...  as we start looking at other parameters (e.g. for quota), the comment
might need to be more general. We need to be sure *no* parameters have to be
changed in flight in order for this optimization to work. I think that's only
handles for the foreseeable future, though, so maybe I'm being paranoid :-)

[bbudge, 2013/11/01 00:34:45]

Done.

+  if (params.handles().empty())
+    output->msg.reset(NULL);

[dmichael (off chromium), 2013/10/31 18:21:33]

you could return early here, I think. Doesn't appear to matter, but might make
the intent clearer.

[bbudge, 2013/11/01 00:34:45]

Done.

+
   // First, if we need to rewrite the message parameters, write everything
   // before the handles (there's nothing after the handles).
-  if (msg) {
-    params.WriteReplyHeader(msg);
+  if (output->msg) {
+    params.WriteReplyHeader(output->msg.get());
     // IPC writes the vector length as an int before the contents of the
     // vector.
-    msg->WriteInt(static_cast<int>(params.handles().size()));
+    output->msg.get()->WriteInt(static_cast<int>(params.handles().size()));
   }
   for (Handles::const_iterator iter = params.handles().begin();
        iter != params.handles().end();
        ++iter) {
     // ConvertHandle will write each handle to |msg|, if necessary.
-    ConvertHandlesInParam(*iter, handles, msg, handle_index);
+    ScanParam(*iter, output);
   }
   // Tell ResourceMessageReplyParams that we have taken the handles, so it
   // shouldn't close them. The NaCl runtime will take ownership of them.
   params.ConsumeHandles();
 }
 
-// This overload is to catch all types other than SerializedHandle or
-// ResourceMessageReplyParams. On Windows, |msg| will be a valid pointer, and we
-// must write |param| to it.
+// Overload to match all other types. If the output msg pointer is not NULL,
+// write the parameter.
 template <class T>
-void ConvertHandlesInParam(const T& param,
-                           Handles* /* handles */,
-                           IPC::Message* msg,
-                           int* /* handle_index */) {
-  // It's not a handle, so just write to the output message, if necessary.
-  if (msg)
-    IPC::WriteParam(msg, param);
+void ScanParam(const T& param,
+               ScanOutput* output) {
+  if (output->msg)
+    IPC::WriteParam(output->msg.get(), param);
 }
 
-// These just break apart the given tuple and run ConvertHandle over each param.
-// The idea is to extract any handles in the tuple, while writing all data to
-// msg (if msg is valid). The msg will only be valid on Windows, where we need
-// to re-write all of the message parameters, writing the handles in POSIX style
-// for NaCl.
+// These just break apart the given tuple and run ScanParam over each param.
+// The idea is to scan elements in the tuple which require special handling,
+// and write any output data into the output struct.
 template <class A>
-void ConvertHandlesImpl(const Tuple1<A>& t1, Handles* handles,
-                        IPC::Message* msg) {
-  int handle_index = 0;
-  ConvertHandlesInParam(t1.a, handles, msg, &handle_index);
+void ScanTuple(const Tuple1<A>& t1, ScanOutput* output) {
+  ScanParam(t1.a, output);
 }
 template <class A, class B>
-void ConvertHandlesImpl(const Tuple2<A, B>& t1, Handles* handles,
-                        IPC::Message* msg) {
-  int handle_index = 0;
-  ConvertHandlesInParam(t1.a, handles, msg, &handle_index);
-  ConvertHandlesInParam(t1.b, handles, msg, &handle_index);
+void ScanTuple(const Tuple2<A, B>& t1, ScanOutput* output) {
+  ScanParam(t1.a, output);
+  ScanParam(t1.b, output);
 }
 template <class A, class B, class C>
-void ConvertHandlesImpl(const Tuple3<A, B, C>& t1, Handles* handles,
-                        IPC::Message* msg) {
-  int handle_index = 0;
-  ConvertHandlesInParam(t1.a, handles, msg, &handle_index);
-  ConvertHandlesInParam(t1.b, handles, msg, &handle_index);
-  ConvertHandlesInParam(t1.c, handles, msg, &handle_index);
+void ScanTuple(const Tuple3<A, B, C>& t1, ScanOutput* output) {
+  ScanParam(t1.a, output);
+  ScanParam(t1.b, output);
+  ScanParam(t1.c, output);
 }
 template <class A, class B, class C, class D>
-void ConvertHandlesImpl(const Tuple4<A, B, C, D>& t1, Handles* handles,
-                        IPC::Message* msg) {
-  int handle_index = 0;
-  ConvertHandlesInParam(t1.a, handles, msg, &handle_index);
-  ConvertHandlesInParam(t1.b, handles, msg, &handle_index);
-  ConvertHandlesInParam(t1.c, handles, msg, &handle_index);
-  ConvertHandlesInParam(t1.d, handles, msg, &handle_index);
+void ScanTuple(const Tuple4<A, B, C, D>& t1, ScanOutput* output) {
+  ScanParam(t1.a, output);
+  ScanParam(t1.b, output);
+  ScanParam(t1.c, output);
+  ScanParam(t1.d, output);
 }
 
 template <class MessageType>
-class HandleConverterImpl {
+class MessageScannerImpl {
  public:
-  explicit HandleConverterImpl(const IPC::Message* msg)
+  explicit MessageScannerImpl(const IPC::Message* msg)
       : msg_(static_cast<const MessageType*>(msg)) {
   }
-  bool ConvertMessage(Handles* handles, IPC::Message* out_msg) {
+  bool ScanMessage(ScanOutput* output) {
     typename TupleTypes<typename MessageType::Schema::Param>::ValueTuple params;
     if (!MessageType::Read(msg_, &params))
       return false;
-    ConvertHandlesImpl(params, handles, out_msg);
+    ScanTuple(params, output);
     return true;
   }
 
-  bool ConvertReply(Handles* handles, IPC::SyncMessage* out_msg) {
+  bool ScanReply(ScanOutput* output) {
     typename TupleTypes<typename MessageType::Schema::ReplyParam>::ValueTuple
         params;
     if (!MessageType::ReadReplyParam(msg_, &params))
       return false;
-    // If we need to rewrite the message (i.e., on Windows), we need to make
-    // sure we write the message id first.
-    if (out_msg) {
-      out_msg->set_reply();
+    // If we need to rewrite the message, we need to write the message id first.
+    if (output->msg) {
+      output->msg.get()->set_reply();

[dmichael (off chromium), 2013/10/31 18:21:33]

Why use get() here?

[bbudge, 2013/11/01 00:34:45]

Done.

       int id = IPC::SyncMessage::GetMessageId(*msg_);
-      out_msg->WriteInt(id);
+      output->msg.get()->WriteInt(id);

[dmichael (off chromium), 2013/10/31 18:21:33]

ditto?

[bbudge, 2013/11/01 00:34:45]

Done.

     }
-    ConvertHandlesImpl(params, handles, out_msg);
+    ScanTuple(params, output);
     return true;
   }
-  // TODO(dmichael): Add ConvertSyncMessage for outgoing sync messages, if we
-  //                 ever pass handles in one of those.
+  // TODO(dmichael): Add ScanSyncMessage for outgoing sync messages, if we ever
+  //                 need to scan those.
 
  private:
   const MessageType* msg_;
@@ -183,17 +177,19 @@ class HandleConverterImpl {
 
 #define CASE_FOR_MESSAGE(MESSAGE_TYPE) \
       case MESSAGE_TYPE::ID: { \
-        HandleConverterImpl<MESSAGE_TYPE> extractor(&msg); \
-        if (!extractor.ConvertMessage(handles, new_msg_ptr->get())) \
+        MessageScannerImpl<MESSAGE_TYPE> scanner(&msg); \
+        if (rewrite_msg) \
+          output.msg.reset(new IPC::Message(msg.routing_id(), msg.type())); \
+        if (!scanner.ScanMessage(&output)) \
           return false; \
         break; \
       }
 #define CASE_FOR_REPLY(MESSAGE_TYPE) \
       case MESSAGE_TYPE::ID: { \
-        HandleConverterImpl<MESSAGE_TYPE> extractor(&msg); \
-        if (!extractor.ConvertReply( \
-                handles, \
-                static_cast<IPC::SyncMessage*>(new_msg_ptr->get()))) \
+        MessageScannerImpl<MESSAGE_TYPE> scanner(&msg); \
+        if (rewrite_msg) \
+          output.msg.reset(new IPC::Message(msg.routing_id(), msg.type())); \
+        if (!scanner.ScanReply(&output)) \
           return false; \
         break; \
       }
@@ -203,40 +199,35 @@ namespace proxy {
 
 class SerializedHandle;
 
-HandleConverter::HandleConverter() {
+NaClMessageScanner::NaClMessageScanner() {
 }
 
-bool HandleConverter::ConvertNativeHandlesToPosix(
+bool NaClMessageScanner::ScanMessage(
     const IPC::Message& msg,
     std::vector<SerializedHandle>* handles,
     scoped_ptr<IPC::Message>* new_msg_ptr) {
   DCHECK(handles);
+  DCHECK(handles->empty());
   DCHECK(new_msg_ptr);
   DCHECK(!new_msg_ptr->get());
 
-  // In Windows, we need to re-write the contents of the message. This is
-  // because in Windows IPC code, native HANDLE values are serialized in the
-  // body of the message.
-  //
-  // In POSIX, we only serialize an index in to a FileDescriptorSet, and the
-  // actual file descriptors are sent out-of-band. So on Windows, to make a
-  // message that's compatible with Windows, we need to write a new message that
-  // has simple indices in the message body instead of the HANDLEs.
+  // On Windows, we must rewrite all IPC messages with handles, because native
+  // HANDLE values are serialized in the body of the message. In POSIX, we only
+  // serialize an index into a FileDescriptorSet, and the actual file
+  // descriptors are sent out-of-band. Note that any rewritten message must be
+  // received in a POSIX environment (in this case, NaCl).

[dmichael (off chromium), 2013/10/31 18:21:33]

"must be received in"
->
"will be in a format compatible with"

or something. I found the comment a little confusing as written, but I'm easily
confused :)

[bbudge, 2013/11/01 00:34:45]

I completely rewrote this. The comment is now above the method definition.

   //
-  // NOTE: This means on Windows, new_msg_ptr's serialized contents are not
-  // compatible with Windows IPC deserialization code; it is intended to be
-  // passed to NaCl.
+  // On POSIX, we rewrite PpapiMsg_CreateNaClChannel messages, because these
+  // contain a handle with an invalid (place holder) descriptor. The message
+  // sending code sees this and doesn't pass the descriptor over correctly.
+  bool rewrite_msg =
 #if defined(OS_WIN)
-  new_msg_ptr->reset(new IPC::Message(msg.routing_id(), msg.type()));
+      true;
 #else
-  // Even on POSIX, we have to rewrite messages to create channels, because
-  // these contain a handle with an invalid (place holder) descriptor. The
-  // message sending code sees this and doesn't pass the descriptor over
-  // correctly.
-  if (msg.type() == PpapiMsg_CreateNaClChannel::ID)
-    new_msg_ptr->reset(new IPC::Message(msg.routing_id(), msg.type()));
+      (msg.type() == PpapiMsg_CreateNaClChannel::ID);
 #endif
 
+  ScanOutput output;
   switch (msg.type()) {
     CASE_FOR_MESSAGE(PpapiMsg_CreateNaClChannel)
     CASE_FOR_MESSAGE(PpapiMsg_PPBAudio_NotifyAudioStreamCreated)
@@ -266,10 +257,17 @@ bool HandleConverter::ConvertNativeHandlesToPosix(
       // Do nothing for messages we don't know.
       break;
   }
+
+  // If no handles were found in the message, then it doesn't need to be
+  // translated, so skip writing any outputs in this case.
+  if (!output.handles.empty()) {

[dmichael (off chromium), 2013/10/31 18:21:33]

We probably fall in here in the PPPMessaging_HandleMessage case. Vars only
*sometimes* contain a handle :-p

[bbudge, 2013/11/01 00:34:45]

I think the "early out" in the SerializedVar overload may be the cause of this.
Apparently this works OK now but if we someday need to rewrite messages with
multiple SerializedVars, the early out won't work.

+    handles->swap(output.handles);
+    *new_msg_ptr = output.msg.Pass();
+  }
   return true;
 }
 
-void HandleConverter::RegisterSyncMessageForReply(const IPC::Message& msg) {
+void NaClMessageScanner::RegisterSyncMessageForReply(const IPC::Message& msg) {
   DCHECK(msg.is_sync());
 
   int msg_id = IPC::SyncMessage::GetMessageId(msg);