Closed Bug 803707 Opened 7 years ago Closed 7 years ago

Don't assume pointers are 64 bits

Categories

(Core :: JavaScript Engine, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: espindola, Assigned: espindola)

References

Details

Attachments

(2 files)

Attached patch patchSplinter Review
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.
Attachment #673432 - Flags: review?(bent.mozilla)
Comment on attachment 673432 [details] [diff] [review]
patch

You'll need a JS peer to look over this.
Attachment #673432 - Flags: review?(bent.mozilla) → review?(jorendorff)
Comment on attachment 673432 [details] [diff] [review]
patch

firebot has not seen jorendorff  for 10 days. Maybe Waldo can review it?
Attachment #673432 - Flags: review?(jwalden+bmo)
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.
Attachment #673432 - Flags: review+
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.)
Attachment #673432 - Flags: review?(jwalden+bmo) → review+
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.
Component: DOM → JavaScript Engine
OS: Linux → All
Hardware: x86_64 → x86
(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.
https://hg.mozilla.org/mozilla-central/rev/dd69215593f6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
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?
Attachment #674081 - Flags: review?(jwalden+bmo)
Attachment #674081 - Flags: review+
Attachment #674081 - Flags: feedback?(gary)
> 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.
> I think asan should catch it, valgrind is the one that cannot.

CC'ing Julian.
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.
Attachment #674081 - Flags: feedback?(gary) → feedback?(choller)
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?
(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.
Attachment #673432 - Flags: review?(jorendorff) → review+
Attachment #674081 - Flags: feedback?(choller)
You need to log in before you can comment on or make changes to this bug.