Closed Bug 1481740 Opened 2 years ago Closed 2 years ago

Big endian bug in StructuredClones destructor

Categories

(Core :: JavaScript Engine, defect)

61 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: martin, Assigned: sfink)

References

Details

Attachments

(2 files)

When a StructuredClone buffer is written and read, the "content" pointer is swapped to little endian byte order. However, on destruction this does not happen. This is easy to overlook as readPtr() does the swapping, but getPtr (used in this case as static function) does not. This may cause a call to js_free() with a byte swapped pointer on big endian platforms, usually causing a crash.

Fix is trivial, see attached patch.
Flags: needinfo?(sphink)
So this is what I'm thinking probably ought to be done to fit in with the current scheme.

But I'm no longer sure the insistence on not byte-swapping pointers makes sense. I think I originally felt that it was another way to ensure that we didn't accidentally write pointers to disk. But I think I did it when I was still writing things out in network byte order? Either way, it might be simpler now to byte swap everywhere, since now that things are written out little-endian, I'm essentially using our big-endian user base as testers.

What do you think?
Attachment #8998593 - Flags: review?(lhansen)
Assignee: nobody → sphink
Status: NEW → ASSIGNED
Comment on attachment 8998593 [details] [diff] [review]
Never do endianness swapping on pointers

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

The patch seems fine.

I observe that SharedArrayBuffer pointers are written using writeBytes() and read using readBytes(), and not endian swapped.  (The WHATWG cabal decided that these objects are not "transfered", so they don't show up in the SC transfer code.)

It would be most consistent to byteswap pointers though I agree it doesn't matter at the moment, with an agent cluster being an intraprocess thing always.  I would be inclined to do it anyway, to be consistent (a rule with no exceptions is easier than a rule with one (two?) exceptions.)
Attachment #8998593 - Flags: review?(lhansen) → review+
Blocks: 1485460
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7d62fc647ec
Never do endianness swapping on pointers, r=lth
Flags: needinfo?(sphink)
Filed a new bug to byteswap everything (bug 1485460), landing this for now.
https://hg.mozilla.org/mozilla-central/rev/c7d62fc647ec
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.