Skip redundant memset(p, 0, 0) calls where p is not valid for writing.
Context: See second section of #15950 (comment)
Nothing urgent obviously and perhaps not even an issue (the spec is a bit unclear) but would be nice to err on the side of caution in unclear cases like this :-)
utACK 953ed47b.
utACK 953ed47b42eac7453ad7e6436098301f0cb523a4
utACK 953ed47b42eac7453ad7e6436098301f0cb523a4
Interesting case since it is not unambiguously needed. I'm curious what the consensus on this will be.
10 | @@ -11,7 +11,9 @@ CHMAC_SHA256::CHMAC_SHA256(const unsigned char* key, size_t keylen) 11 | unsigned char rkey[64]; 12 | if (keylen <= 64) { 13 | memcpy(rkey, key, keylen); 14 | - memset(rkey + keylen, 0, 64 - keylen); 15 | + if (keylen < 64) {
I… don't think this makes the code any more understandable, I had to think for a while to understand why this was <64 and why one'd want to skip the memset if the data is smaller than that.
Calling memset with size 0 is no problem. If calling it with a NULL pointer is, then a check for a null pointer should be added, not a check on the length.
Oh, I might have been a bit brief in the OP - let me clarify:
The goal of this PR is not try to avoid memset(nullptr, …, …) (plus: rkey + keylen cannot be nullptr).
The goal of the PR is to avoid memset(p, …, …) where p is a pointer which is outside of the object and thus not valid for writing.
Please note that rkey + 64 is outside of the object.
See these discussions for context:
Please re-review in light of this clarification :-)
Should I add a comment in the code making the rationale more clear?
Not sure if it's worth a discussion, but I don't see how the call to memset can result in undefined behaviour when memset takes a pointer to one past the last element of the rkey array and the size_t argument is 0. This can't go wrong, can it?
@l2a5b1 Did you read the discussion(s) linked in the OP? :-)
@practicalswift Thanks, yes I did.
From #15950 (comment)
p1 |= ((unsigned char)HexDigit(psz--) << 4);
I agree. Here the behaviour will be undefined when psz takes address one before the initial element.
In case of this pull request, taking a pointer to one past the last element of the rkey array is defined as long as the pointer is not dereferenced or written to.
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:
https://stackoverflow.com/questions/29844298/is-it-legal-to-call-memcpy-with-zero-length-on-a-pointer-just-past-the-end-of-an The discussion is about memcpy but memset should have the same pointer requirements in this case.
I am not sure if I agree with the accepted answer in the stackoverflow thread.
I do agree that the spec leaves room for interpretation.
My interpretation of the spec is that it is perfectly fine that memset takes a pointer that is one past the last element of an array provided that the size_t argument is 0.
But more importantly, if memset wasn't able to handle this case, I assume that it would be very well documented or common knowledge.
Perhaps you can add the arguments that you think are valid or relevant from the stackoverflow thread to the pull request description.
I do agree that the spec leaves room for interpretation.
In cases where the spec is unclear I think it makes sense to err on the side of caution. From the OP:
"Nothing urgent obviously and perhaps not even an issue (the spec is a bit unclear) but would be nice to err on the side of caution in unclear cases like this :-)"
@MarcoFalke Four utACK:s - ready for merge?
utACK 953ed47b42eac7453ad7e6436098301f0cb523a4
NACKish lacking benchmarks showing this has any significance. We shouldn't be adding code to src/crypto without a more compelling reason.
@jamesob Benchmarking? This is not an optimisation :-)
Although I agree on being conservative, I don't see how this could cause a problem (unless there is already a problem and this fixes it).