serialization: Don't invoke undefined behaviour when doing type punning #14479

Don't invoke undefined behaviour when doing type punning.

It is undefined behavior to read a union member with a different type from the one with which it was most recently written. (Even if GCC allows it as non-standard language extension.)

We should not invoke undefined behaviour.

Why should we not invoke undefined behaviour?

A well established result in compiler research is that invoking undefined behaviour means that we allow the compiler to make daemons fly out of our noses.

The causal link between invoking UB and the severe nose problem is called the "nasal daemon" effect in the literature.

Recent studies have shown that invoking undefined behaviour in cryptocurrency projects cause a certain subset of said nasal daemons to emerge: the privkey eating nasal deamons.

The privkey eating nasal deamons are special in that they scan your memory for private keys prior to the nose exit. Once they are outside of your nose they upload these keys to a remote server located in a foreign country (foreign in relation to the nose bearer).

That – ladies and gentlemen – is why we should not invoke undefined behaviour.

Concept ACK

Please combine this into #14475 or vice-versa, as they are similar changes, in the same file and look like they share some of the same code?

@fanquake Good point! Now updated to make non-overlapping. Thanks! :-)

This 'serialization' is pretty severely unportable.

Casting through a union its a almost universally used idiom, and in some cases the only way to deserialize without either violating aliasing or demolishing performance with superfluous copies. (And in C it's unambiguously not undefined behaviour, making be somewhat dubious of the claim that it is in C++) Here, I doubt the performance matter-- but the question I see is: why the heck are we using a completely non-portable serialization of a floating point number in any case? That just sounds like a bug in and of itself.

utACK @gmaxwell I'm pretty sure the generated code is identical or of similar performance with this change. Perhaps someone needs to look at the generated code, but the very similar constructions on https://github.com/bitcoin/bitcoin/blob/v0.17.0/src/crypto/common.h#L17L54 compile to essentially no-ops.

As far as UB goes, https://en.cppreference.com/w/cpp/language/union claims "The details of that allocation are implementation-defined, and it's undefined behavior to read from the member of the union that wasn't most recently written. Many compilers implement, as a non-standard language extension, the ability to read inactive members of a union.", which I believe to be true for gcc and clang.

Finally, indeed it's weird that we rely on the exact memory representation of floating point numbers for serialization. However, it seems that the only place it's used (outside of tests) is for the TxConfirmStats::decay field, when saving/loading fee estimates to/from disk. That doesn't seem too hard to first convert to an integer or so as an independent change.

@gmaxwell

(And in C it's unambiguously not undefined behaviour, making be somewhat dubious of the claim that it is in C++)

The C++ Core Guidelines – with editors Bjarne Stroustrup and Herb Sutter – has this to say: "It is undefined behavior to read a union member with a different type from the one with which it was written."

Casting through a union its a almost universally used idiom, […]

The C++ Core Guidelines has this to say: "Unfortunately, unions are commonly used for type punning. We don’t consider “sometimes, it works as expected” a strong argument."

[…] and in some cases the only way to deserialize without either violating aliasing or demolishing performance with superfluous copies.

Professor John Regehr has this to say: "Again, although we might expect that adding a function call would make the code harder to optimize, both compilers understand memcpy deeply enough that we get the desired object code […] In my opinion c5 is the easiest code to understand out of this little batch of functions because it doesn’t do the messy shifting and also it is totally, completely, obviously free of complications that might arise from the confusing rules for unions and strict aliasing. It became my preferred idiom for type punning a few years ago when I discovered that compilers could see through the memcpy and generate the right code."

NAK. Eliminate the float serialization and just remove this code. This is always going to be non-portable (e.g. can't move data between different architectures without it being silently corrupted)

@practicalswift In case it isn't clear, I think @gmaxwell means we could get rid of the float-to-int conversion functions, and directly write the bytes of the float (which would be less code, and work when transferring a data direction between an little endian and big endian system which the current code does not)

@gmaxwell @sipa Yes, getting rid of this code would be nice. As I understand it the suggestion is to change from:

ser_writedata{32,64}(s, ser_{float,double}_to_uint{32,64}(a));

To:

ser_writedata{32,64}(s, a);

Is that correct?

That change would mean that we change the file format of fee_estimates.dat, right? Perhaps that is not a problem?

No, you'd need to add a ser_writedata_float and ser_writedata_double, mimicking the existing integer write functions, which write the data directly.

That shouldn't break anything, as right now that is essentially what is happening already, except the data is first being reinterpreted as an integer, and then written as an integer.

The issue is that on big-endian systems, the float data is reinterpreted as native (big endian) integers, which are then serialized as little endian. The overall effect of that is that on BE systems, floats and doubles are byteswapped when reading/writing, which makes it incompatible across systems.

The code you suggest would not work, as it'd cast the floating point numbers semantically to integers (rounding them) rather than reinterpreting.

@sipa Oh, yes of course. I misunderstood the suggestion and didn't look at the signature of ser_writedata{32,64}(…) before responding. Thanks for the clarification. Rounding them is obviously not what we want :-)

Sigh, no.

The whole point is not to have the byte swap operations in there.

Drop the htole32 etc.

@sipa Please review d928115718d3b3df69f7188c7344ab814523a1d7 :-)

Let me know if it looks good and I'll squash.

utACK.

Would be nice if someone with a big endian system could test that the resulting fee_estimates.dat files are actually portable across systems.

Reverted to the initial version (2aafe7c60f6e84cea4bbb2ed16c2c6b59a0eb4b3, see #14479 (review)).

Please re-review :-)

@sipa What would be the best way to move forward with this PR?

Adding the static_asserts could still be done without any other changes?