Suppress a harmless variable-time optimization by clang in memczero

[real-or-random]

This has been not been caught by the new constant-time tests because
valgrind currently gives us a zero exit code even if finds errors, see
#723 (comment) .

Note that the timing leak here was the bit whether a secret key was
out of range. This leak is harmless and not exploitable. It is just
our overcautious practice to prefer constant-time code even here.

Here's the "failure" on the current master:
https://travis-ci.org/github/bitcoin-core/secp256k1/jobs/666399947#L462

[real-or-random]

For completeness, this was the code generated by clang:

memczero():
src/util.h:169
static SECP256K1_INLINE void memczero(void *s, size_t len, int flag) {
    unsigned char *p;
    unsigned char mask = -(unsigned char)flag;
    p = (unsigned char *)s;
    while (len) {
        *p ^= *p & mask;
   20405:    0f 10 5b 10              movups 0x10(%rbx),%xmm3
   20409:    0f 10 53 20              movups 0x20(%rbx),%xmm2
   2040d:    0f 10 4b 30              movups 0x30(%rbx),%xmm1
   20411:    0f 57 c0                 xorps  %xmm0,%xmm0
   20414:    74 05                    je     2041b <secp256k1_ec_pubkey_create+0x15b>
   20416:    0f 57 e4                 xorps  %xmm4,%xmm4
   20419:    eb 03                    jmp    2041e <secp256k1_ec_pubkey_create+0x15e>
   2041b:    0f 10 23                 movups (%rbx),%xmm4
   2041e:    0f 11 23                 movups %xmm4,(%rbx)
   20421:    48 8b 04 24              mov    (%rsp),%rax
   20425:    75 42                    jne    20469 <secp256k1_ec_pubkey_create+0x1a9>
   20427:    0f 11 5b 10              movups %xmm3,0x10(%rbx)
   2042b:    75 45                    jne    20472 <secp256k1_ec_pubkey_create+0x1b2>
   2042d:    0f 11 53 20              movups %xmm2,0x20(%rbx)
   20431:    74 03                    je     20436 <secp256k1_ec_pubkey_create+0x176>
   20433:    0f 57 c9                 xorps  %xmm1,%xmm1
   20436:    0f 11 4b 30              movups %xmm1,0x30(%rbx)

This shows that our tests are essentially working. 👍 Except the exit value which will be fixed in #723.

[jonasnick]

Why add this pointer instead of making the flag argument volatile int?

[real-or-random]

This works too but I find volatile function parameters very obscure. Some C++ people agree with me: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p1152r0.html#parret

Also the placement of the comment is more natural with this version.

[jonasnick]

Afaics the argument isn't super strong ("it leaks function implementation information to the caller"). But what about making flagp a volatile int instead of pointer, and copy flag into it? Otherwise flagp is declared as a pointer to volatile int, but does not in fact point to a volatile int.

[real-or-random]

I changed it to your suggestion. (I didn't do this previously because I wrongly assumed it needs one more store).

Otherwise flagp is declared as a pointer to volatile int, but does not in fact point to a volatile int.

When working on this PR I learned the truth about volatile. While the C standards speaks about "volatile objects", what the authors meant and luckily what every compiler implemented are "volatile lvalues" [1]. In other words, being volatile is a property of an access to an object, not a property of the object itself. So both variants with and without pointer are equally good in practice but the one without pointer is simpler and indeed closer the letter of the standard.

[1] Defect report 476. http://www.open-std.org/jtc1/sc22/wg14/www/docs/summary.htm#dr_476

[gmaxwell]

I don't have an opinion on how this is done in general, but the fact that clang would make that non-constant time makes me extremely uneasy about what it will do in the future. It also seems a little inexplicable. A well predicted branch is cheap but still...

[jonasnick]

ACK 0199387