1481740 - Big endian bug in StructuredClones destructor

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Martin Husemann]

Attachment: Swap endianess to native after reading "content" pointer

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.

Comment 1

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

Attachment: Never do endianness swapping on pointers

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?

Comment 2

[Lars T Hansen [:lth]]

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

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7d62fc647ec
Never do endianness swapping on pointers, r=lth

Comment 4

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

Filed a new bug to byteswap everything (bug 1485460), landing this for now.

Comment 5

[Cristian Brindusan [:cbrindusan]]

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