Don't byteswap Fingerprints when encoding

[jcowgill]

Byteswapping Fingerprints when encoding is unnessesary and breaks if the Fingerprint is later decoded on a machine with different endianness to the one it was encoded on.

Fixes#42239

This PR fixes a regression caused by #42082. @michaelwoerister

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @eddyb (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[eddyb]

Can you move it to use #[derive]? Looks like it's what deriving would produce, now.

[eddyb]

r? @michaelwoerister

[michaelwoerister]

Thanks for the PR! I'll review it as soon as I find the time.

[nagisa]

Byteswapping Fingerprints when encoding is unnessesary and breaks if the Fingerprint is later decoded on a machine with different endianness to the one it was encoded on.

This explanation doesn’t exactly make any sense to me. It seems to me like going from machine specific to low endian in encoded form to machine specific after decoding is in fact the correct way to do this.

Maybe instead it is a problem with the comment just below that says:

The bytes returned bytes the Blake2B hasher are always little-endian.

or something like that?

[est31]

This explanation doesn’t exactly make any sense to me. It seems to me like going from machine specific to low endian in encoded form to machine specific after decoding is in fact the correct way to do this.

Not if Decoder::read_u64 and Encoder::emit_u64 already do the host to network conversion themselves. And apparently they do precisely that (that function is used by a macro in the implementation of read_u64).

So if you are doing a superfluous conversion host <-> little endian, it won't mean any problems if you serialize and deserialize on the same host, but if you serialize on a host with one byte order, and deserialize on a host with another, you will get the order mixed up.

Maybe instead it is a problem with the comment just below that says:

Nope, thats a different thing. The comment is correct, we actually encode to little endian. Admittedly, the code smells bad, it shouldn't have to use unsafe, but rather something safe with (hopefully) equivalent performance. That's a different issue though.

[michaelwoerister]

Thanks again, @jcowgill!

Some comments:

• In general it is definitely better to not do any endianess conversion in these encode/decode methods. The Encoder implementation should take care of that (and the OpaqueEncoder does by storing numbers in the endian-independent leb128 format). I overlooked that when introducing this change.
• @eddyb is right, we could just add RustcEncodable, RustcDecodable to Fingerprint at this point.
• It's a bit strange that byte-swapping should break anything here, but here is what's going on (using 291u16 as an example):
  • If HOST is big-endian we encode 291 == 0x01 0x23 to 8961 = 0x23 0x01 and that to leb128(8961).
    • If decoding on big-endian again we convert leb128(8961) to 8961 = 0x23 0x01 and then to 291 = 0x01 0x23 by swapping the bytes again.
    • If decoding on little-endian, we convert leb128(8961) to 8961 = 0x01 0x23 (!) and then we are not swapping bytes, ending up with the wrong value.

Little- to big-endian host is analogous.

So this gets an r+ from me once @eddyb's suggestion is implemented.

[jcowgill]

Hi, I've now changed the fix to use RustcEncodable and RustcDecodable. I just had to make sure things were still working :)

[eddyb]

@bors r=michaelwoerister

[bors]

📌 Commit edefcb2 has been approved by michaelwoerister

[michaelwoerister]

🎉

[bors]

⌛ Testing commit edefcb2 with merge e776af7...

[bors]

💔 Test failed - status-travis

[Mark-Simulacrum]

@bors retry

• Tracking issue for spurious network failures on bots #40474

[00:09:29] curl: (56) SSLRead() return error -36

[Mark-Simulacrum]

@bors rollup