803707 - Don't assume pointers are 64 bits

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: patch

JSStructuredCloneReader::readTransferMap does:

        void *content;

        if (!in.readPair(&tag, &data) || !in.readPtr(&content))
            return false;

and readPtr currently only forwards to read(uint64_t *p). The net result is that a 64 bit value is written to a 32 bit stack location.

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 673432 [details] [diff] [review]
patch

You'll need a JS peer to look over this.

Comment 2

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Comment on attachment 673432 [details] [diff] [review]
patch

firebot has not seen jorendorff  for 10 days. Maybe Waldo can review it?

Comment 3

[:Benjamin Peterson]

Comment on attachment 673432 [details] [diff] [review]
patch

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

::: js/src/jsclone.cpp
@@ +331,4 @@
>  bool
>  SCInput::readPtr(void **p)
>  {
> +    uint64_t tmp;

A comment about the need for a temporary variable would be nice, since this mistake has already been made once.

@@ +332,5 @@
>  SCInput::readPtr(void **p)
>  {
> +    uint64_t tmp;
> +    bool ret = read(&tmp);
> +    *p = (void*) tmp;

(void *) tmp.

Comment 4

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dd69215593f6

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 673432 [details] [diff] [review]
patch

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

::: js/src/jsclone.cpp
@@ +331,4 @@
>  bool
>  SCInput::readPtr(void **p)
>  {
> +    uint64_t tmp;

Honestly I think the kind that casts a pointer-to-pointer type to pointer-to-fixed-size-integer type code is just obviously wrong enough that I wouldn't comment it.  :-)

@@ +332,5 @@
>  SCInput::readPtr(void **p)
>  {
> +    uint64_t tmp;
> +    bool ret = read(&tmp);
> +    *p = (void*) tmp;

Actually these days we'd do static_cast<void*>(tmp).  (Whether there's a space between void and * is at best ambiguous.)

Comment 6

[Jeff Walden [:Waldo]]

Oh, this could use a testcase of some sort -- a JS test, not a mochitest, too, since it's the JS engine ultimately to blame here.  Only a valgrind-ed run would catch the error, perhaps (or do some compilers compile in a way such that this produces buggy behavior?), but that's better than having nothing exercise this.

This seems worth considering for landing on branches.

Comment 7

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: Use reinterpret_cast

Comment 8

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #6)
> Oh, this could use a testcase of some sort -- a JS test, not a mochitest,
> too, since it's the JS engine ultimately to blame here.  Only a valgrind-ed
> run would catch the error, perhaps (or do some compilers compile in a way
> such that this produces buggy behavior?), but that's better than having
> nothing exercise this.

Valgrind would not catch this normally, since it is a stack allocation. Asan should catch it.

Do you have any pointers on how to excite this code from a plain JS test? 
 
> This seems worth considering for landing on branches.

I will upload a combined patch once the cast change and the test are in.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 674081 [details] [diff] [review]
Use reinterpret_cast

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

Re testing, hmm.  On second thought, I'm not sure why an asan build wouldn't catch this, very straightforwardly, running our existing clone tests in js/src/tests/js1_8_5/extensions/clone-*.js.  Gary, you have any insight into this?  Does the current code trigger asan failures at all for any of those tests?

Comment 11

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

> Re testing, hmm.  On second thought, I'm not sure why an asan build wouldn't
> catch this, very straightforwardly, running our existing clone tests in
> js/src/tests/js1_8_5/extensions/clone-*.js.  Gary, you have any insight into
> this?  Does the current code trigger asan failures at all for any of those
> tests?

I think asan should catch it, valgrind is the one that cannot.

Comment 12

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

> I think asan should catch it, valgrind is the one that cannot.

CC'ing Julian.

Comment 13

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 674081 [details] [diff] [review]
Use reinterpret_cast

Passing the baton to Chris - he has run asan on tests longer than I have.

Comment 14

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e7d9b7fadd24

Comment 15

[Christian Holler (:decoder)]

I ran the parent of the revision in comment 9 through the jstests specified in comment 10 using ASan on 32 and 64 bit debug builds and I got no failures. Would this need specific shell flags?

Comment 16

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

(In reply to Christian Holler (:decoder) from comment #15)
> I ran the parent of the revision in comment 9 through the jstests specified
> in comment 10 using ASan on 32 and 64 bit debug builds and I got no
> failures. Would this need specific shell flags?

Is the code in question being executed? If so I think that is an asan bug. If not, can you try mochitest-4? That is the one I was getting failures on.

Comment 17

[Ed Morley [:emorley]]

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