Android: Add chrome-specific dynamic linker.

Android: Add chrome-specific dynamic linker.

This patch adds a new Chrome-specific dynamic linker for Android,
that implements RELRO section sharing in order to save about 1.3 MB
of RAM per renderer process in content-based programs (ContentShell,
ChromiumTestShell, Chrome, etc...)

The linker is disabled by default. For more details, see the corresponding bug entry.

This introduces a new test package (content_linker_test_apk) as well as a new test category. To build and test this feature, do the following:

  ninja -C out/Debug content_linker_test_apk
  build/android/test_runner.py linker


BUG=287739
R=qsr@chromium.org

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

[digit1, 2013-09-03 14:31:54]

Thanks, that looks really good to me. I'll upload soon a new version that only
contains updates to the static library sources (mostly comment updates).

https://chromiumcodereview.appspot.com/23717023/diff/21001/base/android/linke...
File base/android/linker/crazy_linker.cpp (right):

https://chromiumcodereview.appspot.com/23717023/diff/21001/base/android/linke...
base/android/linker/crazy_linker.cpp:17: #include <crazy_linker.h>
Can we rename this source file to crazy_linker_chrome.cpp instead. I'd like to
avoid confusion between the Chrome-independent static library and the
Chrome-specific bridge between them. Keeping the name libcrazy_linker.so for the
actual shared library is ok though.

[digit1, 2013-09-06 15:33:43]

Finally a polished version of this patch. I've added a few reviewers to get
their input:

jar, thakis: Can you take a look at the small changes under base/ ?
joth, marcus: Can you take a look at the changes under base/android/ and
content/public/android/ in particular ?

Note that this patch adds a new third_party/ repository. Feel free to suggest
other reviewers for this code.

[bulach, 2013-09-06 15:45:27]

this is going to be awesome!!! :)) thanks!!

first things first:
the patch description is very, very long.. probably better to move some off to a
BUG=, that perhaps even link to a doc in dev.chromium.org?

third party: strongly recommend doing as a totally separate patch:
http://www.chromium.org/developers/adding-3rd-party-libraries

it's cool to keep in here so we can all see the "big picture" for the time
being, but please get started with third-party-owners..

[digit1, 2013-09-06 16:03:23]

On 2013/09/06 15:45:27, bulach wrote:
> this is going to be awesome!!! :)) thanks!!
> 
> first things first:
> the patch description is very, very long.. probably better to move some off to
a
> BUG=, that perhaps even link to a doc in dev.chromium.org?

Thanks Marcus, I'll move this to the bug. Note that I've started writing an
extensive design doc describing how all of this works, but it's not complete nor
publicly available yet. I'll send it to you reviewers internally.

> 
> third party: strongly recommend doing as a totally separate patch:
> http://www.chromium.org/developers/adding-3rd-party-libraries
> 
> it's cool to keep in here so we can all see the "big picture" for the time
> being, but please get started with third-party-owners..

Sure, will do, thanks.

[jar (doing other things), 2013-09-06 17:08:49]

You asked for a base review stamp... but most of the code is 4 miles deep in a
subdirectory of base (3-5 directories deep?), and there were no apparent
comments on the added files.

Were the files just pulled over from the third party?  Do you have anyone
familiar with the java etc. to review them... and then get a rubber stamp for
the insertion (deep) within base?

I glanced at the files, and made comments... but the comments may not be as
applicable if this is really a copy of a third party library... but the header
suggests it is all new (unreviewed) code.

https://chromiumcodereview.appspot.com/23717023/diff/61001/base/android/linke...
File base/android/linker/crazy_linker_jni.cpp (right):

https://chromiumcodereview.appspot.com/23717023/diff/61001/base/android/linke...
base/android/linker/crazy_linker_jni.cpp:39: public:
Should this comply with Chromium style?  If so, indent public/private by one
space.

https://chromiumcodereview.appspot.com/23717023/diff/61001/base/android/linke...
base/android/linker/crazy_linker_jni.cpp:97: else
nit: no need for else.

https://chromiumcodereview.appspot.com/23717023/diff/61001/base/android/linke...
base/android/linker/crazy_linker_jni.cpp:390: reinterpret_cast<void**>(&env),
JNI_VERSION_1_4) != JNI_OK) {
nit: indent seemed strange.  Better might be:

if (JNI_OK != vm->GetEnv(reinterpret_cast<void**>(&env),
                         JNI_VERSION_1_4)) {

[joth, 2013-09-06 17:20:44]

On 2013/09/06 15:33:43, digit1 wrote:
> Finally a polished version of this patch. I've added a few reviewers to get
> their input:
> 
> jar, thakis: Can you take a look at the small changes under base/ ?
> joth, marcus: Can you take a look at the changes under base/android/ and
> content/public/android/ in particular ?
> 
quick heads up - I'm pretty swamped with other stuff so can't get to this
anytime soon. ppi@ has been doing most work on ChildProcessLauncher.java &
friends recently so definitely invite him to look

> Note that this patch adds a new third_party/ repository. Feel free to suggest
> other reviewers for this code.

[Nico, 2013-09-08 21:24:56]

This is very impressive. However, I can't help feeling that if it makes sense to
ship your own linker with your project, something went wrong somewhere along the
way :-)

* Will the real android linker grow support for these features eventually? If
not, why not?

* Have the security people looked at the same-addresses-in-browser-and-renderer
implication? Are they fine with this?

(* I'd prefer a more descriptive name (or one that's more obviously a proper
name), but I realize that everyone tends an opinion on naming (and the name is
everywhere in the code too))

Why does libchromeview have so many relocations? Because all kinds of stuff
needs to be called from javaland?

https://chromiumcodereview.appspot.com/23717023/diff/61001/base/android/linke...
File base/android/linker/crazy_linker_jni.cpp (right):

https://chromiumcodereview.appspot.com/23717023/diff/61001/base/android/linke...
base/android/linker/crazy_linker_jni.cpp:93: static const char*
GetBaseNamePtr(const char* path) {
(do you need the static? this is already in an unnamed namespace)

https://chromiumcodereview.appspot.com/23717023/diff/61001/third_party/crazy_...
File third_party/crazy_linker/crazy_linker/src/crazy_linker_shared_library.cpp
(right):

https://chromiumcodereview.appspot.com/23717023/diff/61001/third_party/crazy_...
third_party/crazy_linker/crazy_linker/src/crazy_linker_shared_library.cpp:94: //
Call a constructor of destructor function pointer. Ignores
s/of/or/

https://chromiumcodereview.appspot.com/23717023/diff/61001/third_party/crazy_...
third_party/crazy_linker/crazy_linker/src/crazy_linker_shared_library.cpp:490:
// TODO(digit): For some reason, using memcmp() here crashes on Android 4.3.
Weird.

https://chromiumcodereview.appspot.com/23717023/diff/61001/third_party/crazy_...
third_party/crazy_linker/crazy_linker/src/crazy_linker_shared_library.cpp:663:
size_t relro_size,
(nit: indent)

https://chromiumcodereview.appspot.com/23717023/diff/61001/third_party/crazy_...
File third_party/crazy_linker/crazy_linker/src/linker_phdr.h (right):

https://chromiumcodereview.appspot.com/23717023/diff/61001/third_party/crazy_...
third_party/crazy_linker/crazy_linker/src/linker_phdr.h:45:
phdr_table_get_load_size(const ELF::Phdr* phdr_table,
Is this going to cause ODR violations with the same functions in the android
linker? Is the assumption that the android linker's version of these will always
have internal linkage? (I guess that's a good assumption for a linker, and
crazy_linker is built with fvisibility=hidden too)

https://chromiumcodereview.appspot.com/23717023/diff/61001/third_party/crazy_...
File third_party/crazy_linker/crazy_linker/tests/test_relro_sharing.cpp (right):

https://chromiumcodereview.appspot.com/23717023/diff/61001/third_party/crazy_...
third_party/crazy_linker/crazy_linker/tests/test_relro_sharing.cpp:187: if
(!crazy_library_use_relro_sharing(library,
(nit: this reads like a boolean getter to me)

Might be worth testing this with more than one input? From what I can tell,
libfoo_with_relro.so is produced from a fixed source file.

https://chromiumcodereview.appspot.com/23717023/diff/61001/third_party/crazy_...
File third_party/crazy_linker/crazy_linker/tests/zoo.cpp (right):

https://chromiumcodereview.appspot.com/23717023/diff/61001/third_party/crazy_...
third_party/crazy_linker/crazy_linker/tests/zoo.cpp:8: extern "C" bool Zoo() {
(what does this test?)

[bulach, 2013-09-09 11:43:17]

this is really exciting, thanks a ton!! :)
some initial comments, will continue looking into soon..

https://codereview.chromium.org/23717023/diff/61001/base/android/java/src/org...
File base/android/java/src/org/chromium/base/Linker.java (right):

https://codereview.chromium.org/23717023/diff/61001/base/android/java/src/org...
base/android/java/src/org/chromium/base/Linker.java:19: // This is support code
for the custom dynamic linker used in Android
nit: I think java prefers /** .. */ for multi-line comment (also for the
method-level comments below..)

https://codereview.chromium.org/23717023/diff/61001/base/android/java/src/org...
base/android/java/src/org/chromium/base/Linker.java:118: final long
baseAddressMax = baseAddressLimit - 192*1024*1024;
nit: space around *

https://codereview.chromium.org/23717023/diff/61001/base/android/java/src/org...
base/android/java/src/org/chromium/base/Linker.java:362: // don't change them
without modifying base/android/linker/crazy_linker.cpp too.
can we have getters with @CalledByNative to avoid this hidden dependency?
also, since this class is static, perhaps put on its own file?

https://codereview.chromium.org/23717023/diff/61001/base/android/linker/crazy...
File base/android/linker/crazy_linker_jni.cpp (right):

https://codereview.chromium.org/23717023/diff/61001/base/android/linker/crazy...
base/android/linker/crazy_linker_jni.cpp:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
nit: this file should be .cc rather than ".cpp"..

also, "crazy" sounds great :) but it's not too descriptive (or is it??)

how about just "custom_linker" or "shared_sections_linker"?

https://codereview.chromium.org/23717023/diff/61001/base/android/linker/crazy...
base/android/linker/crazy_linker_jni.cpp:19: // Set this to 1 to enable debug
traces to the Android log.
add some comments saying it can't use LOG(..) because it happens before the
LOG(...) module is even initialized..

https://codereview.chromium.org/23717023/diff/61001/base/android/linker/crazy...
base/android/linker/crazy_linker_jni.cpp:37: // a Java jstring handle.
hmm... would base::android::jni_string.h methods work?

std::string str(ConvertJavaStringToUTF8(env, jstr));

https://codereview.chromium.org/23717023/diff/61001/base/android/linker/crazy...
base/android/linker/crazy_linker_jni.cpp:112: static bool InitFieldId(JNIEnv*
env,
it'd be simpler to just use @CalledByNative and the JNI machinery here..

https://codereview.chromium.org/23717023/diff/61001/base/android/linker/crazy...
base/android/linker/crazy_linker_jni.cpp:208: jboolean nativeLoadLibrary(JNIEnv*
env,
nit: LoadLibrary, but again, much simpler to just use the JNI generator
machinery..

https://codereview.chromium.org/23717023/diff/61001/base/android/linker/crazy...
base/android/linker/crazy_linker_jni.cpp:272: jboolean
nativeEnableSharedRelro(JNIEnv* env,
nit: s/native//

[bulach, 2013-09-09 16:14:14]

this is really exciting! sorry I haven't finished yet, but some more comments
here.. I'm adding palmer@ to get some thoughts re: security review...

it's not clear to me what part of the third_party was written by ourselves /
where it comes from, but I'm assuming there should be a full review / follow the
chromium guidelines? wdyt?

https://codereview.chromium.org/23717023/diff/61001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java
(right):

https://codereview.chromium.org/23717023/diff/61001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java:428:
} catch (Exception e) {
is there a more specific Exception to catch?
perhaps Log.e since this would swallow anything?

https://codereview.chromium.org/23717023/diff/61001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
(right):

https://codereview.chromium.org/23717023/diff/61001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:7:
import android.os.Bundle;
nit: sort order

https://codereview.chromium.org/23717023/diff/61001/content/public/android/ja...
File content/public/android/java/templates/NativeLibraries.template (right):

https://codereview.chromium.org/23717023/diff/61001/content/public/android/ja...
content/public/android/java/templates/NativeLibraries.template:10: public static
boolean USE_CRAZY_LINKER = true;
nit: final

https://codereview.chromium.org/23717023/diff/61001/third_party/crazy_linker/...
File third_party/crazy_linker/crazy_linker/src/crazy_linker_debug.cpp (right):

https://codereview.chromium.org/23717023/diff/61001/third_party/crazy_linker/...
third_party/crazy_linker/crazy_linker/src/crazy_linker_debug.cpp:23: char
buffer[4096];
is the stack big enough at this depth?
perhaps allocate this at the heap, or as a static buffer?

https://codereview.chromium.org/23717023/diff/61001/third_party/crazy_linker/...
File third_party/crazy_linker/crazy_linker/src/crazy_linker_debug.h (right):

https://codereview.chromium.org/23717023/diff/61001/third_party/crazy_linker/...
third_party/crazy_linker/crazy_linker/src/crazy_linker_debug.h:23: #define
LOG_ERRNO(...)  ::crazy::LogErrno(__VA_ARGS__)
maybe CRAZY_LOG to avoid collision with the many other LOG?

https://codereview.chromium.org/23717023/diff/61001/third_party/crazy_linker/...
File third_party/crazy_linker/crazy_linker/src/crazy_linker_elf_loader.cpp
(right):

https://codereview.chromium.org/23717023/diff/61001/third_party/crazy_linker/...
third_party/crazy_linker/crazy_linker/src/crazy_linker_elf_loader.cpp:115:
header_.e_ident[EI_MAG3] != ELFMAG3) {
maybe simpler?
memcmp(header_e_ident, ELFMAG, SELFMAG)

https://codereview.chromium.org/23717023/diff/61001/third_party/crazy_linker/...
third_party/crazy_linker/crazy_linker/src/crazy_linker_elf_loader.cpp:125: if
(header_.e_ident[EI_DATA] != ELFDATA2LSB) {
at what level would the endianess cause an issue?
would the higher level fallback to the regular linker if this fails?
I'm not entirely sure we ever tested on LSB, but afaict, arm devices can
potentially support that (and other architectures too)..

https://codereview.chromium.org/23717023/diff/61001/third_party/crazy_linker/...
File third_party/crazy_linker/crazy_linker/src/crazy_linker_library_view.h
(right):

https://codereview.chromium.org/23717023/diff/61001/third_party/crazy_linker/...
third_party/crazy_linker/crazy_linker/src/crazy_linker_library_view.h:72: bool
SafeDeref() {
nit: how about DecrementRef for this one, and "Release" for the next one?

https://codereview.chromium.org/23717023/diff/61001/third_party/crazy_linker/...
File third_party/crazy_linker/crazy_linker/src/crazy_linker_line_reader.cpp
(right):

https://codereview.chromium.org/23717023/diff/61001/third_party/crazy_linker/...
third_party/crazy_linker/crazy_linker/src/crazy_linker_line_reader.cpp:57: const
char* line_end = reinterpret_cast<const char*>(::memchr(line, '\n', buff_size_ -
line_start_));
nit: >80cols here and 62 below

https://codereview.chromium.org/23717023/diff/61001/third_party/crazy_linker/...
File third_party/crazy_linker/crazy_linker/src/crazy_linker_rdebug.cpp (right):

https://codereview.chromium.org/23717023/diff/61001/third_party/crazy_linker/...
third_party/crazy_linker/crazy_linker/src/crazy_linker_rdebug.cpp:29:
readlink("/proc/self/exe", &(*exe_path)[0], exe_path->size()));
can we get a non-const c_str out of exe_path? :) it'd be slightly more
readable..

https://codereview.chromium.org/23717023/diff/61001/third_party/crazy_linker/...
third_party/crazy_linker/crazy_linker/src/crazy_linker_rdebug.cpp:154: public:
nit: space

https://codereview.chromium.org/23717023/diff/61001/third_party/crazy_linker/...
File third_party/crazy_linker/crazy_linker/src/crazy_linker_rdebug.h (right):

https://codereview.chromium.org/23717023/diff/61001/third_party/crazy_linker/...
third_party/crazy_linker/crazy_linker/src/crazy_linker_rdebug.h:87: // variable,
called '_r_debut_tail' that points to the last item in
nit: _r_debug_tail?

https://codereview.chromium.org/23717023/diff/61001/third_party/crazy_linker/...
File third_party/crazy_linker/crazy_linker/src/crazy_linker_shared_library.h
(right):

https://codereview.chromium.org/23717023/diff/61001/third_party/crazy_linker/...
third_party/crazy_linker/crazy_linker/src/crazy_linker_shared_library.h:153:
dep_name_ = NULL;
nit: maybe move this to the .cc file?

I think for chromium proper, clang would complain about these inline methods.. I
think the warning is disabled for third-parties, but it probably wouldn't hurt
to follow the style..

https://codereview.chromium.org/23717023/diff/61001/third_party/crazy_linker/...
third_party/crazy_linker/crazy_linker/src/crazy_linker_shared_library.h:159:
dynamic_ ++;
nit: remove space after dynamic here and 164

[digit1, 2013-09-10 09:11:52]

On 2013/09/08 21:24:56, Nico wrote:
> This is very impressive. However, I can't help feeling that if it makes sense
to
> ship your own linker with your project, something went wrong somewhere along
the
> way :-)
> 
Well, the real issue is that we can't get a Chromium Zygote on Android (for good
technical reasons), so we have to be more creative to get the same RAM-saving
benefits. Note that this was not started in isolation, we asked the Android team
about their input on such ideas an alternate schemes. I would have personally
preferred if the system could support this feature instead.

> * Will the real android linker grow support for these features eventually? If
> not, why not?
> 
The answer is no. I can't discuss Android team plans here, but there's an
internal bug with more details (b/9378107).

> * Have the security people looked at the
same-addresses-in-browser-and-renderer
> implication? Are they fine with this?
> 
I've discussed this with jln@ a few months ago. He said this was acceptable
since this is already what desktop Chrome / ChromeOS do, due to their use of a
Chromium-specific Zygote. Note that, unlike desktop Chrome, the service process
are still running in "sandboxed mode", i.e. with a different Android UID that
has no privileges, so this doesn't affect the sandboxing. I'd be happy to have
Chris' take on this. He should look at the randomizeBaseLoadAddress() method in
org.chromium.base.Linker.


> (* I'd prefer a more descriptive name (or one that's more obviously a proper
> name), but I realize that everyone tends an opinion on naming (and the name is
> everywhere in the code too))
> 

I'm open to suggestions here :-)

> Why does libchromeview have so many relocations? Because all kinds of stuff
> needs to be called from javaland?
> 

It's because it's a huge C++ project (lots of VTables), and also has many
constant tables that include a plethora of pointers.
Even though I've submitted a few patches to reduce these in the past (e.g.
https://code.google.com/p/chromium/issues/detail?id=249747), our RELRO section
keeps growing steadidly due to the influx of new code.

The auto-generated JNI code has a few constant pointers to strings, but doesn't
define a lot new classes, so I wouldn't say it's the root cause of this
explosion.

[digit1, 2013-09-10 09:14:37]

On 2013/09/09 16:14:14, bulach wrote:
> this is really exciting! sorry I haven't finished yet, but some more comments
> here.. I'm adding palmer@ to get some thoughts re: security review...
> 
> it's not clear to me what part of the third_party was written by ourselves /
> where it comes from, but I'm assuming there should be a full review / follow
the
> chromium guidelines? wdyt?
> 
Everything was written by me and qsr, except for a couple NDK/Android source
files (which I happened to write in a previous life :-))
I'm not sure yet whether this will become part of the NDK (which already
includes a few similar support libraries). I'm ok with following the Chromium
coding guidelines though, but I'll do all the required changes after moving the
changes to a different patch.

[digit1, 2013-09-10 09:23:28]

https://chromiumcodereview.appspot.com/23717023/diff/61001/base/android/java/...
File base/android/java/src/org/chromium/base/Linker.java (right):

https://chromiumcodereview.appspot.com/23717023/diff/61001/base/android/java/...
base/android/java/src/org/chromium/base/Linker.java:19: // This is support code
for the custom dynamic linker used in Android
On 2013/09/09 11:43:18, bulach wrote:
> nit: I think java prefers /** .. */ for multi-line comment (also for the
> method-level comments below..)

Ok, I've changed all external multi-line comments to /** .. */
I tend to use // for private comments, and /** .. */ for public / API ones,
which is the scheme I used initially. Thanks.

https://chromiumcodereview.appspot.com/23717023/diff/61001/base/android/java/...
base/android/java/src/org/chromium/base/Linker.java:118: final long
baseAddressMax = baseAddressLimit - 192*1024*1024;
On 2013/09/09 11:43:18, bulach wrote:
> nit: space around *

Done.

https://chromiumcodereview.appspot.com/23717023/diff/61001/base/android/java/...
base/android/java/src/org/chromium/base/Linker.java:362: // don't change them
without modifying base/android/linker/crazy_linker.cpp too.
On 2013/09/09 11:43:18, bulach wrote:
> can we have getters with @CalledByNative to avoid this hidden dependency?
> also, since this class is static, perhaps put on its own file?

The linker source code cannot depend on anything from base/ or the C++ STL, so I
don't think using @CalledByNative here is going to help (since we're not going
to use the auto-generated headers created from them).

The class is really an implementation detail, so I'd rather keep it here. The
only reason it's public is because this is required for Parcelable support
(otherwise the class loader can't see it).

https://chromiumcodereview.appspot.com/23717023/diff/61001/base/android/linke...
File base/android/linker/crazy_linker_jni.cpp (right):

https://chromiumcodereview.appspot.com/23717023/diff/61001/base/android/linke...
base/android/linker/crazy_linker_jni.cpp:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
On 2013/09/09 11:43:18, bulach wrote:
> nit: this file should be .cc rather than ".cpp"..
> 
> also, "crazy" sounds great :) but it's not too descriptive (or is it??)
> 
> how about just "custom_linker" or "shared_sections_linker"?

I've renamed it to crazy_linker_jni.cc for now.

I'm open to different names, but we have plans to experiment with other features
in the future so "shared_sections_linker" sounds a bit too tied to an
implementation detail. libcustom_linker.so doesn't have the same "cachet" to me.
Possible alternatives:

  liblinker.so
  libbase_linker.so
  libcontent_linker.so
  libchromium_linker.so
  libchrome_linker.so

https://chromiumcodereview.appspot.com/23717023/diff/61001/base/android/linke...
base/android/linker/crazy_linker_jni.cpp:19: // Set this to 1 to enable debug
traces to the Android log.
On 2013/09/09 11:43:18, bulach wrote:
> add some comments saying it can't use LOG(..) because it happens before the
> LOG(...) module is even initialized..

Done.

https://chromiumcodereview.appspot.com/23717023/diff/61001/base/android/linke...
base/android/linker/crazy_linker_jni.cpp:37: // a Java jstring handle.
On 2013/09/09 11:43:18, bulach wrote:
> hmm... would base::android::jni_string.h methods work?
> 
> std::string str(ConvertJavaStringToUTF8(env, jstr));

No, the goal is to avoid any code that's part of base/ or the C++ STL to avoid
bloating the linker itself. That could easily a ton of code which would itself
require more RAM pages to load and relocate. This source intentionally avoids
any other dependencies, hence the reimplementations here.

I'll clarify that in a comment here.

https://chromiumcodereview.appspot.com/23717023/diff/61001/base/android/linke...
base/android/linker/crazy_linker_jni.cpp:39: public:
On 2013/09/06 17:08:50, jar wrote:
> Should this comply with Chromium style?  If so, indent public/private by one
> space.

I've done that. By the way, using "git cl format" here generates plenty of lines
that are > 80 characters. Did the style guide changed recently?

https://chromiumcodereview.appspot.com/23717023/diff/61001/base/android/linke...
base/android/linker/crazy_linker_jni.cpp:93: static const char*
GetBaseNamePtr(const char* path) {
Indeed, I've removed the static. Thanks.

https://chromiumcodereview.appspot.com/23717023/diff/61001/base/android/linke...
base/android/linker/crazy_linker_jni.cpp:97: else
On 2013/09/06 17:08:50, jar wrote:
> nit: no need for else.

Done.

https://chromiumcodereview.appspot.com/23717023/diff/61001/base/android/linke...
base/android/linker/crazy_linker_jni.cpp:112: static bool InitFieldId(JNIEnv*
env,
On 2013/09/09 11:43:18, bulach wrote:
> it'd be simpler to just use @CalledByNative and the JNI machinery here..

No, we can't use the machinery, because it relies on helper functions in base/
that I don't need to use here (this bloats the linker to about 300 KB :-(, also
doesn't work well with the component build where you can't statically link base
into this library).

https://chromiumcodereview.appspot.com/23717023/diff/61001/base/android/linke...
base/android/linker/crazy_linker_jni.cpp:208: jboolean nativeLoadLibrary(JNIEnv*
env,
On 2013/09/09 11:43:18, bulach wrote:
> nit: LoadLibrary, but again, much simpler to just use the JNI generator
> machinery..

Done.

https://chromiumcodereview.appspot.com/23717023/diff/61001/base/android/linke...
base/android/linker/crazy_linker_jni.cpp:272: jboolean
nativeEnableSharedRelro(JNIEnv* env,
On 2013/09/09 11:43:18, bulach wrote:
> nit: s/native//

Done.

https://chromiumcodereview.appspot.com/23717023/diff/61001/base/android/linke...
base/android/linker/crazy_linker_jni.cpp:390: reinterpret_cast<void**>(&env),
JNI_VERSION_1_4) != JNI_OK) {
On 2013/09/06 17:08:50, jar wrote:
> nit: indent seemed strange.  Better might be:
> 
> if (JNI_OK != vm->GetEnv(reinterpret_cast<void**>(&env),
>                          JNI_VERSION_1_4)) {

Done.

https://chromiumcodereview.appspot.com/23717023/diff/61001/content/public/and...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java
(right):

https://chromiumcodereview.appspot.com/23717023/diff/61001/content/public/and...
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java:428:
} catch (Exception e) {
Yes, it's RemoteException (any Binder service function may throw one when the
host service process dies). I'll specialize the catch. Thanks for spotting this.

https://chromiumcodereview.appspot.com/23717023/diff/61001/content/public/and...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
(right):

https://chromiumcodereview.appspot.com/23717023/diff/61001/content/public/and...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:7:
import android.os.Bundle;
On 2013/09/09 16:14:15, bulach wrote:
> nit: sort order

Done.

https://chromiumcodereview.appspot.com/23717023/diff/61001/content/public/and...
File content/public/android/java/templates/NativeLibraries.template (right):

https://chromiumcodereview.appspot.com/23717023/diff/61001/content/public/and...
content/public/android/java/templates/NativeLibraries.template:10: public static
boolean USE_CRAZY_LINKER = true;
On 2013/09/09 16:14:15, bulach wrote:
> nit: final

Ah, this is _really_ subtle, this can't get final because it is part of content,
which can be used by several content-based APKs (e.g. content shell, chromium
test shell, chrome, etc...).

The point is that, during the build, this is part of content.jar, but we want to
decide in the final APK target whether to enable this or not.

There is an ugly trick performed in our build system (not invented here, it's
already necessary to handle the list of native libraries to load):

- When building the intermediate content.jar, the NativeLibraries.class file is
removed from the .jar
- Another version of NativeLibraries.class is generated for the given final
target, e.g. goes into content_shell.jar
- When generating the final .dex files, the stripped content.jar + new
content_shell.jar are linked
  together into a single .dex file.

The problem is that if the variable is final, it will be optimized out at
compile time when generating content.jar and will be either disabled / enabled
for all projects using the same intermediate file.

This issue already exists for the list of native libraries defined just below.
So we just reused the system.
I'll add a comment here.

https://chromiumcodereview.appspot.com/23717023/diff/61001/third_party/crazy_...
File third_party/crazy_linker/crazy_linker/src/crazy_linker_debug.cpp (right):

https://chromiumcodereview.appspot.com/23717023/diff/61001/third_party/crazy_...
third_party/crazy_linker/crazy_linker/src/crazy_linker_debug.cpp:23: char
buffer[4096];
On 2013/09/09 16:14:15, bulach wrote:
> is the stack big enough at this depth?
> perhaps allocate this at the heap, or as a static buffer?

I have no reason to think that the stack would be limited here, but I've changed
this to a heap allocation.
static buffers play pretty bad with threads.

https://chromiumcodereview.appspot.com/23717023/diff/61001/third_party/crazy_...
File third_party/crazy_linker/crazy_linker/src/crazy_linker_debug.h (right):

https://chromiumcodereview.appspot.com/23717023/diff/61001/third_party/crazy_...
third_party/crazy_linker/crazy_linker/src/crazy_linker_debug.h:23: #define
LOG_ERRNO(...)  ::crazy::LogErrno(__VA_ARGS__)
On 2013/09/09 16:14:15, bulach wrote:
> maybe CRAZY_LOG to avoid collision with the many other LOG?

what collisions? This is compiled as a standalone static library, and completely
internal (i.e. never exposed through any public header).

https://chromiumcodereview.appspot.com/23717023/diff/61001/third_party/crazy_...
File third_party/crazy_linker/crazy_linker/src/crazy_linker_elf_loader.cpp
(right):

https://chromiumcodereview.appspot.com/23717023/diff/61001/third_party/crazy_...
third_party/crazy_linker/crazy_linker/src/crazy_linker_elf_loader.cpp:115:
header_.e_ident[EI_MAG3] != ELFMAG3) {
On 2013/09/09 16:14:15, bulach wrote:
> maybe simpler?
> memcmp(header_e_ident, ELFMAG, SELFMAG)

Done.

https://chromiumcodereview.appspot.com/23717023/diff/61001/third_party/crazy_...
third_party/crazy_linker/crazy_linker/src/crazy_linker_elf_loader.cpp:125: if
(header_.e_ident[EI_DATA] != ELFDATA2LSB) {
On 2013/09/09 16:14:15, bulach wrote:
> at what level would the endianess cause an issue?
> would the higher level fallback to the regular linker if this fails?
> I'm not entirely sure we ever tested on LSB, but afaict, arm devices can
> potentially support that (and other architectures too)..

First, Android doesn't support little-endian platforms, but this guards about
trying to load the wrong kind of ELF file.

But this linker is designed to be as platform-agnostic as possible. Apart from
the ashmem region (which could theorically be replaced by a SysV shmem handle),
there is nothing really specific to Android.

https://chromiumcodereview.appspot.com/23717023/diff/61001/third_party/crazy_...
File third_party/crazy_linker/crazy_linker/src/crazy_linker_library_view.h
(right):

https://chromiumcodereview.appspot.com/23717023/diff/61001/third_party/crazy_...
third_party/crazy_linker/crazy_linker/src/crazy_linker_library_view.h:72: bool
SafeDeref() {
I've remove Deref(), which isn't used anymore, and changed this to
SafeDecrementRef(). I think it's important to convey that this never destroys
the object.

https://chromiumcodereview.appspot.com/23717023/diff/61001/third_party/crazy_...
File third_party/crazy_linker/crazy_linker/src/crazy_linker_line_reader.cpp
(right):

https://chromiumcodereview.appspot.com/23717023/diff/61001/third_party/crazy_...
third_party/crazy_linker/crazy_linker/src/crazy_linker_line_reader.cpp:57: const
char* line_end = reinterpret_cast<const char*>(::memchr(line, '\n', buff_size_ -
line_start_));
On 2013/09/09 16:14:15, bulach wrote:
> nit: >80cols here and 62 below

Done.

https://chromiumcodereview.appspot.com/23717023/diff/61001/third_party/crazy_...
File third_party/crazy_linker/crazy_linker/src/crazy_linker_rdebug.cpp (right):

https://chromiumcodereview.appspot.com/23717023/diff/61001/third_party/crazy_...
third_party/crazy_linker/crazy_linker/src/crazy_linker_rdebug.cpp:29:
readlink("/proc/self/exe", &(*exe_path)[0], exe_path->size()));
Good idea, done.

https://chromiumcodereview.appspot.com/23717023/diff/61001/third_party/crazy_...
third_party/crazy_linker/crazy_linker/src/crazy_linker_rdebug.cpp:154: public:
Done, as well as in all other sources.

https://chromiumcodereview.appspot.com/23717023/diff/61001/third_party/crazy_...
File third_party/crazy_linker/crazy_linker/src/crazy_linker_rdebug.h (right):

https://chromiumcodereview.appspot.com/23717023/diff/61001/third_party/crazy_...
third_party/crazy_linker/crazy_linker/src/crazy_linker_rdebug.h:87: // variable,
called '_r_debut_tail' that points to the last item in
Thanks for catching this :-) Done.

https://chromiumcodereview.appspot.com/23717023/diff/61001/third_party/crazy_...
File third_party/crazy_linker/crazy_linker/src/crazy_linker_shared_library.cpp
(right):

https://chromiumcodereview.appspot.com/23717023/diff/61001/third_party/crazy_...
third_party/crazy_linker/crazy_linker/src/crazy_linker_shared_library.cpp:94: //
Call a constructor of destructor function pointer. Ignores
On 2013/09/08 21:24:57, Nico wrote:
> s/of/or/

Done.

https://chromiumcodereview.appspot.com/23717023/diff/61001/third_party/crazy_...
third_party/crazy_linker/crazy_linker/src/crazy_linker_shared_library.cpp:490:
// TODO(digit): For some reason, using memcmp() here crashes on Android 4.3.
Indeed :-(

https://chromiumcodereview.appspot.com/23717023/diff/61001/third_party/crazy_...
third_party/crazy_linker/crazy_linker/src/crazy_linker_shared_library.cpp:663:
size_t relro_size,
On 2013/09/08 21:24:57, Nico wrote:
> (nit: indent)

Done.

https://chromiumcodereview.appspot.com/23717023/diff/61001/third_party/crazy_...
File third_party/crazy_linker/crazy_linker/src/crazy_linker_shared_library.h
(right):

https://chromiumcodereview.appspot.com/23717023/diff/61001/third_party/crazy_...
third_party/crazy_linker/crazy_linker/src/crazy_linker_shared_library.h:153:
dep_name_ = NULL;
On 2013/09/09 16:14:15, bulach wrote:
> nit: maybe move this to the .cc file?
> 
> I think for chromium proper, clang would complain about these inline methods..
I
> think the warning is disabled for third-parties, but it probably wouldn't hurt
> to follow the style..

Done.

https://chromiumcodereview.appspot.com/23717023/diff/61001/third_party/crazy_...
third_party/crazy_linker/crazy_linker/src/crazy_linker_shared_library.h:159:
dynamic_ ++;
On 2013/09/09 16:14:15, bulach wrote:
> nit: remove space after dynamic here and 164

Done.

https://chromiumcodereview.appspot.com/23717023/diff/61001/third_party/crazy_...
File third_party/crazy_linker/crazy_linker/src/linker_phdr.h (right):

https://chromiumcodereview.appspot.com/23717023/diff/61001/third_party/crazy_...
third_party/crazy_linker/crazy_linker/src/linker_phdr.h:45:
phdr_table_get_load_size(const ELF::Phdr* phdr_table,
On 2013/09/08 21:24:57, Nico wrote:
> Is this going to cause ODR violations with the same functions in the android
> linker? Is the assumption that the android linker's version of these will
always
> have internal linkage? (I guess that's a good assumption for a linker, and
> crazy_linker is built with fvisibility=hidden too)

There won't be any violations since:

- The crazy linker is built with -fvisibility=hidden by default. Only the
crazy_xxxx functions
  declared in <crazy_linker.h> are actually exposed explicitely through the
_CRAZY_PUBLIC
  annotation macro.

- The system linker is built with the same flags. Moreover, its build system
uses objcopy to
  rename all functions (local or global) with a '__dl_' prefix.

While we're on it, the system linker's official exported symbols (dlopen, etc)
are exposed to
loaded binaries through a special in-memory ELF file matching 'libdl.so' (where
the functions
are supposed to be defined). The symbol entries are linked to the linker's
internal implementation
of the functions (e.g. "dlopen" --> __dl_dlopen)

libdl.so itself is actually an empty "shell" library. All symbols it exposes
(e.g. dlopen())
are backed by empty functions in the library. That's because this enough at link
time (i.e.
when generating some binary with -ldl). At runtime, the in-memory ELF
representation will
override the system library, which will never be loaded.

https://chromiumcodereview.appspot.com/23717023/diff/61001/third_party/crazy_...
File third_party/crazy_linker/crazy_linker/tests/test_relro_sharing.cpp (right):

https://chromiumcodereview.appspot.com/23717023/diff/61001/third_party/crazy_...
third_party/crazy_linker/crazy_linker/tests/test_relro_sharing.cpp:187: if
(!crazy_library_use_relro_sharing(library,
On 2013/09/08 21:24:57, Nico wrote:
> (nit: this reads like a boolean getter to me)
> 
> Might be worth testing this with more than one input? From what I can tell,
> libfoo_with_relro.so is produced from a fixed source file.

This has been tested with the Chrome component build, but yes, I've added a new
unit test that uses two libraries instead (and checks the number of shared
RELROs in each process).

https://chromiumcodereview.appspot.com/23717023/diff/61001/third_party/crazy_...
File third_party/crazy_linker/crazy_linker/tests/zoo.cpp (right):

https://chromiumcodereview.appspot.com/23717023/diff/61001/third_party/crazy_...
third_party/crazy_linker/crazy_linker/tests/zoo.cpp:8: extern "C" bool Zoo() {
On 2013/09/08 21:24:57, Nico wrote:
> (what does this test?)

I've rewritten the test to make it clearer. In essence, it checks that dlopen()
/ dlsym() are properly wrapped for libraries that are loaded by the crazy
linker.

[digit1, 2013-09-10 09:28:48]

By the way, I'm currently writing a unit test for the linker. Given that it is
optional, I plan to write a specific test apk (e.g. content_linker_test_apk).

Any idea of what interesting thing to put there? The 'feature' is supposed to be
completely transparent, but I guess we'd want to run some native code in both a
browser and renderer process and verify that they run properly after it's been
loaded through the linker.

[joth, 2013-09-10 10:01:21]

On 10 September 2013 02:11, <digit@chromium.org> wrote:

> On 2013/09/08 21:24:56, Nico wrote:
>
>> This is very impressive. However, I can't help feeling that if it makes
>> sense
>>
> to
>
>> ship your own linker with your project, something went wrong somewhere
>> along
>>
> the
>
>> way :-)
>>
>
> Well, the real issue is that we can't get a Chromium Zygote on Android
> (for good
> technical reasons), so we have to be more creative to get the same
> RAM-saving
> benefits. Note that this was not started in isolation, we asked the
> Android team
> about their input on such ideas an alternate schemes. I would have
> personally
> preferred if the system could support this feature instead.
>
>
>  * Will the real android linker grow support for these features
>> eventually? If
>> not, why not?
>>
>
> The answer is no. I can't discuss Android team plans here, but there's an
> internal bug with more details (b/9378107).
>
>
>  * Have the security people looked at the
>>
> same-addresses-in-browser-and-**renderer
>
>> implication? Are they fine with this?
>>
>
> I've discussed this with jln@ a few months ago. He said this was
> acceptable
> since this is already what desktop Chrome / ChromeOS do, due to their use
> of a
> Chromium-specific Zygote. Note that, unlike desktop Chrome, the service
> process
> are still running in "sandboxed mode", i.e. with a different Android UID
> that
> has no privileges, so this doesn't affect the sandboxing. I'd be happy to
> have
> Chris' take on this. He should look at the randomizeBaseLoadAddress()
> method in
> org.chromium.base.Linker.
>
>
>
IIUC one exciting bit here is the sandbox processes get a file handle to
the ashmem page containing pointers to [virtual] functions that are
dereferenced in the browser process. Ensuring those pages are read-only is
crucial, right? (A key thing to include in unit tests? perhaps, death tests)



>
>  (* I'd prefer a more descriptive name (or one that's more obviously a
>> proper
>> name), but I realize that everyone tends an opinion on naming (and the
>> name is
>> everywhere in the code too))
>>
>
>
> I'm open to suggestions here :-)
>
>
I only have more variations on the line you already started..
digitlinker
skinnylinker
happylinker



>
>  Why does libchromeview have so many relocations? Because all kinds of
>> stuff
>> needs to be called from javaland?
>>
>
>
> It's because it's a huge C++ project (lots of VTables), and also has many
> constant tables that include a plethora of pointers.
> Even though I've submitted a few patches to reduce these in the past (e.g.
>
https://code.google.com/p/**chromium/issues/detail?id=**249747<https://code.g...),
> our RELRO section
> keeps growing steadidly due to the influx of new code.
>

IIRC doesn't the fact we have to link into into an .so really make the
difference? all other platforms link into a  single monolithic binary that
has more stuff fixed up at static link time? (/hazy on details now)



> The auto-generated JNI code has a few constant pointers to strings, but
> doesn't
> define a lot new classes, so I wouldn't say it's the root cause of this
> explosion.
>
>
>
https://chromiumcodereview.**appspot.com/23717023/<https://chromiumcodereview...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[digit1, 2013-09-10 10:21:41]

On 2013/09/10 10:01:21, joth wrote:
> >
> IIUC one exciting bit here is the sandbox processes get a file handle to
> the ashmem page containing pointers to [virtual] functions that are
> dereferenced in the browser process. Ensuring those pages are read-only is
> crucial, right? (A key thing to include in unit tests? perhaps, death tests)
> 
You're right. The pages are mapped read-only in the renderer process by the
linker (when using the shared RELROs), but a compromised renderer could remap
then read-write. Fortunately, there is an API to force the ASHMEM region to
become forcefully read-only, I'll add that to the linker, but after moving the
code to a third_party/ CL. Thanks.
>
 
> 
> >
> >  (* I'd prefer a more descriptive name (or one that's more obviously a
> >> proper
> >> name), but I realize that everyone tends an opinion on naming (and the
> >> name is
> >> everywhere in the code too))
> >>
> >
> >
> > I'm open to suggestions here :-)
> >
> >
> I only have more variations on the line you already started..
> digitlinker
> skinnylinker
> happylinker
> 
> 
> 
> >
> >  Why does libchromeview have so many relocations? Because all kinds of
> >> stuff
> >> needs to be called from javaland?
> >>
> >
> >
> > It's because it's a huge C++ project (lots of VTables), and also has many
> > constant tables that include a plethora of pointers.
> > Even though I've submitted a few patches to reduce these in the past (e.g.
> >
>
https://code.google.com/p/**chromium/issues/detail?id=**249747%3Chttps://code...>),
> > our RELRO section
> > keeps growing steadidly due to the influx of new code.
> >
> 
> IIRC doesn't the fact we have to link into into an .so really make the
> difference? all other platforms link into a  single monolithic binary that
> has more stuff fixed up at static link time? (/hazy on details now)
> 
> 
> 
> > The auto-generated JNI code has a few constant pointers to strings, but
> > doesn't
> > define a lot new classes, so I wouldn't say it's the root cause of this
> > explosion.
> >
> >
> >
>
https://chromiumcodereview.**appspot.com/23717023/%3Chttps://chromiumcoderevi...>
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[bulach, 2013-09-10 14:21:37]

some more comments, but I'm afraid I'm reaching my limits here..

I think I know someone in NaCl team that perhaps could be able to help, do you
think it'd be useful?

I don't want this to become a bikeshed, so I'd only add more people if you want
more eyes, ok?

https://chromiumcodereview.appspot.com/23717023/diff/92001/base/android/java/...
File base/android/java/src/org/chromium/base/Linker.java (right):

https://chromiumcodereview.appspot.com/23717023/diff/92001/base/android/java/...
base/android/java/src/org/chromium/base/Linker.java:59: public class Linker {
so given the options:
  liblinker.so
  libbase_linker.so
  libcontent_linker.so
  libchromium_linker.so
  libchrome_linker.so

my personal vote would be then to liblinker.. :)
the others seem tied to some other module, and may lead to confusion..
and speaking of which: understood that this can't depend on base/ or STL, thanks
for clarifying!
so how about moving this _out_ of base entirely? :)
it's too tempting to be in base and not depend on it (together with logging,
JNI, etc..).

https://chromiumcodereview.appspot.com/23717023/diff/92001/base/android/java/...
base/android/java/src/org/chromium/base/Linker.java:436: public long
mLoadAddress; // page-aligned load address.
perhaps just to be safe, repeat the comment here:

// Don't change these fields without modifying
base/android/linker/crazy_linker.cpp too.

https://chromiumcodereview.appspot.com/23717023/diff/92001/base/android/java/...
base/android/java/src/org/chromium/base/Linker.java:447: private static class
LibInfoMap extends HashMap<String, LibInfo> {
given the limits on number of classes, could this be made as free-functions?

private static Bundle createBundleFromLibInfoMap(HashMap<String, LibInfo>
libInfoMap) {
  ...
}

private static HashMap<String, LibInfo> createLibInfoMapfromBundle(Bundle) {
  ...
}

https://chromiumcodereview.appspot.com/23717023/diff/92001/base/android/linke...
File base/android/linker/crazy_linker_jni.cc (right):

https://chromiumcodereview.appspot.com/23717023/diff/92001/base/android/linke...
base/android/linker/crazy_linker_jni.cc:13: // This source code *cannot* depend
on anything from base/ or the C++
yeah, we should certainly move it _out_ of base :)

(and maybe a small explanation, something like:
// and avoid ugly dependency issues:
// this linker library is loaded before base
// and even STL libraries are loaded.

https://chromiumcodereview.appspot.com/23717023/diff/92001/base/android/linke...
base/android/linker/crazy_linker_jni.cc:142: static JavaVM* s_java_vm;
nit: I think g_ is more common than s_ (ditto for s_lib_info_fields and
s_crazy_context below..)

https://chromiumcodereview.appspot.com/23717023/diff/92001/base/android/linke...
base/android/linker/crazy_linker_jni.cc:264: memset(&info, 0, sizeof(info));
nit: I think "crazy_library_info_t info = {0};" would work?

https://chromiumcodereview.appspot.com/23717023/diff/92001/content/public/and...
File content/public/android/java/src/org/chromium/content/app/LibraryLoader.java
(left):

https://chromiumcodereview.appspot.com/23717023/diff/92001/content/public/and...
content/public/android/java/src/org/chromium/content/app/LibraryLoader.java:112:
Log.i(TAG, "loading: " + sLibrary);
perhaps keep this log entries? sometimes it's helpful..

https://chromiumcodereview.appspot.com/23717023/diff/92001/content/public/and...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java
(right):

https://chromiumcodereview.appspot.com/23717023/diff/92001/content/public/and...
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java:93:
public static final String EXTRA_CHROME_LINKER_ADDRESS =
how about EXTRA_LINKER_BASE_LOAD_ADDRESS, and similarly for the value?

https://chromiumcodereview.appspot.com/23717023/diff/92001/third_party/crazy_...
File third_party/crazy_linker/crazy_linker/minitest/minitest.cc (right):

https://chromiumcodereview.appspot.com/23717023/diff/92001/third_party/crazy_...
third_party/crazy_linker/crazy_linker/minitest/minitest.cc:1: #include
"minitest.h"
copyright info..

https://chromiumcodereview.appspot.com/23717023/diff/92001/third_party/crazy_...
File third_party/crazy_linker/crazy_linker/src/crazy_linker_elf_loader.cpp
(right):

https://chromiumcodereview.appspot.com/23717023/diff/92001/third_party/crazy_...
third_party/crazy_linker/crazy_linker/src/crazy_linker_elf_loader.cpp:209:
error->Format("Could not reserve %d bytes of address space",
maybe "at addr %d" ?
I mean, it'd be an error condition to load reserve the same, or overlapping
addresses, right?

https://chromiumcodereview.appspot.com/23717023/diff/92001/third_party/crazy_...
File third_party/crazy_linker/crazy_linker/src/crazy_linker_elf_relocator.h
(right):

https://chromiumcodereview.appspot.com/23717023/diff/92001/third_party/crazy_...
third_party/crazy_linker/crazy_linker/src/crazy_linker_elf_relocator.h:57: bool
Apply(SymbolResolver* resolver,
this has a good potential to be tested... I think it should be possible to
synthesize / have a golden ELF data snippet, then pass it down here with a
mocked SymbolResolver and ensure the various relocation types were applied
correctly, wdyt?

https://chromiumcodereview.appspot.com/23717023/diff/92001/third_party/crazy_...
File third_party/crazy_linker/crazy_linker/src/crazy_linker_globals.cpp (right):

https://chromiumcodereview.appspot.com/23717023/diff/92001/third_party/crazy_...
third_party/crazy_linker/crazy_linker/src/crazy_linker_globals.cpp:21: static
Globals* s_globals;
should s_globals and the lock be statics?
maybe also have "Get()" using the lock, at least to test / initialize g_globals?

https://chromiumcodereview.appspot.com/23717023/diff/92001/third_party/crazy_...
File third_party/crazy_linker/crazy_linker/src/crazy_linker_wrappers.cpp
(right):

https://chromiumcodereview.appspot.com/23717023/diff/92001/third_party/crazy_...
third_party/crazy_linker/crazy_linker/src/crazy_linker_wrappers.cpp:73:
ScopedGlobalLock lock;
nit: indent

[digit1, 2013-09-10 14:28:37]

On 2013/09/10 10:21:41, digit1 wrote:
> On 2013/09/10 10:01:21, joth wrote:
> > >
> > IIUC one exciting bit here is the sandbox processes get a file handle to
> > the ashmem page containing pointers to [virtual] functions that are
> > dereferenced in the browser process. Ensuring those pages are read-only is
> > crucial, right? (A key thing to include in unit tests? perhaps, death tests)
> > 
> You're right. The pages are mapped read-only in the renderer process by the
> linker (when using the shared RELROs), but a compromised renderer could remap
> then read-write. Fortunately, there is an API to force the ASHMEM region to
> become forcefully read-only, I'll add that to the linker, but after moving the
> code to a third_party/ CL. Thanks.
> >

Ok, I've done it in patch set 15, and didn't move this out of this CL yet.
I've also included a new target (content_linker_unittests_apk) that checks
that the shared RELRO sections exist, at least in the browser process. I'm still
looking at how to run a similar test inside a renderer process though.

>  
> > 
> > >
> > >  (* I'd prefer a more descriptive name (or one that's more obviously a
> > >> proper
> > >> name), but I realize that everyone tends an opinion on naming (and the
> > >> name is
> > >> everywhere in the code too))
> > >>
> > >
> > >
> > > I'm open to suggestions here :-)
> > >
> > >
> > I only have more variations on the line you already started..
> > digitlinker
> > skinnylinker
> > happylinker
> > 
> > 
> > 
> > >
> > >  Why does libchromeview have so many relocations? Because all kinds of
> > >> stuff
> > >> needs to be called from javaland?
> > >>
> > >
> > >
> > > It's because it's a huge C++ project (lots of VTables), and also has many
> > > constant tables that include a plethora of pointers.
> > > Even though I've submitted a few patches to reduce these in the past (e.g.
> > >
> >
>
https://code.google.com/p/**chromium/issues/detail?id=**249747%253Chttps://co...>),
> > > our RELRO section
> > > keeps growing steadidly due to the influx of new code.
> > >
> > 
> > IIRC doesn't the fact we have to link into into an .so really make the
> > difference? all other platforms link into a  single monolithic binary that
> > has more stuff fixed up at static link time? (/hazy on details now)
> > 
> > 
> > 
> > > The auto-generated JNI code has a few constant pointers to strings, but
> > > doesn't
> > > define a lot new classes, so I wouldn't say it's the root cause of this
> > > explosion.
> > >
> > >
> > >
> >
>
https://chromiumcodereview.**appspot.com/23717023/%253Chttps://chromiumcodere...>
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.

[digit1, 2013-09-10 14:54:49]

On 2013/09/10 14:21:37, bulach wrote:
> some more comments, but I'm afraid I'm reaching my limits here..
> 
> I think I know someone in NaCl team that perhaps could be able to help, do you
> think it'd be useful?
> 
> I don't want this to become a bikeshed, so I'd only add more people if you
want
> more eyes, ok?
> 
I'm ok for more eyes at this point, let's have some fun, but I'll never have
time to finish my design doc :-)

>
https://chromiumcodereview.appspot.com/23717023/diff/92001/base/android/java/...
> File base/android/java/src/org/chromium/base/Linker.java (right):
> 
>
https://chromiumcodereview.appspot.com/23717023/diff/92001/base/android/java/...
> base/android/java/src/org/chromium/base/Linker.java:59: public class Linker {
> so given the options:
>   liblinker.so
>   libbase_linker.so
>   libcontent_linker.so
>   libchromium_linker.so
>   libchrome_linker.so
> 
> my personal vote would be then to liblinker.. :)

I'm perfectly ok with that.

> the others seem tied to some other module, and may lead to confusion..
> and speaking of which: understood that this can't depend on base/ or STL,
thanks
> for clarifying!
> so how about moving this _out_ of base entirely? :)
> it's too tempting to be in base and not depend on it (together with logging,
> JNI, etc..).
> 

Do you want to move it to content/ where LibraryLoader already lives, or to a
new top-level directory? I have a hard time figuring where to place this
Chromium-but-does-not-depend-on-base code with Java APIs :-) I don't have any
preference really.

[Nico, 2013-09-10 14:56:49]

On Tue, Sep 10, 2013 at 2:11 AM, <digit@chromium.org> wrote:

> On 2013/09/08 21:24:56, Nico wrote:
>
>> This is very impressive. However, I can't help feeling that if it makes
>> sense
>>
> to
>
>> ship your own linker with your project, something went wrong somewhere
>> along
>>
> the
>
>> way :-)
>>
>
> Well, the real issue is that we can't get a Chromium Zygote on Android
> (for good
> technical reasons), so we have to be more creative to get the same
> RAM-saving
> benefits. Note that this was not started in isolation, we asked the
> Android team
> about their input on such ideas an alternate schemes. I would have
> personally
> preferred if the system could support this feature instead.
>
>
>  * Will the real android linker grow support for these features
>> eventually? If
>> not, why not?
>>
>
> The answer is no. I can't discuss Android team plans here, but there's an
> internal bug with more details (b/9378107).
>
>
>  * Have the security people looked at the
>>
> same-addresses-in-browser-and-**renderer
>
>> implication? Are they fine with this?
>>
>
> I've discussed this with jln@ a few months ago. He said this was
> acceptable
> since this is already what desktop Chrome / ChromeOS do, due to their use
> of a
> Chromium-specific Zygote.


The Zygote process is Linux-only in desktop chrome, right? (Not that that's
relevant for this CL.)


> Note that, unlike desktop Chrome, the service process
> are still running in "sandboxed mode", i.e. with a different Android UID
> that
> has no privileges, so this doesn't affect the sandboxing. I'd be happy to
> have
> Chris' take on this. He should look at the randomizeBaseLoadAddress()
> method in
> org.chromium.base.Linker.
>
>
>
>  (* I'd prefer a more descriptive name (or one that's more obviously a
>> proper
>> name), but I realize that everyone tends an opinion on naming (and the
>> name is
>> everywhere in the code too))
>>
>
>
> I'm open to suggestions here :-)


Descriptive names:
runtime_linker
startup_linker
userspace_linker (hm, I guess ld.so also runs in userspace)
in_process_linker (I guess ld.so runs in process too)
manual_linker
explicit_linker


Proper names:
ashmeld (ashmem + ld linker suffix)
weld ("web browser ld", also happens to be somewhat descriptive)


>
>
>  Why does libchromeview have so many relocations? Because all kinds of
>> stuff
>> needs to be called from javaland?
>>
>
>
> It's because it's a huge C++ project (lots of VTables), and also has many
> constant tables that include a plethora of pointers.
> Even though I've submitted a few patches to reduce these in the past (e.g.
>
https://code.google.com/p/**chromium/issues/detail?id=**249747<https://code.g...),
> our RELRO section
> keeps growing steadidly due to the influx of new code.
>
> The auto-generated JNI code has a few constant pointers to strings, but
> doesn't
> define a lot new classes, so I wouldn't say it's the root cause of this
> explosion.
>
>
>
https://chromiumcodereview.**appspot.com/23717023/<https://chromiumcodereview...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nico, 2013-09-10 14:57:19]

https://chromiumcodereview.appspot.com/23717023/diff/61001/base/android/linke...
File base/android/linker/crazy_linker_jni.cpp (right):

https://chromiumcodereview.appspot.com/23717023/diff/61001/base/android/linke...
base/android/linker/crazy_linker_jni.cpp:39: public:
On 2013/09/10 09:23:30, digit1 wrote:
> On 2013/09/06 17:08:50, jar wrote:
> > Should this comply with Chromium style?  If so, indent public/private by one
> > space.
> 
> I've done that. By the way, using "git cl format" here generates plenty of
lines
> that are > 80 characters. Did the style guide changed recently?

No. That's probably a clang-format bug. Make sure you're using clang-format
head, and file a bug at http://llvm.org/bugs (component clang subcomponent
formatter) (or go/clang-format-bug)

https://chromiumcodereview.appspot.com/23717023/diff/121001/base/android/link...
File base/android/linker/crazy_linker_jni.cc (right):

https://chromiumcodereview.appspot.com/23717023/diff/121001/base/android/link...
base/android/linker/crazy_linker_jni.cc:110: static bool
InitClassReference(JNIEnv* env,
still static

https://chromiumcodereview.appspot.com/23717023/diff/121001/base/android/link...
base/android/linker/crazy_linker_jni.cc:125: static bool InitFieldId(JNIEnv*
env,
still static

[digit1, 2013-09-10 16:40:26]

https://chromiumcodereview.appspot.com/23717023/diff/92001/base/android/java/...
File base/android/java/src/org/chromium/base/Linker.java (right):

https://chromiumcodereview.appspot.com/23717023/diff/92001/base/android/java/...
base/android/java/src/org/chromium/base/Linker.java:436: public long
mLoadAddress; // page-aligned load address.
On 2013/09/10 14:21:39, bulach wrote:
> perhaps just to be safe, repeat the comment here:
> 
> // Don't change these fields without modifying
> base/android/linker/crazy_linker.cpp too.

Done.

https://chromiumcodereview.appspot.com/23717023/diff/92001/base/android/java/...
base/android/java/src/org/chromium/base/Linker.java:447: private static class
LibInfoMap extends HashMap<String, LibInfo> {
I see, I've removed the LibInfoMap class.

https://chromiumcodereview.appspot.com/23717023/diff/92001/content/public/and...
File content/public/android/java/src/org/chromium/content/app/LibraryLoader.java
(left):

https://chromiumcodereview.appspot.com/23717023/diff/92001/content/public/and...
content/public/android/java/src/org/chromium/content/app/LibraryLoader.java:112:
Log.i(TAG, "loading: " + sLibrary);
On 2013/09/10 14:21:39, bulach wrote:
> perhaps keep this log entries? sometimes it's helpful..

Done.

https://chromiumcodereview.appspot.com/23717023/diff/92001/content/public/and...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java
(right):

https://chromiumcodereview.appspot.com/23717023/diff/92001/content/public/and...
content/public/android/java/src/org/chromium/content/browser/ChildProcessConnection.java:93:
public static final String EXTRA_CHROME_LINKER_ADDRESS =
On 2013/09/10 14:21:39, bulach wrote:
> how about EXTRA_LINKER_BASE_LOAD_ADDRESS, and similarly for the value?

Done.

https://chromiumcodereview.appspot.com/23717023/diff/92001/third_party/crazy_...
File third_party/crazy_linker/crazy_linker/minitest/minitest.cc (right):

https://chromiumcodereview.appspot.com/23717023/diff/92001/third_party/crazy_...
third_party/crazy_linker/crazy_linker/minitest/minitest.cc:1: #include
"minitest.h"
On 2013/09/10 14:21:39, bulach wrote:
> copyright info..

This comes from the NDK, but I've updated anyway (I wrote this).

By the way, I've created https://chromiumcodereview.appspot.com/23542017/ to
hold the third_party/ library, but I just realize that this makes this CL
un-testable with "git cl try" without a lot of stupid hackery. I'm going to
leave the code here for now then, if this is ok with you.

https://chromiumcodereview.appspot.com/23717023/diff/92001/third_party/crazy_...
File third_party/crazy_linker/crazy_linker/src/crazy_linker_elf_loader.cpp
(right):

https://chromiumcodereview.appspot.com/23717023/diff/92001/third_party/crazy_...
third_party/crazy_linker/crazy_linker/src/crazy_linker_elf_loader.cpp:209:
error->Format("Could not reserve %d bytes of address space",
On 2013/09/10 14:21:39, bulach wrote:
> maybe "at addr %d" ?
> I mean, it'd be an error condition to load reserve the same, or overlapping
> addresses, right?

Done.

https://chromiumcodereview.appspot.com/23717023/diff/92001/third_party/crazy_...
File third_party/crazy_linker/crazy_linker/src/crazy_linker_elf_relocator.h
(right):

https://chromiumcodereview.appspot.com/23717023/diff/92001/third_party/crazy_...
third_party/crazy_linker/crazy_linker/src/crazy_linker_elf_relocator.h:57: bool
Apply(SymbolResolver* resolver,
On 2013/09/10 14:21:39, bulach wrote:
> this has a good potential to be tested... I think it should be possible to
> synthesize / have a golden ELF data snippet, then pass it down here with a
> mocked SymbolResolver and ensure the various relocation types were applied
> correctly, wdyt?

That's a good idea, but is a lot of work, crafting an ELF symbol/hash table is
tedious. I'll see if I can come up with something.

https://chromiumcodereview.appspot.com/23717023/diff/92001/third_party/crazy_...
File third_party/crazy_linker/crazy_linker/src/crazy_linker_globals.cpp (right):

https://chromiumcodereview.appspot.com/23717023/diff/92001/third_party/crazy_...
third_party/crazy_linker/crazy_linker/src/crazy_linker_globals.cpp:21: static
Globals* s_globals;
On 2013/09/10 14:21:39, bulach wrote:
> should s_globals and the lock be statics?
I don't want to make s_globals static to avoid creating a liberal static
initializer, they're bad.
On the other hand, I've added a pthread_once to make the Get() function
thread-safe.

> maybe also have "Get()" using the lock, at least to test / initialize
g_globals?
This is already handled by ScopedGlobalsLock, the Get() is supposed to be used
when the variable is locked.

https://chromiumcodereview.appspot.com/23717023/diff/92001/third_party/crazy_...
File third_party/crazy_linker/crazy_linker/src/crazy_linker_wrappers.cpp
(right):

https://chromiumcodereview.appspot.com/23717023/diff/92001/third_party/crazy_...
third_party/crazy_linker/crazy_linker/src/crazy_linker_wrappers.cpp:73:
ScopedGlobalLock lock;
Done, but I'll submit a single patch that "git cl format" all this later.

https://chromiumcodereview.appspot.com/23717023/diff/121001/base/android/link...
File base/android/linker/crazy_linker_jni.cc (right):

https://chromiumcodereview.appspot.com/23717023/diff/121001/base/android/link...
base/android/linker/crazy_linker_jni.cc:110: static bool
InitClassReference(JNIEnv* env,
On 2013/09/10 14:57:20, Nico wrote:
> still static

Done.

https://chromiumcodereview.appspot.com/23717023/diff/121001/base/android/link...
base/android/linker/crazy_linker_jni.cc:125: static bool InitFieldId(JNIEnv*
env,
On 2013/09/10 14:57:20, Nico wrote:
> still static

Done.

[digit1, 2013-09-11 07:36:13]

For the record, patch 17 applies a "git cl format" to the whole file.

I found out that the tool was not generating lines over 80 characters wide, like
I feared. Instead, my text editor (Kate) is drunk when I use a certain font,
with a certain text size: It displays the static border at the wrong location.
Ah ah Linux...

[digit1, 2013-09-11 13:32:07]

Good news, Andrew asked me to add crazy_linker to the NDK's list of helper
libraries. I've upload an AOSP patch at [1]
What this means is:

  1/ Once accepted into the AOSP NDK directory, the crazy_linker code should be
copied into
     third_party/android_tools/ndk/

     Since this comes from the NDK repository, we won't lose them on the next
refresh, i.e.
     updating to the next NDK release in the future.

  2/ There is no need to ask for a new third_party/ directory.

On the other hand, I still don't know where to put the stuff that is currently
under base/android/linker/

I think I'll keep it there for the moment since it's a single source file.

[joth__google, 2013-09-11 16:07:39]

Naming: don't we have possible isuues if the platform ever adds system lib
with same name in future ?
+Torne

Boring and obvious may win... libchromium_linker


On Tuesday, 10 September 2013,  <digit@chromium.org> wrote:
> On 2013/09/10 14:21:37, bulach wrote:
>>
>> some more comments, but I'm afraid I'm reaching my limits here..
>
>> I think I know someone in NaCl team that perhaps could be able to help,
do you
>> think it'd be useful?
>
>> I don't want this to become a bikeshed, so I'd only add more people if
you
>
> want
>>
>> more eyes, ok?
>
> I'm ok for more eyes at this point, let's have some fun, but I'll never
have
> time to finish my design doc :-)
>
>
>
https://chromiumcodereview.appspot.com/23717023/diff/92001/base/android/java/...
>>
>> File base/android/java/src/org/chromium/base/Linker.java (right):
>
>
>
https://chromiumcodereview.appspot.com/23717023/diff/92001/base/android/java/...
>>
>> base/android/java/src/org/chromium/base/Linker.java:59: public class
Linker {
>> so given the options:
>>    liblinker.so
>>    libbase_linker.so
>>    libcontent_linker.so
>>    libchromium_linker.so
>>    libchrome_linker.so
>
>> my personal vote would be then to liblinker.. :)
>
> I'm perfectly ok with that.
>
>> the others seem tied to some other module, and may lead to confusion..
>> and speaking of which: understood that this can't depend on base/ or STL,
>
> thanks
>>
>> for clarifying!
>> so how about moving this _out_ of base entirely? :)
>> it's too tempting to be in base and not depend on it (together with
logging,
>> JNI, etc..).
>
>
> Do you want to move it to content/ where LibraryLoader already lives, or
to a
> new top-level directory? I have a hard time figuring where to place this
> Chromium-but-does-not-depend-on-base code with Java APIs :-) I don't have
any
> preference really.
>
>
> https://chromiumcodereview.appspot.com/23717023/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Torne, 2013-09-11 16:12:55]

On 2013/09/11 16:07:39, joth__google wrote:
> Naming: don't we have possible isuues if the platform ever adds system lib
> with same name in future ?
> +Torne
> 
> Boring and obvious may win... libchromium_linker

If the system adds a library of the same name and it gets loaded in the zygote
then things will break, yes, so a generic name related to linking is probably a
bad idea. I'd suggest including the name "chrome" specifically since it avoids
potential clashes with webviewchromium :)

[digit1, 2013-09-11 16:23:02]

I agree that a Chrome-specific name is better, even if it may eventually end up
in a non-Chrome content-based application (e.g. Opera, who knows).

Note that the linker is explicitely disabled for webviewchromium builds, and I
don't think there is any reason to use it there (if the library is already
loaded by the Zygote, then its RELRO section is already shared by all processes
that use it, even when they don't belong to the same application :-))

I'll change the name to libchrome_linker.so then.

[Nico, 2013-09-11 18:18:10]

On Wed, Sep 11, 2013 at 9:23 AM, <digit@chromium.org> wrote:

> I agree that a Chrome-specific name is better, even if it may eventually
> end up
> in a non-Chrome content-based application (e.g. Opera, who knows).
>
> Note that the linker is explicitely disabled for webviewchromium builds,
> and I
> don't think there is any reason to use it there (if the library is already
> loaded by the Zygote, then its RELRO section is already shared by all
> processes
> that use it, even when they don't belong to the same application :-))
>
> I'll change the name to libchrome_linker.so then.
>

libchrome_mobile_linker.so? libchrome_android_linker.so? Something that
doesn't suggest that we intend to use this on desktop (since most desktop
platforms don't use ELF)


>
>
https://chromiumcodereview.**appspot.com/23717023/<https://chromiumcodereview...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[palmer, 2013-09-11 23:03:17]

From the bug:

"""- The crazy linker ensures that libraries are loaded at the same addresses in
all processes, for a specific instance of the library. They are still randomized
for security reasons, but will be the same in the browser process and any
service (renderer) process."""

So, it doesn't matter how random the addresses are, if the adversary (a
compromised renderer process) knows the address locations for the defender
(primarily the browser, but secondarily other isolated Service processes).

So at that point you effectively do not have ASLR.

Also: Isn't distinguishing between "CRAZY_STATUS_KO" and "CRAZY_STATUS_OK" ripe
for error?

Are there unit tests ensuring that if the ashmem segment could not be made
read-only, that no read-write pages full of useful pointers are left behind?
There might be (I haven't read all of this yet), but if there are not you should
add them.

But overall I'm uncomfortable with what I understand of this CL so far.

https://codereview.chromium.org/23717023/diff/133001/third_party/crazy_linker...
File third_party/crazy_linker/crazy_linker/README.TXT (right):

https://codereview.chromium.org/23717023/diff/133001/third_party/crazy_linker...
third_party/crazy_linker/crazy_linker/README.TXT:5: address. The system linker
always randomizes the address.
ASLR is a very important security feature. We can't give it up.

https://codereview.chromium.org/23717023/diff/133001/third_party/crazy_linker...
third_party/crazy_linker/crazy_linker/README.TXT:30: - Support sharing of RELRO
sections. When two processes load the same
This is what we want, right? Wouldn't it be better to fix this in the Android
system linker, thus benefitting all applications as well as Chrome?

https://codereview.chromium.org/23717023/diff/133001/third_party/crazy_linker...
third_party/crazy_linker/crazy_linker/README.TXT:53: - Libraries loaded by the
crazy linker are not automatically closed when
Potentially exacerbating a resource consumption problem?

[palmer, 2013-09-11 23:46:42]

Regarding this:

"""
* Have the security people looked at the same-addresses-in-browser-and-renderer
implication? Are they fine with this?

I've discussed this with jln@ a few months ago. He said this was acceptable
since this is already what desktop Chrome / ChromeOS do, due to their use of a
Chromium-specific Zygote. Note that, unlike desktop Chrome, the service process
are still running in "sandboxed mode", i.e. with a different Android UID that
has no privileges, so this doesn't affect the sandboxing. I'd be happy to have
Chris' take on this. He should look at the randomizeBaseLoadAddress() method in
org.chromium.base.Linker.
"""

We (Chrome security team) don't believe it is true that Linux and ChromeOS
Chrome have the same addresses in renderers and browser processes. The browser
process is *not* spawned by the Chrome zygote; only the renderers are. So
renderers share mappings with each other, but not with the browser. We don't
believe it is wise or good to have the same mappings on both sides of a sandbox.

https://codereview.chromium.org/23717023/diff/133001/base/android/java/src/or...
File base/android/java/src/org/chromium/base/Linker.java (right):

https://codereview.chromium.org/23717023/diff/133001/base/android/java/src/or...
base/android/java/src/org/chromium/base/Linker.java:74: private static long
sBaseLoadAddress = 0;
Java long is 64 bits, but currently Android runs only on 32-bit machines. Are we
sure this class will behave gracefully if the upper 32 bits are not all 0?

https://codereview.chromium.org/23717023/diff/133001/base/android/java/src/or...
base/android/java/src/org/chromium/base/Linker.java:180: * load address,
typically in non-initial processes. This must be called
Is that "must" enforced anywhere?

[palmer, 2013-09-12 00:05:21]

> We (Chrome security team) don't believe it is true that Linux and ChromeOS
> Chrome have the same addresses in renderers and browser processes. The browser
> process is *not* spawned by the Chrome zygote; only the renderers are. So
> renderers share mappings with each other, but not with the browser. We don't
> believe it is wise or good to have the same mappings on both sides of a
sandbox.

That said, we do think it is OK (not ideal, but acceptable) for all renderers to
have the same mappings, as long as the browser has its own, completely different
mappings. So I think you can still do this, as long as the browser gets its own
mappings. And there should be a unit test, and a run-time CHECK to ensure that
the renderers can't write to the ashmem handle.

[digit1, 2013-09-12 15:48:34]

On 2013/09/11 18:18:10, Nico wrote:
> On Wed, Sep 11, 2013 at 9:23 AM, <mailto:digit@chromium.org> wrote:
> 
> > I agree that a Chrome-specific name is better, even if it may eventually
> > end up
> > in a non-Chrome content-based application (e.g. Opera, who knows).
> >
> > Note that the linker is explicitely disabled for webviewchromium builds,
> > and I
> > don't think there is any reason to use it there (if the library is already
> > loaded by the Zygote, then its RELRO section is already shared by all
> > processes
> > that use it, even when they don't belong to the same application :-))
> >
> > I'll change the name to libchrome_linker.so then.
> >
> 
> libchrome_mobile_linker.so? libchrome_android_linker.so? Something that
> doesn't suggest that we intend to use this on desktop (since most desktop
> platforms don't use ELF)
> 
In patchset 18, I've renamed the library to libchrome_android_linker.so to make
everything explicit.
The content-based unit-test for the linker has also been renamed
content_android_linker_unittests.

Hope that helps.

> 
> >
> >
>
https://chromiumcodereview.**appspot.com/23717023/%3Chttps://chromiumcoderevi...>
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[digit1, 2013-09-12 15:58:09]

On 2013/09/11 23:03:17, Chromium Palmer wrote:
> From the bug:
> 
> """- The crazy linker ensures that libraries are loaded at the same addresses
in
> all processes, for a specific instance of the library. They are still
randomized
> for security reasons, but will be the same in the browser process and any
> service (renderer) process."""
> 
> So, it doesn't matter how random the addresses are, if the adversary (a
> compromised renderer process) knows the address locations for the defender
> (primarily the browser, but secondarily other isolated Service processes).
> 
> So at that point you effectively do not have ASLR.
> 
Well, you do still have a form of ASLR that prevents a drive-by payload to
contain working hard-coded function addresses, that's why the feature is enabled
for normal single-process Android applications. However, I see your point,
having the addresses equal in the browser and renderer processes makes things a
bit easier for adversaries.

I didn't realize the fine details on the way the Chrome Linux / ChromeOS Zygote
works. I'm sorry about this because, as I wrote before, I did discuss this with
people from the Chrome security team before.

> Also: Isn't distinguishing between "CRAZY_STATUS_KO" and "CRAZY_STATUS_OK"
ripe
> for error?
> 
I've changed them to CRAZY_STATUS_SUCCESS and CRAZY_STATUS_FAILURE. The
documentation makes it clear that the values should be treated as bool, with
true == success, and false == failure, but I agree there is no reason to use
something that easy to mistake. Thanks.

> Are there unit tests ensuring that if the ashmem segment could not be made
> read-only, that no read-write pages full of useful pointers are left behind?
> There might be (I haven't read all of this yet), but if there are not you
should
> add them.
>

Yes, see content/test/content_android_linker_unittest.cc in patch 18. I also
added similar checks to the static library test suite, as well as a runtime
check in the implementation of crazy_library_use_relro_sharing() (in
third_party/crazy_library/crazy_library/src/crazy_library_shared_library.cpp).
Now the library will refuse to reuse an ashmem file descriptor if it allows
writable mappings at all. Also, the ashmem file descriptor is always closed when
this function returns (even in case of failure), to ensure that it can't later
be retrieved (e.g. by parsing /proc/self/fd) by the renderer process.

 
> But overall I'm uncomfortable with what I understand of this CL so far.
> 
>
https://codereview.chromium.org/23717023/diff/133001/third_party/crazy_linker...
> File third_party/crazy_linker/crazy_linker/README.TXT (right):
> 
>
https://codereview.chromium.org/23717023/diff/133001/third_party/crazy_linker...
> third_party/crazy_linker/crazy_linker/README.TXT:5: address. The system linker
> always randomizes the address.
> ASLR is a very important security feature. We can't give it up.
> 
Yes, but this is the README for the non-Chrome-specific static library, so I
think we can leave this here.

>
https://codereview.chromium.org/23717023/diff/133001/third_party/crazy_linker...
> third_party/crazy_linker/crazy_linker/README.TXT:30: - Support sharing of
RELRO
> sections. When two processes load the same
> This is what we want, right? Wouldn't it be better to fix this in the Android
> system linker, thus benefitting all applications as well as Chrome?
> 
No, but I'll send the details off-list.

>
https://codereview.chromium.org/23717023/diff/133001/third_party/crazy_linker...
> third_party/crazy_linker/crazy_linker/README.TXT:53: - Libraries loaded by the
> crazy linker are not automatically closed when
> Potentially exacerbating a resource consumption problem?

No, it's just that atexit() hasn't been hooked in the current library code, and
we don't need it for Android (the processes never exit(), they are killed by the
system).

[digit1, 2013-09-12 16:03:11]

https://codereview.chromium.org/23717023/diff/133001/base/android/java/src/or...
File base/android/java/src/org/chromium/base/Linker.java (right):

https://codereview.chromium.org/23717023/diff/133001/base/android/java/src/or...
base/android/java/src/org/chromium/base/Linker.java:74: private static long
sBaseLoadAddress = 0;
Dalvik has already been ported to 64-bits systems, and there are ongoing
third-party contributions to port the rest of Android to these too [1]. That's
why I'm using longs here.

The randomized value itself is always between 0x2000_00000 and 0x4000_0000 so
should never exceed the 32-bit range. I'd be interesting to see how ASLR works
on Android 64-bit systems, but these don't exist yet.

The value is passed to the native code that will always cast it as a size_t at
this point. I'll change it to check for out-of-range values in the next patch.

https://codereview.chromium.org/23717023/diff/133001/base/android/java/src/or...
base/android/java/src/org/chromium/base/Linker.java:180: * load address,
typically in non-initial processes. This must be called
On 2013/09/11 23:46:44, Chromium Palmer wrote:
> Is that "must" enforced anywhere?

I'll make the comment clearer: This simply doesn't have any effect on libraries
that have already been loaded. That's all.

[digit1, 2013-09-12 16:05:26]

On 2013/09/12 00:05:21, Chromium Palmer wrote:
> That said, we do think it is OK (not ideal, but acceptable) for all renderers
to
> have the same mappings, as long as the browser has its own, completely
different
> mappings. So I think you can still do this, as long as the browser gets its
own
> mappings. And there should be a unit test, and a run-time CHECK to ensure that
> the renderers can't write to the ashmem handle.

Yes, I've started looking into doing this instead.

Too bad this won't save as much RAM as we'd like, but it will still be useful
when we have more than one tab.

This requires changing the protocol used by the browser process to start
service/renderer ones. It's a bit subtle but hopefully should only require
changes in ChildProcessLauncher / ChildProcessService.

[JF, 2013-09-13 05:40:58]

bulach@ asked me to review. I started yesterday but haven't gotten very far yet.
Here's what I have, will do more later.

https://chromiumcodereview.appspot.com/23717023/diff/118001/base/android/java...
File base/android/java/src/org/chromium/base/Linker.java (right):

https://chromiumcodereview.appspot.com/23717023/diff/118001/base/android/java...
base/android/java/src/org/chromium/base/Linker.java:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
s/2012/2013/

https://chromiumcodereview.appspot.com/23717023/diff/118001/base/android/java...
base/android/java/src/org/chromium/base/Linker.java:70: // Set to true to when
shared RELRO creation is enabled in this process.
s/to true to/to true/

https://chromiumcodereview.appspot.com/23717023/diff/118001/base/android/java...
base/android/java/src/org/chromium/base/Linker.java:328: Log.e(TAG, "Unable to
load library: " + libName);
Shouldn't this set sLoadAtFixedAddressFailed?

https://chromiumcodereview.appspot.com/23717023/diff/118001/base/android/java...
base/android/java/src/org/chromium/base/Linker.java:336: sCurrentLoadAddress =
libInfo.mLoadAddress + libInfo.mLoadSize;
What's the reasoning in loading things linearly one after the other (expected
behavior, avoid fragmentation, ...)? I'm also not 100% sure that things work as
expected if one of the libraries fails in the middle (leaves a hole). I also
assume that you don't expect any overflow here (a developer is free to shoot
himself in the foot in many other ways, and the linker will just fail). It's not
clear to me that the native linker will forbid e.g. mapping above 0x40000000.

Maybe just comment on these things here.

https://chromiumcodereview.appspot.com/23717023/diff/118001/base/android/java...
base/android/java/src/org/chromium/base/Linker.java:403: public LibInfo(Parcel
in) {
I'm not sure this is the right place to add an assertion, but your comments
assume that this is page aligned.

https://chromiumcodereview.appspot.com/23717023/diff/118001/base/android/link...
File base/android/linker/crazy_linker_jni.cc (right):

https://chromiumcodereview.appspot.com/23717023/diff/118001/base/android/link...
base/android/linker/crazy_linker_jni.cc:63: ptr_ =
reinterpret_cast<char*>(::malloc(size_ + 1));
static_cast?

[digit1, 2013-09-18 15:17:28]

Patch set 19 contains a big refactor of this feature in order to harden the
security as suggested by Chris:

- Libraries will only be loaded at fixed/common address
  in service (renderer) processes. I.e. the address where
  they are loaded in the browser process is still
  randomized independently.

- Testing shows that there are old Android kernels which
  didn't check the permissions of Ashmem regions correctly.
  This patch includes a runtime check to detect if this
  is the case. If so, shared RELROs / common load addresses
  will be completely disabled. For the record, this happens
  in Android emulator system images (which kernel is still
  based on 2.6.29). See CVE-2011-1149 for the details.

- The shared RELRO section is always created in the browser
  process, forced read-only, then sent to each service
  process. Note that the RELRO is pre-relocated for its
  future usage address in service processes, it cannot be
  used in the browser process, and actually is not mapped
  in it.

- When a service process maps a shared RELRO region in its
  address space, it then immediately closes the
  corresponding file descriptor (even a compromised
  renderer cannot retrieve it by parsing /proc/self/fds)

In addition to this, the code has been cleaned up / simplified. There is a large
piece of documentation in Linker.java that explains how things work in more
details.

The larger change is that the Linker code has been moved
to content/ from base/. Given that so much of its API relies on multiple
processes, it makes more sense. The library is still called
"libchrome_android_linker.so" though.

This is still not the final version of the patch since:

  - The crazy_linker stuff needs to move into
    third_party/android_tools/, which is another
    repository

  - The content_android_linker_unittests now fails,
    since there is no shared RELRO section mapped in
    the browser process.

We're working on updating the test to run properly both in the browser process
and one or more renderer ones. It seems this requires a new kind of test Android
program / package though, so this may take a couple of days.

In the meantime, this should be considered feature-complete, and I welcome your
input.

https://chromiumcodereview.appspot.com/23717023/diff/118001/base/android/java...
File base/android/java/src/org/chromium/base/Linker.java (right):

https://chromiumcodereview.appspot.com/23717023/diff/118001/base/android/java...
base/android/java/src/org/chromium/base/Linker.java:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
Done. Thanks :)

https://chromiumcodereview.appspot.com/23717023/diff/118001/base/android/link...
File base/android/linker/crazy_linker_jni.cc (right):

https://chromiumcodereview.appspot.com/23717023/diff/118001/base/android/link...
base/android/linker/crazy_linker_jni.cc:63: ptr_ =
reinterpret_cast<char*>(::malloc(size_ + 1));
On 2013/09/13 05:41:00, JF wrote:
> static_cast?

Done.

[digit1, 2013-09-18 15:51:05]

Raaaah, this needs a good rebase, I'm working on it.

[digit1, 2013-09-18 16:41:40]

On 2013/09/18 15:51:05, digit1 wrote:
> Raaaah, this needs a good rebase, I'm working on it.

And done in #20

[palmer, 2013-09-18 19:48:19]

More to come, gotta head to another meeting...

https://chromiumcodereview.appspot.com/23717023/diff/19001/content/public/and...
File content/public/android/java/src/org/chromium/content/common/Linker.java
(right):

https://chromiumcodereview.appspot.com/23717023/diff/19001/content/public/and...
content/public/android/java/src/org/chromium/content/common/Linker.java:48: *
that accesses it during it. In practice, this means the swap must happen
How do you guarantee/check for this? We (jln and I) could really use it for
applying Seccomp-BPF policies. As far as we can tell, the Android Zygote spawns
various threads in the child before our code gets to have a say. Those threads
might do anything...

https://chromiumcodereview.appspot.com/23717023/diff/19001/content/public/and...
content/public/android/java/src/org/chromium/content/common/Linker.java:64: *  
browser-side buffer-overflows more difficult from a compromised
Not just buffer overflows; also use-after-free and so on. I would say simply
"This maintains the efficacy of ASLR as an exploit mitigation across the render
<-> browser privilege boundary."

https://chromiumcodereview.appspot.com/23717023/diff/19001/content/public/and...
content/public/android/java/src/org/chromium/content/common/Linker.java:75: *  
disable shared RELROs if it detects the problem. For the record, this is
Thank you. :)

Question: Are the low-end devices we want to run on still running kernels that
old?

https://chromiumcodereview.appspot.com/23717023/diff/19001/content/public/and...
content/public/android/java/src/org/chromium/content/common/Linker.java:101: * 
- Before loading any library, prepareLibraryLoad() should be called.
Is there a way to enforce/check that this happens correctly?

https://chromiumcodereview.appspot.com/23717023/diff/19001/content/public/and...
content/public/android/java/src/org/chromium/content/common/Linker.java:181: //
Used internally to initialize the linker's static data. Assume lock is held.
Can we enforce rather than assume?

https://chromiumcodereview.appspot.com/23717023/diff/19001/content/public/and...
content/public/android/java/src/org/chromium/content/common/Linker.java:184: if
(!sInitialized) {
What is the purpose of this?

https://chromiumcodereview.appspot.com/23717023/diff/19001/content/public/and...
content/public/android/java/src/org/chromium/content/common/Linker.java:237: if
(DEBUG) Log.i(TAG, "prepareLibraryLoad() called");
Use Log.d instead of checking |DEBUG|?

https://chromiumcodereview.appspot.com/23717023/diff/19001/content/public/and...
content/public/android/java/src/org/chromium/content/common/Linker.java:253: if
(DEBUG) Log.i(TAG, "finishLibraryLoad() called");
Log.d here too?

[palmer, 2013-09-18 19:51:09]

https://chromiumcodereview.appspot.com/23717023/diff/19001/content/public/and...
File content/public/android/java/src/org/chromium/content/common/Linker.java
(right):

https://chromiumcodereview.appspot.com/23717023/diff/19001/content/public/and...
content/public/android/java/src/org/chromium/content/common/Linker.java:398:
Random r = new Random();
This is not random for the purposes of security. We need SecureRandom (and,
since that is broken, we need to explicitly seed it with 16 bytes from
/dev/urandom. Or, just create a long by reading directly from urandom).

[palmer, 2013-09-18 21:56:34]

https://chromiumcodereview.appspot.com/23717023/diff/19001/content/public/and...
File content/public/android/java/src/org/chromium/content/common/Linker.java
(right):

https://chromiumcodereview.appspot.com/23717023/diff/19001/content/public/and...
content/public/android/java/src/org/chromium/content/common/Linker.java:382: //
Used internally to lazily setup the common random base load address.
As with the other method that assumes its callers have acquired the lock, I'd
document it (and preferably enforce it) here, too.

https://chromiumcodereview.appspot.com/23717023/diff/19001/content/public/and...
content/public/android/java/src/org/chromium/content/common/Linker.java:398:
Random r = new Random();
On 2013/09/18 19:51:11, Chromium Palmer wrote:
> This is not random for the purposes of security. We need SecureRandom (and,
> since that is broken, we need to explicitly seed it with 16 bytes from
> /dev/urandom. Or, just create a long by reading directly from urandom).

Sorry, wrote that in a hurry. SecureRandom is documented as the "secure" way to
get random values that attackers cannot predict or learn; Random is explicitly
*not* that (and is correctly documented to be predictable).

But, we have recently learned that Android's SecureRandom had a weak
implementation (it's fixed now, but the fix is not shipped). So, we need to
ensure that the SecureRandom generator is seeded with truly unpredictable
values, such as some bytes from /dev/urandom. It might even be more
straightforward to just generate the random (pointer-sized) int you want
directly from bytes from /dev/urandom.

(Honestly, everything wrapping a simple read from urandom is likely to be a
vulnerability factory, and in any case more complicated.)

https://chromiumcodereview.appspot.com/23717023/diff/19001/content/public/and...
content/public/android/java/src/org/chromium/content/common/Linker.java:432: //
Used for debugging only.
Might be nice to wrap the body of this method in an if (DEBUG) { ... }. The
compiler might even be able to optimize it away when |DEBUG| is false.

https://chromiumcodereview.appspot.com/23717023/diff/19001/content/public/and...
content/public/android/java/src/org/chromium/content/common/Linker.java:520: 
Nit: Only one or zero blank lines.

https://chromiumcodereview.appspot.com/23717023/diff/19001/content/public/and...
content/public/android/java/src/org/chromium/content/common/Linker.java:541:
Log.e(TAG, "Unable to load library: " + libName);
Nit: I'd construct the string only once, (a) to ensure the log message is the
same as the exception message, to avoid confusion; and (b) to avoid creating
more Strings than necessary.

https://chromiumcodereview.appspot.com/23717023/diff/19001/content/public/and...
content/public/android/java/src/org/chromium/content/common/Linker.java:574: if
(DEBUG) Log.i(TAG, "Library details " + libInfo.toString());
Nit: Another thought on this code pattern: If you want to avoid the cost of a
call to Log.foo when |DEBUG| is false --- I assume that's why you wrap them in
checks against |DEBUG| --- you can take advantage of the private static
final-ness of DEBUG and do:

private static void logDebug(String tag, String message) {
    if (DEBUG) {
        Log.d(tag, message);
    }
}

and then all those calls will not only be cheap, they will (likely) be compiled
away. And then the call sites will look cleaner, too.

https://chromiumcodereview.appspot.com/23717023/diff/19001/content/public/and...
content/public/android/java/src/org/chromium/content/common/Linker.java:627: *
IMPORTANT: Native code knows anout this class's fields, so
Typo: "about"

[digit1, 2013-09-19 17:23:48]

Yet another new version, there are quite a few changes actually:

- third_party/crazy_linker/ is gone, the sources are now
  part of third_party/android_tools/ndk which has been
  rolled by https://chromiumcodereview.appspot.com/24251007/

- had to move Linker.java et al to org.chromium.content.app/
  (from content.common/) because our checkdeps.py tool was
  very confused about the location of NativeLibraries.java
  and related imports. There is little reason to have
  LibraryLoader.java and Linker.java under .app, but moving
  all of them to .common (the more logical place) would
  require changing even more sources, so I'll leave this
  for a future patch.

- Browser process' usage of the shared RELRO is controlled
  by a new configuration variable (see Linker.java), the
  default behaviour is to only use it on low-memory devices
  (e.g. the Nexus S), where the RAM saving is much more
  significant than others.

- Changed the computation of the random common fixed address
  by reading 32-bit integers from /dev/urandom bytes 
  directly, and stopping until we get one in the appropriate
  range.

- Linker.wantSharedRelros() renamed to
  Linker.initServiceProcess() to be more explicit.

- Looks like my previous manual rebase missed some
  small but important changes in ChildProcessConnection.

- Finally, the shared library is renamed to 
  libcontent_android_linker.so. I've been told that "Chrome"
  is trade-marked, and we don't want other content-based
  projects to end up embedding a shared library with this
  name in their own APKs. So be it :)

Note that content_android_linker_unittests is still broken (only runs in the
browser process, fails when the RELRO section is not used in it). Still working
on it.

https://chromiumcodereview.appspot.com/23717023/diff/19001/content/public/and...
File content/public/android/java/src/org/chromium/content/common/Linker.java
(right):

https://chromiumcodereview.appspot.com/23717023/diff/19001/content/public/and...
content/public/android/java/src/org/chromium/content/common/Linker.java:48: *
that accesses it during it. In practice, this means the swap must happen
This is simply guaranteed by the library loader that uses this API. See
LibraryLoader changes.

Note that there are still have other Java threads running when this happen, the
guarantee is that none of these can actually run the native code from the loaded
libraries before the RELRO creation.

I believe this does not apply to your situation, which requires that no other
threads in the process exist before setting up the Seccomp-BPF policies, which
is probably impossible to enforce unless you modify the Zygote, and probably the
framework's initialization path :-(

https://chromiumcodereview.appspot.com/23717023/diff/19001/content/public/and...
content/public/android/java/src/org/chromium/content/common/Linker.java:64: *  
browser-side buffer-overflows more difficult from a compromised
Good point. Done.

https://chromiumcodereview.appspot.com/23717023/diff/19001/content/public/and...
content/public/android/java/src/org/chromium/content/common/Linker.java:75: *  
disable shared RELROs if it detects the problem. For the record, this is
On 2013/09/18 19:48:21, Chromium Palmer wrote:
> Thank you. :)
> 
> Question: Are the low-end devices we want to run on still running kernels that
> old?

No, not that old, the issue  was discovered by running the unit tests for the
crazy_linker library inside emulators (to check the x86 and MIPS
implementations), and these have a really dated kernel. For real devices, the
issue seems to have been fixed after Gingerbread, and Chromium doesn't try to
target anything before 4.0.1.

And for the record, we do a lot of performance work using humble Nexus S devices
running 4.1 or 4.2, that's as low end as we can get.

Still, I think it's important to keep the runtime check, just in case :)

https://chromiumcodereview.appspot.com/23717023/diff/19001/content/public/and...
content/public/android/java/src/org/chromium/content/common/Linker.java:101: * 
- Before loading any library, prepareLibraryLoad() should be called.
Currently, this is done by the LibraryLoader class, but I've added an assert.

https://chromiumcodereview.appspot.com/23717023/diff/19001/content/public/and...
content/public/android/java/src/org/chromium/content/common/Linker.java:181: //
Used internally to initialize the linker's static data. Assume lock is held.
On 2013/09/18 19:48:21, Chromium Palmer wrote:
> Can we enforce rather than assume?
I've added an assert Thread.holdsLock() call here, but generally, methods named
with a Locked() suffix indicate that they work on protected data assuming the
lock is held. This is a huge convention for Chromium on Android (and the Android
platform itself), and is used all over the place :-)

https://chromiumcodereview.appspot.com/23717023/diff/19001/content/public/and...
content/public/android/java/src/org/chromium/content/common/Linker.java:184: if
(!sInitialized) {
Nothing really, bad refactoring let this through :-) Also found by findbugs
apparently. Fixed.

https://chromiumcodereview.appspot.com/23717023/diff/19001/content/public/and...
content/public/android/java/src/org/chromium/content/common/Linker.java:237: if
(DEBUG) Log.i(TAG, "prepareLibraryLoad() called");
On 2013/09/18 19:48:21, Chromium Palmer wrote:
> Use Log.d instead of checking |DEBUG|?

That's a really bad idea. The documentations states that Log.d() are "compiled
in but stripped at runtime" but this is misleading. What really happens is that
the Log.d() implementation does nothing on production devices (it works when you
run the program on a userdebug image, i.e. when running in the emulator, or
doing Android system development).

However, the function call, and any operation needed to craft its parameter is
still in the application code, and you don't want that when DEBUG is set to
false.

Log.v() is similarly unuseful here, so Log.i() is needed, controlled explicitely
by the "if (DEBUG) ...". More on this below.

https://chromiumcodereview.appspot.com/23717023/diff/19001/content/public/and...
content/public/android/java/src/org/chromium/content/common/Linker.java:382: //
Used internally to lazily setup the common random base load address.
assert added. Thanks.

https://chromiumcodereview.appspot.com/23717023/diff/19001/content/public/and...
content/public/android/java/src/org/chromium/content/common/Linker.java:398:
Random r = new Random();
I'm not sure I fully understand your last paragraph. Do you mean that it's
better to read directly from /dev/urandom that using a helper class like
SecureRandom() which does who knows what? If so, I've implemented reading from
/dev/urandom here.

https://chromiumcodereview.appspot.com/23717023/diff/19001/content/public/and...
content/public/android/java/src/org/chromium/content/common/Linker.java:432: //
Used for debugging only.
I'm relying on proguard removing unused private methods from the final .class,
which happens here if DEBUG is not defined, but your suggestion is valid and
more explicit, so I've implemented it.

https://chromiumcodereview.appspot.com/23717023/diff/19001/content/public/and...
content/public/android/java/src/org/chromium/content/common/Linker.java:520: 
On 2013/09/18 21:56:36, Chromium Palmer wrote:
> Nit: Only one or zero blank lines.

Done.

https://chromiumcodereview.appspot.com/23717023/diff/19001/content/public/and...
content/public/android/java/src/org/chromium/content/common/Linker.java:541:
Log.e(TAG, "Unable to load library: " + libName);
On 2013/09/18 21:56:36, Chromium Palmer wrote:
> Nit: I'd construct the string only once, (a) to ensure the log message is the
> same as the exception message, to avoid confusion; and (b) to avoid creating
> more Strings than necessary.

Done.

https://chromiumcodereview.appspot.com/23717023/diff/19001/content/public/and...
content/public/android/java/src/org/chromium/content/common/Linker.java:574: if
(DEBUG) Log.i(TAG, "Library details " + libInfo.toString());
I don't know where you learnt about this technique, but it is _definitely_
wrong.

The Java compiler does optimizes constant final values, which means that if
DEBUG is defined as false, then a statement like:

   if (DEBUG) doSomethingCostly();

Does not get compiled in, that why all calls to Log.i() here use this idiom.

On the other hand, with your proposal, what will happen is that:

  - logDebug() will be compiled as an empty function.
  - any call to logDebug() will still be compiled in, including any costly
operation needed to craft its parameters

In other words, the compiler is not capable of optimizing out calls to empty
private static functions. Proguard won't do it either.

That's a major pet peeve of the Java language and why the "if (DEBUG)
doCostlyStuff();" idiom must be used everywhere you need that kind of
conditional logging. Again, this is used all over the place on both the Android
framework and Chrome for Android sources.

https://chromiumcodereview.appspot.com/23717023/diff/19001/content/public/and...
content/public/android/java/src/org/chromium/content/common/Linker.java:627: *
IMPORTANT: Native code knows anout this class's fields, so
On 2013/09/18 21:56:36, Chromium Palmer wrote:
> Typo: "about"

Done.

[digit1, 2013-09-19 19:40:32]

I'm not sure why all try builds fail, it seems the trybot have not rolled DEPS
properly, will look into this.

[digit1, 2013-09-19 21:46:57]

I think I got it:

- The 'includes' added in content/content.gyp refers to
third_party/android_tools/ that is not checked out in a non-Android gclient
sync. Even though it is in a conditional "os=="android"' block.

- The reason this is an include to a .gypi instead of a depends on a .gyp target
is because I wanted to be sure that crazy_library is always statically linked
into libcontent_android_linker.so, even in the case of a component build. I was
told that 'static_library' GYP target types didn't work in component builds, but
experimenting shows this is not really the case.

So the fix is to:

  - get rid of third_party/android_tools/ndk/crazy_linker.gypi and replace it
with third_party/android_tools/ndk/crazy_linker.gyp which defines a target with
type 'static_library'.
  - replace the 'includes' in content/content.gyp with a 'depends' instead.
These are properly conditionalized and don't affect non-Android builds.

This means first a patch to third_party/android_tools + a new DEPS roll, then an
update here. I'm on it.

[Nico, 2013-09-19 21:53:08]

On Thu, Sep 19, 2013 at 2:46 PM, <digit@chromium.org> wrote:

> I think I got it:
>
> - The 'includes' added in content/content.gyp refers to
> third_party/android_tools/ that is not checked out in a non-Android gclient
> sync. Even though it is in a conditional "os=="android"' block.
>
> - The reason this is an include to a .gypi instead of a depends on a .gyp
> target
> is because I wanted to be sure that crazy_library is always statically
> linked
> into libcontent_android_linker.so, even in the case of a component build.
> I was
> told that 'static_library' GYP target types didn't work in component
> builds, but
> experimenting shows this is not really the case.
>

Yes, that's not true. Who told you that?


>
> So the fix is to:
>
>   - get rid of third_party/android_tools/ndk/**crazy_linker.gypi and
> replace it
> with third_party/android_tools/ndk/**crazy_linker.gyp which defines a
> target with
> type 'static_library'.
>   - replace the 'includes' in content/content.gyp with a 'depends' instead.
> These are properly conditionalized and don't affect non-Android builds.
>
> This means first a patch to third_party/android_tools + a new DEPS roll,
> then an
> update here. I'm on it.
>
>
>
https://chromiumcodereview.**appspot.com/23717023/<https://chromiumcodereview...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[digit1, 2013-09-19 22:36:12]

On 2013/09/19 21:53:08, Nico wrote:
> Yes, that's not true. Who told you that?
>
qsr told me that a few days ago, but he may have been confused about what I was
trying to do.
Anyway, waiting for https://codereview.chromium.org/24297002/ to land to upload
the next version here.

[palmer, 2013-09-19 22:42:58]

Sigh, these draft comments didn't get submitted yesterday. Sorry about that.

https://codereview.chromium.org/23717023/diff/19001/content/common/android/li...
File content/common/android/linker/chrome_linker_jni.cc (right):

https://codereview.chromium.org/23717023/diff/19001/content/common/android/li...
content/common/android/linker/chrome_linker_jni.cc:62: ptr_ =
reinterpret_cast<char*>(::malloc(size_ + 1));
I normally don't explicitly cast the result of malloc at all.

Also, why not use new/delete instead of malloc/free? Probably doesn't matter,
just curious.

https://codereview.chromium.org/23717023/diff/19001/content/common/android/li...
content/common/android/linker/chrome_linker_jni.cc:98: // Return a pointer to
the base name from an input |path| string.
This re-defines the term "basename".

pathname = "/etc/noodles/tasty.conf"
basename(pathname) --> "tasty.conf"
dirname(pathname) --> "etc/noodles"

Since libc is loaded, just use the libc basename(3) function.

If you need the specific semantics of this function, don't call it "basename".
(Maybe GetPathnameTail or something.)

Or, if you really want the semantics of basename(3), but don't like its absurd
calling semantics, you can use strrchr(3) (note the extra "r").

https://codereview.chromium.org/23717023/diff/19001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/common/LinkerParams.java
(right):

https://codereview.chromium.org/23717023/diff/19001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/common/LinkerParams.java:1:
// Copyright 2012 The Chromium Authors. All rights reserved.
Typo: 2013

https://codereview.chromium.org/23717023/diff/19001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/common/LinkerParams.java:26:
private final String EXTRA_LINKER_PARAMS_BASE_LOAD_ADDRESS =
Can this, and |EXTRA_LINKER_PARAMS_WAIT_FOR_SHARED_RELRO| also be static?

[palmer, 2013-09-19 23:55:03]

> I believe this does not apply to your situation, which requires that no other
> threads in the process exist before setting up the Seccomp-BPF policies, which
> is probably impossible to enforce unless you modify the Zygote, and probably
the
> framework's initialization path :-(

Yeah, that's what I figured. Thanks. :)

> I'm not sure I fully understand your last paragraph. Do you mean that it's
> better to read directly from /dev/urandom that using a helper class like
> SecureRandom() which does who knows what? If so, I've implemented reading from
> /dev/urandom here.

Yes, exactly. (Just for fun, here is an example of the problem in C:
http://emboss.github.io/blog/2013/08/21/openssl-prng-is-not-really-fork-safe/)

> On the other hand, with your proposal, what will happen is that:
> 
>   - logDebug() will be compiled as an empty function.
>   - any call to logDebug() will still be compiled in, including any costly
> operation needed to craft its parameters

Ahh, that is sad. I was, full of hope, imagining that empty private functions
*and their call sites* would get optimized away, C-style. Perhaps javac has to
leave them in, in case you introspect to discover them, but maybe the JIT can
figure out that you never refer to them?

Ah well, we're digressing now. :)

Looking at the code more now...

[digit1, 2013-09-20 00:47:12]

DEPS rolled succesfully, here comes the new patch.

content_android_linker_unittests now works on low-memory devices (because the
browser process uses the shared RELRO there). However, the test still needs some
love.

I really need to go to sleep though, so see you in a few hours.

https://codereview.chromium.org/23717023/diff/19001/content/common/android/li...
File content/common/android/linker/chrome_linker_jni.cc (right):

https://codereview.chromium.org/23717023/diff/19001/content/common/android/li...
content/common/android/linker/chrome_linker_jni.cc:62: ptr_ =
reinterpret_cast<char*>(::malloc(size_ + 1));
On 2013/09/19 22:43:00, Chromium Palmer wrote:
> I normally don't explicitly cast the result of malloc at all.
>
I changed it to a static_cast<>, without a cast, this results in a compilation
error (invalid conversion from void* to char*).

> Also, why not use new/delete instead of malloc/free? Probably doesn't matter,
> just curious.

new[] will typically add an 8-byte header to each memory block it allocates, I
generally tend to avoid it when allocating simple arrays of bytes. Actually, the
issue is a little bit more complex, the C++ ABI simply mandates that the
"cookie" / header is not _required_ for elements types with trivial / empty
destructors, but the generated machine code really depends on the compiler, and
not all of them are created equal. So I developed this a long time ago out of
habit. I'll have a look at the disassembly for new char[] on this build though.

https://codereview.chromium.org/23717023/diff/19001/content/common/android/li...
content/common/android/linker/chrome_linker_jni.cc:98: // Return a pointer to
the base name from an input |path| string.
Aargh, that should be a strrchr() below, not strchr(). The code works because
the inputs never have a directory separator, and because the result is only used
in debug traces. Nice catch, I'll fix this.

https://codereview.chromium.org/23717023/diff/19001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/common/LinkerParams.java
(right):

https://codereview.chromium.org/23717023/diff/19001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/common/LinkerParams.java:1:
// Copyright 2012 The Chromium Authors. All rights reserved.
On 2013/09/19 22:43:00, Chromium Palmer wrote:
> Typo: 2013

Done.

https://codereview.chromium.org/23717023/diff/19001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/common/LinkerParams.java:26:
private final String EXTRA_LINKER_PARAMS_BASE_LOAD_ADDRESS =
Yes, this  was already fixed in #21.

[palmer, 2013-10-01 00:11:20]

https://codereview.chromium.org/23717023/diff/175001/content/common/android/l...
File content/common/android/linker/linker_jni.cc (right):

https://codereview.chromium.org/23717023/diff/175001/content/common/android/l...
content/common/android/linker/linker_jni.cc:240: jlong load_address,
Sometimes you use jlong for addresses, and other times you use size_t. I am not
sure which is right, honestly. Probably jlong, validated by IsValidAddress?

https://codereview.chromium.org/23717023/diff/175001/content/common/android/l...
content/common/android/linker/linker_jni.cc:259: // the library, even in case of
error, is intentional.
Why is it intentional?

https://codereview.chromium.org/23717023/diff/175001/content/common/android/l...
content/common/android/linker/linker_jni.cc:294: LOG_ERROR("%s: Invalid address
0x%llx", __FUNCTION__, load_address);
Nit: check our coding standards, but "%" PRIx64 may be more correct than "%llx".
(You have to #define __STDC_FORMAT_MACROS and then #include <inttypes.h> to get
PRIx64.)

(If you decide to fix it, there are a few other places in this CL to fix it too)

https://codereview.chromium.org/23717023/diff/175001/content/common/android/l...
content/common/android/linker/linker_jni.cc:387: // available since the library
hasn't been loaded yet.
Can we #include something from base/, though, to get those constants? Otherwise,
over time, these values will get out of sync...

https://codereview.chromium.org/23717023/diff/175001/content/common/android/l...
content/common/android/linker/linker_jni.cc:390: const size_t kMegaBytes = 1024
* 1024;
These can be static, too? Probably doesn't matter.

https://codereview.chromium.org/23717023/diff/175001/content/common/android/l...
content/common/android/linker/linker_jni.cc:398: LOG_INFO("%s: System physical
size is %d MB\n",
Should be %zu instead of %d, since size_t

https://codereview.chromium.org/23717023/diff/175001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java
(right):

https://codereview.chromium.org/23717023/diff/175001/content/public/android/j...
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:97:
Linker.useSharedRelros(sharedRelros);
How sure are we that only the browser could invoke this? I think this guarantees
it (from AndroidManifest.xml):

 12     <permission
android:name="com.google.android.apps.chrome.permission.CHILD_SERVICE"
 13         android:protectionLevel="signature" />

And then each SandboxedProcessService* requires callers to have this Permission.

That works for me, but I wanted to make sure it matches your intent.

https://codereview.chromium.org/23717023/diff/175001/content/public/android/j...
File content/public/android/java/src/org/chromium/content/app/LibraryLoader.java
(right):

https://codereview.chromium.org/23717023/diff/175001/content/public/android/j...
content/public/android/java/src/org/chromium/content/app/LibraryLoader.java:165:
// This is called once the library loading is done and successful.
Nit: Indentation

https://codereview.chromium.org/23717023/diff/175001/content/public/android/j...
File content/public/android/java/src/org/chromium/content/app/Linker.java
(right):

https://codereview.chromium.org/23717023/diff/175001/content/public/android/j...
content/public/android/java/src/org/chromium/content/app/Linker.java:75: *   for
details. This linker probes the system on startup and will completely
Where is this check done? Looking for call sites of disableSharedRelros doesn't
show anything likely.

https://codereview.chromium.org/23717023/diff/175001/content/public/android/j...
content/public/android/java/src/org/chromium/content/app/Linker.java:167: //  
ALWAYS       -> It is always used (not recommended).
This seems ripe for mis-use. Why have it?

https://codereview.chromium.org/23717023/diff/175001/content/public/android/j...
content/public/android/java/src/org/chromium/content/app/Linker.java:249: // use
the shared RELRO too, unless sDisallowSharedRelroInBrowser is set.
This comment is incorrect; this is the only instance of the string
"sDisallowSharedRelroInBrowser" in this file (and CL).

https://codereview.chromium.org/23717023/diff/175001/content/public/android/j...
content/public/android/java/src/org/chromium/content/app/Linker.java:288: config
== MEMORY_DEVICE_CONFIG_NORMAL;;
Nit: only need one ";"

https://codereview.chromium.org/23717023/diff/175001/content/public/android/j...
content/public/android/java/src/org/chromium/content/app/Linker.java:507: //
explicitely to avoid random conflicts.
Typo: "explicitly"

https://codereview.chromium.org/23717023/diff/175001/content/public/android/j...
content/public/android/java/src/org/chromium/content/app/Linker.java:545: *
@param input A FileInputStream opened on a cryptographically strong
The function doesn't take this argument anymore.

https://codereview.chromium.org/23717023/diff/175001/content/public/android/j...
content/public/android/java/src/org/chromium/content/app/Linker.java:558: result
= (result << 8) | (input.read() & 255);
My testing shows this code can go through the loop a surprising number of times
(tens of thousands), and it's doing I/O each time, and (jln tells me, in the
general case) that is subject to information disclosure via timing attack.

* Can we limit the amount of I/O? Such as by reading 4 bytes, masking off the
bits that are higher-order than we need, and only then doing the check for <=
maxLimit. That would greatly reduce the number of times through the loop.

* Can we make sure maxLimit is always a power of 2? If we could, we could get a
number by masking off high-order bits without introducing a bias, and then only
read 4 bytes once instead of needing a loop.

[digit1, 2013-10-01 15:40:19]

Note: the latest patch introduces a new test suite under
build/android/pylib/linker/, that can be run with:

  build/android/test_runner.py linker

After building the new content_linker_test_apk target.

I have spent the last week trying to write a test that would suit the existing
gtest and instrumentation test cases we already have, but the peculiarities of
this "feature" make it extremely difficult, inconvenient. The current code
reflects the simplest way I found to reliably test the feature in an automated
way that fit our model.

Note that content_linker_test_apk actually runs two test cases to simulate both
low-memory and regular devices.

https://chromiumcodereview.appspot.com/23717023/diff/175001/content/common/an...
File content/common/android/linker/linker_jni.cc (right):

https://chromiumcodereview.appspot.com/23717023/diff/175001/content/common/an...
content/common/android/linker/linker_jni.cc:240: jlong load_address,
On 2013/10/01 00:11:22, Chromium Palmer wrote:
> Sometimes you use jlong for addresses, and other times you use size_t. I am
not
> sure which is right, honestly. Probably jlong, validated by IsValidAddress?

jlong is only used to communicate with Java. After IsValidAddress() passes,
everything should use a size_t.

https://chromiumcodereview.appspot.com/23717023/diff/175001/content/common/an...
content/common/android/linker/linker_jni.cc:259: // the library, even in case of
error, is intentional.
On 2013/10/01 00:11:22, Chromium Palmer wrote:
> Why is it intentional?

Because otherwise the library would be closed / unloaded on scope exit.
I've reworked the code a little to make this more explicit.

https://chromiumcodereview.appspot.com/23717023/diff/175001/content/common/an...
content/common/android/linker/linker_jni.cc:294: LOG_ERROR("%s: Invalid address
0x%llx", __FUNCTION__, load_address);
But jlong is actually defined as "long long" by <jni.h>, so "lld" is the right
formatter in this case.

Actually, <jni.h> would define jlong as int64_t if HAVE_INTTYPES_H is defined,
which only happens when the header is used in the Android platform build, not
with the NDK or the Chrome build, as far as I know. That could be defined at the
top of this source, but I don't really see the point though, it only makes
things more confusing.

https://chromiumcodereview.appspot.com/23717023/diff/175001/content/common/an...
content/common/android/linker/linker_jni.cc:387: // available since the library
hasn't been loaded yet.
I really don't like that, but I've done it though. Hopefully the reviewers here
are also OWNERS of base/android/

https://chromiumcodereview.appspot.com/23717023/diff/175001/content/common/an...
content/common/android/linker/linker_jni.cc:390: const size_t kMegaBytes = 1024
* 1024;
On 2013/10/01 00:11:22, Chromium Palmer wrote:
> These can be static, too? Probably doesn't matter.
Indeed, that doesn't change anything for local integral variables.

https://chromiumcodereview.appspot.com/23717023/diff/175001/content/common/an...
content/common/android/linker/linker_jni.cc:398: LOG_INFO("%s: System physical
size is %d MB\n",
On 2013/10/01 00:11:22, Chromium Palmer wrote:
> Should be %zu instead of %d, since size_t

Done.

https://chromiumcodereview.appspot.com/23717023/diff/175001/content/public/an...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java
(right):

https://chromiumcodereview.appspot.com/23717023/diff/175001/content/public/an...
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:97:
Linker.useSharedRelros(sharedRelros);
Yes, this is part of the existing code for starting child processes, and if
anything could invoke it, there would be much more serious problems here.

https://chromiumcodereview.appspot.com/23717023/diff/175001/content/public/an...
File content/public/android/java/src/org/chromium/content/app/LibraryLoader.java
(right):

https://chromiumcodereview.appspot.com/23717023/diff/175001/content/public/an...
content/public/android/java/src/org/chromium/content/app/LibraryLoader.java:165:
// This is called once the library loading is done and successful.
On 2013/10/01 00:11:22, Chromium Palmer wrote:
> Nit: Indentation

Done.

https://chromiumcodereview.appspot.com/23717023/diff/175001/content/public/an...
File content/public/android/java/src/org/chromium/content/app/Linker.java
(right):

https://chromiumcodereview.appspot.com/23717023/diff/175001/content/public/an...
content/public/android/java/src/org/chromium/content/app/Linker.java:75: *   for
details. This linker probes the system on startup and will completely
It is done by the crazy_linker static library itself. It's invoked by
nativeCanUseSharedRelros().

See the InitFrom() method in 
https://android.googlesource.com/platform/ndk/+/master/sources/android/crazy_...

which ends up calling CheckFileDescriptorIsReadOnly() in:
https://android.googlesource.com/platform/ndk/+/master/sources/android/crazy_...

This test is also performed in the linker test program.

https://chromiumcodereview.appspot.com/23717023/diff/175001/content/public/an...
content/public/android/java/src/org/chromium/content/app/Linker.java:249: // use
the shared RELRO too, unless sDisallowSharedRelroInBrowser is set.
Yes, it was obsolete, I've removed it.
In case the code here is not clear enough, there is a sanity check on line 269
below.

https://chromiumcodereview.appspot.com/23717023/diff/175001/content/public/an...
content/public/android/java/src/org/chromium/content/app/Linker.java:288: config
== MEMORY_DEVICE_CONFIG_NORMAL;;
On 2013/10/01 00:11:22, Chromium Palmer wrote:
> Nit: only need one ";"

Done.

https://chromiumcodereview.appspot.com/23717023/diff/175001/content/public/an...
content/public/android/java/src/org/chromium/content/app/Linker.java:507: //
explicitely to avoid random conflicts.
On 2013/10/01 00:11:22, Chromium Palmer wrote:
> Typo: "explicitly"

Done.

https://chromiumcodereview.appspot.com/23717023/diff/175001/content/public/an...
content/public/android/java/src/org/chromium/content/app/Linker.java:545: *
@param input A FileInputStream opened on a cryptographically strong
On 2013/10/01 00:11:22, Chromium Palmer wrote:
> The function doesn't take this argument anymore.

Done.

https://chromiumcodereview.appspot.com/23717023/diff/175001/content/public/an...
content/public/android/java/src/org/chromium/content/app/Linker.java:558: result
= (result << 8) | (input.read() & 255);
On 2013/10/01 00:11:22, Chromium Palmer wrote:
> * Can we limit the amount of I/O? Such as by reading 4 bytes, masking off the
> bits that are higher-order than we need, and only then doing the check for <=
> maxLimit. That would greatly reduce the number of times through the loop.
> 
> * Can we make sure maxLimit is always a power of 2? If we could, we could get
a
> number by masking off high-order bits without introducing a bias, and then
only
> read 4 bytes once instead of needing a loop.

I've implemented this scheme in the latest patch. Note that maxLimit value was
0x14000,
the new code uses an equivalent of 0x8000, which still gives 32768 possible
random locations.

[digit1, 2013-10-01 15:44:40]

Another note for Chris. I added new methods in Linker.java to support testing,
but their behaviour is disabled until NativeLibraries.ENABLE_LINKER_TESTS is
set. The value of this variable is determined at build time and always false,
unless a target APK has set 'enable_chrome_linker_tests' to 1 in its GYP
definition. Right now, only content_linker_test_apk does that, and I don't plan
to change this.

Please have a look at the definition of the TestRunner interface and how it is
used here. Unfortunately, I didn't find a way to perform the same thing without
modifying the code in Linker.java itself.

[frankf, 2013-10-01 18:07:57]

On 2013/10/01 15:44:40, digit1 wrote:
> Another note for Chris. I added new methods in Linker.java to support testing,
> but their behaviour is disabled until NativeLibraries.ENABLE_LINKER_TESTS is
> set. The value of this variable is determined at build time and always false,
> unless a target APK has set 'enable_chrome_linker_tests' to 1 in its GYP
> definition. Right now, only content_linker_test_apk does that, and I don't
plan
> to change this.
> 
> Please have a look at the definition of the TestRunner interface and how it is
> used here. Unfortunately, I didn't find a way to perform the same thing
without
> modifying the code in Linker.java itself.

Adding a new test type to test_runner.py is fine (although very specific), but
the python changes deserve their
own CL (~400 lines). WDYT?

[digit1, 2013-10-01 19:46:09]

On 2013/10/01 18:07:57, frankf wrote:
> Adding a new test type to test_runner.py is fine (although very specific), but
> the python changes deserve their
> own CL (~400 lines). WDYT?

Thanks, I've created https://codereview.chromium.org/25525003
Ideally, I'd prefer everything in a single CL, but I'd happily do what the
reviewers ask me to get this reviewed more easily :-)

[digit1, 2013-10-03 14:03:12]

The 25525003 CL has been submited. I've updated a new rebased version of the
patch. Is there anything left to address here? Chris, can you take a look at CL
30? Thanks in advance.

[Chris Palmer, 2013-10-03 17:22:57]

Yeah I'll do it today.
On Oct 3, 2013 7:03 AM, <digit@chromium.org> wrote:

> The 25525003 CL has been submited. I've updated a new rebased version of
> the
> patch. Is there anything left to address here? Chris, can you take a look
> at CL
> 30? Thanks in advance.
>
>
https://chromiumcodereview.**appspot.com/23717023/<https://chromiumcodereview...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[palmer, 2013-10-04 00:05:33]

A nit and a request for another test, if practical. LGTM.

https://codereview.chromium.org/23717023/diff/209001/content/shell/android/li...
File content/shell/android/linker_test_apk/content_linker_test_linker_tests.cc
(right):

https://codereview.chromium.org/23717023/diff/209001/content/shell/android/li...
content/shell/android/linker_test_apk/content_linker_test_linker_tests.cc:31:
jboolean RunChecks(bool inBrowserProcess, bool needRelros) {
Is it possible to write a test to ensure that the library is mapped in at
different adresses (a) in browser vs. renderer, and (b) in renderers across
distinct loads (probably == process creation)?

https://codereview.chromium.org/23717023/diff/209001/content/shell/android/li...
content/shell/android/linker_test_apk/content_linker_test_linker_tests.cc:58:
std::string relro_section_prefix(kRelroSectionPrefix);
Nit: I think you can just do

    static const std::string kRelroSectionPrefix("/dev/ashmem/RELRO:");

Or, you can just use the char[] and pass it to region.path.find below. In any
case you don't need both a named char[] constant and a non-static, non-const
std::string.

[digit1, 2013-10-07 13:32:27]

Note: The new tests change the content of build/android significantly, again.
I'm creating a separate patch to address these, just as before.

https://codereview.chromium.org/23717023/diff/209001/content/shell/android/li...
File content/shell/android/linker_test_apk/content_linker_test_linker_tests.cc
(right):

https://codereview.chromium.org/23717023/diff/209001/content/shell/android/li...
content/shell/android/linker_test_apk/content_linker_test_linker_tests.cc:31:
jboolean RunChecks(bool inBrowserProcess, bool needRelros) {
Yes, I've added two four new tests that check the library load addresses, and
also their randomization (see the changes in build/android/pylib/linker).

Note that I've discovered experimentally that the randomization provided by the
system itself is apparently poor in the context of running the test. See the
comments in LinkerRandomizationTest._RunTest() in
build/android/pylib/linker/test_case.py for more details.

https://codereview.chromium.org/23717023/diff/209001/content/shell/android/li...
content/shell/android/linker_test_apk/content_linker_test_linker_tests.cc:58:
std::string relro_section_prefix(kRelroSectionPrefix);
I tend to avoid static std::string variables, since they introduce static
initializers for no good reason. I've just changed the code to use
kRelroSectionPrefix directly.

[digit1, 2013-10-07 13:40:25]

Related build/android patch at: https://codereview.chromium.org/26251003/

[digit1, 2013-10-08 11:36:59]

26251003 has been submitted, and patch set 33 rebases on top of it.

I've added jochen@ and creis@ as suggested reviewers for the following files:

  content/content.gyp
  content/content_shell.gyp
  content/content_tests.gyp

Guys, can I ask you to take a look at these three file modifications and let us
know what you think about them.

Thanks.

[Charlie Reis, 2013-10-08 20:45:29]

Sorry, this goes beyond my skill with gyp.  I'll defer to John (or Jochen, if he
sees it first).

[jochen (gone - plz use gerrit), 2013-10-09 12:08:21]

some comments.

I'll defer to jam@ on this, esp. for the base/ include

https://codereview.chromium.org/23717023/diff/234001/content/content.gyp
File content/content.gyp (right):

https://codereview.chromium.org/23717023/diff/234001/content/content.gyp#newc...
content/content.gyp:512: # This is necessary to include
base/android/sys_utils_constants.h
shouldn't you depend on the target that has this header if you use it?

https://codereview.chromium.org/23717023/diff/234001/content/content_shell.gypi
File content/content_shell.gypi (right):

https://codereview.chromium.org/23717023/diff/234001/content/content_shell.gy...
content/content_shell.gypi:656:
'shell/android/linker_test_apk/src/org/chromium/content_linker_test_apk/LinkerTests.java',
nit. alphabetical ordering

[digit1, 2013-10-09 13:33:47]

https://codereview.chromium.org/23717023/diff/234001/content/content.gyp
File content/content.gyp (right):

https://codereview.chromium.org/23717023/diff/234001/content/content.gyp#newc...
content/content.gyp:512: # This is necessary to include
base/android/sys_utils_constants.h
Actually no, it's a very special case: the header needs to be included to get a
constant value that is being used in a source file that _cannot_ depend on base/
at all. See the comments in sys_utils_constants.h and in linker_jni.cc

I've made the comment more explicit about this.

https://codereview.chromium.org/23717023/diff/234001/content/content_shell.gypi
File content/content_shell.gypi (right):

https://codereview.chromium.org/23717023/diff/234001/content/content_shell.gy...
content/content_shell.gypi:656:
'shell/android/linker_test_apk/src/org/chromium/content_linker_test_apk/LinkerTests.java',
On 2013/10/09 12:08:23, jochen wrote:
> nit. alphabetical ordering

Done.

[digit1, 2013-10-10 08:34:26]

On 2013/10/09 12:08:21, jochen wrote:
> some comments.
> 
> I'll defer to jam@ on this, esp. for the base/ include
> 
Note that none of the changes here will affect non-Android builds, and the
linker itself is only used for Android packages that explicitly want it.

Currently, I'm looking for a LGTM from the following owners:

  android_webview/ -> torne, or joth
  base/android/ -> bulach or joth
  content/app/android/ -> bulach or yfriedman
  content/public/android/ -> bulach, joth or yfriedman
  content/shell/android/ -> yfriedman
  content/*.gyp -> jam, creis or jochen?

[Yaron, 2013-10-10 10:21:10]

fyi: bulach is ooo for the rest of the week. Took a pass at everything except
linker tests but this is huge! I'll look more

https://chromiumcodereview.appspot.com/23717023/diff/245001/build/android/gyp...
File build/android/gyp/gcc_preprocess.py (right):

https://chromiumcodereview.appspot.com/23717023/diff/245001/build/android/gyp...
build/android/gyp/gcc_preprocess.py:21: '-D', 'ANDROID',
This is duplicated on L23

https://chromiumcodereview.appspot.com/23717023/diff/245001/build/android/gyp...
build/android/gyp/gcc_preprocess.py:41: parser.add_option('-D',
help='Pre-defines macros', action='append')
Nit: --defines (for consistency with above? I don't mind if you prefer to keep)

https://chromiumcodereview.appspot.com/23717023/diff/245001/build/java_apk.gypi
File build/java_apk.gypi (right):

https://chromiumcodereview.appspot.com/23717023/diff/245001/build/java_apk.gy...
build/java_apk.gypi:210:
'<(PRODUCT_DIR)/lib/libcontent_android_linker.>(android_product_extension)',
Although this works for our needs, it's more portable to use <(SHARED_LIB_DIR>
instead of <(PRODUCT_DIR)/lib

https://chromiumcodereview.appspot.com/23717023/diff/245001/content/common/an...
File content/common/android/linker/linker_jni.cc (right):

https://chromiumcodereview.appspot.com/23717023/diff/245001/content/common/an...
content/common/android/linker/linker_jni.cc:22: // A special header that
contains only constant definitions to avoid dragging
I think you can add a DEPS file in this dir that ensures you can't add those
dependencies (any only allows the one header file)

https://chromiumcodereview.appspot.com/23717023/diff/245001/content/public/an...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java
(right):

https://chromiumcodereview.appspot.com/23717023/diff/245001/content/public/an...
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:60:
// Becomes true once the service is bound.
Rather than having to loop below, you could replace this with a semaphore. OTOH,
I guess that could be worse since you're already dealing with a separate
critical section/locking mechanism so mixing techniques might be unwise. Leaving
this for consideration, but fine if you leave it.

https://chromiumcodereview.appspot.com/23717023/diff/245001/content/public/an...
File content/public/android/java/src/org/chromium/content/app/Linker.java
(right):

https://chromiumcodereview.appspot.com/23717023/diff/245001/content/public/an...
content/public/android/java/src/org/chromium/content/app/Linker.java:312:
synchronized (Linker.class) {
Can you synchronize on TAG instead? I believe we have a findbugs check for this
(have tryjobs been ok?) because technically anyone can synchronize on this and
cause surprises for you

https://chromiumcodereview.appspot.com/23717023/diff/245001/content/public/an...
content/public/android/java/src/org/chromium/content/app/Linker.java:330: /**
nit: ws

https://chromiumcodereview.appspot.com/23717023/diff/245001/content/public/an...
File content/public/android/java/src/org/chromium/content/app/LinkerParams.java
(right):

https://chromiumcodereview.appspot.com/23717023/diff/245001/content/public/an...
content/public/android/java/src/org/chromium/content/app/LinkerParams.java:58:
public LinkerParams(Intent intent) {
Nit: keep ctors next to each other (should come after fields and before other
methods0

https://chromiumcodereview.appspot.com/23717023/diff/245001/content/public/an...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
(right):

https://chromiumcodereview.appspot.com/23717023/diff/245001/content/public/an...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:168:
Log.i(TAG, String.format("sLinkerLoadAddress = 0x%x", sLinkerLoadAddress));
Does it help to put this in the log? Is it dangerous?

[jochen (gone - plz use gerrit), 2013-10-10 11:39:28]

https://codereview.chromium.org/23717023/diff/234001/content/content.gyp
File content/content.gyp (right):

https://codereview.chromium.org/23717023/diff/234001/content/content.gyp#newc...
content/content.gyp:512: # This is necessary to include
base/android/sys_utils_constants.h
On 2013/10/09 13:33:49, digit1 wrote:
> Actually no, it's a very special case: the header needs to be included to get
a
> constant value that is being used in a source file that _cannot_ depend on
base/
> at all. See the comments in sys_utils_constants.h and in linker_jni.cc
> 
> I've made the comment more explicit about this.


I'd either put this into a gyp variable, or introduce a new base target that is
safe to depend on.

[digit1, 2013-10-10 12:20:08]

https://chromiumcodereview.appspot.com/23717023/diff/245001/build/android/gyp...
File build/android/gyp/gcc_preprocess.py (right):

https://chromiumcodereview.appspot.com/23717023/diff/245001/build/android/gyp...
build/android/gyp/gcc_preprocess.py:21: '-D', 'ANDROID',
Thanks, I've removed it. I guess it came with a rebase.

https://chromiumcodereview.appspot.com/23717023/diff/245001/build/android/gyp...
build/android/gyp/gcc_preprocess.py:41: parser.add_option('-D',
help='Pre-defines macros', action='append')
Good idea. Done.

https://chromiumcodereview.appspot.com/23717023/diff/245001/build/java_apk.gypi
File build/java_apk.gypi (right):

https://chromiumcodereview.appspot.com/23717023/diff/245001/build/java_apk.gy...
build/java_apk.gypi:210:
'<(PRODUCT_DIR)/lib/libcontent_android_linker.>(android_product_extension)',
On 2013/10/10 10:21:12, Yaron wrote:
> Although this works for our needs, it's more portable to use <(SHARED_LIB_DIR>
> instead of <(PRODUCT_DIR)/lib

Done.

https://chromiumcodereview.appspot.com/23717023/diff/245001/content/common/an...
File content/common/android/linker/linker_jni.cc (right):

https://chromiumcodereview.appspot.com/23717023/diff/245001/content/common/an...
content/common/android/linker/linker_jni.cc:22: // A special header that
contains only constant definitions to avoid dragging
Excellent idea, done!

https://chromiumcodereview.appspot.com/23717023/diff/245001/content/public/an...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java
(right):

https://chromiumcodereview.appspot.com/23717023/diff/245001/content/public/an...
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:60:
// Becomes true once the service is bound.
The looping construct is already used in several other places in this source
file, so I'd prefer to keep it like that.

https://chromiumcodereview.appspot.com/23717023/diff/245001/content/public/an...
File content/public/android/java/src/org/chromium/content/app/Linker.java
(right):

https://chromiumcodereview.appspot.com/23717023/diff/245001/content/public/an...
content/public/android/java/src/org/chromium/content/app/Linker.java:312:
synchronized (Linker.class) {
Actually, I used to do it, and findbugs complained about it (apparently, one
shouldn't use a static final String object for synchronization purpose).

https://chromiumcodereview.appspot.com/23717023/diff/245001/content/public/an...
content/public/android/java/src/org/chromium/content/app/Linker.java:330: /**
On 2013/10/10 10:21:12, Yaron wrote:
> nit: ws

Done.

https://chromiumcodereview.appspot.com/23717023/diff/245001/content/public/an...
File content/public/android/java/src/org/chromium/content/app/LinkerParams.java
(right):

https://chromiumcodereview.appspot.com/23717023/diff/245001/content/public/an...
content/public/android/java/src/org/chromium/content/app/LinkerParams.java:58:
public LinkerParams(Intent intent) {
On 2013/10/10 10:21:12, Yaron wrote:
> Nit: keep ctors next to each other (should come after fields and before other
> methods0

Done.

https://chromiumcodereview.appspot.com/23717023/diff/245001/content/public/an...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
(right):

https://chromiumcodereview.appspot.com/23717023/diff/245001/content/public/an...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:168:
Log.i(TAG, String.format("sLinkerLoadAddress = 0x%x", sLinkerLoadAddress));
That was a debugging leftover, I've removed it. I've kept the one on 166 because
it can be useful to detect issues.

[digit1, 2013-10-10 12:21:39]

https://codereview.chromium.org/23717023/diff/234001/content/content.gyp
File content/content.gyp (right):

https://codereview.chromium.org/23717023/diff/234001/content/content.gyp#newc...
content/content.gyp:512: # This is necessary to include
base/android/sys_utils_constants.h
Ok, I have added build/android/low_memory_device.gypi to address this. Now I
need a base/ reviewer though :-)

thakis, could you look at the latest change in base/base.gypi?

[Yaron, 2013-10-10 13:27:26]

high-level OWNERS lgtm for the overall structure and folders I cover. I honestly
didn't look very in-depth at the linker code but it seems like other
more-qualified folk have reviewed fairly extensively

https://chromiumcodereview.appspot.com/23717023/diff/265001/content/shell/and...
File content/shell/android/linker_test_apk/AndroidManifest.xml (right):

https://chromiumcodereview.appspot.com/23717023/diff/265001/content/shell/and...
content/shell/android/linker_test_apk/AndroidManifest.xml:99: <uses-sdk
android:minSdkVersion="14" android:targetSdkVersion="17" />
18 (probably rev'd since you started)

https://chromiumcodereview.appspot.com/23717023/diff/265001/content/shell/and...
File content/shell/android/linker_test_apk/content_linker_test_linker_tests.h
(right):

https://chromiumcodereview.appspot.com/23717023/diff/265001/content/shell/and...
content/shell/android/linker_test_apk/content_linker_test_linker_tests.h:5:
#ifndef CONTENT_SHELL_ANDROID_LINKER_TEST_APK_LINKER_TESTS_H
Doesn't fully match filename.

[digit1, 2013-10-10 13:37:21]

https://chromiumcodereview.appspot.com/23717023/diff/265001/content/shell/and...
File content/shell/android/linker_test_apk/AndroidManifest.xml (right):

https://chromiumcodereview.appspot.com/23717023/diff/265001/content/shell/and...
content/shell/android/linker_test_apk/AndroidManifest.xml:99: <uses-sdk
android:minSdkVersion="14" android:targetSdkVersion="17" />
Indeed :-) Fixed.

https://chromiumcodereview.appspot.com/23717023/diff/265001/content/shell/and...
File content/shell/android/linker_test_apk/content_linker_test_linker_tests.h
(right):

https://chromiumcodereview.appspot.com/23717023/diff/265001/content/shell/and...
content/shell/android/linker_test_apk/content_linker_test_linker_tests.h:5:
#ifndef CONTENT_SHELL_ANDROID_LINKER_TEST_APK_LINKER_TESTS_H
Fixed. My god, the new macro uses 'LINKER' and 'TEST' three times each :-)

[Torne, 2013-10-10 15:49:40]

LGTM for android_webview. As far as I can see this is all disabled and fine for
us.

[Nico, 2013-10-10 16:51:08]

https://chromiumcodereview.appspot.com/23717023/diff/278001/base/base.gypi
File base/base.gypi (right):

https://chromiumcodereview.appspot.com/23717023/diff/278001/base/base.gypi#ne...
base/base.gypi:790: }],
I'm not sure we want a dependency from base on build. I didn't follow the review
too closely, maybe you added this because someone complained about the
include-only-don't-depend-on-base .h file you had previously.

Maybe this gypi file could just live in base/android ? Or can't the target that
must not depend on base include a gypi file from base/android either?

[digit1, 2013-10-11 08:00:10]

https://chromiumcodereview.appspot.com/23717023/diff/278001/base/base.gypi
File base/base.gypi (right):

https://chromiumcodereview.appspot.com/23717023/diff/278001/base/base.gypi#ne...
base/base.gypi:790: }],
On 2013/10/10 16:51:10, Nico wrote:
> I'm not sure we want a dependency from base on build. I didn't follow the
review
> too closely, maybe you added this because someone complained about the
> include-only-don't-depend-on-base .h file you had previously.
> 

Yes, that's exactly what happened. For the record, we already have
build/android/cpufeatures.gypi which is used by base/, content/, skia/ and
third_party/libwebp/. This makes sense because it is used to define a common
module that cannot be in base/

I think this use case is very similar, and I don't see the point of putting the
file under base/android/ given that another module which cannot depend on base/
will need to include it.

> Maybe this gypi file could just live in base/android ? Or can't the target
that
> must not depend on base include a gypi file from base/android either?

Yes, the other target that includes this gypi must _not_ depend on base, and
even has a DEPS file that checks for this. Hence the need for a common location,
outside of base/.

The alternative is to define something in common.gypi, but I find this less
elegant because it would become visible to all targets.

I can't help feeling that maybe we're over-doing this, the only reason for this
is to share the value of a single constant (512) which is very, very, unlikely
to change in the future :-)

[Nico, 2013-10-11 13:44:40]

OK, lgtm
On Oct 11, 2013 1:00 AM, <digit@chromium.org> wrote:

>
> https://chromiumcodereview.**appspot.com/23717023/diff/**
>
278001/base/base.gypi<https://chromiumcodereview.appspot.com/23717023/diff/278001/base/base.gypi>
> File base/base.gypi (right):
>
> https://chromiumcodereview.**appspot.com/23717023/diff/**
>
278001/base/base.gypi#**newcode790<https://chromiumcodereview.appspot.com/23717023/diff/278001/base/base.gypi#newcode790>
> base/base.gypi:790: }],
> On 2013/10/10 16:51:10, Nico wrote:
>
>> I'm not sure we want a dependency from base on build. I didn't follow
>>
> the review
>
>> too closely, maybe you added this because someone complained about the
>> include-only-don't-depend-on-**base .h file you had previously.
>>
>
>
> Yes, that's exactly what happened. For the record, we already have
> build/android/cpufeatures.gypi which is used by base/, content/, skia/
> and third_party/libwebp/. This makes sense because it is used to define
> a common module that cannot be in base/
>
> I think this use case is very similar, and I don't see the point of
> putting the file under base/android/ given that another module which
> cannot depend on base/ will need to include it.
>
>  Maybe this gypi file could just live in base/android ? Or can't the
>>
> target that
>
>> must not depend on base include a gypi file from base/android either?
>>
>
> Yes, the other target that includes this gypi must _not_ depend on base,
> and even has a DEPS file that checks for this. Hence the need for a
> common location, outside of base/.
>
> The alternative is to define something in common.gypi, but I find this
> less elegant because it would become visible to all targets.
>
> I can't help feeling that maybe we're over-doing this, the only reason
> for this is to share the value of a single constant (512) which is very,
> very, unlikely to change in the future :-)
>
>
https://chromiumcodereview.**appspot.com/23717023/<https://chromiumcodereview...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[digit1, 2013-10-11 14:02:25]

On 2013/10/11 13:44:40, Nico wrote:
> OK, lgtm

Thanks, it seems that we only need a lgtm from one of the content/*.gyp owners.
Any recommendation on this?

[Nico, 2013-10-11 14:06:31]

On Fri, Oct 11, 2013 at 7:02 AM, <digit@chromium.org> wrote:

> On 2013/10/11 13:44:40, Nico wrote:
>
>> OK, lgtm
>>
>
> Thanks, it seems that we only need a lgtm from one of the content/*.gyp
> owners.
> Any recommendation on this?
>

piman is probably a good match.


>
>
>
https://chromiumcodereview.**appspot.com/23717023/<https://chromiumcodereview...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[digit1, 2013-10-11 14:08:42]

Thanks, I've added piman as a reviewer, and send an email to jam.

[jam, 2013-10-11 14:40:21]

->Darin

[digit1, 2013-10-14 13:29:02]

On 2013/10/11 14:40:21, jam wrote:
> ->Darin

Thanks, I've also added avi@ and brettw@ as well. For those that recently got
added here: Can I ask you to take a look at the changes in these 3 files:

  content/content.gyp
  content/content_shell.gypi
  content/content_tests.gypi

I'm sorry for adding so many people here, but so far 3 other OWNERS explicitly
declined to review these changes, and I don't know how to escalate this. 
For the record, this CL shall not affect any non-Android build. If you're
curious about the motivations for this patch, please read go/chrome-linker
(warning: long design doc).

Thanks for your time.

[jar (doing other things), 2013-10-14 19:36:43]

https://codereview.chromium.org/23717023/diff/278001/base/base.gypi
File base/base.gypi (right):

https://codereview.chromium.org/23717023/diff/278001/base/base.gypi#newcode790
base/base.gypi:790: }],
On 2013/10/11 08:00:12, digit1 wrote:
> On 2013/10/10 16:51:10, Nico wrote:
> > I'm not sure we want a dependency from base on build. I didn't follow the
> review
> > too closely, maybe you added this because someone complained about the
> > include-only-don't-depend-on-base .h file you had previously.
> > 
> 
> Yes, that's exactly what happened. For the record, we already have
> build/android/cpufeatures.gypi which is used by base/, content/, skia/ and
> third_party/libwebp/. This makes sense because it is used to define a common
> module that cannot be in base/
> 
> I think this use case is very similar, and I don't see the point of putting
the
> file under base/android/ given that another module which cannot depend on
base/
> will need to include it.
> 
> > Maybe this gypi file could just live in base/android ? Or can't the target
> that
> > must not depend on base include a gypi file from base/android either?
> 
> Yes, the other target that includes this gypi must _not_ depend on base, and
> even has a DEPS file that checks for this. Hence the need for a common
location,
> outside of base/.
> 
> The alternative is to define something in common.gypi, but I find this less
> elegant because it would become visible to all targets.
> 
> I can't help feeling that maybe we're over-doing this, the only reason for
this
> is to share the value of a single constant (512) which is very, very, unlikely
> to change in the future :-)
> 

I think that usually such "sharing dependencies" are accomplished by calling
into base to force the setting (somewhere during startup?), rather than by
inclusion (by base) during compilation of external (non-base) code.

Is that workable?

[digit1, 2013-10-14 20:01:06]

https://codereview.chromium.org/23717023/diff/278001/base/base.gypi
File base/base.gypi (right):

https://codereview.chromium.org/23717023/diff/278001/base/base.gypi#newcode790
base/base.gypi:790: }],
On 2013/10/14 19:36:45, jar wrote:
> 
> I think that usually such "sharing dependencies" are accomplished by calling
> into base to force the setting (somewhere during startup?), rather than by
> inclusion (by base) during compilation of external (non-base) code.
> 
> Is that workable?
>
The main issue is that this injection needs to happen before any use of the
corresponding value, which is difficult to guarantee due to the complicated
initialization code paths on Android (yes, I'm writing code pathS, because the
way initialization works varies heavily depending on which Chromium-based APK is
being built). I really think this is going to add a lot of complexity for very
little benefits.

Again, I'm thinking we're overdoing this, I'm really tempted to hard-coding the
value in each respective component given it's so unlikely to change anyway.

[digit1, 2013-10-14 20:49:36]

Following off-line discussion with chris palmer, and isaac levy, patch set 38
changes the Linker's behaviour to use a shared RELRO in the browser process to.
It should be possible to switch back to the previous behaviour easily if needed.
Note that this doesn't change low_memory_device.gypi, and we're still looking
for content/content*gyp reviews. Thanks.

[jar (doing other things), 2013-10-14 22:30:56]

On 2013/10/14 20:01:06, digit1 wrote:
> https://codereview.chromium.org/23717023/diff/278001/base/base.gypi
> File base/base.gypi (right):
> 
> https://codereview.chromium.org/23717023/diff/278001/base/base.gypi#newcode790
> base/base.gypi:790: }],
> On 2013/10/14 19:36:45, jar wrote:
> > 
> > I think that usually such "sharing dependencies" are accomplished by calling
> > into base to force the setting (somewhere during startup?), rather than by
> > inclusion (by base) during compilation of external (non-base) code.
> > 
> > Is that workable?
> >
> The main issue is that this injection needs to happen before any use of the
> corresponding value, which is difficult to guarantee due to the complicated
> initialization code paths on Android (yes, I'm writing code pathS, because the
> way initialization works varies heavily depending on which Chromium-based APK
is
> being built). I really think this is going to add a lot of complexity for very
> little benefits.
> 
> Again, I'm thinking we're overdoing this, I'm really tempted to hard-coding
the
> value in each respective component given it's so unlikely to change anyway.

The existing (old) code has a hard-coded value (admittedly, not shared) which
seems like what you're after.

It is sad (surprising) that you can't hook a standard part of *all*
initialization pathS to achieve the setting.

You could also hard-code a "default" value, and then have the setter (called on
each path?) validate said value, if that is all you need. That would "fail safe"
and provide the semantics it sounds like you want.

[digit1, 2013-10-15 10:17:22]

On 2013/10/14 22:30:56, jar wrote:
> 
> The existing (old) code has a hard-coded value (admittedly, not shared) which
> seems like what you're after.
> 
> It is sad (surprising) that you can't hook a standard part of *all*
> initialization pathS to achieve the setting.
> 
That's the current situation and solving this would require refactoring a lot of
code, unfortunately.

> You could also hard-code a "default" value, and then have the setter (called
on
> each path?) validate said value, if that is all you need. That would "fail
safe"
> and provide the semantics it sounds like you want.

I fear this doesn't get rid of the initial problem (ensuring all paths do this),
and it risks introducing more flakiness (how do you guarantee that the function
is not called before the non-default value is injected).

I have uploaded patch set 39 which goes back to hard-coding the constant, but
adds a new linker-specific tests that will scan each source file, extract the
values and compare them. I think this takes care of the root problem much more
elegantly.

It is also possible to only change the definition of BROWSER_SHARED_RELRO_CONFIG
in Linker.java to modify the corresponding build-time-only configuration option.
All tests will adapt to the new value appropriately. I've manually verified that
they behave as expected in all three possible cases. This is to take care of
Chris' concern that there is an easy way to get back to the behaviour that was
defined in patch set 37 (to do this, change the value from
BROWSER_SHARED_RELRO_CONFIG_ALWAYS to BROWSER_SHARED_RELRO_CONFIG_LOW_RAM_ONLY).

I hope this satisifies everyone :)

[bulach, 2013-10-15 10:45:55]

lgtm, as yaron said people more qualified had looked more in depth on specific
files.. just some nits.. 

awesome work, thanks david!

https://chromiumcodereview.appspot.com/23717023/diff/300001/content/public/an...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java
(right):

https://chromiumcodereview.appspot.com/23717023/diff/300001/content/public/an...
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:60:
// Becomes true once the service is bound.
nit: only accessed while holding the mMainThread monitor.

https://chromiumcodereview.appspot.com/23717023/diff/300001/content/shell/and...
File content/shell/android/linker_test_apk/content_linker_test_linker_tests.cc
(right):

https://chromiumcodereview.appspot.com/23717023/diff/300001/content/shell/and...
content/shell/android/linker_test_apk/content_linker_test_linker_tests.cc:31:
jboolean RunChecks(bool inBrowserProcess, bool needRelros) {
nit: c_style instead of javaStyle

https://chromiumcodereview.appspot.com/23717023/diff/300001/content/shell/and...
content/shell/android/linker_test_apk/content_linker_test_linker_tests.cc:147:
LOG(INFO) << prefix << "FAIL Bad Relros sections in this process";
nit: should this be LOG(ERROR) ?

https://chromiumcodereview.appspot.com/23717023/diff/300001/content/shell/and...
File
content/shell/android/linker_test_apk/src/org/chromium/content_linker_test_apk/ContentLinkerTestApplication.java
(right):

https://chromiumcodereview.appspot.com/23717023/diff/300001/content/shell/and...
content/shell/android/linker_test_apk/src/org/chromium/content_linker_test_apk/ContentLinkerTestApplication.java:16:
private static final String PRIVATE_DATA_DIRECTORY_SUFFIX = "content_shell";
nit: perhaps change the data directory?

[digit1, 2013-10-15 13:18:43]

https://chromiumcodereview.appspot.com/23717023/diff/300001/content/public/an...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java
(right):

https://chromiumcodereview.appspot.com/23717023/diff/300001/content/public/an...
content/public/android/java/src/org/chromium/content/app/ChildProcessService.java:60:
// Becomes true once the service is bound.
On 2013/10/15 10:45:57, bulach wrote:
> nit: only accessed while holding the mMainThread monitor.

Done.

https://chromiumcodereview.appspot.com/23717023/diff/300001/content/shell/and...
File content/shell/android/linker_test_apk/content_linker_test_linker_tests.cc
(right):

https://chromiumcodereview.appspot.com/23717023/diff/300001/content/shell/and...
content/shell/android/linker_test_apk/content_linker_test_linker_tests.cc:31:
jboolean RunChecks(bool inBrowserProcess, bool needRelros) {
On 2013/10/15 10:45:57, bulach wrote:
> nit: c_style instead of javaStyle

Done.

https://chromiumcodereview.appspot.com/23717023/diff/300001/content/shell/and...
content/shell/android/linker_test_apk/content_linker_test_linker_tests.cc:147:
LOG(INFO) << prefix << "FAIL Bad Relros sections in this process";
Yes, thanks (though the test script doesn't care). Done.

https://chromiumcodereview.appspot.com/23717023/diff/300001/content/shell/and...
File
content/shell/android/linker_test_apk/src/org/chromium/content_linker_test_apk/ContentLinkerTestApplication.java
(right):

https://chromiumcodereview.appspot.com/23717023/diff/300001/content/shell/and...
content/shell/android/linker_test_apk/src/org/chromium/content_linker_test_apk/ContentLinkerTestApplication.java:16:
private static final String PRIVATE_DATA_DIRECTORY_SUFFIX = "content_shell";
I wasn't sure about this, BrowserTestsApplication.java uses this as well, so I
didn't want to change it.
I've changed it to 'content_linker_test' instead, seems to work.

[digit1, 2013-10-15 21:14:09]

friendly ping.

[jar (doing other things), 2013-10-15 23:25:31]

Patch set 40, base/android/sys_utils.cc LGTM

[jam, 2013-10-16 23:10:58]

lgtm with "chrome linker" -> "content linker". and please try to reduce the
dependencies listed out if possible

https://codereview.chromium.org/23717023/diff/318001/content/content_tests.gypi
File content/content_tests.gypi (right):

https://codereview.chromium.org/23717023/diff/318001/content/content_tests.gy...
content/content_tests.gypi:1351: '../webkit/glue/webkit_glue.gyp:glue',
are you certain that all these dependencies are needed? i.e. doesn't
content_shell_lib give most of these transitively?

[digit1, 2013-10-17 15:42:37]

On 2013/10/16 23:10:58, jam wrote:
> lgtm with "chrome linker" -> "content linker". and please try to reduce the
> dependencies listed out if possible
> 

Done! Thanks.

>
https://codereview.chromium.org/23717023/diff/318001/content/content_tests.gypi
> File content/content_tests.gypi (right):
> 
>
https://codereview.chromium.org/23717023/diff/318001/content/content_tests.gy...
> content/content_tests.gypi:1351: '../webkit/glue/webkit_glue.gyp:glue',
> are you certain that all these dependencies are needed? i.e. doesn't
> content_shell_lib give most of these transitively?

I have actually removed all superfluous dependencies, and removed any change
from content/content_shell.gypi.

On the other hand, I have no idea why the checkdeps/ is failing on Windows,
while it works on other platforms. Any one has an idea? The new DEPS file seems
ok to me.

[digit1, 2013-10-17 16:06:51]

On 2013/10/17 15:42:37, digit1 wrote:
> On the other hand, I have no idea why the checkdeps/ is failing on Windows,
> while it works on other platforms. Any one has an idea? The new DEPS file
seems
> ok to me.

And the answer is that the DEPS 'include_rules' should use [], instead of {} to
define a list, unlike what's stated in the documentation in
tools/checkdeps/builddeps.py.
I'll update it in a different CL.

[commit-bot: I haz the power, 2013-10-21 11:06:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/digit@chromium.org/23717023/343001

[commit-bot: I haz the power, 2013-10-21 11:17:48]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2013-10-21 15:52:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/digit@chromium.org/23717023/571001

[commit-bot: I haz the power, 2013-10-21 16:24:23]

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

[commit-bot: I haz the power, 2013-10-21 17:12:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/digit@chromium.org/23717023/821001

[commit-bot: I haz the power, 2013-10-21 21:40:46]

Change committed as 229921