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

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?

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.

Jan, are you seeing this with the 32-bit Linux nightlies?

(In reply to Boris Zbarsky (:bz) from comment #3) > Jan, are you seeing this with the 32-bit Linux nightlies? Yes

OK. Could you possibly create a smallish testcase I can run to reproduce the bug?

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

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.

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.

This is not an xpconnect bug. Things go wrong in jseng.

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

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.

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.

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.

We should probably backport this to 10 and 11, right?

Oh, and if someone knows how to write a regression test for this, please do.

This is almost certainly the cause of bug 715907.

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)

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

(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?)

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.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b4195741db43 pushed with those changes. Going to request branch approval for this....

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.

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.

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

timeless points out that we could, in parallel to shipping the fix, block Firebug for users of the affected versions.

(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

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.

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. :-/

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?

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.

Comment on attachment 589107 [details] [diff] [review] Test alignment of jsval across C and C++ r=me

(In reply to Boris Zbarsky (:bz) from comment #32) > Alex, is this effectively wontfix for fx9? Yep - that ship's sailed.

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

Filed bug 718831 to get Firebug blocklisted in Firefox 9.

(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).

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

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.

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.

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

(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

Test: https://hg.mozilla.org/mozilla-central/rev/62635aac38dd

Verified in FF 10.0.3 ESR, FF 10.0.2, FF 11 Beta, FF 12 Aurora, FF 13 Nightly.