1276961 - Load the child binary once instead of for each sandboxed child process launch.

Status: RESOLVED WONTFIX

(Core :: Security: Process Sandboxing, defect, P3)

Description

[Bob Owen (:bobowen)]

As part of sandbox initialisation on the child process, the sandbox code loads and unloads the child executable.
Load it upfront to prevent this churn.

Comment 1

[Bob Owen (:bobowen)]

Attachment: MozReview Request: Bug 1276961: Load the child binary once instead of for each sandboxed child process launch.

Review commit: https://reviewboard.mozilla.org/r/56596/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56596/

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8758314 [details]
MozReview Request: Bug 1276961: Load the child binary once instead of for each sandboxed child process launch.

https://reviewboard.mozilla.org/r/56596/#review53164

r+ provided that isses are resolved.

::: security/sandbox/win/src/sandboxbroker/moz.build:18
(Diff revision 1)
>  ]
>  
>  for var in ('UNICODE', '_UNICODE', 'SANDBOX_EXPORTS'):
>      DEFINES[var] = True
>  
> +DEFINES['MOZ_WIDE_CHILD_PROCESS_NAME'] = 'L"%s"' % CONFIG['MOZ_CHILD_PROCESS_NAME']

Can you run this by a build system peer? I don't really care whether you add the 'L' prefix here or in the C++ code, but I'm wondering whether or not they have a preference.

::: security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp:33
(Diff revision 1)
> +  // Load and hold an HMODULE upfront to prevent this churn.
> +  WCHAR exePath[MAX_PATH];
> +  DWORD len = ::GetModuleFileNameW(NULL, exePath, MAX_PATH);
> +
> +  // Check that GetModuleFileNameW worked.
> +  if (len <= 0 || len >= MAX_PATH) {

Nit: len can't be less than 0 since it is unsigned.

::: security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp:43
(Diff revision 1)
> +  if (!::PathRemoveFileSpecW(exePath)) {
> +    return;
> +  }
> +
> +  // Check we have enough room for the child EXE name, backslash and null.
> +  if (wcslen(exePath) + wcslen(MOZ_WIDE_CHILD_PROCESS_NAME) + 2 > MAX_PATH) {

Since MOZ_WIDE_CHILD_PROCESS_NAME is a literal, you should be able to use ArrayLength here instead of wcslen.

Maybe declare a NS_NAMED_LITERAL_STRING above this and then use that in the rest of the function?

::: security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp:61
(Diff revision 1)
>  /* static */
>  void
>  SandboxBroker::Initialize(sandbox::BrokerServices* aBrokerServices)
>  {
>    sBrokerService = aBrokerServices;
> +  LoadChildBinary();

Food for thought: I wonder if we should trigger off-main-thread readahead for this binary.

Something to think about in the future if we discover that we're paying a disk I/O penalty.

::: xpcom/base/nsWindowsHelpers.h:323
(Diff revision 1)
> +  void operator()(pointer hModule)
> +  {
> +    ::FreeLibrary(hModule);
> +  }
> +};
> +typedef UniquePtr<HMODULE, HModuleFreePolicy> UniqueHModule;

We already have nsModuleHandle for this (see the typedefs above this block).

Comment 3

[Jim Mathies [:jimm]]

Is this a valid bug still? If not please close it out or knock it down below P3.

Comment 4

[Bob Owen (:bobowen)]

The bug that this might have fixed, hasn't been seen since Fx51, so I think we can wontfix this.