588607 - Implement custom dlopen to load libraries from apk

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Comment 1

[Michael Wu [:mwu]]

Attachment: 0. Add bionic linker files to other-licenses/android

From http://android.git.kernel.org/?p=platform/bionic.git;a=commit;h=312be567a03aaf851707a268807ee666b12f8c74

Comment 2

[Michael Wu [:mwu]]

Attachment: 1. Add support for reading libraries out of the apk

This patch adds a APKOpen.cpp to read libraries to read APKs, extract them, load them, and then forward the functions called by the java wrapper to libxul. It also hacks the bionic linker to read libraries out of memory without an associated fd.

Comment 3

[Michael Wu [:mwu]]

Attachment: 2. Make the Java wrapper use the new library loader

Comment 4

[Michael Wu [:mwu]]

Attachment: 3. Add support for the custom library loader to plugin-container

This patch is pretty terrible. Should I fork this file into an Android specific version?

Comment 5

[Michael Wu [:mwu]]

Attachment: 4. Wrap the dl* functions with our own version

Pretty ugly. Guess we'll need our own wrapper variables? Or maybe turn the wrapper vars into something more generic.

Comment 6

[Michael Wu [:mwu]]

For anyone testing these patches, the nspr configure needs to be regenerated after applying these patches.

Comment 7

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

Comment on attachment 478944 [details] [diff] [review]
1. Add support for reading libraries out of the apk

you should use hg rename for moving these files

Comment 8

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

Comment on attachment 478945 [details] [diff] [review]
2. Make the Java wrapper use the new library loader

>-libs/armeabi/%: $(DIST)/lib/%
>-	@$(NSINSTALL) -D libs/armeabi
>+lib/%: $(DIST)/lib/%
>+	@$(NSINSTALL) -D lib/armeabi
why do you need to create the armeabi folder here?

seems like you create it here again:
>+	$(NSINSTALL) -D lib/armeabi
>+	mv lib/libmozutils.so lib/armeabi

Comment 9

[Michael Wu [:mwu]]

(In reply to comment #8)
> Comment on attachment 478945 [details] [diff] [review]
> 2. Make the Java wrapper use the new library loader
> 
> >-libs/armeabi/%: $(DIST)/lib/%
> >-	@$(NSINSTALL) -D libs/armeabi
> >+lib/%: $(DIST)/lib/%
> >+	@$(NSINSTALL) -D lib/armeabi
> why do you need to create the armeabi folder here?
> 
> seems like you create it here again:
> >+	$(NSINSTALL) -D lib/armeabi
> >+	mv lib/libmozutils.so lib/armeabi

I'll remove one of them.

Comment 10

[Luis Villa [Outside Counsel; for non-law use [e-mail]]]

Comment on attachment 478935 [details] [diff] [review]
0. Add bionic linker files to other-licenses/android

r+ generally (to the extent I can really r anything :), but note that you should put this in about:license as well. Might want to file a separate bug for that.

Comment 11

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

Comment on attachment 478945 [details] [diff] [review]
2. Make the Java wrapper use the new library loader

>-libs/armeabi/%: $(DIST)/lib/%
>-	@$(NSINSTALL) -D libs/armeabi
>+lib/%: $(DIST)/lib/%
>+	@$(NSINSTALL) -D lib/armeabi
no need to create the armeabi dir here

> 	@cp -L -v $< $@
> 	@$(DO_STRIP) $@
> 
> # Bug 567873 - Android packaging should use standard packaging code
>-dist: FORCE
>+dist: FORCE $(FULL_LIBS)
as I've understand it, if you have FORCE here, adding anything else is superfluous
 

>+dist.zip: dist
>+	rm -f dist.zip
>+	cd dist && zip -r8D ../dist.zip .
Does this pull armeabi/libgeckoutils.so into the zip? I'd expect that to be unneeded

Comment 12

[(dormant account)]

Comment on attachment 478944 [details] [diff] [review]
1. Add support for reading libraries out of the apk

r-
* for commented out code
* using raw numbers instead of constants

r+ will come with the agreement that the extra memcpy will be removed in a future enhancement.

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 478946 [details] [diff] [review]
3. Add support for the custom library loader to plugin-container

Yeah, let's have a different file for this.

Comment 14

[Michael Wu [:mwu]]

Attachment: 4a. Wrap the dl* functions with our own version

Comment 15

[Michael Wu [:mwu]]

Attachment: 4b. Wrap the dl* functions with our own version (NSPR)

Comment 16

[Michael Wu [:mwu]]

Attachment: 4a. Wrap the dl* functions with our own version, v2

System zlib required to build.

Comment 17

[Michael Wu [:mwu]]

Attachment: 3. Add support for the custom library loader to plugin-container, v2

Comment 18

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 480748 [details] [diff] [review]
3. Add support for the custom library loader to plugin-container, v2

>--- /dev/null
>+++ b/ipc/app/MozillaRuntimeMainAndroid.cpp
>+int
>+main(int argc, char* argv[])
>+{
>+    // Check for the absolute minimum number of args we need to move
>+    // forward here. We expect the last arg to be the child process type.
>+    if (argc < 1)

ITYM < 2 here.

>+    void *mozloader_handle = dlopen("libmozutils.so", RTLD_LAZY);
>+    if (!mozloader_handle)
>+        __android_log_print(ANDROID_LOG_ERROR, "GeckoChildLoad",
>+                            "Couldn't load mozloader because %s", dlerror());
>+

exit on error.

>+    typedef int (*ChildProcessInit_t)(int, char**);
>+    ChildProcessInit_t fChildProcessInit =
>+        (ChildProcessInit_t)dlsym(mozloader_handle, "ChildProcessInit");
>+    if (!fChildProcessInit)
>+        __android_log_print(ANDROID_LOG_ERROR, "GeckoChildLoad",
>+                            "Couldn't load cpi_t because %s", dlerror());
>+

exit on error.

Comment 19

[Michael Wu [:mwu]]

(In reply to comment #18)
> Comment on attachment 480748 [details] [diff] [review]
> 3. Add support for the custom library loader to plugin-container, v2
> 
> >--- /dev/null
> >+++ b/ipc/app/MozillaRuntimeMainAndroid.cpp
> >+int
> >+main(int argc, char* argv[])
> >+{
> >+    // Check for the absolute minimum number of args we need to move
> >+    // forward here. We expect the last arg to be the child process type.
> >+    if (argc < 1)
> 
> ITYM < 2 here.
> 
This was code copied from MozillaRuntimeMain.cpp. Should it be 2 there too?

Comment 20

[Michael Wu [:mwu]]

Attachment: 1a. Fix review comments, add passthrough mode

This fixes review comments and adds a new mode for debug builds which extracts the libraries and loads them through the system linker.

Comment 21

[Michael Wu [:mwu]]

Attachment: 3. Add support for the custom library loader to plugin-container, v3

Comment 22

[(not currently active) Ted Mielczarek]

Comment on attachment 479977 [details] [diff] [review]
4b. Wrap the dl* functions with our own version (NSPR)

># HG changeset patch
># Parent 0bc7433dc518b4d49f47f2356774406a4422b018
>
>diff --git a/nsprpub/configure.in b/nsprpub/configure.in
>--- a/nsprpub/configure.in
>+++ b/nsprpub/configure.in
>@@ -194,6 +194,8 @@ if test "$target" = "arm-android-eabi" ;
>         HOST_LDFLAGS=" "
>     fi
> 
>+    WRAP_MALLOC_CFLAGS="-Wl,--wrap -Wl,dlopen -Wl,--wrap -Wl,dlclose -Wl,--wrap -Wl,dlerror -Wl,--wrap -Wl,dlsym -Wl,--wrap -Wl,dladdr"

Nitpicky, but is it possible to write this as:
WRAP_MALLOC_CFLAGS='-Wl,"--wrap dlopen --wrap dlclose ..."'
?

Won't work as easily for the hunk below because you want variable interpolation.

Comment 23

[(dormant account)]

Comment on attachment 481369 [details] [diff] [review]
1a. Fix review comments, add passthrough mode

>+static void
>+extractFile(const char * path, const struct cdir_entry *entry, void * data)

As discussed on irc, should just mmap the file for writing, instead of C convulsions.
Low priority.

>+static void
>+extractLib(const struct cdir_entry *entry, void * data, void * buf)
>+{
buf is a bad name, say dest or something.

Comment 24

[Michael Wu [:mwu]]

Attachment: 0. Add bionic linker files to other-licenses/android (hg patch ready)

Comment 25

[Michael Wu [:mwu]]

Attachment: 1. Add support for reading libraries out of the apk, v2 (hg patch ready)

Comment 26

[Michael Wu [:mwu]]

Attachment: 2. Make the Java wrapper use the new library loader, v2 (hg patch ready)

Comment 27

[Michael Wu [:mwu]]

Attachment: 3. Add support for the custom library loader to plugin-container, v3 (hg patch ready)

Comment 28

[Michael Wu [:mwu]]

Attachment: 4a. Wrap the dl* functions with our own version, v3 (hg patch ready)

Comment 29

[Michael Wu [:mwu]]

Attachment: 4b. Wrap the dl* functions with our own version (NSPR) (hg patch ready)

Comment 30

[(not currently active) Ted Mielczarek]

Comment on attachment 483310 [details] [diff] [review]
4b. Wrap the dl* functions with our own version (NSPR) (hg patch ready)

Checking in configure;
/cvsroot/mozilla/nsprpub/configure,v  <--  configure
new revision: 1.289; previous revision: 1.288
done
Checking in configure.in;
/cvsroot/mozilla/nsprpub/configure.in,v  <--  configure.in
new revision: 1.292; previous revision: 1.291
done

Comment 31

[Michael Wu [:mwu]]

http://hg.mozilla.org/mozilla-central/rev/6277ca1ac576 Patch 0
http://hg.mozilla.org/mozilla-central/rev/266535ecad79 Patch 1
http://hg.mozilla.org/mozilla-central/rev/a6b6ebdd29a2 Patch 2
http://hg.mozilla.org/mozilla-central/rev/884b917c7628 Patch 3
http://hg.mozilla.org/mozilla-central/rev/e3cdf1619342 Patch 4a