712289 - jsval has different alignments in C and C++ (jsdIValue.getWrappedValue broken on 32-bit)

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

[Bobby Holley (:bholley)]

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

[Simon Lindholm]

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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 5

[Boris Zbarsky [:bzbarsky]]

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

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: Test extension

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

Comment 7

[Boris Zbarsky [:bzbarsky]]

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.

Comment 8

[Boris Zbarsky [:bzbarsky]]

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.

Comment 9

[Boris Zbarsky [:bzbarsky]]

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

Comment 10

[Boris Zbarsky [:bzbarsky]]

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

Comment 11

[Boris Zbarsky [:bzbarsky]]

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.

Comment 12

[Boris Zbarsky [:bzbarsky]]

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.

Comment 13

[Boris Zbarsky [:bzbarsky]]

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.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Make sure that alignment requirements agree for js::Value and jsval_layout.

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

Comment 15

[Boris Zbarsky [:bzbarsky]]

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

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

This is almost certainly the cause of bug 715907.

Comment 17

[Luke Wagner [:luke]]

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)

Comment 18

[Boris Zbarsky [:bzbarsky]]

> 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

[Luke Wagner [:luke]]

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

[Boris Zbarsky [:bzbarsky]]

Attachment: With those changes

Comment 21

[Luke Wagner [:luke]]

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.

Comment 22

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b4195741db43 pushed with those changes.

Going to request branch approval for this....

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: Make sure that alignment requirements agree for js::Value and jsval_layout.

Comment 24

[Boris Zbarsky [:bzbarsky]]

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.

Comment 25

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/mozilla-central/rev/b4195741db43

Comment 26

[Jason Orendorff [:jorendorff]]

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.

Comment 27

[Boris Zbarsky [:bzbarsky]]

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

Comment 28

[Jason Orendorff [:jorendorff]]

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

Comment 29

[Luke Wagner [:luke]]

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

[Alex Keybl [:akeybl]]

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.

Comment 31

[Robert Kaiser]

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

Comment 32

[Boris Zbarsky [:bzbarsky]]

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

[Steve Fink [:sfink] [:s:]]

Attachment: Test alignment of jsval across C and C++

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 34

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 35

[Alex Keybl [:akeybl]]

(In reply to Boris Zbarsky (:bz) from comment #32)
> Alex, is this effectively wontfix for fx9?

Yep - that ship's sailed.

Comment 36

[Boris Zbarsky [:bzbarsky]]

> 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

[Jason Orendorff [:jorendorff]]

Filed bug 718831 to get Firebug blocklisted in Firefox 9.

Comment 38

[Alex Keybl [:akeybl]]

(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

[Alex Keybl [:akeybl]]

(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

[Daniel Veditz [:dveditz]]

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.

Comment 41

[Ginn Chen]

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.

Comment 42

[Boris Zbarsky [:bzbarsky]]

> 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

[Steve Fink [:sfink] [:s:]]

(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

[Ed Morley [:emorley]]

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

Comment 45

[Jason Smith [:jsmith]]

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