This looks like an issue in the current SHA256/512 code, where a pointer outside of the area pointed to may be constructed (this is UB in theory, though in practice every supported platform treats pointers as integers).
I discovered this while investigating #14580. Sadly, it does not fix it.
Rewritten; a much simpler change was possible to accomplish the same.
sha1 and ripemd160 need the same treatment.
Very nice find! Thanks for fixing!
Similar issues:
When looking for pointer arithmetic issues in the SHA256 and SHA512 code I stumbled across these two:
Consider the case where keylen is 64:
Consider the case where keylen is 128:
AFAIK p in memset(p, …, n) must be valid for writing even in the case where n is 0 to avoid UB.
Anyways -- concept ACK of course:
Getting rid of also the "not-yet-(ab-)used-by-compiler-authors-for-optimisation" class of UB makes perfect sense:
Avoiding UB is largely about avoiding future problems. This is a frequently missed point when discussing UB.
We should assume that current and future compiler writers care about benchmarks and that they will (ab-)use all optimisation opportunities given to them in the form of UB - regardless of if such optimisations would be "good" or "bad" for us. As been shown historically it is a risky strategy to assume otherwise: Changes in GCC Code Optimization Can Cause a Crash in BIND, D J Bernstein boringcc announcement, etc.
@practicalswift I don't see the issue with the uint256 code (as far as I can see, all pointers are to objects or one-past the object which is legal). I also can't find a reference for requiring that the memset argument needs to be a writable pointer if length is 0 (it needs to be a valid pointer, but it is; just not a writable one).
Also no reason to explain me why fixing UB is a good idea; I obviously agree, or I wouldn't have opened this PR.
I don't see the issue with the uint256 code (as far as I can see, all pointers are to objects or one-past the object which is legal).
Consider the case SetHex("1000000000000000000000000000000000000000000000000000000000000000"):
When the reverse processing reaches the leading 1 we have psz == pbegin and the following happens:
*p1 |= ((unsigned char)HexDigit(*psz--) << 4);
Please note that psz has now been decremented to pbegin - 1 which is outside of the object.
Let me know if I'm reading this incorrectly.
I also can't find a reference for requiring that the memset argument needs to be a writable pointer if length is 0 (it needs to be a valid pointer, but it is; just not a writable one).
Unfortunately the spec is not entirely clear on this (see 7.24.1.2. and 7.1.4.1. in ISO/IEC 9899:201x N1570).
Arguments for why the pointer should be valid for writing even in the case of n is 0 are given here:
The discussion is about memcpy but memset should have the same pointer requirements in this case.
Also no reason to explain me why fixing UB is a good idea; I obviously agree, or I wouldn't have opened this PR.
I was just giving a proper rationale for my concept ACK. Let me know if I'm too verbose in my concept ACKs and I'll adjust :-)
utACK 7a3426096fede34607695ee1523b795784a3a40f
utACK 7a3426096fede34607695ee1523b795784a3a40f
Consider the case SetHex("1000000000000000000000000000000000000000000000000000000000000000"):
Could add a unit testcase for this if that exact code path is not already covered?
162 | @@ -163,7 +163,7 @@ CSHA1& CSHA1::Write(const unsigned char* data, size_t len) 163 | sha1::Transform(s, buf); 164 | bufsize = 0; 165 | } 166 | - while (end >= data + 64) { 167 | + while (end -data >= 64) {
nit, space
Fixed.
utACK 7a34260.
utACK c01c065b9ded3399a6a480f15543827dd5e8eb4d
ACK