1141693 - Plugin container must be PIE on Android 5.0+

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

Executables on Lollipop must be position independent. We don't do that right now, because some older versions of Android (2.3?) don't support it. As a result, libplugin-container.so is busted on Android 5.0 right now. In practice I think that mainly just means that OpenH264 won't work, but we also use it to run some tests.

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

I think we have at least two options:

1) Build everything with PIE, including all of the test helpers we have running around. We would then use the new APK restriction stuff to make sure that will only install on 4.0 or higher, which is allegedly the minimum you need to run/link a PIE executable. There is some info here about the flags you need to make it run on 4.0. http://stackoverflow.com/questions/24818902/running-a-native-library-on-android-l-error-only-position-independent-executab

If we don't care about 3.x, we could actually use the existing APK split we have now, which is one APK for 2.3 and another for everything else.

2) Build two versions of the plugin container executable, and keep everything in the same APK. We'd need smarts to figure out which executable to run based on OS version and/or architecture.

I like option 1) the best, especially if we can live without creating yet another APK split. Mike do you have an opinion? I remember we talked about this before, but can't remember if we came to a decision.

Comment 2

[Mike Hommey [:glandium]]

Unfortunately, PIE executables are only supported on 4.1 onwards, not 4.0.

Comment 3

[Richard Newman [:rnewman]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #1)
> 1) Build everything with PIE, including all of the test helpers we have
> running around. We would then use the new APK restriction stuff to make sure
> that will only install on 4.0 or higher…

Adding another APK split is a lot of release engineering work, and I would expect it to take multiple weeks. If you decide to go this route, discuss with jlund, see how enthusiastic he is about making it happen!


> If we don't care about 3.x, we could actually use the existing APK split we
> have now, which is one APK for 2.3 and another for everything else.

That's really tempting, but if it's really 4.1 up, then it doesn't help.

Comment 4

[Richard Newman [:rnewman]]

Oh, and if we were to do another split, I'd think carefully about where we draw the line. It doesn't have to be 4.1 up; if we have a lot of conditionals for 4.4 or 5.0, we can put the split there and have low/mid/high.

Comment 5

[Kevin Brosnan [Ex-Mozilla]]

Android 4.0 is the next lowest population after Froyo http://developer.android.com/about/dashboards/index.html

Do our Android version breakdowns show similar results to Google Play? Marking ICS as unsupported for Flash/OpenH264 is an option.

Comment 6

[Mike Hommey [:glandium]]

Note that there /might/ be a way to create PIE executables that work on 4.0, depending on what exactly bionic does. No guarantee that it's possible, but if it is, that would surely simplify things.

Comment 7

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #6)
> Note that there /might/ be a way to create PIE executables that work on 4.0.

Yeah, no, this is actually not possible.

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

Yeah I thought from the stackoverflow comments that it was possible on 4.0, but it seems that the wrapper is required.

I think we're probably ok with breaking plugin container for 4.0 if it comes to that. I'll get a patch together.

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

Jordan, we can fix this with another APK split. How much work is that on your end? We would want to have three APKs instead of the two we have now. The resulting splits would be:

1) Gingerbread
2) Honeycomb and ICS (though we are considering dropping Honeycomb, bug 1155801)
3) JellyBean+

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

can we ship two versions of the plugin container and only use the PIE one where it is supported?

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #10)
> can we ship two versions of the plugin container and only use the PIE one
> where it is supported?

We can, but it's a little gross. I'm fighting the build system right now to figure that out.

Comment 12

[Jordan Lund (:jlund)]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #9)
> Jordan, we can fix this with another APK split. How much work is that on
> your end? We would want to have three APKs instead of the two we have now.
> The resulting splits would be:
> 
> 1) Gingerbread
> 2) Honeycomb and ICS (though we are considering dropping Honeycomb, bug
> 1155801)
> 3) JellyBean+

the first split spread over a few weeks with long tail of fixes due to hacks required. If we were to do it again, the time frame should could be reduced to a few days for continuous-integration and a week for the other bits (releases, updates) but, IMO, waiting until end of Q2 when we will be doing android builds through Taskcluster (https://bugzil.la/1118394) and not Buildbot (our current scheduler, c-i tool) would make the process dramatically simpler. Can this wait? 

I'm actually on PTO until May 13th. coop, would you mind triaging the releng side of communication for this bug?

Comment 13

[Chris Cooper [:coop] (he/him)]

(In reply to Jordan Lund (:jlund) [PTO until May 13th] from comment #12)
> I'm actually on PTO until May 13th. coop, would you mind triaging the releng
> side of communication for this bug?

Is there a timing or milestone requirement on the mobile side that I should be aware of?

If the sky is indeed not falling on Lollipop, this may sit until June. Realistically speaking, no one in releng would be able to get up-to-speed on the split APK work before Jordan gets back from PTO. There is already enough special, one-off stuff happening for Fx38 that is keeping releng occupied otherwise.

Comment 14

[Mike Hommey [:glandium]]

Attachment: Build a PIE plugin container

This is the base of what you'd need to build a second plugin-container with PIE on Android. This would need to only be enabled on the HC+ builds, hooked to be copied in the right place in the APK, and hooked to be used accordingly. I'll leave all that to snorp :)

Comment 15

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Build and use a separate PIE-enabled plugin container for Android 5.0+

Comment 16

[Mike Hommey [:glandium]]

Comment on attachment 8601691 [details] [diff] [review]
Build and use a separate PIE-enabled plugin container for Android 5.0+

Review of attachment 8601691 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/app/android/pie/moz.build
@@ +6,5 @@
> +
> +Program(CONFIG['MOZ_CHILD_PROCESS_NAME_PIE'])
> +include('../common.build')
> +
> +CFLAGS += ['-pie']

That's unnecessary (and CFLAGS are not used to compile C++ ;) )

@@ +7,5 @@
> +Program(CONFIG['MOZ_CHILD_PROCESS_NAME_PIE'])
> +include('../common.build')
> +
> +CFLAGS += ['-pie']
> +LDFLAGS += ['-fPIE', '-pie']
\ No newline at end of file

-fPIE is a compiler flag, and we don't need it anyways, we're already building everything with -fPIC, which has the same effect. So you only really need -pie in LDFLAGS.

::: ipc/app/moz.build
@@ +100,5 @@
> +    if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gonk':
> +        OS_LIBS += [
> +            'binder',
> +            'utils',
> +        ]

Indenting this whole block seems overkill.

::: mobile/android/geckoview_library/Makefile.in
@@ +48,5 @@
>  	$(RM) libs/gecko-R.jar
>  
>  	# Copy the SOs. The latter should be $(MOZ_CHILD_PROCESS_NAME), but
>  	# it includes a "lib/" prefix.
> +	cp $(_ABS_DIST)/bin/libmozglue.so $(_ABS_DIST)/bin/libplugin-container.so $(_ABS_DIST)/bin/libplugin-container-pie.so libs/$(ANDROID_CPU_ARCH)/

might as well use the MOZ_CHILD_PROCESS_NAME* variables while you're here.

Comment 17

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Mike Hommey [:glandium] from comment #14)
> Created attachment 8599007 [details] [diff] [review]
> Build a PIE plugin container
> 
> This is the base of what you'd need to build a second plugin-container with
> PIE on Android. This would need to only be enabled on the HC+ builds, hooked
> to be copied in the right place in the APK, and hooked to be used
> accordingly. I'll leave all that to snorp :)

I didn't even see this patch, sorry! I think I'll rework mine to be more like that.

Comment 18

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Build and use a PIE plugin-container on Android 5.0+. Based on a patch by Mike Hommey.

Comment 19

[Mike Hommey [:glandium]]

Comment on attachment 8602128 [details] [diff] [review]
Build and use a PIE plugin-container on Android 5.0+. Based on a patch by Mike Hommey.

Review of attachment 8602128 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/app/pie/Makefile.in
@@ +3,5 @@
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +ifneq ($(dir $(PROGRAM)),./)
> +  GENERATED_DIRS = $(dir $(PROGRAM))
> +endif

Considering you're removing the lib/ in MOZ_CHILD_PROCESS_NAME*, that's not necessary (and can be removed in ipc/app/Makefile.in too)

::: ipc/app/pie/moz.build
@@ +28,5 @@
> +	LDFLAGS += ['--param lto-partitions=1']
> +
> +LDFLAGS += ['-pie']
> +
> +FAIL_ON_WARNINGS = True

Please add a note in ipc/app/moz.build that pie/moz.build need to be kept in sync.

::: ipc/glue/moz.build
@@ +156,5 @@
>  include('/ipc/chromium/chromium-config.mozbuild')
>  
>  FINAL_LIBRARY = 'xul'
>  
> +for var in ('MOZ_CHILD_PROCESS_NAME', 'MOZ_CHILD_PROCESS_NAME_PIE', 'MOZ_CHILD_PROCESS_BUNDLE',

Please wrap.

::: mobile/android/geckoview_library/Makefile.in
@@ +48,5 @@
>  	$(RM) libs/gecko-R.jar
>  
>  	# Copy the SOs. The latter should be $(MOZ_CHILD_PROCESS_NAME), but
>  	# it includes a "lib/" prefix.
> +	cp $(_ABS_DIST)/bin/libmozglue.so $(_ABS_DIST)/bin/$(MOZ_CHILD_PROCESS_NAME) $(_ABS_DIST)/bin/$(MOZ_CHILD_PROCESS_NAME_PIE) libs/$(ANDROID_CPU_ARCH)/

$(addprefix $(_ABS_DIST)/bin,libmozglue.so $(MOZ_CHILD_PROCESS_NAME) $(MOZ_CHILD_PROCESS_NAME_PIE))

Comment 20

[Nick Alexander :nalexander [he/him]]

NI to me to verify that this is handled smoothly by the Gradle configuration.  (I expect it will be, since the new library is written to lib/ like the old.)

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb6329699801

Comment 22

[James Willcox (:snorp) ([e-mail]) (he/him)]

This bounced, because it failed to build on Android x86. https://treeherder.mozilla.org/logviewer.html#?job_id=9858899&repo=mozilla-inbound

The failure is:

/builds/slave/m-in-and-x86-00000000000000000/build/android-ndk/toolchains/x86-4.7/prebuilt/linux-x86_64/bin/../lib/gcc/i686-linux-android/4.7/../../../../i686-linux-android/bin/ld: error: read-only segment has dynamic relocations 

It built fine with here with NDK r10d, but fails with r8e. Mike do you have any idea? Linker bug? I'll try to figure it out, but don't see any obvious cause.

Comment 23

[Mike Hommey [:glandium]]

It's likely to be something similar to:
https://code.google.com/p/android/issues/detail?id=23203

Comment 24

[James Willcox (:snorp) ([e-mail]) (he/him)]

Indeed, passing -nostartfiles seems to avoid that linking error (though I get something else after, and the resulting binary would not be usable). I think we just need to use the newer NDK to get around this on x86.

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c0aa1d6f1ce4

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c0aa1d6f1ce4

Comment 27

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #20)
> NI to me to verify that this is handled smoothly by the Gradle
> configuration.  (I expect it will be, since the new library is written to
> lib/ like the old.)

Locally, it looks fine here.