Last Comment Bug 647288 - Crash loading binary components bigger than 4KB
: Crash loading binary components bigger than 4KB
Status: RESOLVED FIXED
QA?
:
Product: Core
Classification: Components
Component: Widget: Android (show other bugs)
: Trunk
: ARM Android
: -- critical (vote)
: mozilla8
Assigned To: Mike Hommey [:glandium]
:
Mentors:
: 612544 674570 (view as bug list)
Depends on:
Blocks: 588607 668210
  Show dependency treegraph
 
Reported: 2011-04-01 12:08 PDT by Mike Hommey [:glandium]
Modified: 2013-12-27 14:25 PST (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
+
fixed
7+


Attachments
Make our Android linker properly load libraries outside the .apk (4.35 KB, patch)
2011-07-06 06:52 PDT, Mike Hommey [:glandium]
mwu.code: review+
blassey.bugs: approval‑mozilla‑aurora+
Details | Diff | Review

Description Mike Hommey [:glandium] 2011-04-01 12:08:13 PDT
The code path used to load libraries that are already mapped in memory leads to a crash if a library is bigger than a page, because load_library passes only a page worth of the library to load_segments as 'header' argument, vs. the full library when coming from load_mapped_library.
But load_segments assumes the 'header' argument to contain the whole library here:
http://hg.mozilla.org/mozilla-central/file/84034146670c/other-licenses/android/linker.c#l943
Comment 1 Mike Hommey [:glandium] 2011-07-06 06:52:51 PDT
Created attachment 544224 [details] [diff] [review]
Make our Android linker properly load libraries outside the .apk
Comment 2 Michael Wu [:mwu] 2011-07-07 06:46:56 PDT
Comment on attachment 544224 [details] [diff] [review]
Make our Android linker properly load libraries outside the .apk

Well, looks fine. There's some additional improvements that could be made to in the case of fd's point to real files but I don't know the changes to APKOpen well enough to comment on how to do it.
Comment 3 Mike Hommey [:glandium] 2011-07-07 07:34:23 PDT
(In reply to comment #2)
> Comment on attachment 544224 [details] [diff] [review] [review]
> Make our Android linker properly load libraries outside the .apk
> 
> Well, looks fine. There's some additional improvements that could be made to
> in the case of fd's point to real files but I don't know the changes to
> APKOpen well enough to comment on how to do it.

Actually, this takes care of the case of real files, which is what was broken.
Comment 4 Mike Hommey [:glandium] 2011-07-07 07:38:27 PDT
http://hg.mozilla.org/mozilla-central/rev/337618a7bc79
Comment 5 Michael Wu [:mwu] 2011-07-07 08:16:41 PDT
(In reply to comment #3)
> Actually, this takes care of the case of real files, which is what was
> broken.

I'm talking about the case where a real file FD gets passed to load_mapped_library. In that case, you would be able to not set FLAG_MMAPPED. This is not a particularly important case since it's only relevant when there are uncompressed libraries in the apk.
Comment 6 Mike Hommey [:glandium] 2011-07-07 08:59:40 PDT
(In reply to comment #5)
> I'm talking about the case where a real file FD gets passed to
> load_mapped_library. In that case, you would be able to not set
> FLAG_MMAPPED. This is not a particularly important case since it's only
> relevant when there are uncompressed libraries in the apk.

... which doesn't work anyway, for other reasons.
Comment 7 Mike Hommey [:glandium] 2011-07-07 13:36:20 PDT
Without the patch, Firefox crashes on Android when using extensions with binary components or using js-ctypes.
Comment 8 Mike Hommey [:glandium] 2011-07-07 13:37:05 PDT
(In reply to comment #7)
> Without the patch, Firefox crashes on Android when using extensions with
> binary components or using js-ctypes.

(for anything that is not in the Firefox apk ; using js-ctypes on e.g. nss works)
Comment 9 Asa Dotzler [:asa] 2011-07-07 14:40:37 PDT
we're interested in this for aurora after it's baked on the trunk for a bit. not sure about beta yet.
Comment 10 Ted Mielczarek [:ted.mielczarek] 2011-07-07 16:07:29 PDT
It's unfortunate, but I'd guess there aren't any current extensions being impacted, because this has never worked in a shipping release, I think. It does block off one avenue of extension development, which kind of sucks, but I don't know that it's bad enough to bother branch-landing.
Comment 11 [:Cww] 2011-07-11 14:37:40 PDT
This is Android specific.  Release drivers want to know if this affects Firefox 6 (beta) and if we should be tracking.  We didn't have mobile quorum to decide that.
Comment 12 Mike Hommey [:glandium] 2011-07-11 14:49:06 PDT
I affects all versions of Firefox mobile >= 4.
Comment 13 Asa Dotzler [:asa] 2011-07-13 11:37:55 PDT
the mobile team will track this but not the larger release team.
Comment 14 Asa Dotzler [:asa] 2011-07-21 14:33:02 PDT
We didn't have a mobile rep at today's triage but desktop has no concerns here so I'm delegating the yes/no to which ever of the Mobile drivers gets here first.
Comment 15 Mike Hommey [:glandium] 2011-07-21 23:37:04 PDT
http://hg.mozilla.org/releases/mozilla-aurora/rev/7a76cd285475
Comment 16 Bryan Ashby 2011-07-25 09:00:50 PDT
Using Android/Fennec and the above patch, I am still crashing when attempting to use dlopen() and friends from any thread using the specialized linker. This seems to happen when trying a specific library or dlopen(NULL, ...) attempting to fetch the currently loaded table.

I have created a new bug: #673937
Comment 17 Mike Hommey [:glandium] 2011-07-27 13:19:12 PDT
*** Bug 674570 has been marked as a duplicate of this bug. ***
Comment 18 Mike Hommey [:glandium] 2011-07-27 13:20:13 PDT
*** Bug 612544 has been marked as a duplicate of this bug. ***
Comment 19 Mike Hommey [:glandium] 2011-07-27 13:28:56 PDT
Note that as bug 612544 witnesses, this does affect plugin loading, too...

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