Cleaning up pgcrypto/crypt-des.c to avoid compiler warnings

Hi,

On Fri, 25 Sept 2026 at 23:28, Tom Lane <tgl@sss.pgh.pa.us> wrote:

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.

Yeah I agree that it's pretty ugly.

We had an open patch for that obscure loop earlier[1]/messages/by-id/CAJTYsWWkNO0fKZit+_hRUUgrP-Gyu6pGDT5w0w+4dYOM7HMF1Q@mail.gmail.com.
(I'll close that commitfest item)

I reviewed your patch and it looks good and clearer to me.

Thanks for the patch.

Regards,
Ayush

[1]: /messages/by-id/CAJTYsWWkNO0fKZit+_hRUUgrP-Gyu6pGDT5w0w+4dYOM7HMF1Q@mail.gmail.com
/messages/by-id/CAJTYsWWkNO0fKZit+_hRUUgrP-Gyu6pGDT5w0w+4dYOM7HMF1Q@mail.gmail.com

On Fri, Sep 25, 2026 at 01:58:17PM -0400, Tom Lane wrote:

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.

Getting rid of the "q" variable altogether might have a better chance of
clearing the warnings, and IMHO it further improves readability:

+	q = &keybuf.bytes[0];
+	while (q < &keybuf.bytes[8])
{
*q++ = *key << 1;

for (i = 0; i < 8; i++)
{
keybuf.bytes[i] = *key << 1;

+			q = &keybuf.bytes[0];
+			while (q < &keybuf.bytes[8] && *key)
*q++ ^= *key++ << 1;

for (i = 0; i < 8 && *key; i++)
keybuf.bytes[i] ^= *key++ << 1;

--
nathan

Ayush Tiwari <ayushtiwari.slg01@gmail.com> writes:

On Fri, 25 Sept 2026 at 23:28, Tom Lane <tgl@sss.pgh.pa.us> wrote:

For some time now, buildfarm member serinus has been complaining
about pgcrypto/crypt-des.c:
...
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.

Yeah I agree that it's pretty ugly.
We had an open patch for that obscure loop earlier[1].
(I'll close that commitfest item)

Oh! I'd missed that thread. Thank you for pointing me to it, because
I had been unable to reproduce the warning, but now I know that it's
necessary to use -O3 along with a recent gcc to see it. Armed with
that knowledge, I find that my patch does not suppress the warning :-(
I'll use this variant of your proposal instead:

for (size_t q = 0; q < lengthof(keybuf.bytes); q++)
{
keybuf.bytes[q] = *key << 1;
...

That does suppress the warning, at least with the gcc 16.2.1 that
I'm testing with.

Thanks for the review!

regards, tom lane

Nathan Bossart <nathandbossart@gmail.com> writes:

On Fri, Sep 25, 2026 at 01:58:17PM -0400, Tom Lane wrote:

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.

Getting rid of the "q" variable altogether might have a better chance of
clearing the warnings, and IMHO it further improves readability:

Yeah, I adopted something close to Ayush's variant of that.

regards, tom lane