Last Comment Bug 588607 - Implement custom dlopen to load libraries from apk
: Implement custom dlopen to load libraries from apk
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget: Android (show other bugs)
: Trunk
: All Android
: -- normal (vote)
: ---
Assigned To: Michael Wu [:mwu]
:
Mentors:
Depends on: 595989 647288
Blocks: 608593 603592 607534
  Show dependency treegraph
 
Reported: 2010-08-18 16:13 PDT by Michael Wu [:mwu]
Modified: 2011-04-01 12:08 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
2.0b2+


Attachments
0. Add bionic linker files to other-licenses/android (135.17 KB, patch)
2010-09-27 17:58 PDT, Michael Wu [:mwu]
villalu: review+
Details | Diff | Review
1. Add support for reading libraries out of the apk (39.98 KB, patch)
2010-09-27 18:29 PDT, Michael Wu [:mwu]
taras.mozilla: review-
Details | Diff | Review
2. Make the Java wrapper use the new library loader (5.08 KB, patch)
2010-09-27 18:31 PDT, Michael Wu [:mwu]
blassey.bugs: review+
Details | Diff | Review
3. Add support for the custom library loader to plugin-container (1.86 KB, patch)
2010-09-27 18:33 PDT, Michael Wu [:mwu]
cjones.bugs: feedback-
Details | Diff | Review
4. Wrap the dl* functions with our own version (3.62 KB, patch)
2010-09-27 18:39 PDT, Michael Wu [:mwu]
no flags Details | Diff | Review
4a. Wrap the dl* functions with our own version (2.04 KB, patch)
2010-09-30 17:49 PDT, Michael Wu [:mwu]
no flags Details | Diff | Review
4b. Wrap the dl* functions with our own version (NSPR) (1.65 KB, patch)
2010-09-30 17:50 PDT, Michael Wu [:mwu]
ted: review+
Details | Diff | Review
4a. Wrap the dl* functions with our own version, v2 (2.23 KB, patch)
2010-10-04 14:04 PDT, Michael Wu [:mwu]
ted: review+
Details | Diff | Review
3. Add support for the custom library loader to plugin-container, v2 (3.56 KB, patch)
2010-10-04 14:37 PDT, Michael Wu [:mwu]
no flags Details | Diff | Review
1a. Fix review comments, add passthrough mode (13.67 KB, patch)
2010-10-06 16:34 PDT, Michael Wu [:mwu]
taras.mozilla: review+
Details | Diff | Review
3. Add support for the custom library loader to plugin-container, v3 (3.61 KB, patch)
2010-10-06 16:36 PDT, Michael Wu [:mwu]
cjones.bugs: review+
Details | Diff | Review
0. Add bionic linker files to other-licenses/android (hg patch ready) (135.27 KB, patch)
2010-10-14 15:15 PDT, Michael Wu [:mwu]
no flags Details | Diff | Review
1. Add support for reading libraries out of the apk, v2 (hg patch ready) (43.91 KB, patch)
2010-10-14 15:16 PDT, Michael Wu [:mwu]
no flags Details | Diff | Review
2. Make the Java wrapper use the new library loader, v2 (hg patch ready) (4.31 KB, patch)
2010-10-14 15:17 PDT, Michael Wu [:mwu]
no flags Details | Diff | Review
3. Add support for the custom library loader to plugin-container, v3 (hg patch ready) (3.82 KB, patch)
2010-10-14 15:18 PDT, Michael Wu [:mwu]
no flags Details | Diff | Review
4a. Wrap the dl* functions with our own version, v3 (hg patch ready) (2.33 KB, patch)
2010-10-14 15:19 PDT, Michael Wu [:mwu]
no flags Details | Diff | Review
4b. Wrap the dl* functions with our own version (NSPR) (hg patch ready) (1.75 KB, patch)
2010-10-14 15:20 PDT, Michael Wu [:mwu]
no flags Details | Diff | Review

Description Michael Wu [:mwu] 2010-08-18 16:13:31 PDT

    
Comment 1 Michael Wu [:mwu] 2010-09-27 17:58:23 PDT
Created attachment 478935 [details] [diff] [review]
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] 2010-09-27 18:29:35 PDT
Created attachment 478944 [details] [diff] [review]
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] 2010-09-27 18:31:22 PDT
Created attachment 478945 [details] [diff] [review]
2. Make the Java wrapper use the new library loader
Comment 4 Michael Wu [:mwu] 2010-09-27 18:33:36 PDT
Created attachment 478946 [details] [diff] [review]
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] 2010-09-27 18:39:41 PDT
Created attachment 478947 [details] [diff] [review]
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] 2010-09-27 18:42:11 PDT
For anyone testing these patches, the nspr configure needs to be regenerated after applying these patches.
Comment 7 Brad Lassey [:blassey] (use needinfo?) 2010-09-27 21:31:11 PDT
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?) 2010-09-27 21:42:05 PDT
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] 2010-09-28 00:10:04 PDT
(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 luis@tieguy.org] 2010-09-28 08:41:45 PDT
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?) 2010-09-28 16:35:40 PDT
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) 2010-09-29 15:15:11 PDT
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 2010-09-30 15:16:51 PDT
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] 2010-09-30 17:49:14 PDT
Created attachment 479976 [details] [diff] [review]
4a. Wrap the dl* functions with our own version
Comment 15 Michael Wu [:mwu] 2010-09-30 17:50:30 PDT
Created attachment 479977 [details] [diff] [review]
4b. Wrap the dl* functions with our own version (NSPR)
Comment 16 Michael Wu [:mwu] 2010-10-04 14:04:01 PDT
Created attachment 480732 [details] [diff] [review]
4a. Wrap the dl* functions with our own version, v2

System zlib required to build.
Comment 17 Michael Wu [:mwu] 2010-10-04 14:37:09 PDT
Created attachment 480748 [details] [diff] [review]
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 2010-10-05 14:06:24 PDT
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] 2010-10-06 13:23:37 PDT
(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] 2010-10-06 16:34:51 PDT
Created attachment 481369 [details] [diff] [review]
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] 2010-10-06 16:36:19 PDT
Created attachment 481370 [details] [diff] [review]
3. Add support for the custom library loader to plugin-container, v3
Comment 22 Ted Mielczarek [:ted.mielczarek] 2010-10-08 08:21:53 PDT
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) 2010-10-08 15:57:24 PDT
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] 2010-10-14 15:15:36 PDT
Created attachment 483300 [details] [diff] [review]
0. Add bionic linker files to other-licenses/android (hg patch ready)
Comment 25 Michael Wu [:mwu] 2010-10-14 15:16:08 PDT
Created attachment 483302 [details] [diff] [review]
1. Add support for reading libraries out of the apk, v2 (hg patch ready)
Comment 26 Michael Wu [:mwu] 2010-10-14 15:17:30 PDT
Created attachment 483305 [details] [diff] [review]
2. Make the Java wrapper use the new library loader, v2 (hg patch ready)
Comment 27 Michael Wu [:mwu] 2010-10-14 15:18:21 PDT
Created attachment 483306 [details] [diff] [review]
3. Add support for the custom library loader to plugin-container, v3 (hg patch ready)
Comment 28 Michael Wu [:mwu] 2010-10-14 15:19:13 PDT
Created attachment 483308 [details] [diff] [review]
4a. Wrap the dl* functions with our own version, v3 (hg patch ready)
Comment 29 Michael Wu [:mwu] 2010-10-14 15:20:04 PDT
Created attachment 483310 [details] [diff] [review]
4b. Wrap the dl* functions with our own version (NSPR) (hg patch ready)
Comment 30 Ted Mielczarek [:ted.mielczarek] 2010-10-15 08:19:05 PDT
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

Note You need to log in before you can comment on or make changes to this bug.