Non-SFI mode: Quick workaround of unexpected CHECK failure.

Non-SFI mode: Quick workaround of unexpected CHECK failure.

Recently, crrev.com/418423002 is landed, but it has a bug in Non-SFI mode.
It introduces CHECK for the file token in ManifestService::OpenResource().
However, in Non-SFI mode, there is no NaClIPCAdapter, so the token is passed
from the renderer directly. (Actually the IPC channel is connected directly
to the renderer).
As a result, if the renderer fills the file token properly, it crashes.
As far as I investigated, it happens, at least, when the fast-path is triggered
(i.e. OpenNaClExecutable works in DownloadFile in ppb_nacl_private_impl.cc).
Anyway, we can ignore file tokens in Non-SFI mode, because it is for
SFI NaCl's validation cache.

BUG=394130
TEST=Ran trybots. Patched locally and run our Non-SFI NaCl app.
CQ_EXTRA_TRYBOTS=tryserver.chromium.linux:linux_rel_precise32

Committed: https://crrev.com/b16b57b17667f75ee80b3abe5dd59529bc48a8e5
Cr-Commit-Position: refs/heads/master@{#294396}

[hidehiko, 2014-09-11 05:17:03]

hidehiko@chromium.org changed reviewers:
+ teravest@chromium.org

[hidehiko, 2014-09-11 05:17:04]

Hi Justin,

crrev.com/418423002 breaks Non-SFI mode.
It triggers CHECK failure in ManifestService::OpenResource, which causes
seccomp-bpf failure with rt_sigprocmask invocation.
We need to fix it ASAP. This CL is a quick workaround for the crash. I'm not
sure this works for you well. If not, please revert your CL for now.

Also, the failure case does not seem to be covered by browser_tests, which is
irt_manifest testing with the fast-path triggering. Could you kindly add the
case for both SFI and Non-SFI mode?

Thanks!
- hidehiko

[teravest, 2014-09-11 14:57:26]

lgtm

[teravest, 2014-09-11 14:58:32]

The CQ bit was checked by teravest@chromium.org

[commit-bot: I haz the power, 2014-09-11 14:59:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/560983003/1

[Mark Seaborn, 2014-09-11 15:04:41]

This is only good as a temporary fix, though.

This exposes the token values to untrusted code in Non-SFI Mode, which isn't
ideal.  It would be better never to generate them in Non-SFI Mode.  In Non-SFI
Mode, can we get the renderer to skip its request to the browser for generating
the token?  Without skipping this, open_resource() will be slower than
necessary, and some excess handles/FDs will be kept around in the browser
process's token mapping (since the tokens will never be claimed).

Can you also add a test case to cover open_resource() in Non-SFI Mode with a
packaged app?

[commit-bot: I haz the power, 2014-09-11 15:59:18]

Committed patchset #1 (id:1) as 968f0cba660c61c8f440b181262532da4090458e

[teravest, 2014-09-11 16:02:20]

On 2014/09/11 05:17:04, hidehiko wrote:
> Hi Justin,
> 
> crrev.com/418423002 breaks Non-SFI mode.
> It triggers CHECK failure in ManifestService::OpenResource, which causes
> seccomp-bpf failure with rt_sigprocmask invocation.
> We need to fix it ASAP. This CL is a quick workaround for the crash. I'm not
> sure this works for you well. If not, please revert your CL for now.
> 
> Also, the failure case does not seem to be covered by browser_tests, which is
> irt_manifest testing with the fast-path triggering. Could you kindly add the
> case for both SFI and Non-SFI mode?
> 
> Thanks!
> - hidehiko

What do you mean by "the failure case" does not seem to be covered? I don't
understand.
I believe the fast-path case is tested for SFI mode, but a test needs to be
added for Non-SFI mode.

[teravest, 2014-09-11 16:03:19]

On 2014/09/11 15:04:41, Mark Seaborn wrote:
> This is only good as a temporary fix, though.
> 
> This exposes the token values to untrusted code in Non-SFI Mode, which isn't
> ideal.  It would be better never to generate them in Non-SFI Mode.  In Non-SFI
> Mode, can we get the renderer to skip its request to the browser for
generating
> the token?

Yes, I think that should be pretty easy.

> Without skipping this, open_resource() will be slower than
> necessary, and some excess handles/FDs will be kept around in the browser
> process's token mapping (since the tokens will never be claimed).
> 
> Can you also add a test case to cover open_resource() in Non-SFI Mode with a
> packaged app?

Do we have any packaged app testing for Non-SFI mode today I can look to for
reference?

[commit-bot: I haz the power, 2014-09-11 16:06:23]

Patchset 1 (id:??) landed as
https://crrev.com/b16b57b17667f75ee80b3abe5dd59529bc48a8e5
Cr-Commit-Position: refs/heads/master@{#294396}