False clang-scan warning, likely from be64enc

[gperciva]

Notes mainly to myself. I've run out of steam right now, but I'll come back to it later (albeit possibly after clang 12.0 is out).

The clang static analyzer produces the scary-looking:

../../crypto/crypto_aesctr_shared.c:47:30: warning: The right operand of '^' is a garbage value [core.UndefinedBinaryOperatorResult]
                (*outbuf)[i] = (*inbuf)[i] ^ stream->buf[bytemod + i];
                                           ^ ~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.

The first instance of this arose in 9ff90c2, which is baffling since there's nothing there that would suggest this flaw.

Tracing it backwards, it appears that the analyzer doesn't believe that be64enc() initializes the memory. This is even true if we use #include <sys/endian.h> intsead of our own be64enc.

1. first level of investigation: if we do

diff --git a/crypto/crypto_aesctr_shared.c b/crypto/crypto_aesctr_shared.c
index 531bbeb8..a27e4e02 100644
--- a/crypto/crypto_aesctr_shared.c
+++ b/crypto/crypto_aesctr_shared.c
@@ -32,6 +32,7 @@ crypto_aesctr_stream_cipherblock_generate(struct crypto_aesctr * stream)
 
        /* Encrypt the cipherblock. */
        crypto_aes_encrypt_block(stream->pblk, stream->buf, stream->key);
+       stream->buf[0] = stream->buf[0];
 }
 
 /* Encrypt ${nbytes} bytes, then update ${inbuf}, ${outbuf}, and ${buflen}. */

Then we get this warning instead of the original one:

../../crypto/crypto_aesctr_shared.c:35:17: warning: Assigned value is garbage or undefined [core.uninitialized.Assign]
        stream->buf[0] = stream->buf[0];
                       ^ ~~~~~~~~~~~~~~
1 warning generated.

So at least it doesn't have anything to do with using the cipherblock.

2. We can silence the warning entirely by doing:

diff --git a/crypto/crypto_aesctr.c b/crypto/crypto_aesctr.c
index 2b4e018e..b6483f29 100644
--- a/crypto/crypto_aesctr.c
+++ b/crypto/crypto_aesctr.c
@@ -86,6 +86,10 @@ crypto_aesctr_init2(struct crypto_aesctr * stream,
        be64enc(stream->pblk, nonce);
        stream->bytectr = 0;
 
+       /* Silence the warning?!?! */
+       for (int i = 0; i < 4; i++)
+               stream->pblk[i] = stream->pblk[i];
+
        /*
         * Set the counter such that the least significant byte will wrap once
         * incremented.

This should be a no-op, but it's enough to make clang realize that we've initialized the first 4 bytes of stream->pblk. (And yes, we must use i < 4; 3 is not enough.)

So: it appears that clang's static analyzer isn't convinced that be64enc() initializes the first 32 bits of its output. I wonder if this it's getting caught up in the compiler's "try to recognize a byte-swap" code? The assembly output shows that it did recognize it as a byte-swap:

;               stream->key = key;
                mov     qword ptr [rdi], rsi
;       p[0] = (x >> 56) & 0xff;
                bswap   rdx
;       stream->bytectr = 0;
                mov     qword ptr [rdi + 8], 0
;       p[0] = (x >> 56) & 0xff;
                mov     qword ptr [rdi + 32], rdx
;       stream->pblk[15] = 0xff;
                mov     byte ptr [rdi + 47], -1

but I know that some implementations do a pair of be32enc(). For example, FreeBSD's /usr/include/sys/endian.h contains the alignment-agnostic:

static __inline void
be64enc(void *pp, uint64_t u)
{
        uint8_t *p = (uint8_t *)pp;

        be32enc(p, (uint32_t)(u >> 32));
        be32enc(p + 4, (uint32_t)(u & 0xffffffffU));
}

So maybe there's a convoluted path of the analyzer recognizing that the first 4 bytes come from nonce >> 32, noticing that the hard-coded nonce in our test file isn't bigger than 2^32, and then... forgetting that a right-shift will introduce 0s, and is thus well-defined? Hmm, no, that's nonsense. At least, if it was the case, we'd see these warnings in earlier versions of our code, not introduced at that particular commit.

FWIW, I see this warning on linux t4g as well, so it's not just a FreeBSD amd64 thing.

[gperciva]

I believe that this is an instance of https://bugs.llvm.org/show_bug.cgi?id=44114. Comment from a developer in that thread:

... our memory model fails to confirm that writing a 32-bit integer would initialize all 4 bytes of that integer

I've uploaded a fix in the https://github.com/Tarsnap/libcperciva/tree/fix-clang-scan-ridiculous branch, in case anybody is interested in investigating further. I've confirmed that the bug still exists in clang 12.

(I won't be proposing that fix for merging, because we don't want to clutter our code with #ifdefs for specific compilers / tools, and this isn't important enough to warrant a POSIXFAIL define.)

EDIT: confirmed that clang 13 also has this bug.