647288 - Crash loading binary components bigger than 4KB

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

[Mike Hommey [:glandium]]

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]]

Attachment: Make our Android linker properly load libraries outside the .apk

Comment 2

[Michael Wu [:mwu]]

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]]

(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]]

http://hg.mozilla.org/mozilla-central/rev/337618a7bc79

Comment 5

[Michael Wu [:mwu]]

(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]]

(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]]

Without the patch, Firefox crashes on Android when using extensions with binary components or using js-ctypes.

Comment 8

[Mike Hommey [:glandium]]

(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]]

we're interested in this for aurora after it's baked on the trunk for a bit. not sure about beta yet.

Comment 10

[(not currently active) Ted Mielczarek]

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]]

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]]

I affects all versions of Firefox mobile >= 4.

Comment 13

[Asa Dotzler [:asa]]

the mobile team will track this but not the larger release team.

Comment 14

[Asa Dotzler [:asa]]

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]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/7a76cd285475

Comment 16

[Bryan Ashby]

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 19

[Mike Hommey [:glandium]]

Note that as bug 612544 witnesses, this does affect plugin loading, too...