On library load failure, retry without shared RELRO.

On library load failure, retry without shared RELRO.

https://codereview.chromium.org/443393003 introduced code for service
processes that turns off RELRO sharing and retries a library load in
the event of an address clash or other failure to load a library with
shared RELRO.

This change adds the same back off and retry strategy to the libraries
loaded in the main browser process, for cases where low memory devices
aggressively share RELRO as widely as possible.

BUG=411928
TBR=rmcilroy@chromium.org

Committed: https://crrev.com/20eb5c542bbae695fe468509bcc10353c83587de
Cr-Commit-Position: refs/heads/master@{#294393}

[simonb (inactive), 2014-09-09 15:46:24]

simonb@chromium.org changed reviewers:
+ andrewhayden@chromium.org, pasko@chromium.org

[Andrew Hayden (chromium.org), 2014-09-10 10:13:32]

A few small nits.

https://codereview.chromium.org/551373004/diff/1/base/android/java/src/org/ch...
File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
(right):

https://codereview.chromium.org/551373004/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:181:
Linker.loadLibraryInZipFile(zipfile, library);
Above, we wrap the load attempt in a try/catch; and in your service code you did
the same, outputting an error message if the retry fails. Should we do the same
thing here for consistency?

https://codereview.chromium.org/551373004/diff/1/base/android/java/src/org/ch...
File base/android/java/src/org/chromium/base/library_loader/Linker.java (right):

https://codereview.chromium.org/551373004/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/library_loader/Linker.java:380: public
static boolean isBrowserUsingSharedRelro() {
The name of the method is somewhat at odds with the comment. Could we rename the
method to something like didBrowserWantSharedRelro()?

[simonb (inactive), 2014-09-10 10:58:35]

Thanks for the review.  Comment responses below.

https://codereview.chromium.org/551373004/diff/1/base/android/java/src/org/ch...
File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
(right):

https://codereview.chromium.org/551373004/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:181:
Linker.loadLibraryInZipFile(zipfile, library);
On 2014/09/10 10:13:32, Andrew Hayden wrote:
> Above, we wrap the load attempt in a try/catch; and in your service code you
did
> the same, outputting an error message if the retry fails. Should we do the
same
> thing here for consistency?

We're already in a try/catch for this, lines 144-224, so behaviour when browser
shared relro is not enabled, or when it is but an initial attempt to load failed
and we turned it off, is identical to that before this change.  That is, throws
LoaderErrors.LOADER_ERROR_NATIVE_LIBRARY_LOAD_FAILED to its caller, which
eventually logs the exception to logcat (no gain in logging it twice).

Service process retry logged on the second case because it doesn't/can't send
status to its caller -- on failure to load, the ChildProcessService simply calls
System.exit(-1) (brutal, eh?!).

[ I'm not a fan of very large try blocks and this is certainly one.  At some
point it could be tidied further.  But it is not mine originally, is a sensitive
area, and I would like to restrict this change to just adding turn-of-relro-and
retry rather than more general cleanup. ]

https://codereview.chromium.org/551373004/diff/1/base/android/java/src/org/ch...
File base/android/java/src/org/chromium/base/library_loader/Linker.java (right):

https://codereview.chromium.org/551373004/diff/1/base/android/java/src/org/ch...
base/android/java/src/org/chromium/base/library_loader/Linker.java:380: public
static boolean isBrowserUsingSharedRelro() {
On 2014/09/10 10:13:32, Andrew Hayden wrote:
> The name of the method is somewhat at odds with the comment. Could we rename
the
> method to something like didBrowserWantSharedRelro()?

Naming it succinctly yet descriptively is surprisingly hard.  This suggestion is
at odds too, since the browser isn't yet running!

Whether or not the linker sets shared relros for the browser depends on its
determination that this is a 'low-end' device (or config override).

How about Linker.isUsingBrowserSharedRelros()?

[Andrew Hayden (chromium.org), 2014-09-10 11:36:03]

On 2014/09/10 10:58:35, simonb wrote:
> Thanks for the review.  Comment responses below.
> 
>
https://codereview.chromium.org/551373004/diff/1/base/android/java/src/org/ch...
> File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
> (right):
> 
>
https://codereview.chromium.org/551373004/diff/1/base/android/java/src/org/ch...
> base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:181:
> Linker.loadLibraryInZipFile(zipfile, library);
> On 2014/09/10 10:13:32, Andrew Hayden wrote:
> > Above, we wrap the load attempt in a try/catch; and in your service code you
> did
> > the same, outputting an error message if the retry fails. Should we do the
> same
> > thing here for consistency?
> 
> We're already in a try/catch for this, lines 144-224, so behaviour when
browser
> shared relro is not enabled, or when it is but an initial attempt to load
failed
> and we turned it off, is identical to that before this change.  That is,
throws
> LoaderErrors.LOADER_ERROR_NATIVE_LIBRARY_LOAD_FAILED to its caller, which
> eventually logs the exception to logcat (no gain in logging it twice).
> 
> Service process retry logged on the second case because it doesn't/can't send
> status to its caller -- on failure to load, the ChildProcessService simply
calls
> System.exit(-1) (brutal, eh?!).
> 
> [ I'm not a fan of very large try blocks and this is certainly one.  At some
> point it could be tidied further.  But it is not mine originally, is a
sensitive
> area, and I would like to restrict this change to just adding
turn-of-relro-and
> retry rather than more general cleanup. ]

Yeah, ok, that's fair. I had noticed the other try/catch but didn't realize all
you are catching in all of these cases is UnsatisfiedLinkError. That's the only
error that can be thrown, presumably? At any rate your point about limiting
scope here is reasonable to me, it's certainly no more broken than it has been.
So yeah, let's go with this.

> 
>
https://codereview.chromium.org/551373004/diff/1/base/android/java/src/org/ch...
> File base/android/java/src/org/chromium/base/library_loader/Linker.java
(right):
> 
>
https://codereview.chromium.org/551373004/diff/1/base/android/java/src/org/ch...
> base/android/java/src/org/chromium/base/library_loader/Linker.java:380: public
> static boolean isBrowserUsingSharedRelro() {
> On 2014/09/10 10:13:32, Andrew Hayden wrote:
> > The name of the method is somewhat at odds with the comment. Could we rename
> the
> > method to something like didBrowserWantSharedRelro()?
> 
> Naming it succinctly yet descriptively is surprisingly hard.  This suggestion
is
> at odds too, since the browser isn't yet running!
> 
> Whether or not the linker sets shared relros for the browser depends on its
> determination that this is a 'low-end' device (or config override).
> 
> How about Linker.isUsingBrowserSharedRelros()?

That sounds good to me.

[simonb (inactive), 2014-09-10 12:05:14]

Thanks.  Updates in patch set 2.

[Andrew Hayden (chromium.org), 2014-09-10 12:09:14]

LGTM

[simonb (inactive), 2014-09-11 10:49:21]

The CQ bit was checked by simonb@chromium.org

[commit-bot: I haz the power, 2014-09-11 10:51:07]

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

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

The CQ bit was unchecked by commit-bot@chromium.org

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

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[simonb (inactive), 2014-09-11 12:52:45]

simonb@chromium.org changed reviewers:
+ skyostil@chromium.org

[simonb (inactive), 2014-09-11 12:52:45]

+sami, for committer lgtm, thanks.

[Sami, 2014-09-11 14:05:36]

lgtm

[simonb (inactive), 2014-09-11 14:58:16]

The CQ bit was checked by simonb@chromium.org

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

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

[commit-bot: I haz the power, 2014-09-11 15:06:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-11 15:06:32]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[simonb (inactive), 2014-09-11 15:41:33]

The CQ bit was checked by simonb@chromium.org

[commit-bot: I haz the power, 2014-09-11 15:45:33]

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

[commit-bot: I haz the power, 2014-09-11 15:50:04]

Committed patchset #2 (id:20001) as ebf5ac2f4fb6464a2c443f05583f7675954056c1

[commit-bot: I haz the power, 2014-09-11 15:53:07]

Patchset 2 (id:??) landed as
https://crrev.com/20eb5c542bbae695fe468509bcc10353c83587de
Cr-Commit-Position: refs/heads/master@{#294393}