Never unmap memory reserved for RELRO file creation.

Never unmap memory reserved for RELRO file creation.

After loading a library with android_dlopen_ext and passing the flag
ANDROID_DLEXT_RESERVED_ADDRESS, a subsequent dlclose will unmap the
part of the reservation occupied by the library, but leave the
remainder of the reservation mapped. If we ourselves later unmap the
entire reservation, we may also unmap data from a thread that happened
to map into the hole created in our reservation by the dlclose unmap.
This is an unpleasant mmap/munmap threads race.

Because dlclose's unmap is opaque to us, we cannot readily unmap the
remaining portions of our reservation because we do not know exactly
where they now start and end. However, we only dlclose on relro
creation, and this occurs just once, on browser process startup. By
leaving these remaining reservation portions mapped but unused, we
'waste' a few Mb of virtual address space, but crucially we do not
waste actual memory because these addresses are never used by anything.

Implemented by explicitly releasing the reservation scoped mapping
*before* the dlclose call in relro creation.

Also, make failure to trim address space on library load for run a
non-fatal error (warning). Failure to munmap can really only be due
to coding error. And even if it does fail the library code can and
will still run okay, albeit again with minor loss of some virtual
address space that we otherwise might have recovered.

BUG=568880
Committed: https://crrev.com/b857f7676bc16665ca86d81db877dc11094bcb7a
Cr-Commit-Position: refs/heads/master@{#370015}

[simonb (inactive), 2016-01-15 13:26:12]

The CQ bit was checked by simonb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-15 13:26:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1583093007/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1583093007/1

[simonb (inactive), 2016-01-15 13:27:19]

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

[commit-bot: I haz the power, 2016-01-15 13:58:55]

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

[commit-bot: I haz the power, 2016-01-15 13:58:55]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2016-01-15 15:56:00]

LGTM.

https://codereview.chromium.org/1583093007/diff/1/base/android/linker/modern_...
File base/android/linker/modern_linker_jni.cc (right):

https://codereview.chromium.org/1583093007/diff/1/base/android/linker/modern_...
base/android/linker/modern_linker_jni.cc:316: LOG_ERROR("WARNING: library
reservation was too small");
LOG_WARN ?

https://codereview.chromium.org/1583093007/diff/1/base/android/linker/modern_...
base/android/linker/modern_linker_jni.cc:545: // Release the scoped mapping. See
comment in LoadLibrary() above for more.
Isn't this covered by the comment below on dlclose ?

https://codereview.chromium.org/1583093007/diff/1/base/android/linker/modern_...
base/android/linker/modern_linker_jni.cc:559: // significant issue.
Please add the TODO here as discussed.

[simonb (inactive), 2016-01-15 16:18:18]

https://codereview.chromium.org/1583093007/diff/1/base/android/linker/modern_...
File base/android/linker/modern_linker_jni.cc (right):

https://codereview.chromium.org/1583093007/diff/1/base/android/linker/modern_...
base/android/linker/modern_linker_jni.cc:316: LOG_ERROR("WARNING: library
reservation was too small");
On 2016/01/15 at 15:56:00, rmcilroy wrote:
> LOG_WARN ?

Doesn't exist, unfortunately. The linker JNI code has its own cut-down version
of base logging (can't use base directly), and LOG_WARN is one of the missing
bits.

https://codereview.chromium.org/1583093007/diff/1/base/android/linker/modern_...
base/android/linker/modern_linker_jni.cc:545: // Release the scoped mapping. See
comment in LoadLibrary() above for more.
On 2016/01/15 at 15:56:00, rmcilroy wrote:
> Isn't this covered by the comment below on dlclose ?

Sort of, but also for reasons in LoadLibrary(), which are valid in addition to
the note before dlclose().

https://codereview.chromium.org/1583093007/diff/1/base/android/linker/modern_...
base/android/linker/modern_linker_jni.cc:559: // significant issue.
On 2016/01/15 at 15:56:00, rmcilroy wrote:
> Please add the TODO here as discussed.

Done.

[dimitry, 2016-01-15 22:18:10]

dimitry@google.com changed reviewers:
+ dimitry@google.com

[dimitry, 2016-01-15 22:18:10]

https://codereview.chromium.org/1583093007/diff/20001/base/android/linker/mod...
File base/android/linker/modern_linker_jni.cc (right):

https://codereview.chromium.org/1583093007/diff/20001/base/android/linker/mod...
base/android/linker/modern_linker_jni.cc:311: if (munmap(unmap, length) == -1) {
what is going to happen here once bionic linker starts unmapping the whole
reserved segment?

[simonb (inactive), 2016-01-18 11:43:30]

On 2016/01/15 at 22:18:10, dimitry wrote:
>
https://codereview.chromium.org/1583093007/diff/20001/base/android/linker/mod...
> File base/android/linker/modern_linker_jni.cc (right):
> 
>
https://codereview.chromium.org/1583093007/diff/20001/base/android/linker/mod...
> base/android/linker/modern_linker_jni.cc:311: if (munmap(unmap, length) == -1)
{
> what is going to happen here once bionic linker starts unmapping the whole
reserved segment?

Is it the proposal, then, that future bionic linkers should unmap the whole
reserved segment?

I think my suggestion would actually be for bionic to take the opposite
approach. That is, for dlclose to not unmap anything that dlopen[_ext] did not
map. That preserves conventional memory allocation and ownership semantics.

If future bionic linkers do indeed unmap the whole reservation, this unmap might
require(*) some form of version-specific handling. It is hard to see exactly how
to achieve the required result in that case though, since dlclose would only
occur after the library is no longer required, meaning lost virtual address
space. Unfortunately it is not at all trivial for the libdl client code to find
the size of virtual address space needed for a library ahead of actually loading
it -- the only way is to read its ELF headers and parse them directly, which is
what the crazy linker did and is also what we wanted to get away from by moving
this into the system linker.

(*) In practice, we never dlclose or unload the running libchrome.so on Android,
just terminate the process. So no problem should arise here. But... this is
specific to the way the chromium linker uses dlopen_ext. Other caller code may
not be so lucky.

[simonb (inactive), 2016-01-18 12:03:46]

The CQ bit was checked by simonb@chromium.org

[simonb (inactive), 2016-01-18 12:03:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1583093007/#ps20001
(title: "Update for review feedback.")

[commit-bot: I haz the power, 2016-01-18 12:03:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1583093007/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1583093007/20001

[commit-bot: I haz the power, 2016-01-18 12:44:22]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-01-18 12:45:17]

Description was changed from

==========
Never unmap memory reserved for RELRO file creation.

After loading a library with android_dlopen_ext and passing the flag
ANDROID_DLEXT_RESERVED_ADDRESS, a subsequent dlclose will unmap the
part of the reservation occupied by the library, but leave the
remainder of the reservation mapped. If we ourselves later unmap the
entire reservation, we may also unmap data from a thread that happened
to map into the hole created in our reservation by the dlclose unmap.
This is an unpleasant mmap/munmap threads race.

Because dlclose's unmap is opaque to us, we cannot readily unmap the
remaining portions of our reservation because we do not know exactly
where they now start and end. However, we only dlclose on relro
creation, and this occurs just once, on browser process startup. By
leaving these remaining reservation portions mapped but unused, we
'waste' a few Mb of virtual address space, but crucially we do not
waste actual memory because these addresses are never used by anything.

Implemented by explicitly releasing the reservation scoped mapping
*before* the dlclose call in relro creation.

Also, make failure to trim address space on library load for run a
non-fatal error (warning). Failure to munmap can really only be due
to coding error. And even if it does fail the library code can and
will still run okay, albeit again with minor loss of some virtual
address space that we otherwise might have recovered.

BUG=568880
==========

to

==========
Never unmap memory reserved for RELRO file creation.

After loading a library with android_dlopen_ext and passing the flag
ANDROID_DLEXT_RESERVED_ADDRESS, a subsequent dlclose will unmap the
part of the reservation occupied by the library, but leave the
remainder of the reservation mapped. If we ourselves later unmap the
entire reservation, we may also unmap data from a thread that happened
to map into the hole created in our reservation by the dlclose unmap.
This is an unpleasant mmap/munmap threads race.

Because dlclose's unmap is opaque to us, we cannot readily unmap the
remaining portions of our reservation because we do not know exactly
where they now start and end. However, we only dlclose on relro
creation, and this occurs just once, on browser process startup. By
leaving these remaining reservation portions mapped but unused, we
'waste' a few Mb of virtual address space, but crucially we do not
waste actual memory because these addresses are never used by anything.

Implemented by explicitly releasing the reservation scoped mapping
*before* the dlclose call in relro creation.

Also, make failure to trim address space on library load for run a
non-fatal error (warning). Failure to munmap can really only be due
to coding error. And even if it does fail the library code can and
will still run okay, albeit again with minor loss of some virtual
address space that we otherwise might have recovered.

BUG=568880

Committed: https://crrev.com/b857f7676bc16665ca86d81db877dc11094bcb7a
Cr-Commit-Position: refs/heads/master@{#370015}
==========

[commit-bot: I haz the power, 2016-01-18 12:45:18]

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