910845 - ia64, Firefox 17, mozjs17 segfaults because JS ptrs haven't their high 17 bit cleared

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Stephan Schreiber]

User Agent: Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 5.2; WOW64; Trident/4.0; .NET CLR 2.0.50727; .NET CLR 3.0.04506.648; .NET CLR 3.5.21022; .NET CLR 3.0.4506.2152; .NET CLR 3.5.30729)

Steps to reproduce:

Debian Linux ia64 (Itanium) sid (unstable).
Start Iceweasel 17.0.8esr.


Actual results:

Iceweasel crashes before it shows its window.
This is Debian bug#719802 (http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=719802).



Expected results:

Iceweasel should start with success.

Comment 1

[Stephan Schreiber]

Attachment: patch proposal 1/2

Comment 2

[Stephan Schreiber]

Attachment: patch proposal 2/2, 17.0.8-esr

Comment 3

[Stephan Schreiber]

The fix is also important for mozjs17 which will be used by gnome-shell soon.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Luke, who's a good reviewer for this?

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 797415 [details] [diff] [review]
patch proposal 1/2

Bill (re)wrote static strings and I assume is familiar with the gc memory mapping code, so I'd suggest him.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 797415 [details] [diff] [review]
patch proposal 1/2

Review of attachment 797415 [details] [diff] [review]:
-----------------------------------------------------------------

I think it should be possible to write a wrapper around mmap that does what you need for IA64. That way we would avoid writing the same code twice. Could you please do that?

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 797418 [details] [diff] [review]
patch proposal 2/2, 17.0.8-esr

Review of attachment 797418 [details] [diff] [review]:
-----------------------------------------------------------------

I don't understand what this patch is for. However, I doubt that it's needed. Since bug 675806, static strings have been allocated the same way as every other string. So the changes to Memory.cpp should be sufficient to make static strings work. If I'm wrong, can you explain more specifically what problem the patch addresses?

Comment 8

[Stephan Schreiber]

I wrote a mapper for mmap() as you want (fix-map-pages-on-ia64-v2.patch).

Bill McClosky wrote:
> I doubt that it's needed. Since bug 675806, static strings have been allocated the same
> way as every other string. So the changes to Memory.cpp should be sufficient to make
> static strings work.

You are right. I didn't realize that it doesn't matter that the three arrays (of pointers) may be on high memory addresses:
    JSAtom *length2StaticTable[NUM_SMALL_CHARS * NUM_SMALL_CHARS];
    JSAtom *intStaticTable[INT_STATIC_LIMIT];
    JSAtom *unitStaticTable[UNIT_STATIC_LIMIT];
Addresses of the elements of the fourth array aren't provided to somewhere. So, does not matter either:
    static const SmallChar toSmallChar[];


So, only the fix-map-pages-on-ia64-v2.patch remains.


I want to have the patch in Firefox 24 and the trunk as well.
I guess I have to file individual bug reports for each of them - after this bug report for Firefox 17 is solved?

Comment 9

[Stephan Schreiber]

Attachment: fix-map-pages-on-ia64-v2.patch for 17.0.8-esr

The revised patch proposal; uses a wrapper for mmap().

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 798319 [details] [diff] [review]
fix-map-pages-on-ia64-v2.patch for 17.0.8-esr

Review of attachment 798319 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks very much! If you don't have permissions to check into mozilla-central, can you please update the patch as described below and set the checkin-needed flag? Then someone will be able to check it in for you. After that's done, we can ask for approval to land the patch on other versions (in the same bug). Is there a reason that you need it in previous versions?

::: iceweasel-17.0.8esr-orig/js/src/gc/Memory.cpp
@@ +327,5 @@
> +     * mmap an "addr" parameter with those bits clear. The mmap will return that address,
> +     * or the nearest available memory above that address, providing a near-guarantee
> +     * that those bits are clear. If they are not, we return NULL below to indicate
> +     * out-of-memory.
> +     * 

There are spaces at the end of some of the lines in these new comments. Can you please remove them?

@@ +363,5 @@
>      int flags = MAP_PRIVATE | MAP_ANON;
>  
>      /* Special case: If we want page alignment, no further work is needed. */
>      if (alignment == rt->gcSystemAllocGranularity) {
> +        return MapMemory(size, prot, flags, -1, 0);

This isn't your fault, but I just noticed a problem here. We will return MAP_FAILED here if the mmap fails, and we should instead be returning NULL. Would you mind changing it to this instead?

void *region = MapMemory(size, prot, flags, -1, 0);
if (region == MAP_FAILED)
    return NULL;
return region;

Comment 11

[Stephan Schreiber]

Done (fix-map-pages-on-ia64-v3.patch). I don't have permissions to check into mozilla-central.

I want to have the patch in previous versions
- Firefox 17.0.8-esr because it's the most recent esr version. Debian is still distributing it. Furthermore, it would be nice if the mozjs17 library would have the patch because Debian attempts to use it with gnome-shell soon.
- Firefox 24.0 beta; it will be the base for the next Firefox esr version.

Other versions are not important for me.

Comment 12

[Stephan Schreiber]

Attachment: fix-map-pages-on-ia64-v3.patch; 17.0.8-esr

Revised patch. Removed trailing spaces; fixed the MAP_FAILED/NULL mismatch.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 798986 [details] [diff] [review]
fix-map-pages-on-ia64-v3.patch; 17.0.8-esr

checkin+ is used to indicate that a patch had already landed (most useful for bugs with multiple patches attached). checkin-needed is what you want.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Also, current mozilla-central has seen some pretty significant changes compared to esr17. As such, the patch as-posted doesn't apply cleanly. Can you please attach a patch against m-c for landing? Also, when you do so, please be mindful of the page the below so that it includes all the necessary commit information. Thanks!
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Stephan, this can't land on the ESR releases until it's fixed on mozilla-central, so I can't help move this bug forward until I have a patch that applies.

Comment 16

[Stephan Schreiber]

Attachment: fix-map-pages-on-ia64-v4.patch for mozilla-central

Here is the patch for mozilla-central. I hope it satisfies your needs.

It *should* be applicable on Firefox 24 beta as well, but not on Firefox 17-esr/mozjs17.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 798986 [details] [diff] [review]
fix-map-pages-on-ia64-v3.patch; 17.0.8-esr

We can use this for esr17 still :)

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/45c336307136

Thanks for the patch, Stephan! Assuming this doesn't cause any bustage or test failures, this should be merged to mozilla-central within the next day or so and the bug resolved. Once it is, go ahead and request branch approval for the two patches. It's worth emphasizing that these patches are effectively NPOTB for non-IA64 so that the risk can be properly assessed.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
http://hg.mozilla.org/integration/mozilla-inbound/rev/97f0b4398600

https://tbpl.mozilla.org/php/getParsedLog.php?id=27506773&tree=Mozilla-Inbound

Presumably you'll need to refresh the esr17 patch as well once this is fixed, so please attach a new version of that patch as well.

Comment 20

[Stephan Schreiber]

Attachment: fix-map-pages-on-ia64-v5.patch for mozilla-central

Apologies for the bustage.

Here is a revised patch for mozilla-central.
I realized that a build of Firefox would fail with a similar compile error due to the MapAlignedPagesRecursively() function on OS/2. I fixed it, too.

The patch *should* be applicable on Firefox 24 beta again.

The path for Firefox 17-esr remains unchanged.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/66c4c1a9b697

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/66c4c1a9b697

Feel free to request uplift now :)

Comment 23

[Ryan VanderMeulen [:RyanVM]]

You need to answer the questions asked when requesting approval.

Comment 24

[bhavana bajaj [:bajaj]]

Stephan,

 Is this a recent regression in Firefox ? Unclear why this is nominated for esr as we only tend to uplift serious stability bug that needs to be addressed for the ESR, or a security fix that's landed on a branch affects the ESR.

Regarding uplift,

a)Please help fill the form that comes up when you set the approval flag on esr/beta 
b)We are already done with our final beta for Fx24 and have gone to build with our release candidate so it is too late to get it in there.
c) Based on the risk/reward we may still  be able to uplift to aurora(Fx25) else may have to ride the trains and eventually get fixed in Fx26

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Basic gist is that this is an IA64-only patch that's wanted for the standalone JS release (hence the esr requests). But yes, Stephen still needs to fill in the details.

Comment 26

[Stephan Schreiber]

Comment on attachment 798986 [details] [diff] [review]
fix-map-pages-on-ia64-v3.patch; 17.0.8-esr

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
17-esr. Debian. There is an attempt to use mozjs17d for gnome-shell soon; there will be a separate mozjs17d package with separate source code. Thus, it would simplify the work if esr-17 would also have the patch.

User impact if declined: Firefox/anything which uses the mozjs library remains useless on Linux ia64. Segfault upon start.

Fix Landed on Version: 26

Risk to taking this patch (and alternatives if risky): 
The question isn't applicable on Linux ia64; you can't do anything without the patch.
Low risk on other OS or Linux on other archs than ia64 since the patch doesn't change the generated object code: Just a new inline function which passes any arguments to mmap() in straight way. The compiler likely generates exactly the code as before since it unfolds the inline function.

String or UUID changes made by this patch: 
66c4c1a9b697
Bug 910845 - Add a wrapper for mmap() in order to keep the high 17-bits of JS pointers cleared on IA64. r=billm

Comment 27

[Stephan Schreiber]

Comment on attachment 801153 [details] [diff] [review]
fix-map-pages-on-ia64-v5.patch for mozilla-central

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
24-esr. This would simplify the work of Debian since we can stop patching every Firefox version for ia64. Gentoo would have a benefit as well. Debian only uses the esr versions in stable/testing/unstable.

User impact if declined: Firefox/anything which uses the mozjs library remains useless on Linux ia64. Segfault upon start.

Fix Landed on Version: 26

Risk to taking this patch (and alternatives if risky): 
The question isn't applicable on Linux ia64; you can't do anything without the patch.
Low risk on other OS or Linux on other archs than ia64 since the patch doesn't change the generated object code: Just a new inline function which passes any arguments to mmap() in straight way. The compiler likely generates exactly the code as before since it unfolds the inline function.

String or UUID changes made by this patch: 
66c4c1a9b697
Bug 910845 - Add a wrapper for mmap() in order to keep the high 17-bits of JS pointers cleared on IA64. r=billm

Comment 28

[Daniel Veditz [:dveditz]]

The risk to Mozilla's Firefox builds is minimal, most of the patch is #ifdef their architecture. We should approve this for ESRs

Comment 29

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I agree. I think we should take this for ESR.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Unfortunately, this has now missed the boat for ESR17. Debian's going to have to patch it locally :(

Alex, can we please get this approved for ESR24? The request has been pending for nearly 2 months now.

Comment 31

[bhavana bajaj [:bajaj]]

Comment on attachment 798986 [details] [diff] [review]
fix-map-pages-on-ia64-v3.patch; 17.0.8-esr

Clearing the Fx17 approval as this will be a wontfix on that branch at this point.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr24/rev/ff1049809bec

Sorry for the awful delay on this :(

Comment 33

[Matt Wobensmith [:mwobensmith][:matt:]]

Stephan, if you haven't already, please download the latest builds of 24esr/26 to confirm that this is correct, on affected platforms. Thank you.

Comment 34

[Ian Stakenvicius]

... has anyone successfully run tests on an ia64 platform yet, with the above fixes applied?  My experience so far, both on sm-24.2.0 and on sm-17 (backported patch above) has been failure and/or segfaults of testTrap_gc , and/or of later tests (like bug438633_JS_CompileFileHandleForPrincipals).  I'm also seeing a ton of "unaligned access" messages.

I don't want to reopen this bug, but....

Comment 35

[Émeric Maschino]

(In reply to Ian Stakenvicius from comment #34)
> ... has anyone successfully run tests on an ia64 platform yet, with the
> above fixes applied?  My experience so far, both on sm-24.2.0 and on sm-17
> (backported patch above) has been failure and/or segfaults of testTrap_gc ,
> and/or of later tests (like bug438633_JS_CompileFileHandleForPrincipals). 
> I'm also seeing a ton of "unaligned access" messages.
> 
> I don't want to reopen this bug, but....

You already know where I am with sm-17 and sm-24 on Gentoo ;-)
Furthermore, I can't even build neither Firefox 17 ESR nor 24 ESR [1].

I'm however replying here from a working Firefox 26 binary (though I don't know whether sm-26 exists and will pass tests successfully or not). I first was hitting bug #812647, but patch in bug #878791 [2] did the trick.

Regarding the unaligned accesses, Firefox/sm is unfortunately not the only piece of software having this bad habit on ia64... I thought that with other 64-bit architectures going mainstream (most notably x86_64) data would be properly aligned, but it appears that's not the case in the end.

     Émeric


[1] https://bugs.gentoo.org/show_bug.cgi?id=497514
[2] https://bug878791.bugzilla.mozilla.org/attachment.cgi?id=8358332