1015824 - 64-bit ints in StructuredClone are bogus on big-endian

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Cameron Kaiser [:spectre]]

Attachment: structured_clone_bige.diff

"TenFourFox: Lie Cannot I And Endian Big Like I"

The StructuredClone implementation passes pointers in 64-bit ints that are actually little endian, even on big endian platforms, and causes naughty little assertions like

Assertion failure: contents, at /Volumes/BruceDeuce/src/mozilla-31a/js/src/vm/ArrayBufferObject.cpp:1055

The simple solution is just to wordswap the 64-bit ints as per the patch.

Comment 1

[Cameron Kaiser [:spectre]]

Review ping-a-ling (advise, Steve, if someone else should review).

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8428493 [details] [diff] [review]
structured_clone_bige.diff

Shouldn't that be swapFromLittleEndian?  They accomplish the same thing, in this case, but it reads more correctly for big-endian machines, since presumably things are little-endian when they're being passed around, and you're trying to get the data back into native endianness.  I'd expect swapToLittleEndian to be used while writing, per the comments in Endian.h.

Comment 3

[Cameron Kaiser [:spectre]]

OK, that makes sense. I'll change it when I'm back at my desk.

Comment 4

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

Comment on attachment 8428493 [details] [diff] [review]
structured_clone_bige.diff

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

::: js/src/vm/StructuredClone.cpp
@@ +572,5 @@
>  {
>      uint64_t u;
>      if (!readNativeEndian(&u))
>          return false;
> +    *p = reinterpret_cast<void*>(NativeEndian::swapToLittleEndian(u));

Yeah, this should be swapFromLittleEndian.

readPtr/writePtr should die. But that's a separate bug. (Specifically, transferable clones should not pretend to serialize the transferred data. It should be in a separate auxiliary data structure using native types.)

I think my intent here was actually to not change endianness at all for pointers, since it's a lie that they can be serialized in the first place. But I'm ok either way, especially since I plan to eliminate it eventually anyway.

In other words: r=me if you change the above to swapFromLittleEndian. Alternatively, r=me if you change the body of writePtr:

-    return write(reinterpret_cast<uint64_t>(p));
+    return buf.append(reinterpret_cast<uint64_t>(p));

and then eliminate the endianness conversion entirely in readPtr.

Comment 5

[Cameron Kaiser [:spectre]]

Thanks for the review!

>+    return buf.append(reinterpret_cast<uint64_t>(p));

I'm puzzling over this. Unless I misunderstand what write() is doing, wouldn't this have the same problem?

Comment 6

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

(In reply to Cameron Kaiser [:spectre] from comment #5)
> Thanks for the review!
> 
> >+    return buf.append(reinterpret_cast<uint64_t>(p));
> 
> I'm puzzling over this. Unless I misunderstand what write() is doing,
> wouldn't this have the same problem?

Currently, pointers' endianness is flipped when writing them out (because writePtr calls write(uint64_t) which does NativeEndian::swapToLittleEndian) but not when reading them in.

So your fix is to flip them on both sides. An alternative would be to never flip them. This would be done by removing the swapToLittleEndian call, which ends up with the buf.append line of code above. It's the current SCOutput::write but without the endian swap.

Comment 7

[Cameron Kaiser [:spectre]]

Let me make sure we're talking about the right patch:

 bool
 SCOutput::writePtr(const void *p)
 {
-    return write(reinterpret_cast<uint64_t>(p));
+    return buf.append(reinterpret_cast<uint64_t>(p));
 }

still blows up. Did I need something else in there?

Comment 8

[Cameron Kaiser [:spectre]]

Attachment: For check-in

The swapFromLittleEndian() of course works, so I think I'll go with that, since you gave me the choice. Patch for checkin. Carrying r+.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Can you please run this through Try (builds only is fine) just as a sanity check? :)

Comment 10

[Cameron Kaiser [:spectre]]

I don't have the ability to push to try. If someone can on my behalf I would appreciate it.

Comment 11

[Jan Beich]

Comment on attachment 8439669 [details] [diff] [review]
For check-in

s/r=sphink/r=sfink/ and green on Try

https://tbpl.mozilla.org/?tree=Try&rev=438f9bf04477

Comment 12

[Cameron Kaiser [:spectre]]

Thanks, Jan!

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9cd95132bc9f

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9cd95132bc9f