Plugin container must be PIE on Android 5.0+

RESOLVED FIXED in Firefox 41

Status

()

Firefox for Android
General
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: snorp, Assigned: snorp)

Tracking

unspecified
Firefox 41
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
OS: Mac OS X → Android
Hardware: x86 → ARM
Version: Firefox 34 → unspecified
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.
Flags: needinfo?(mh+mozilla)
Unfortunately, PIE executables are only supported on 4.1 onwards, not 4.0.
Flags: needinfo?(mh+mozilla)
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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.
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.
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.
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.
(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.
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.
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+
Flags: needinfo?(jlund)
can we ship two versions of the plugin container and only use the PIE one where it is supported?
(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.
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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?
Flags: needinfo?(jlund) → needinfo?(coop)
(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.
Flags: needinfo?(coop)
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 :)
Created attachment 8601691 [details] [diff] [review]
Build and use a separate PIE-enabled plugin container for Android 5.0+
Attachment #8601691 - Flags: review?(nchen)
Attachment #8601691 - Flags: review?(mh+mozilla)
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.
Attachment #8601691 - Flags: review?(mh+mozilla) → review+
(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.
Created attachment 8602128 [details] [diff] [review]
Build and use a PIE plugin-container on Android 5.0+. Based on a patch by Mike Hommey.
Attachment #8599007 - Attachment is obsolete: true
Attachment #8601691 - Attachment is obsolete: true
Attachment #8601691 - Flags: review?(nchen)
Attachment #8602128 - Flags: review?(mh+mozilla)
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))
Attachment #8602128 - Flags: review?(mh+mozilla) → feedback+
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.)
Assignee: nobody → snorp
Status: NEW → ASSIGNED
Flags: needinfo?(nalexander)
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.
Flags: needinfo?(mh+mozilla)
It's likely to be something similar to:
https://code.google.com/p/android/issues/detail?id=23203
Flags: needinfo?(mh+mozilla)
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.
Depends on: 1165460
Blocks: 1170323
https://hg.mozilla.org/mozilla-central/rev/c0aa1d6f1ce4
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
(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.
Flags: needinfo?(nalexander)

Updated

2 years ago
See Also: → bug 1338809
You need to log in before you can comment on or make changes to this bug.