Hi, > > > On Wed, 29 Apr 2026 at 23:17, Andres Freund <[email protected]> wrote: > >> Hi, >> >> Recently I got a bit of a shock building postgres with gcc-16: >> >> ../../../../../home/andres/src/postgresql/contrib/pgcrypto/crypt-des.c: >> In function ‘px_crypt_des’: >> ../../../../../home/andres/src/postgresql/contrib/pgcrypto/crypt-des.c:675:22: >> warning: writing 8 bytes into a region of size 7 [-Wstringop-overflow=] >> 675 | *q++ = *key << 1; >> | ~~~~~^~~~~~~~~~~ >> ../../../../../home/andres/src/postgresql/contrib/pgcrypto/crypt-des.c:659:33: >> note: at offset [1, 2] into destination object ‘keybuf’ of size 8 >> 659 | keybuf[2]; >> | ^~~~~~ >> ../../../../../home/andres/src/postgresql/contrib/pgcrypto/crypt-des.c:659:33: >> note: at offset [2, 3] into destination object ‘keybuf’ of size 8 >> ../../../../../home/andres/src/postgresql/contrib/pgcrypto/crypt-des.c:659:33: >> note: at offset [3, 4] into destination object ‘keybuf’ of size 8 >> ../../../../../home/andres/src/postgresql/contrib/pgcrypto/crypt-des.c:659:33: >> note: at offset [4, 5] into destination object ‘keybuf’ of size 8 >> ../../../../../home/andres/src/postgresql/contrib/pgcrypto/crypt-des.c:659:33: >> note: at offset [5, 6] into destination object ‘keybuf’ of size 8 >> ../../../../../home/andres/src/postgresql/contrib/pgcrypto/crypt-des.c:659:33: >> note: at offset [6, 7] into destination object ‘keybuf’ of size 8 >> ../../../../../home/andres/src/postgresql/contrib/pgcrypto/crypt-des.c:659:33: >> note: at offset [7, 8] into destination object ‘keybuf’ of size 8 >> ../../../../../home/andres/src/postgresql/contrib/pgcrypto/crypt-des.c:675:22: >> warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=] >> 675 | *q++ = *key << 1; >> | ~~~~~^~~~~~~~~~~ >> ../../../../../home/andres/src/postgresql/contrib/pgcrypto/crypt-des.c:659:33: >> note: at offset 8 into destination object ‘keybuf’ of size 8 >> 659 | keybuf[2]; >> | ^~~~~~ >> ../../../../../home/andres/src/postgresql/contrib/pgcrypto/crypt-des.c:659:33: >> note: at offset [9, 10] into destination object ‘keybuf’ of size 8 >> ../../../../../home/andres/src/postgresql/contrib/pgcrypto/crypt-des.c:659:33: >> note: at offset [10, 11] into destination object ‘keybuf’ of size 8 >> ../../../../../home/andres/src/postgresql/contrib/pgcrypto/crypt-des.c:659:33: >> note: at offset [11, 12] into destination object ‘keybuf’ of size 8 >> ../../../../../home/andres/src/postgresql/contrib/pgcrypto/crypt-des.c:659:33: >> note: at offset [12, 13] into destination object ‘keybuf’ of size 8 >> ../../../../../home/andres/src/postgresql/contrib/pgcrypto/crypt-des.c:659:33: >> note: at offset [13, 14] into destination object ‘keybuf’ of size 8 >> ../../../../../home/andres/src/postgresql/contrib/pgcrypto/crypt-des.c:659:33: >> note: at offset [14, 15] into destination object ‘keybuf’ of size 8 >> >> >> Luckily it turns out that that the warning is spurious, due to a bug in >> gcc >> [1]. >> >> However, it took me quite a while to figure out what the hell the code was >> doing: >> >> char * >> px_crypt_des(const char *key, const char *setting) >> { >> uint32 keybuf[2]; >> ... >> uint8 *q; >> ... >> >> /* >> * Copy the key, shifting each character up by one bit and padding >> with >> * zeros. >> */ >> q = (uint8 *) keybuf; >> while (q - (uint8 *) keybuf - 8) >> { >> *q++ = *key << 1; >> if (*key != '\0') >> key++; >> } >> >> Like, it's far from immediately obvious where the 8 is coming from (it's >> the >> size of keybuf), whether there are precedence issues or what this is even >> trying to achieve. >> >> And it's still not clear to me why on earth it makes sense to write it >> that >> complicated, when it seems something like >> >> for (int byteno = 0; byteno < sizeof(keybuf); byteno++) >> { >> *q++ = *key << 1; >> if (*key != '\0') >> key++; >> } >> >> would do the same thing, except be trivially understandable for humans and >> compilers. >> >> I've created a patch for it.
It replaces the obscure "while (q - (uint8 *) keybuf - 8)" loop conditions in px_crypt_des() with for-loops bounded by sizeof(keybuf). To avoid introducing a new -Wsign-compare warning against sizeof, I used the new loop counter (bytenum) as size_t. The logic remains equivalent, it preserves the exact iteration counts and the *key short-circuit in the extended-DES loop, but makes the bounds obvious to both readers and the compiler. Please find the patch attached. Thoughts? Regards, Ayush
v1-0001-Avoid-obscure-DES-key-buffer-loop-bounds.patch
Description: Binary data
