Cleaning up pgcrypto/crypt-des.c to avoid compiler warnings - Mailing list pgsql-hackers

For some time now, buildfarm member serinus has been complaining
about pgcrypto/crypt-des.c:

[1601/2408] /usr/bin/ccache /usr/lib/gcc-snapshot/bin/gcc -Icontrib/pgcrypto/pgcrypto.so.p -Isrc/include
-I../pgsql/src/include-I/usr/include/libxml2 -fdiagnostics-color=never -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -O2
-g-fno-strict-aliasing -fwrapv -fexcess-precision=standard -fpch-deps -D_GNU_SOURCE -Wpointer-arith -Werror=vla
-Wmissing-format-attribute-Wcast-function-type -Wshadow=compatible-local -Wformat-security -Wmissing-prototypes
-Wold-style-declaration-Wold-style-definition -Wstrict-prototypes -Wimplicit-fallthrough=5
-Wdeclaration-after-statement-Wmissing-variable-declarations -Wno-format-truncation -Wno-stringop-truncation -O1 -ggdb
-g3-fno-omit-frame-pointer -Wall -Wextra -Wno-unused-parameter -Wno-sign-compare -Wno-missing-field-initializers -O3
-Wno-unterminated-string-initialization-Wno-clobbered -fPIC -isystem /usr/include/mit-krb5 -isystem
/usr/include/mit-krb5-pthread -fvisibility=hidden -MD -MQ contrib/pgcrypto/pgcrypto.so.p/crypt-des.c.o -MF
contrib/pgcrypto/pgcrypto.so.p/crypt-des.c.o.d-o contrib/pgcrypto/pgcrypto.so.p/crypt-des.c.o -c
../pgsql/contrib/pgcrypto/crypt-des.c
../pgsql/contrib/pgcrypto/crypt-des.c: In function 'px_crypt_des':
../pgsql/contrib/pgcrypto/crypt-des.c:675:22: warning: writing 8 bytes into a region of size 7 [-Wstringop-overflow=]
  675 |                 *q++ = *key << 1;
      |                 ~~~~~^~~~~~~~~~~
../pgsql/contrib/pgcrypto/crypt-des.c:659:33: note: at offset [1, 2] into destination object 'keybuf' of size 8
  659 |                                 keybuf[2];
      |                                 ^~~~~~
../pgsql/contrib/pgcrypto/crypt-des.c:659:33: note: at offset [2, 3] into destination object 'keybuf' of size 8
../pgsql/contrib/pgcrypto/crypt-des.c:659:33: note: at offset [3, 4] into destination object 'keybuf' of size 8
../pgsql/contrib/pgcrypto/crypt-des.c:659:33: note: at offset [4, 5] into destination object 'keybuf' of size 8
../pgsql/contrib/pgcrypto/crypt-des.c:659:33: note: at offset [5, 6] into destination object 'keybuf' of size 8
../pgsql/contrib/pgcrypto/crypt-des.c:659:33: note: at offset [6, 7] into destination object 'keybuf' of size 8
../pgsql/contrib/pgcrypto/crypt-des.c:659:33: note: at offset [7, 8] into destination object 'keybuf' of size 8
../pgsql/contrib/pgcrypto/crypt-des.c:675:22: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=]
  675 |                 *q++ = *key << 1;
      |                 ~~~~~^~~~~~~~~~~
../pgsql/contrib/pgcrypto/crypt-des.c:659:33: note: at offset 8 into destination object 'keybuf' of size 8
  659 |                                 keybuf[2];
      |                                 ^~~~~~
../pgsql/contrib/pgcrypto/crypt-des.c:659:33: note: at offset [9, 10] into destination object 'keybuf' of size 8
../pgsql/contrib/pgcrypto/crypt-des.c:659:33: note: at offset [10, 11] into destination object 'keybuf' of size 8
../pgsql/contrib/pgcrypto/crypt-des.c:659:33: note: at offset [11, 12] into destination object 'keybuf' of size 8
../pgsql/contrib/pgcrypto/crypt-des.c:659:33: note: at offset [12, 13] into destination object 'keybuf' of size 8
../pgsql/contrib/pgcrypto/crypt-des.c:659:33: note: at offset [13, 14] into destination object 'keybuf' of size 8
../pgsql/contrib/pgcrypto/crypt-des.c:659:33: note: at offset [14, 15] into destination object 'keybuf' of size 8

It's possible to see by inspection that this loop doesn't actually
overrun the buffer.  But the termination condition is oddly written:

    q = (uint8 *) keybuf;
    while (q - (uint8 *) keybuf - 8)

I think this odd coding, perhaps combined with all the cowboy casting
that's going on here, is what's confusing gcc into giving a warning.
I propose the attached patch to make this code less ugly and (with
luck) suppress the warning.

            regards, tom lane

From 15f26029193d922468ceed0c0cef11627770bcd8 Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Fri, 25 Sep 2026 13:47:38 -0400
Subject: [PATCH v1] Modernize crypt-des.c a bit, avoiding dubious casts.

The header comment for this file says it assumes that "the CPU is not
picky about alignment", which we wouldn't really accept for Postgres.
The two functions that have alignment requirements have been hacked in
ways that are inconsistent and yet both ugly.  des_setkey just casts
its "const char *" argument to "const uint32 *", relying on the caller
to ensure that that pointer is actually word-aligned, which the caller
does by declaring the buffer as uint32[] and then casting to "char *".
Meanwhile des_cipher carefully memcpy's to and from an internal
uint32[] buffer, which is pretty silly given that what it's passed
must be word-aligned for the benefit of des_setkey.  And on top of
that we have a bunch of random casting and strangely-written pointer
arithmetic in the caller px_crypt_des, which I think is what's
confusing buildfarm member serinus into issuing scary-looking warnings
about buffer overruns.

Let's clean this up by converting the buffer variable into a union
of a uint8 array and a uint32 array, so that we can remove all these
casts, and with a little luck satisfy serinus that the code's okay.
---
 contrib/pgcrypto/crypt-des.c | 50 +++++++++++++++---------------------
 1 file changed, 21 insertions(+), 29 deletions(-)

diff --git a/contrib/pgcrypto/crypt-des.c b/contrib/pgcrypto/crypt-des.c
index 98c30ea122e..4afb39615f2 100644
--- a/contrib/pgcrypto/crypt-des.c
+++ b/contrib/pgcrypto/crypt-des.c
@@ -53,11 +53,6 @@
  * pbox, and final permutations are inverted (this has been brought to the
  * attention of the author).  A list of errata for this book has been
  * posted to the sci.crypt newsgroup by the author and is available for FTP.
- *
- * ARCHITECTURE ASSUMPTIONS:
- *    It is assumed that the 8-byte arrays passed by reference can be
- *    addressed as arrays of uint32's (ie. the CPU is not picky about
- *    alignment).
  */

 #include "postgres.h"
@@ -393,7 +388,7 @@ setup_salt(long salt)
 }

 static int
-des_setkey(const char *key)
+des_setkey(const uint32 *key)
 {
     uint32        k0,
                 k1,
@@ -405,8 +400,8 @@ des_setkey(const char *key)
     if (!des_initialised)
         des_init();

-    rawkey0 = pg_ntoh32(*(const uint32 *) key);
-    rawkey1 = pg_ntoh32(*(const uint32 *) (key + 4));
+    rawkey0 = pg_ntoh32(key[0]);
+    rawkey1 = pg_ntoh32(key[1]);

     if ((rawkey0 | rawkey1)
         && rawkey0 == old_rawkey0
@@ -614,9 +609,8 @@ do_des(uint32 l_in, uint32 r_in, uint32 *l_out, uint32 *r_out, int count)
 }

 static int
-des_cipher(const char *in, char *out, long salt, int count)
+des_cipher(const uint32 *in, uint32 *out, long salt, int count)
 {
-    uint32        buffer[2];
     uint32        l_out,
                 r_out,
                 rawl,
@@ -628,21 +622,15 @@ des_cipher(const char *in, char *out, long salt, int count)

     setup_salt(salt);

-    /* copy data to avoid assuming input is word-aligned */
-    memcpy(buffer, in, sizeof(buffer));
-
-    rawl = pg_ntoh32(buffer[0]);
-    rawr = pg_ntoh32(buffer[1]);
+    rawl = pg_ntoh32(in[0]);
+    rawr = pg_ntoh32(in[1]);

     retval = do_des(rawl, rawr, &l_out, &r_out, count);
     if (retval)
         return retval;

-    buffer[0] = pg_hton32(l_out);
-    buffer[1] = pg_hton32(r_out);
-
-    /* copy data to avoid assuming output is word-aligned */
-    memcpy(out, buffer, sizeof(buffer));
+    out[0] = pg_hton32(l_out);
+    out[1] = pg_hton32(r_out);

     return retval;
 }
@@ -655,8 +643,12 @@ px_crypt_des(const char *key, const char *setting)
                 salt,
                 l,
                 r0,
-                r1,
-                keybuf[2];
+                r1;
+    union
+    {
+        uint8        bytes[8];
+        uint32        ints[2];
+    }            keybuf;
     char       *p;
     uint8       *q;
     static char output[21];
@@ -669,14 +661,14 @@ px_crypt_des(const char *key, const char *setting)
      * Copy the key, shifting each character up by one bit and padding with
      * zeros.
      */
-    q = (uint8 *) keybuf;
-    while (q - (uint8 *) keybuf - 8)
+    q = &keybuf.bytes[0];
+    while (q < &keybuf.bytes[8])
     {
         *q++ = *key << 1;
         if (*key != '\0')
             key++;
     }
-    if (des_setkey((char *) keybuf))
+    if (des_setkey(keybuf.ints))
         return NULL;

 #ifndef DISABLE_XDES
@@ -707,17 +699,17 @@ px_crypt_des(const char *key, const char *setting)
             /*
              * Encrypt the key with itself.
              */
-            if (des_cipher((char *) keybuf, (char *) keybuf, 0L, 1))
+            if (des_cipher(keybuf.ints, keybuf.ints, 0L, 1))
                 return NULL;

             /*
              * And XOR with the next 8 characters of the key.
              */
-            q = (uint8 *) keybuf;
-            while (q - (uint8 *) keybuf - 8 && *key)
+            q = &keybuf.bytes[0];
+            while (q < &keybuf.bytes[8] && *key)
                 *q++ ^= *key++ << 1;

-            if (des_setkey((char *) keybuf))
+            if (des_setkey(keybuf.ints))
                 return NULL;
         }
         strlcpy(output, setting, 10);
--
2.52.0