Expose NaClAppSetDesc() to Chromium for use at startup

Expose NaClAppSetDesc() to Chromium for use at startup

Some refactoring of NaCl startup, required for implementing Bare Metal
Mode, involves providing two Chrome IPC FDs to untrusted code at
startup instead of the original one FD.  Rather than converting
initial_ipc_fd into an array (or worse, adding initial_ipc_fd2), I
think it's cleaner to expose NaClAppSetDesc() to the embedder.

So, split up NaClChromeMainStart() into two parts:
 * NaClAppCreate()
 * NaClChromeMainStartApp()
so that NaClAppSetDesc() may be called inbetween on the NaClApp.

This also involves changing when NaClAllModulesInit() gets called.  On
Windows, NaClAppCtor() crashes if NaClSecureRngModuleInit() hasn't
been called (because Windows IMC wants a random number), but on Unix,
in the Chrome sandbox, NaClSecureRngModuleSetUrandomFd() must be
called before NaClAllModulesInit().

So add NaClChromeMainInit() and NaClChromeMainSetUrandomFd() to deal
with that.

The new and old usages are explained in chrome_main.h.

Move NaClChromeMainStart()'s memset() call to
NaClAppWithSyscallTableCtor().

BUG=https://code.google.com/p/nativeclient/issues/detail?id=3734
TEST=run_sel_main_chrome_test
R=bradnelson@google.com

Committed: https://src.chromium.org/viewvc/native_client?view=rev&revision=12710

[bradn, 2014-02-05 19:47:22]

lgtm

[Mark Seaborn, 2014-02-05 23:01:59]

Committed patchset #3 manually as r12710 (presubmit successful).

[teravest, 2014-02-06 15:05:22]

I'm confused.

You added NaClAppCreate() in this change, but you proposed earlier the following
as part of the embedding API:
  struct NaClApp *NaClChromeMainCreateApp(struct NaClChromeMainArgs *args);

Why doesn't NaClAppCreate take args as an argument?

[Mark Seaborn, 2014-02-06 22:26:24]

On 6 February 2014 07:05, <teravest@chromium.org> wrote:

> I'm confused.
>
> You added NaClAppCreate() in this change, but you proposed earlier the
> following
> as part of the embedding API:
>   struct NaClApp *NaClChromeMainCreateApp(struct NaClChromeMainArgs
> *args);
>
> Why doesn't NaClAppCreate take args as an argument?


They were different changes in response to different needs, but your
proposal to expose a NaClAppCreate() function did influence me in adding
that here.

When you proposed exposing NaClAppCreate() in
https://codereview.chromium.org/135853021/#ps40001, I wasn't necessarily
against it, but I did want to check two things:

 1) Check that any initialisation order problems had been resolved.
 Specifically, calling NaClAppCtor() before NaClAllModulesInit() aborts on
Windows but not on Unix.  (BTW, running "run_sel_main_chrome_test" from
NaCl's Scons tests detects that.  So a NaCl try job detects this problem,
without having to test the NaCl change in Chromium.)  I addressed that in
https://codereview.chromium.org/153453002/ by splitting up initialisation.

 2) Check the reasons for exposing the NaClApp pointer.  If the Chromium
side was going to #include sel_ldr.h and call NaClAppLoadModule(), that is
not so good, because we are trying to avoid having the Chromium side
#include catch-all headers like sel_ldr.h (see
https://code.google.com/p/nativeclient/issues/detail?id=2832).  For
https://codereview.chromium.org/153453002/, the reason for exposing the
NaClApp pointer is to be able to call NaClAppSetDesc() on it, which I've
moved to be exposed via a header in public/.

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.
For more options, visit https://groups.google.com/groups/opt_out.

[teravest, 2014-02-06 22:52:48]

On Thu, Feb 6, 2014 at 3:26 PM, Mark Seaborn <mseaborn@chromium.org> wrote:
> On 6 February 2014 07:05, <teravest@chromium.org> wrote:
>>
>> I'm confused.
>>
>> You added NaClAppCreate() in this change, but you proposed earlier the
>> following
>> as part of the embedding API:
>>   struct NaClApp *NaClChromeMainCreateApp(struct NaClChromeMainArgs
>> *args);
>>
>> Why doesn't NaClAppCreate take args as an argument?
>
>
> They were different changes in response to different needs, but your
> proposal to expose a NaClAppCreate() function did influence me in adding
> that here.
>
> When you proposed exposing NaClAppCreate() in
> https://codereview.chromium.org/135853021/#ps40001, I wasn't necessarily
> against it, but I did want to check two things:
>
>  1) Check that any initialisation order problems had been resolved.
> Specifically, calling NaClAppCtor() before NaClAllModulesInit() aborts on
> Windows but not on Unix.  (BTW, running "run_sel_main_chrome_test" from
> NaCl's Scons tests detects that.  So a NaCl try job detects this problem,
> without having to test the NaCl change in Chromium.)  I addressed that in
> https://codereview.chromium.org/153453002/ by splitting up initialisation.
>
>  2) Check the reasons for exposing the NaClApp pointer.  If the Chromium
> side was going to #include sel_ldr.h and call NaClAppLoadModule(), that is
> not so good, because we are trying to avoid having the Chromium side
> #include catch-all headers like sel_ldr.h (see
> https://code.google.com/p/nativeclient/issues/detail?id=2832).  For
> https://codereview.chromium.org/153453002/, the reason for exposing the
> NaClApp pointer is to be able to call NaClAppSetDesc() on it, which I've
> moved to be exposed via a header in public/.

I'll ask this another way: Should I plan on the NaClApp creation in
the embedding API looking as it does in this change?

I've had to add other methods to public interface
(https://codereview.chromium.org/141413007/).
Should operations like LoadModule() and StartModule() which use
NaClApp take NaClChromeMainArgs as an argument, then?

From the review of https://codereview.chromium.org/153453002/, you
asked for a prototype with Chromium and NaCl-side changes, which I
later provided, but haven't heard anything back since. I've updated
the links in the changes to make it more obvious.

What's needed to make progress here?

>
> 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.
For more options, visit https://groups.google.com/groups/opt_out.