This particular mistake is all the more infuriating because it comes from a precaution. Or trying to silence a compiler warning:
partialBlock = (unsigned int)(dataByteLen - i);
Where both `dataByteLen` and `i` where actually `size_t`.
Assuming this is close enough to C, what happens is that we're converting a difference between `size_t` into a mere `unsigned`, and since they're not the same sizes on 64-bit platforms this can give `partialBlock` the wrong value, and the whole thing then snowballs into a catastrophic error that is not trivial to test because it only happens with huge buffer sizes.
The biggest mistake here is having written `(unsigned int)` instead of `(size_t)`. But the reason it happened in the first place is because they tried to do the right thing: writing the cast as a precaution, even though the following would have worked:
partialBlock = dataByteLen - i;
I really can't fault them: because it was a difference it could theoretically yield a "negative" result, and therefore intuitively the type of a difference should be signed, so we should cast it back to unsigned to be crystal clear. I knew C was dangerous, but to be honest I didn't expect such a wicked mind game.
But some people might think that it could, so putting the cast could increase readability for them. A similar reasoning applies with operator precedence: even though it's very well defined, we tend to forget it, so instead we put additional parentheses. But in a couple specific cases it hurts more than it helps.
So, (unsigned int) - (unsigned int) → (unsigned int) is guaranteed on every platform.
But relevant to the bad SHA-3 code, the resulting type of size_t - size_t can be surprising. First, size_t is not guaranteed to have any relation with unsigned int; it could be narrower, the same, or wider. For example, size_t could map to unsigned short, unsigned int, unsigned long, or unsigned long long.
If size_t is defined as unsigned short (note that size_t must be at least 16 bits), and short is 16 bits wide, and int is 32 bits wide, then the calculation of size_t - size_t will be promoted to (signed int) - (signed int), and thus the result will have the type signed int.
Assuming this is close enough to C, what happens is that we're converting a difference between `size_t` into a mere `unsigned`, and since they're not the same sizes on 64-bit platforms this can give `partialBlock` the wrong value, and the whole thing then snowballs into a catastrophic error that is not trivial to test because it only happens with huge buffer sizes.
The biggest mistake here is having written `(unsigned int)` instead of `(size_t)`. But the reason it happened in the first place is because they tried to do the right thing: writing the cast as a precaution, even though the following would have worked:
I really can't fault them: because it was a difference it could theoretically yield a "negative" result, and therefore intuitively the type of a difference should be signed, so we should cast it back to unsigned to be crystal clear. I knew C was dangerous, but to be honest I didn't expect such a wicked mind game.Now I'm going to have to take a look at my code.