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

RESOLVED FIXED in Firefox 26

Status

()

defect
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: info, Assigned: info)

Tracking

17 Branch
mozilla26
Other
Linux
Points:
---

Firefox Tracking Flags

(firefox26 fixed, firefox-esr17 wontfix, firefox-esr24 fixed)

Details

()

Attachments

(2 attachments, 4 obsolete attachments)

Assignee

Description

6 years ago
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.
Assignee

Comment 1

6 years ago
Posted patch patch proposal 1/2 (obsolete) — Splinter Review
Assignee

Updated

6 years ago
OS: Windows Server 2003 → Linux
Hardware: x86_64 → Other
Assignee

Comment 2

6 years ago
Assignee

Comment 3

6 years ago
The fix is also important for mozjs17 which will be used by gnome-shell soon.
Luke, who's a good reviewer for this?
Flags: needinfo?(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.
Attachment #797415 - Flags: review?(wmccloskey)

Updated

6 years ago
Attachment #797418 - Flags: review?(wmccloskey)
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?
Attachment #797415 - Flags: review?(wmccloskey)
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?
Assignee

Comment 8

6 years ago
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?
Flags: needinfo?(luke)
Assignee

Comment 9

6 years ago
The revised patch proposal; uses a wrapper for mmap().
Attachment #797415 - Attachment is obsolete: true
Attachment #797418 - Attachment is obsolete: true
Attachment #797418 - Flags: review?(wmccloskey)
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;
Attachment #798319 - Flags: review+
Assignee

Comment 11

6 years ago
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.
Assignee

Comment 12

6 years ago
Revised patch. Removed trailing spaces; fixed the MAP_FAILED/NULL mismatch.
Attachment #798319 - Attachment is obsolete: true
Attachment #798986 - Flags: checkin+
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.
Attachment #798986 - Flags: checkin+
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
Keywords: checkin-needed
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.
Flags: needinfo?(info)
Assignee

Comment 16

6 years ago
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.
Attachment #798986 - Attachment is obsolete: true
Flags: needinfo?(info)
Assignee

Updated

6 years ago
Keywords: checkin-needed
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 :)
Attachment #798986 - Attachment is obsolete: false
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.
Keywords: checkin-needed
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.
Assignee

Comment 20

6 years ago
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.
Attachment #800950 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/66c4c1a9b697

Feel free to request uplift now :)
Status: UNCONFIRMED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Assignee: general → info
Assignee

Updated

6 years ago
Attachment #801153 - Flags: approval-mozilla-esr24?
Attachment #801153 - Flags: approval-mozilla-beta?
Assignee

Updated

6 years ago
Attachment #798986 - Flags: approval-mozilla-esr17?
Assignee

Updated

6 years ago
You need to answer the questions asked when requesting approval.
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
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.
Attachment #801153 - Flags: approval-mozilla-beta?
Assignee

Comment 26

6 years ago
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
Assignee

Comment 27

6 years ago
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
The risk to Mozilla's Firefox builds is minimal, most of the patch is #ifdef their architecture. We should approve this for ESRs
I agree. I think we should take this for ESR.
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.
Flags: needinfo?(akeybl)
Attachment #801153 - Flags: approval-mozilla-esr24? → approval-mozilla-esr24+
Flags: needinfo?(akeybl)
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.
Attachment #798986 - Flags: approval-mozilla-esr17?
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

5 years ago
... 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

5 years ago
(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
You need to log in before you can comment on or make changes to this bug.