Spurious warnings in crypto-des.c when building with gcc-16 -O3 - Mailing list pgsql-hackers

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
ofsize 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
size0 [-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
destinationobject ‘keybuf’ of size 8
 
../../../../../home/andres/src/postgresql/contrib/pgcrypto/crypt-des.c:659:33: note: at offset [11, 12] into
destinationobject ‘keybuf’ of size 8
 
../../../../../home/andres/src/postgresql/contrib/pgcrypto/crypt-des.c:659:33: note: at offset [12, 13] into
destinationobject ‘keybuf’ of size 8
 
../../../../../home/andres/src/postgresql/contrib/pgcrypto/crypt-des.c:659:33: note: at offset [13, 14] into
destinationobject ‘keybuf’ of size 8
 
../../../../../home/andres/src/postgresql/contrib/pgcrypto/crypt-des.c:659:33: note: at offset [14, 15] into
destinationobject ‘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.


Am I missing something or is what I suggest equivalent?  Any reason to not
change it that way, both to clarify the code and to work around the spurious
warning?


Greetings,

Andres Freund


[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113664