Do not translate the file open flags other than read/write.

chrome/nacl/nacl_ipc_adapter.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/nacl/nacl_ipc_adapter.h"
 
 #include <limits.h>
 #include <string.h>
 
 #include "base/basictypes.h"
@@ skipping 78 matching lines @@
   funcs.SendMsg = NaClDescCustomSendMsg;
   funcs.RecvMsg = NaClDescCustomRecvMsg;
   // NaClDescMakeCustomDesc gives us a reference on the returned NaClDesc.
   return NaClDescMakeCustomDesc(new DescThunker(adapter), &funcs);
 }
 
 void DeleteChannel(IPC::Channel* channel) {
   delete channel;
 }
 
 int TranslatePepperFileOpenFlags(int32_t pp_open_flags) {

[bsy, 2013/05/17 17:35:03]

If the intention is to use this only for NaClDescIoDescFromHandleAllocCtor and
never for a "raw" open like NaClDescIoDescOpen, then please rename this since
the name makes it sound like it'd be usable in that scenario.  Otherwise, there
should be another function to filter out the irrelevant flag bits for uses such
as on line 452 below.

[mazda, 2013/05/17 22:09:55]

Renamed TranslatePepperFileOpenFlags to TranslatePepperFileReadWriteOpenFlags
and added comments in order to make it clear that the function only translate
read/write open flags.

   int nacl_open_flag;
   switch (pp_open_flags & (PP_FILEOPENFLAG_READ | PP_FILEOPENFLAG_WRITE)) {
     case PP_FILEOPENFLAG_READ:
       nacl_open_flag = NACL_ABI_O_RDONLY;
       break;
     case PP_FILEOPENFLAG_WRITE:
       nacl_open_flag = NACL_ABI_O_WRONLY;
       break;
     case PP_FILEOPENFLAG_READ | PP_FILEOPENFLAG_WRITE:
       nacl_open_flag = NACL_ABI_O_RDWR;
       break;
     default:
       // NACL_ABI_O_RDONLY == 0, so make this ambiguous case readonly as a safe
       // fallback.
       nacl_open_flag = NACL_ABI_O_RDONLY;
       break;
   }
 
   if (pp_open_flags & PP_FILEOPENFLAG_CREATE)
     nacl_open_flag |= NACL_ABI_O_CREAT;
   if (pp_open_flags & PP_FILEOPENFLAG_TRUNCATE)
     nacl_open_flag |= NACL_ABI_O_TRUNC;
+#if 0

[Mark Seaborn, 2013/05/17 14:02:35]

Don't disable code with "#if 0" -- why don't you just remove the code?

Can you add a test for O_EXCL?

[mazda, 2013/05/17 22:09:55]

Deleted the code and added a test.

+  // NaCl does not support NACL_ABI_O_EXCL yet and sending the flag can cause
+  // NaCl module to crash, so we discard the flag here for now.
   if (pp_open_flags & PP_FILEOPENFLAG_EXCLUSIVE)
     nacl_open_flag |= NACL_ABI_O_EXCL;

[bsy, 2013/05/17 16:38:52]

TranslatePepperFileOpenFlags is used at line 452 below, to generate an argument
to NaClDescIoDescFromHandleAllocCtor.  The comment above that function in the
header file says:

/*
 * NaClDescIoDescFromHandleAllocCtor takes ownership of the |handle|
 * argument -- when the returned NaClDesc object is destroyed
 * (refcount goes to 0), the handle will be closed.  The |flags|
 * argument should be one of NACL_ABI_O_RDONLY, NACL_ABI_O_RDWR,
 * NACL_ABI_O_WRONLY possibly bitwise ORed with NACL_ABI_O_APPEND.
 */
struct NaClDesc *NaClDescIoDescFromHandleAllocCtor(NaClHandle handle,
                                                   int flags);

and NACL_ABI_O_EXCL is not allowed.  The same applies to NACL_ABI_O_TRUNC,
NACL_ABI_O_CREAT, and similar flags.

Exclusive open and truncation do not apply to already opened descriptors. 
Exclusive open is enforced by the underlying host OS -- the semantics are that
we will, when used with NACL_ABI_O_CREAT,  ensure that the file is created if
the file does not already exist, and will otherwise fail.  It clearly makes no
sense for an already-opened descriptor, since the creation had already happened.

Truncation happens at the time of the open.  Clearly, it makes no sense to ask
for it when passing ownership of the descriptor, because we wouldn't want to
truncate the file again.

Please change this to follow the contract -- do not include flags that are not
mentioned.

On the NaCl side, we will modify the interface to check and abort earlier
if/when unexpected/illegal flags are supplied.

[mazda, 2013/05/17 22:09:55]

Changed the code not to include NACL_ABI_O_EXCL, NACL_ABI_O_TRUNC, and
NACL_ABI_O_CREAT.

+#endif
   return nacl_open_flag;
 }
 
 class NaClDescWrapper {
  public:
   explicit NaClDescWrapper(NaClDesc* desc): desc_(desc) {}
   ~NaClDescWrapper() {
     NaClDescUnref(desc_);
   }
 
@@ skipping 443 matching lines @@
 
   header.payload_size = static_cast<uint32>(msg.payload_size());
   header.routing = msg.routing_id();
   header.type = msg.type();
   header.flags = msg.flags();
   header.num_fds = static_cast<int>(rewritten_msg->desc_count());
 
   rewritten_msg->SetData(header, msg.payload(), msg.payload_size());
   locked_data_.to_be_received_.push(rewritten_msg);
 }