Simplify serialize.h's exception handling #4618

Remove the 'state' and 'exceptmask' from serialize.h's stream implementations, as well as related methods.

As exceptmask always included 'failbit', and setstate was always called with bits = failbit, all it did was immediately raise an exception. Get rid of those variables, and replace the setstate with direct exception throwing (which also removes some dead code).

As a result, good() is never reached after a failure (there are only 2 calls, one of which is in tests), and can just be replaced by !eof().

fail(), clear(n) and exceptions() are just never called. Delete them.

Larger goal: simplify the serialize.h interfaces enough to make a planned refactor easier (I want to get rid of IMPLEMENT_SERIALIZE, and replace it with a single templated method).

Looks good to me. Untested ACK.

ut ACK once memset() comment resolved.

+1 on removing state. Looks like the memset was just used to sanitize the return buffer in the case of a failed read. Anything that relied on that would be quite broken already, but it's worth double-checking if @sipa didn't already.

@theuni Nod, it looked like "secure erase" style exit, warranting further review to ensure we are not accidentally leaking some data on error.

Commented on @jgarzik's comments elsewhere (but they seem to have disappeared from the PR discussion).

• The memset was unreachable code, as it occurs after a setstate that already always throws an exception.
• Failure on short read is perfectly reasonable IMHO, as it will inevitably lead to failed deserialization.

@sipa Failure on short read presumes binary, which is not the case if e.g. you are reading text files.

... our serialization has no text file format.

@sipa It is CAutoFile being modified. That is "RAII wrapper for FILE_" and is used outside the network wire protocol and core serialization of data structures. Both [1] and [2] want to read text files, and using an RAII wrapper for FILE_ is the logical choice.

[1] https://github.com/bitcoin/bitcoin/blob/master/src/bitcoin-tx.cpp#L126 [2] https://github.com/jgarzik/bitcoin/blob/2014_ext_services/src/extservices.cpp

In that case we should probably separate the RAII wrapper part from serialize.h, and just have an additional FIlE* wrapper for serialization there.

Still, I think those textfile usages are probably better served with native c++ file streams rather than using FILE*...

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4618_eb0b56b19017ab5c16c745e6da39c53126924ed6/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

I'd feel better if that crazy CAutoFile wrapper was phased out eventually. Let's at least not use it for any new things not related to serialization.

To read and write from text files I'd suggest the use of ofstream/ifstream as this would be consistent with code to handle text files already in rpcdump.cpp.

@jgarzik You consider the memset() issue resolved?

@sipa Well, strictly speaking, you are correct. It is dead code, since at least 0.1.5.

It seems reasonable to examine the intent behind this, though. The intent appears to be:

• If we want more data than is available (overrun), clear the caller-provided buffer.

Naively, a working memset() in this location would seem to be wise security practice, and thus, a minor security bug. If you throw an error but catch it later (ie. process does not exit), then potentially sensitive wallet data is left in unguarded memory, if you are de-serializing sensitive data at the time of thrown exception.

We are deserializing non-sensitive data 99% of the time, so this is a rare error case, but it does seem to fall within the rubric of secure allocation/clearing.

the intent was emulating c++'s stream interface, which has this configurable set of exception flags.

The intent of the memset(), specifically.

ACK if you move the memset() before exception throw.

The memset makes little sense to me. pch points to caller-allocated memory. It's up to the caller to securely erase the passed-in memory before deallocating it - irrespective of whether it ended with an error. Secure erasing the caller's memory is not the responsibility of the serialization system. If it was local state that would otherwise linger around I could understand.

I think the original purpose wasn't security either, but just preventing any short read from returning meaningful data, causing a fast fail if someone would rely on the return value. That's already made impossible by the exception being thrown in the alternative (and so far, always guaranteed) case, what also explains why it's done after the throw.

@laanwj That logic does not sound applicable/realistic here: The caller is automatically generated from serialization macros.

RE responsibility, it is good practice to avoid returning garbage when we know it's garbage.

Well, this is no worse than what we have now... ut ACK, and this memset() issue can be split from this PR.