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

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

After r200088, NACL_ABI_O_CREAT, NACL_ABI_O_TRUNC, and NACL_ABI_O_EXCL are passed to NaClDescIoDescFromHandleAllocCtor.
But the function does not expect these flags and that causes NaCl to abort.
This CL changes NaClIPCAdapter so that is does not translate the file open flags other than PP_FILEOPENFLAG_READ and PP_FILEOPENFLAG_WRITE.

BUG=241726
TEST=unit_tests --gtest_filter="NaClIPCAdapterTest.*"

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202027

[mazda, 2013-05-17 08:55:33]

Hi,
Could you review this change?

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

https://codereview.chromium.org/15274002/diff/1/chrome/nacl/nacl_ipc_adapter.cc
File chrome/nacl/nacl_ipc_adapter.cc (right):

https://codereview.chromium.org/15274002/diff/1/chrome/nacl/nacl_ipc_adapter....
chrome/nacl/nacl_ipc_adapter.cc:122: #if 0
Don't disable code with "#if 0" -- why don't you just remove the code?

Can you add a test for O_EXCL?

[victorhsieh, 2013-05-17 16:05:47]

Is there any plan to support the flag in NaCl?  I wonder if it's better to fix
it from NaCl side as it's a NACL_ABI flag.

[Mark Seaborn, 2013-05-17 16:21:24]

On 17 May 2013 09:05, <victorhsieh@chromium.org> wrote:

> Is there any plan to support the flag in NaCl?  I wonder if it's better to
> fix
> it from NaCl side as it's a NACL_ABI flag.
>

O_EXCL is an open-time-only flag.  It's not relevant after a file is
opened, so since the PPAPI layer is responsible for opening the file (not
NaCl), presumably it doesn't make sense to pass the flag through to NaCl.

Cheers,
Mark

[Mark Seaborn, 2013-05-17 16:21:33]

On 17 May 2013 09:05, <victorhsieh@chromium.org> wrote:

> Is there any plan to support the flag in NaCl?  I wonder if it's better to
> fix
> it from NaCl side as it's a NACL_ABI flag.
>

O_EXCL is an open-time-only flag.  It's not relevant after a file is
opened, so since the PPAPI layer is responsible for opening the file (not
NaCl), presumably it doesn't make sense to pass the flag through to NaCl.

Cheers,
Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.

[victorhsieh, 2013-05-17 16:36:58]

lgtm

I see.  Make sense to me.

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

https://codereview.chromium.org/15274002/diff/1/chrome/nacl/nacl_ipc_adapter.cc
File chrome/nacl/nacl_ipc_adapter.cc (right):

https://codereview.chromium.org/15274002/diff/1/chrome/nacl/nacl_ipc_adapter....
chrome/nacl/nacl_ipc_adapter.cc:126: nacl_open_flag |= NACL_ABI_O_EXCL;
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.

[bsy, 2013-05-17 17:35:02]

https://codereview.chromium.org/15274002/diff/1/chrome/nacl/nacl_ipc_adapter.cc
File chrome/nacl/nacl_ipc_adapter.cc (right):

https://codereview.chromium.org/15274002/diff/1/chrome/nacl/nacl_ipc_adapter....
chrome/nacl/nacl_ipc_adapter.cc:99: int TranslatePepperFileOpenFlags(int32_t
pp_open_flags) {
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]

PTAL

https://codereview.chromium.org/15274002/diff/1/chrome/nacl/nacl_ipc_adapter.cc
File chrome/nacl/nacl_ipc_adapter.cc (right):

https://codereview.chromium.org/15274002/diff/1/chrome/nacl/nacl_ipc_adapter....
chrome/nacl/nacl_ipc_adapter.cc:99: int TranslatePepperFileOpenFlags(int32_t
pp_open_flags) {
On 2013/05/17 17:35:03, bsy wrote:
> 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.

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

https://codereview.chromium.org/15274002/diff/1/chrome/nacl/nacl_ipc_adapter....
chrome/nacl/nacl_ipc_adapter.cc:122: #if 0
On 2013/05/17 14:02:35, Mark Seaborn wrote:
> Don't disable code with "#if 0" -- why don't you just remove the code?
> 
> Can you add a test for O_EXCL?

Deleted the code and added a test.

https://codereview.chromium.org/15274002/diff/1/chrome/nacl/nacl_ipc_adapter....
chrome/nacl/nacl_ipc_adapter.cc:126: nacl_open_flag |= NACL_ABI_O_EXCL;
On 2013/05/17 16:38:52, bsy wrote:
> 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.

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

[bsy, 2013-05-20 22:40:09]

red bots.

[mazda, 2013-05-21 00:40:21]

On 2013/05/20 22:40:09, bsy wrote:
> red bots.

It looks the failure of linux_rel is not related to my change and now it's
green.
win_rel is due to one of flake tests, which is disabled now
(https://src.chromium.org/viewvc/chrome?revision=200907&view=revision).
The failures of linux_rel:swarm_triggered and win_rel:swarm_triggered seem to be
a known issue of test infrastructure.
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/erppzhd1TH4/OeiHK...

[mazda, 2013-05-21 18:00:57]

On 2013/05/21 00:40:21, mazda wrote:
> On 2013/05/20 22:40:09, bsy wrote:
> > red bots.
win_rel is also green now. PTAL.

[bsy, 2013-05-21 18:46:50]

lgtm for nacl code usage

[mazda, 2013-05-21 22:34:57]

mseaborn@
Could you do an OWNERS review for chrome/nacl?
bradnelson seems OOO.

[bradn, 2013-05-23 04:06:08]

LGTM

But alter (in change description):
This CL makes CaClIPCAdapter not to translate the file open flags other than
PP_FILEOPENFLAG_READ and PP_FILEOPENFLAG_WRITE. 

to:
This CL changes NaClIPCAdapter so that is does not translate the file open flags
other than PP_FILEOPENFLAG_READ and PP_FILEOPENFLAG_WRITE.

[mazda, 2013-05-23 08:16:57]

On 2013/05/23 04:06:08, bradn wrote:
> LGTM
> 
> But alter (in change description):
> This CL makes CaClIPCAdapter not to translate the file open flags other than
> PP_FILEOPENFLAG_READ and PP_FILEOPENFLAG_WRITE. 
> 
> to:
> This CL changes NaClIPCAdapter so that is does not translate the file open
flags
> other than PP_FILEOPENFLAG_READ and PP_FILEOPENFLAG_WRITE.

I modified the description. Thanks.

[commit-bot: I haz the power, 2013-05-23 08:21:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mazda@chromium.org/15274002/18001

[commit-bot: I haz the power, 2013-05-23 08:30:58]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[bradnelson, 2013-05-23 23:11:44]

lgtm

[commit-bot: I haz the power, 2013-05-23 23:23:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mazda@chromium.org/15274002/18001

[commit-bot: I haz the power, 2013-05-24 09:21:02]

Change committed as 202027