Closed
Bug 712289
Opened 13 years ago
Closed 13 years ago
jsval has different alignments in C and C++ (jsdIValue.getWrappedValue broken on 32-bit)
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
VERIFIED
FIXED
mozilla12
People
(Reporter: Honza, Assigned: bzbarsky)
References
Details
(Whiteboard: [sg:critical][qa!])
Attachments
(4 files, 1 obsolete file)
3.31 KB,
application/x-xpinstall
|
Details | |
3.20 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
4.13 KB,
patch
|
akeybl
:
approval-mozilla-aurora+
akeybl
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
2.35 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
Firebug is using jsdIValue.getWrappedValue() API to get list of of local (closure) scope variables (names and values).
To get list of a scope properties, Firebug is using:
var listValue = {value: null}, lengthValue = {value: 0};
jsdIStackFrame frame.scope.getProperties(listValue, lengthValue);
It works on Windows, but in Ubuntu - applying getWrappedValue() on returned properties (prop.name and prop.value), returns a number instead of a name or a value.
Here is the related code in Firebug SVN:
http://code.google.com/p/fbug/source/browse/branches/firebug1.9/content/firebug/lib/wrapper.js#73
STR:
1) Install Firebug e.g. from here: http://code.google.com/p/fbug/issues/detail?id=5058#c4
2) Load http://getfirebug.com/tests/content/branches/1.9/script/watch/5019/issue5019.html
3) Follow steps on the test page.
The Watch side panel properly displays local variables a, b, c and their values in Windows, but in Ubuntu I am seeing something like: -4.428469523771077e-75, 2.12199573e-314, etc. (looks like a reference/address instead of an actual value)
Tested with Firefox Nightly:
Built from http://hg.mozilla.org/mozilla-central/rev/2afd7ae68e8b
Honza
Comment 1•13 years ago
|
||
Assuming this just appeared on the nightlies, this is likely a regression from bug 705444 (or maybe bug 708754). Ms2ger, can you look into this?
Comment 2•13 years ago
|
||
Sadly I can reproduce this behavior in Firefox 9 and up. i686-pc-linux-gnu, Ubuntu 11.10. Not all variables show up weird, for example in the test http://getfirebug.com/tests/content/branches/1.9/commandLine/4434/issue4434.html precisely function parameters seem to be affected.
Assignee | ||
Comment 3•13 years ago
|
||
Jan, are you seeing this with the 32-bit Linux nightlies?
Reporter | ||
Comment 4•13 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #3)
> Jan, are you seeing this with the 32-bit Linux nightlies?
Yes
Assignee | ||
Comment 5•13 years ago
|
||
OK. Could you possibly create a smallish testcase I can run to reproduce the bug?
Reporter | ||
Comment 6•13 years ago
|
||
Sure thing, here you go
1) Install the attached extension
2) Open test.html file (you need to extract it from the xpi), you should use http: or file: to open that file
3) Click the button on the page
4) Watch system console (dump) or Firefox Error Console
I am seeing, in Windows:
scope: Call
scope vars: c: false, b: 100, a: hello
in Linux (Ubuntu 9.04):
scope: Call
scope vars: c: false, -2.21420102685212e-75: 2.143215748267e-312, undefined: a
Honza
Assignee | ||
Comment 7•13 years ago
|
||
OK. I don't have Windows, but I _can_ reproduce the bug on both Linux and Mac... in a 32-bit build. In a 64-bit build, I get the expected output.
Assignee | ||
Comment 8•13 years ago
|
||
OK. So we get into _buildProps with this obj:
(gdb) call js_DumpObject(obj)
object 0x19b78060
class 0x139596a0 Call
flags:
proto null
parent <Window object at 0x19b1c0d0>
private 0x195140a8
reserved slots:
0 (reserved) = <Window object at 0x19b1c0d0>
1 (reserved) = <function executeTest (file:///Users/bzbarsky/test/test.html:26) at 0x19b2f2e0>
2 (reserved) = undefined
properties:
((Shape *) 0x19b21ce8) enumerate permanent getterOp=0x1351d286 setterOp=0x13521326 "a": slot 3 = undefined
((Shape *) 0x19b21d00) enumerate permanent getterOp=0x1351d286 setterOp=0x13521326 "b": slot 4 = undefined
((Shape *) 0x19b7de20) enumerate permanent getterOp=0x1351d286 setterOp=0x13521326 "c": slot 5 = undefined
Then we call JS_GetPropertyDescArray on this object. And we get a result which is bogus. The first entry is ok, but after that:
(gdb) p/x pda.array[1].id
$41 = {
asBits = 0x19a00630ffffff82,
s = {
payload = {
i32 = 0xffffff82,
u32 = 0xffffff82,
boo = 0xffffff82,
str = 0xffffff82,
obj = 0xffffff82,
ptr = 0xffffff82,
why = 0xffffff82,
word = 0xffffff82
},
tag = 0x19a00630
},
asDouble = 0x0,
asPtr = 0xffffff82
}
Now the weird thing is that if I look at pda->array[1].id right before the return JS_TRUE in JS_GetPropertyDescArray, it looks like this:
(gdb) p/x pda->array[1].id
$88 = {
data = {
asBits = 0xffffff8519a00630,
s = {
payload = {
i32 = 0x19a00630,
u32 = 0x19a00630,
boo = 0x19a00630,
str = 0x19a00630,
obj = 0x19a00630,
ptr = 0x19a00630,
why = 0x19a00630,
word = 0x19a00630
},
tag = 0xffffff85
},
asDouble = 0x8000000000000000,
asPtr = 0x19a00630
}
}
which is a perfectly reasonable string.
Assignee | ||
Comment 9•13 years ago
|
||
This is not an xpconnect bug. Things go wrong in jseng.
Assignee: nobody → general
Component: XPConnect → JavaScript Engine
QA Contact: xpconnect → general
Assignee | ||
Comment 10•13 years ago
|
||
OK. So inside JS_GetPropertyDescArray we have:
(gdb) p &pda->array[0]
$105 = (JSPropertyDesc *) 0x46d3e0
(gdb) p sizeof(pda->array[0])
$106 = 32
and once we return we have:
(gdb) p &pda.array[0]
$108 = (JSPropertyDesc *) 0x46d3e0
(gdb) p sizeof(pda.array[0])
$109 = 28
Assignee | ||
Comment 11•13 years ago
|
||
Inside JS_GetPropertyDescArray:
(gdb) p &pda->array[0]
$121 = (JSPropertyDesc *) 0x416bd0
(gdb) p (char*)&$121->flags - (char*)$121
$122 = 16
(gdb) p (char*)&$121->alias - (char*)$121
$123 = 24
After we return:
(gdb) p &pda.array[0]
$125 = (JSPropertyDesc *) 0x416bd0
Current language: auto; currently c
(gdb) p (char*)&$125->flags - (char*)$125
$126 = 16
(gdb) p (char*)&$125->alias - (char*)$125
$127 = 20
Somehow the two compilation units are seeing different alignment requirements for jsval.
Assignee | ||
Comment 12•13 years ago
|
||
The version of jsval that JS_GetPropertyDescArray is a struct that has a single member called "data". The version of jsval in the C caller does not.
Assignee | ||
Comment 13•13 years ago
|
||
OK, so jsval is jsval_layout in C code. jsval_layout is a union, but all the union members only require 4-byte alignment with GCC in 32-bit mode (yes, including the double; I just tested with a small testcase; I knew gcc did that for 64-bit ints, but the fact that it does it for doubles is a surprise).
In C++ code, jsval is js::Value, which has explicit JSVAL_ALIGNMENT to make it require 8-byte alignment.
Assignee | ||
Comment 14•13 years ago
|
||
We should probably backport this to 10 and 11, right?
Attachment #588450 -
Flags: review?(luke)
Assignee | ||
Comment 15•13 years ago
|
||
Oh, and if someone knows how to write a regression test for this, please do.
Assignee: general → bzbarsky
Whiteboard: [need review]
Comment 17•13 years ago
|
||
Comment on attachment 588450 [details] [diff] [review]
Make sure that alignment requirements agree for js::Value and jsval_layout.
Thanks bz!
>+ struct jsval_layoutAlignmentTester {
>+ char c;
>+ jsval_layout l;
>+ };
Could you rename to LayoutAlignmentTester?
>+struct ValueAlignmentTester {
>+ char c;
>+ Value v;
>+};
>+JS_STATIC_ASSERT(sizeof(ValueAlignmentTester) == 16);
and move ValueAlignmentTester right below LayoutAlignmentTester and put the static assert in staticAssertions() (complete type is visible in memfuns)
Attachment #588450 -
Flags: review?(luke) → review+
Assignee | ||
Comment 18•13 years ago
|
||
> Could you rename to LayoutAlignmentTester?
Yes.
> and move ValueAlignmentTester right below LayoutAlignmentTester
Doesn't compile, because it claims that Value is not a complete type when defining ValueAlignmentTester. I actually had it that way to start with, but had to change to this setup...
Comment 19•13 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #18)
Ah, of course; then how about:
void staticAsserts() {
struct ValueAlignmentTester { char c; Value v; };
JS_STATIC_ASSERT(sizeof(ValueAlignmentTester) == 16);
}
? (And, for symmetry, could you put LayoutAlignmentTester right below it?)
Assignee | ||
Comment 20•13 years ago
|
||
Attachment #588591 -
Flags: review?(luke)
Assignee | ||
Updated•13 years ago
|
Attachment #588450 -
Attachment is obsolete: true
Comment 21•13 years ago
|
||
Comment on attachment 588591 [details] [diff] [review]
With those changes
I just realized, though, that this whole block is only #ifdef __cplusplus. I think, ideally, the jsval_layout static assert would be right after the lines:
#endif /* defined (__cplusplus) */
next to the:
JS_STATIC_ASSERT(sizeof(jsval_layout) == sizeof(jsval));
and the JS::Value assert would be right above the
#else /* defined(__cplusplus) */
Secondly, since the *Tester classes are outside any namespace in this context, they'd need a JS- prefix. Thanks again, sorry for the hassle.
Attachment #588591 -
Flags: review?(luke) → review+
Assignee | ||
Comment 22•13 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4195741db43 pushed with those changes.
Going to request branch approval for this....
tracking-firefox10:
--- → ?
tracking-firefox11:
--- → ?
Whiteboard: [need review]
Target Milestone: --- → mozilla12
Assignee | ||
Comment 23•13 years ago
|
||
Assignee | ||
Comment 24•13 years ago
|
||
Comment on attachment 588708 [details] [diff] [review]
Make sure that alignment requirements agree for js::Value and jsval_layout.
[Approval Request Comment]
Regression caused by (bug #): bug 684526
User impact if declined: Using some JS debugger APIs at best returns garbage data and at worst crashes. The crashes may or may not be exploitable.
Testing completed (on m-c, etc.): The code compiles and fixes the testcase in this bug. It also passes our various other tests.
Risk to taking this patch (and alternatives if risky): Low risk, I believe, but please double-check with Luke.
Attachment #588708 -
Flags: approval-mozilla-beta?
Attachment #588708 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•13 years ago
|
Flags: in-testsuite?
Comment 25•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 26•13 years ago
|
||
This bug appears to be biting people using release Firefox. See bug 715907.
The user there claims Firefox crashes when using Firebug, once an hour or so.
Folks this is bad. The fact that it's crashing means the bug can trigger in practice. Likely a Web page can use it to introduce garbage jsvals into the engine. I have a hard time believing it's not exploitable.
Assignee | ||
Comment 27•13 years ago
|
||
Agreed that this is probably exploitable, for what it's worth. I personally think we should fix this on 10 and 11 and at least consider a 9.0.2 (though 10 will ship pretty soon anyway, so maybe not worth worrying about).
I wonder whether this bites things that don't use jsd...
tracking-firefox9:
--- → ?
Updated•13 years ago
|
Group: core-security
status-firefox10:
--- → affected
status-firefox11:
--- → affected
status-firefox9:
--- → affected
Comment 28•13 years ago
|
||
timeless points out that we could, in parallel to shipping the fix, block Firebug for users of the affected versions.
Comment 29•13 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #24)
> Risk to taking this patch (and alternatives if risky): Low risk, I believe,
> but please double-check with Luke.
Agreed
Updated•13 years ago
|
Summary: jsdIValue.getWrappedValue doesn't seem to work in Ubuntu 9.04 → jsval has different alignments in C and C++ (jsdIValue.getWrappedValue broken on 32-bit)
Updated•13 years ago
|
Comment 30•13 years ago
|
||
Comment on attachment 588708 [details] [diff] [review]
Make sure that alignment requirements agree for js::Value and jsval_layout.
[Triage Comment]
Deemed low risk and fixes crashes that may be significantly affecting a part of our user population. Also may prevent a security vulnerability. Approving for aurora and beta.
Attachment #588708 -
Flags: approval-mozilla-beta?
Attachment #588708 -
Flags: approval-mozilla-beta+
Attachment #588708 -
Flags: approval-mozilla-aurora?
Attachment #588708 -
Flags: approval-mozilla-aurora+
Updated•13 years ago
|
Comment 31•13 years ago
|
||
Sorry for resetting the flags, that was due to reloading the bug to dodge a mid-air collision. Unfortunately I can't undo that change myself so I guess Alex need to set those flags again. Sorry. :-/
Updated•13 years ago
|
Assignee | ||
Comment 32•13 years ago
|
||
http://hg.mozilla.org/releases/mozilla-aurora/rev/8b89bc45b439
http://hg.mozilla.org/releases/mozilla-beta/rev/59d8e5608ab7
Alex, is this effectively wontfix for fx9?
Comment 33•13 years ago
|
||
I think this test is testing the right thing. No need to land it until the fix is in a release, though, especially with the static assert in place already.
Attachment #589107 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 34•13 years ago
|
||
Comment on attachment 589107 [details] [diff] [review]
Test alignment of jsval across C and C++
r=me
Attachment #589107 -
Flags: review?(bzbarsky) → review+
Comment 35•13 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #32)
> Alex, is this effectively wontfix for fx9?
Yep - that ship's sailed.
Assignee | ||
Comment 36•13 years ago
|
||
> Yep - that ship's sailed.
Just to be clear about what I was asking. I know we've shipped Firefox 9. The question was about whether we should be considering a chemspill for this before the Fx10 release. If we're not, that's fine by me.
Comment 37•13 years ago
|
||
Filed bug 718831 to get Firebug blocklisted in Firefox 9.
Comment 38•13 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #36)
> > Yep - that ship's sailed.
>
> Just to be clear about what I was asking. I know we've shipped Firefox 9.
> The question was about whether we should be considering a chemspill for this
> before the Fx10 release. If we're not, that's fine by me.
The possibility of a 9.0.2 for this issue is off the table due to our proximity to 10's release and the fact that crashes are intermittent (according to blocked bugs).
Comment 39•13 years ago
|
||
(In reply to Jason Orendorff [:jorendorff] from comment #37)
> Filed bug 718831 to get Firebug blocklisted in Firefox 9.
Is the motivation for this blocklist bug due to security concerns? Otherwise, this would appear to be blocklisting a usable add-on that causes intermittent crashes.
Comment 40•13 years ago
|
||
This is clearly sg:critical for Firebug users, but if it isn't exploitable for users w/out a debugger the overall severity might be sg:moderate. Probably doesn't matter much since it's fixed.
Does not affect the old 1.9.2 branch.
status1.9.2:
--- → unaffected
Comment 26 is private:
false
Comment 27 is private:
false
Comment 39 is private:
false
Whiteboard: [sg:critical]
Comment 41•13 years ago
|
||
Why do we need #ifdef DEBUG for those JS_STATIC_ASSERTs?
Anyway, the asserts don't work with Solaris Studio compiler, because we don't use JSVAL_ALIGNMENT, js::Value and jsval_layout are both 4-byte alignment.
(If I use align attribute with Solaris Studio compiler, it has problems with C_ValueToObject, it might be a bug of Solaris Studio.)
BTW: the "Test alignment of jsval across C and C++" patch was not committed.
And it has an extra "return sizeof(AlignTest);" in BEGIN_TEST(testValueABI_alignment).
So it didn't actually do the test.
Assignee | ||
Comment 42•13 years ago
|
||
> Why do we need #ifdef DEBUG for those JS_STATIC_ASSERTs?
I didn't want to add new symbols in release bulds. It's not strictly needed.
> Anyway, the asserts don't work with Solaris Studio compiler
Hmm. I guess we should assert that the two structs have the same alignment, not that they have 8-byte alignment. Do you want to do that, or should I? Probably in a separate bug.
> BTW: the "Test alignment of jsval across C and C++" patch was not committed.
Indeed. Steve?
Comment 43•13 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #42)
> > BTW: the "Test alignment of jsval across C and C++" patch was not committed.
>
> Indeed. Steve?
I wasn't sure what the policy was for tests of sg:crit bugs, so I stalled on landing it until the fix made it to a release. And then, of course, never got around to landing it after 10 was released.
(In reply to Ginn Chen from comment #41)
> And it has an extra "return sizeof(AlignTest);" in
> BEGIN_TEST(testValueABI_alignment).
> So it didn't actually do the test.
Doh! I thought I tried this before and after the patch.
Oh. No, I didn't, because it would never fail on my development machine. Well, unless I cross-compiled to 32-bit, doing that now... yep, it works.
Thanks.
http://hg.mozilla.org/integration/mozilla-inbound/rev/62635aac38dd
Comment 44•13 years ago
|
||
Flags: in-testsuite? → in-testsuite+
Updated•13 years ago
|
status-firefox-esr10:
--- → fixed
Updated•13 years ago
|
Updated•13 years ago
|
Updated•13 years ago
|
Comment 45•13 years ago
|
||
Verified in FF 10.0.3 ESR, FF 10.0.2, FF 11 Beta, FF 12 Aurora, FF 13 Nightly.
Status: RESOLVED → VERIFIED
Whiteboard: [sg:critical][qa+] → [sg:critical][qa!]
Updated•13 years ago
|
tracking-firefox-esr10:
--- → 10+
Updated•13 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•