Closed Bug 647288 Opened 10 years ago Closed 9 years ago

Crash loading binary components bigger than 4KB

Categories

(Core :: Widget: Android, defect)

ARM
Android
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla8
Tracking Status
firefox6 - ---
firefox7 + fixed
fennec 7+ ---

People

(Reporter: glandium, Assigned: glandium)

References

Details

(Whiteboard: QA?)

Attachments

(1 file)

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
Blocks: 668210
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.
Attachment #544224 - Flags: review?(mwu) → review+
(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.
http://hg.mozilla.org/mozilla-central/rev/337618a7bc79
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
(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.
(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.
Without the patch, Firefox crashes on Android when using extensions with binary components or using js-ctypes.
(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)
we're interested in this for aurora after it's baked on the trunk for a bit. not sure about beta yet.
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.
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.
I affects all versions of Firefox mobile >= 4.
the mobile team will track this but not the larger release team.
tracking-fennec: --- → 7+
Attachment #544224 - Flags: approval-mozilla-aurora?
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.
Attachment #544224 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
Duplicate of this bug: 674570
Duplicate of this bug: 612544
Note that as bug 612544 witnesses, this does affect plugin loading, too...
Whiteboard: QA?
You need to log in before you can comment on or make changes to this bug.