Subject: pgsql: Make cancel request keys longer

From: Heikki Linnakangas <[email-masked]>
Date: Wed, 02 Apr 2025 13:43:17 +0000

Make cancel request keys longer

Currently, the cancel request key is a 32-bit token, which isn't very
much entropy. If you want to cancel another session's query, you can
brute-force it. In most environments, an unauthorized cancellation of
a query isn't very serious, but it nevertheless would be nice to have
more protection from it. Hence make the key longer, to make it harder
to guess.

The longer cancellation keys are generated when using the new protocol
version 3.2. For connections using version 3.0, short 4-bytes keys are
still used.

The new longer key length is not hardcoded in the protocol anymore,
the client is expected to deal with variable length keys, up to 256
bytes. This flexibility allows e.g. a connection pooler to add more
information to the cancel key, which might be useful for finding the
connection.

Reviewed-by: Jelte Fennema-Nio <[email-masked]>
Reviewed-by: Robert Haas <[email-masked]> (earlier versions)
Discussion: https://www.postgresql.org/message-id/[email-masked]

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/a460251f0a1ac987f0225203ff9593704da0b1a9

Modified Files
--------------
doc/src/sgml/protocol.sgml                         |  29 +++++-
src/backend/storage/ipc/procsignal.c               |  23 ++---
src/backend/tcop/backend_startup.c                 |  55 ++++++-----
src/backend/tcop/postgres.c                        |  15 ++-
src/backend/utils/init/globals.c                   |   5 +-
src/backend/utils/init/postinit.c                  |   2 +-
src/include/libpq/pqcomm.h                         |   8 +-
src/include/miscadmin.h                            |   4 +-
src/include/storage/procsignal.h                   |  14 ++-
src/interfaces/libpq/fe-cancel.c                   | 102 +++++++++++++++++----
src/interfaces/libpq/fe-connect.c                  |  15 ++-
src/interfaces/libpq/fe-protocol3.c                |  45 ++++++++-
src/interfaces/libpq/libpq-int.h                   |   7 +-
.../modules/libpq_pipeline/t/001_libpq_pipeline.pl |  12 ++-
14 files changed, 252 insertions(+), 84 deletions(-)

---

From: Peter Eisentraut <[email-masked]>
Date: Tue, 8 Apr 2025 19:06:33 +0200

> Make cancel request keys longer

This patch changed the signature of ProcSignal()

-ProcSignalInit(bool cancel_key_valid, int32 cancel_key)
+ProcSignalInit(char *cancel_key, int cancel_key_len)

but did not update the caller in auxprocess.c:

ProcSignalInit(false, 0);

This gives a warning with clang.

While I was looking at this, I suggest to make the first argument void 
*.  This is consistent for passing binary data.

Also, I wonder why MyCancelKeyLength is of type uint8 rather than 
something more mundane like int.  There doesn't seem to be any API 
reason for this type.

See attached patch for possible changes.

From 8abe6e66521de7fe5d2132fd5471265ef1bf6b0e Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <[email-masked]>
Date: Tue, 8 Apr 2025 19:01:29 +0200
Subject: [PATCH] WIP: Fix cancel key stuff

---
 src/backend/postmaster/auxprocess.c  | 2 +-
 src/backend/storage/ipc/procsignal.c | 2 +-
 src/backend/utils/init/globals.c     | 2 +-
 src/include/miscadmin.h              | 2 +-
 src/include/storage/procsignal.h     | 2 +-
 5 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/backend/postmaster/auxprocess.c b/src/backend/postmaster/auxprocess.c
index 4f6795f7265..a6d3630398f 100644
--- a/src/backend/postmaster/auxprocess.c
+++ b/src/backend/postmaster/auxprocess.c
@@ -66,7 +66,7 @@ AuxiliaryProcessMainCommon(void)
 
 	BaseInit();
 
-	ProcSignalInit(false, 0);
+	ProcSignalInit(NULL, 0);
 
 	/*
 	 * Auxiliary processes don't run transactions, but they may need a
diff --git a/src/backend/storage/ipc/procsignal.c b/src/backend/storage/ipc/procsignal.c
index a3c2cd12277..33b1a5be276 100644
--- a/src/backend/storage/ipc/procsignal.c
+++ b/src/backend/storage/ipc/procsignal.c
@@ -162,7 +162,7 @@ ProcSignalShmemInit(void)
  *		Register the current process in the ProcSignal array
  */
 void
-ProcSignalInit(char *cancel_key, int cancel_key_len)
+ProcSignalInit(const void *cancel_key, int cancel_key_len)
 {
 	ProcSignalSlot *slot;
 	uint64		barrier_generation;
diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
index 92304a1f124..1847e7c85d3 100644
--- a/src/backend/utils/init/globals.c
+++ b/src/backend/utils/init/globals.c
@@ -51,7 +51,7 @@ TimestampTz MyStartTimestamp;
 struct ClientSocket *MyClientSocket;
 struct Port *MyProcPort;
 char		MyCancelKey[MAX_CANCEL_KEY_LENGTH];
-uint8		MyCancelKeyLength = 0;
+int			MyCancelKeyLength = 0;
 int			MyPMChildSlot;
 
 /*
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index 58b2496a9cb..72f5655fb34 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -193,7 +193,7 @@ extern PGDLLIMPORT TimestampTz MyStartTimestamp;
 extern PGDLLIMPORT struct Port *MyProcPort;
 extern PGDLLIMPORT struct Latch *MyLatch;
 extern PGDLLIMPORT char MyCancelKey[];
-extern PGDLLIMPORT uint8 MyCancelKeyLength;
+extern PGDLLIMPORT int MyCancelKeyLength;
 extern PGDLLIMPORT int MyPMChildSlot;
 
 extern PGDLLIMPORT char OutputFileName[];
diff --git a/src/include/storage/procsignal.h b/src/include/storage/procsignal.h
index cfe14631445..08108a5d7de 100644
--- a/src/include/storage/procsignal.h
+++ b/src/include/storage/procsignal.h
@@ -73,7 +73,7 @@ typedef enum
 extern Size ProcSignalShmemSize(void);
 extern void ProcSignalShmemInit(void);
 
-extern void ProcSignalInit(char *cancel_key, int cancel_key_len);
+extern void ProcSignalInit(const void *cancel_key, int cancel_key_len);
 extern int	SendProcSignal(pid_t pid, ProcSignalReason reason,
 						   ProcNumber procNumber);
 extern void SendCancelRequest(int backendPID, char *cancel_key, int cancel_key_len);
-- 
2.49.0

---

From: Heikki Linnakangas <[email-masked]>
Date: Tue, 8 Apr 2025 22:41:31 +0300

> This gives a warning with clang.

Good catch. I wonder why the cirrus CI didn't complain, it has a step to 
check for warnings with clang.

> *.  This is consistent for passing binary data.

Ok, sure.

> something more mundane like int.  There doesn't seem to be any API 

Agreed. The cancel key length is documented to be at most 256 bytes, but 
that's more of a coincidence, nothing depends on that variable being uint8.

> See attached patch for possible changes.

Looks good to me. I can commit these tomorrow, or feel free to do it 
yourself too.

Thank you!

-- 
Heikki Linnakangas
Neon (https://neon.tech)

---

From: Heikki Linnakangas <[email-masked]>
Date: Wed, 9 Apr 2025 11:53:20 +0300

>> *.  This is consistent for passing binary data.

On second thoughts, -1 on that. 'void *' is appropriate for functions 
like libc's read() or pq_sendbytes(), where the buffer can point to 
anything. In other words, the caller is expected to have a pointer like 
'foobar *', and it gets cast to 'void *' when you call the function. 
That's not the case with the cancellation key. The cancellation key is 
just an array of bytes, the caller is expected to pass an array of 
bytes, not a struct.

The right precedent for that are e.g. SCRAM functions in scram-common.h, 
for example. They use "const uint8 *" for the hashes.

I'll switch to "const uint *" everywhere that deals with cancel keys. 
There are a few more variables elsewhere in the backend and in libpq.

> Looks good to me. I can commit these tomorrow, or feel free to do it 

I'm on this now.

-- 
Heikki Linnakangas
Neon (https://neon.tech)

---

From: Peter Eisentraut <[email-masked]>
Date: Wed, 9 Apr 2025 11:39:49 +0200

On 09.04.25 10:53, Heikki Linnakangas wrote:
>>> While I was looking at this, I suggest to make the first argument 
>>> void *.  This is consistent for passing binary data.
>>
>> Ok, sure.
> 

I was having the same second thoughts overnight.  I agree with your 
conclusion.

---

From: Heikki Linnakangas <[email-masked]>
Date: Wed, 9 Apr 2025 13:28:03 +0300

(moving to pgsql-hackers)

On 09/04/2025 12:39, Peter Eisentraut wrote:
>> anything. In other words, the caller is expected to have a pointer 
>> like 'foobar *', and it gets cast to 'void *' when you call the 
>> function. That's not the case with the cancellation key. The 
>> cancellation key is just an array of bytes, the caller is expected to 
>> pass an array of bytes, not a struct.
>>
>> The right precedent for that are e.g. SCRAM functions in scram- 
>> common.h, for example. They use "const uint8 *" for the hashes.
>>

Here's a patch to change cancellation keys to "uint8 *". I did the same 
for a few other places, namely the new scram_client_key_binary and 
scram_server_key_binary fields in pg_conn, and a few libpq functions 
that started to give compiler warnings after that. There probably would 
be more code that could be changed to follow this convention, but I 
didn't look hard. What do you think?

I'm on the edge with the pg_b64_encode/decode functions, whether they 
should work on "uint8 *" or "void *". On one hand, you do base64 
encoding on a byte array, which would support "uint8 *". But on the 
other hand, you might use it for encoding things with more structure, 
which would support "void *". I went with "void *", mostly out of 
convenience as many of the SCRAM functions that currently use 
pg_b64_encode/decode, use "char *" to represent byte arrays. But 
arguably those should be changed to use "uint8 *" too.

I committed the other parts of your original patch, thanks!

-- 
Heikki Linnakangas
Neon (https://neon.tech)
From 6d3a0ac89b76fe5f82aad5a3522e55fc165cd360 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <[email-masked]>
Date: Wed, 9 Apr 2025 13:17:17 +0300
Subject: [PATCH 1/1] Use 'void *' for arbitrary buffers, 'uint8 *' for byte
 arrays

A 'void *' argument suggests that the caller might pass an arbitrary
struct, which is appropriate for functions like libc's read/write, or
pq_sendbytes(). 'uint8 *' is more appropriate for byte arrays that
have no structure, like the cancellation keys or SCRAM tokens. Change
a few places to follow that convention.

Discussion: https://www.postgresql.org/message-id/[email-masked]
---
 src/backend/storage/ipc/procsignal.c |  6 +++---
 src/backend/utils/init/globals.c     |  2 +-
 src/common/base64.c                  | 16 ++++++++--------
 src/include/common/base64.h          |  4 ++--
 src/include/libpq/pqcomm.h           |  2 +-
 src/include/miscadmin.h              |  2 +-
 src/include/storage/procsignal.h     |  4 ++--
 src/interfaces/libpq/fe-cancel.c     |  2 +-
 src/interfaces/libpq/fe-misc.c       | 10 +++++-----
 src/interfaces/libpq/fe-protocol3.c  |  6 +++---
 src/interfaces/libpq/libpq-int.h     | 12 ++++++------
 11 files changed, 33 insertions(+), 33 deletions(-)

diff --git a/src/backend/storage/ipc/procsignal.c b/src/backend/storage/ipc/procsignal.c
index a3c2cd12277..f9dafbcaf22 100644
--- a/src/backend/storage/ipc/procsignal.c
+++ b/src/backend/storage/ipc/procsignal.c
@@ -64,7 +64,7 @@ typedef struct
 {
 	pg_atomic_uint32 pss_pid;
 	int			pss_cancel_key_len; /* 0 means no cancellation is possible */
-	char		pss_cancel_key[MAX_CANCEL_KEY_LENGTH];
+	uint8		pss_cancel_key[MAX_CANCEL_KEY_LENGTH];
 	volatile sig_atomic_t pss_signalFlags[NUM_PROCSIGNALS];
 	slock_t		pss_mutex;		/* protects the above fields */
 
@@ -162,7 +162,7 @@ ProcSignalShmemInit(void)
  *		Register the current process in the ProcSignal array
  */
 void
-ProcSignalInit(char *cancel_key, int cancel_key_len)
+ProcSignalInit(const uint8 *cancel_key, int cancel_key_len)
 {
 	ProcSignalSlot *slot;
 	uint64		barrier_generation;
@@ -728,7 +728,7 @@ procsignal_sigusr1_handler(SIGNAL_ARGS)
  * fields in the ProcSignal slots.
  */
 void
-SendCancelRequest(int backendPID, char *cancel_key, int cancel_key_len)
+SendCancelRequest(int backendPID, const uint8 *cancel_key, int cancel_key_len)
 {
 	Assert(backendPID != 0);
 
diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c
index 1847e7c85d3..92b0446b80c 100644
--- a/src/backend/utils/init/globals.c
+++ b/src/backend/utils/init/globals.c
@@ -50,7 +50,7 @@ pg_time_t	MyStartTime;
 TimestampTz MyStartTimestamp;
 struct ClientSocket *MyClientSocket;
 struct Port *MyProcPort;
-char		MyCancelKey[MAX_CANCEL_KEY_LENGTH];
+uint8		MyCancelKey[MAX_CANCEL_KEY_LENGTH];
 int			MyCancelKeyLength = 0;
 int			MyPMChildSlot;
 
diff --git a/src/common/base64.c b/src/common/base64.c
index 6028f413472..05e9d18824f 100644
--- a/src/common/base64.c
+++ b/src/common/base64.c
@@ -46,11 +46,11 @@ static const int8 b64lookup[128] = {
  * for safety.
  */
 int
-pg_b64_encode(const char *src, int len, char *dst, int dstlen)
+pg_b64_encode(const void *src, int len, char *dst, int dstlen)
 {
 	char	   *p;
 	const char *s,
-			   *end = src + len;
+			   *end = (const char *) src + len;
 	int			pos = 2;
 	uint32		buf = 0;
 
@@ -113,7 +113,7 @@ error:
  * buffer zeroed for safety.
  */
 int
-pg_b64_decode(const char *src, int len, char *dst, int dstlen)
+pg_b64_decode(const char *src, int len, void *dst, int dstlen)
 {
 	const char *srcend = src + len,
 			   *s = src;
@@ -172,21 +172,21 @@ pg_b64_decode(const char *src, int len, char *dst, int dstlen)
 			 * Leave if there is an overflow in the area allocated for the
 			 * decoded string.
 			 */
-			if ((p - dst + 1) > dstlen)
+			if ((p - (char *) dst + 1) > dstlen)
 				goto error;
 			*p++ = (buf >> 16) & 255;
 
 			if (end == 0 || end > 1)
 			{
 				/* overflow check */
-				if ((p - dst + 1) > dstlen)
+				if ((p - (char *) dst + 1) > dstlen)
 					goto error;
 				*p++ = (buf >> 8) & 255;
 			}
 			if (end == 0 || end > 2)
 			{
 				/* overflow check */
-				if ((p - dst + 1) > dstlen)
+				if ((p - (char *) dst + 1) > dstlen)
 					goto error;
 				*p++ = buf & 255;
 			}
@@ -204,8 +204,8 @@ pg_b64_decode(const char *src, int len, char *dst, int dstlen)
 		goto error;
 	}
 
-	Assert((p - dst) <= dstlen);
-	return p - dst;
+	Assert((p - (char *) dst) <= dstlen);
+	return p - (char *) dst;
 
 error:
 	memset(dst, 0, dstlen);
diff --git a/src/include/common/base64.h b/src/include/common/base64.h
index 3f74aa301f0..0a9fb047169 100644
--- a/src/include/common/base64.h
+++ b/src/include/common/base64.h
@@ -11,8 +11,8 @@
 #define BASE64_H
 
 /* base 64 */
-pg_nodiscard extern int pg_b64_encode(const char *src, int len, char *dst, int dstlen);
-pg_nodiscard extern int pg_b64_decode(const char *src, int len, char *dst, int dstlen);
+pg_nodiscard extern int pg_b64_encode(const void *src, int len, char *dst, int dstlen);
+pg_nodiscard extern int pg_b64_decode(const char *src, int len, void *dst, int dstlen);
 extern int	pg_b64_enc_len(int srclen);
 extern int	pg_b64_dec_len(int srclen);
 
diff --git a/src/include/libpq/pqcomm.h b/src/include/libpq/pqcomm.h
index d11069cf8dc..f04ca135653 100644
--- a/src/include/libpq/pqcomm.h
+++ b/src/include/libpq/pqcomm.h
@@ -141,7 +141,7 @@ typedef struct CancelRequestPacket
 	/* Note that each field is stored in network byte order! */
 	MsgType		cancelRequestCode;	/* code to identify a cancel request */
 	uint32		backendPID;		/* PID of client's backend */
-	char		cancelAuthCode[FLEXIBLE_ARRAY_MEMBER];	/* secret key to
+	uint8		cancelAuthCode[FLEXIBLE_ARRAY_MEMBER];	/* secret key to
 														 * authorize cancel */
 } CancelRequestPacket;
 
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index 72f5655fb34..258e5624ad0 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -192,7 +192,7 @@ extern PGDLLIMPORT pg_time_t MyStartTime;
 extern PGDLLIMPORT TimestampTz MyStartTimestamp;
 extern PGDLLIMPORT struct Port *MyProcPort;
 extern PGDLLIMPORT struct Latch *MyLatch;
-extern PGDLLIMPORT char MyCancelKey[];
+extern PGDLLIMPORT uint8 MyCancelKey[];
 extern PGDLLIMPORT int MyCancelKeyLength;
 extern PGDLLIMPORT int MyPMChildSlot;
 
diff --git a/src/include/storage/procsignal.h b/src/include/storage/procsignal.h
index cfe14631445..345d5a0ecb1 100644
--- a/src/include/storage/procsignal.h
+++ b/src/include/storage/procsignal.h
@@ -73,10 +73,10 @@ typedef enum
 extern Size ProcSignalShmemSize(void);
 extern void ProcSignalShmemInit(void);
 
-extern void ProcSignalInit(char *cancel_key, int cancel_key_len);
+extern void ProcSignalInit(const uint8 *cancel_key, int cancel_key_len);
 extern int	SendProcSignal(pid_t pid, ProcSignalReason reason,
 						   ProcNumber procNumber);
-extern void SendCancelRequest(int backendPID, char *cancel_key, int cancel_key_len);
+extern void SendCancelRequest(int backendPID, const uint8 *cancel_key, int cancel_key_len);
 
 extern uint64 EmitProcSignalBarrier(ProcSignalBarrierType type);
 extern void WaitForProcSignalBarrier(uint64 generation);
diff --git a/src/interfaces/libpq/fe-cancel.c b/src/interfaces/libpq/fe-cancel.c
index e84e64bf2a7..d51a2d2f70a 100644
--- a/src/interfaces/libpq/fe-cancel.c
+++ b/src/interfaces/libpq/fe-cancel.c
@@ -463,7 +463,7 @@ PQsendCancelRequest(PGconn *cancelConn)
 	memset(&req, 0, offsetof(CancelRequestPacket, cancelAuthCode));
 	req.cancelRequestCode = (MsgType) pg_hton32(CANCEL_REQUEST_CODE);
 	req.backendPID = pg_hton32(cancelConn->be_pid);
-	if (pqPutnchar((char *) &req, offsetof(CancelRequestPacket, cancelAuthCode), cancelConn))
+	if (pqPutnchar(&req, offsetof(CancelRequestPacket, cancelAuthCode), cancelConn))
 		return STATUS_ERROR;
 	if (pqPutnchar(cancelConn->be_cancel_key, cancelConn->be_cancel_key_len, cancelConn))
 		return STATUS_ERROR;
diff --git a/src/interfaces/libpq/fe-misc.c b/src/interfaces/libpq/fe-misc.c
index d78445c70af..c74fe404bbe 100644
--- a/src/interfaces/libpq/fe-misc.c
+++ b/src/interfaces/libpq/fe-misc.c
@@ -67,7 +67,7 @@ PQlibVersion(void)

 /*
- * pqGetc: get 1 character from the connection
+ * pqGetc: read 1 character from the connection
  *
  *	All these routines return 0 on success, EOF on error.
  *	Note that for the Get routines, EOF only means there is not enough
@@ -100,7 +100,7 @@ pqPutc(char c, PGconn *conn)
 
 /*
  * pqGets[_append]:
- * get a null-terminated string from the connection,
+ * read a null-terminated string from the connection,
  * and store it in an expansible PQExpBuffer.
  * If we run out of memory, all of the string is still read,
  * but the excess characters are silently discarded.
@@ -159,10 +159,10 @@ pqPuts(const char *s, PGconn *conn)
 
 /*
  * pqGetnchar:
- *	get a string of exactly len bytes in buffer s, no null termination
+ *	read exactly len bytes in buffer s, no null termination
  */
 int
-pqGetnchar(char *s, size_t len, PGconn *conn)
+pqGetnchar(void *s, size_t len, PGconn *conn)
 {
 	if (len > (size_t) (conn->inEnd - conn->inCursor))
 		return EOF;
@@ -199,7 +199,7 @@ pqSkipnchar(size_t len, PGconn *conn)
  *	write exactly len bytes to the current message
  */
 int
-pqPutnchar(const char *s, size_t len, PGconn *conn)
+pqPutnchar(const void *s, size_t len, PGconn *conn)
 {
 	if (pqPutMsgBytes(s, len, conn))
 		return EOF;
diff --git a/src/interfaces/libpq/fe-protocol3.c b/src/interfaces/libpq/fe-protocol3.c
index d85910f41fc..289d1beca75 100644
--- a/src/interfaces/libpq/fe-protocol3.c
+++ b/src/interfaces/libpq/fe-protocol3.c
@@ -1532,7 +1532,7 @@ getParameterStatus(PGconn *conn)
 static int
 getBackendKeyData(PGconn *conn, int msgLength)
 {
-	uint8		cancel_key_len;
+	int			cancel_key_len;
 
 	if (conn->be_cancel_key)
 	{
@@ -2121,7 +2121,7 @@ pqFunctionCall3(PGconn *conn, Oid fnid,
 		}
 		else
 		{
-			if (pqPutnchar((char *) args[i].u.ptr, args[i].len, conn))
+			if (pqPutnchar(args[i].u.ptr, args[i].len, conn))
 				return NULL;
 		}
 	}
@@ -2215,7 +2215,7 @@ pqFunctionCall3(PGconn *conn, Oid fnid,
 					}
 					else
 					{
-						if (pqGetnchar((char *) result_buf,
+						if (pqGetnchar(result_buf,
 									   *actual_result_len,
 									   conn))
 							continue;
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index 9369c217fb5..a6cfd7f5c9d 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -539,16 +539,16 @@ struct pg_conn
 								 * tried host */
 	bool		send_appname;	/* okay to send application_name? */
 	size_t		scram_client_key_len;
-	void	   *scram_client_key_binary;	/* binary SCRAM client key */
+	uint8	   *scram_client_key_binary;	/* binary SCRAM client key */
 	size_t		scram_server_key_len;
-	void	   *scram_server_key_binary;	/* binary SCRAM server key */
+	uint8	   *scram_server_key_binary;	/* binary SCRAM server key */
 	ProtocolVersion min_pversion;	/* protocol version to request */
 	ProtocolVersion max_pversion;	/* protocol version to request */
 
 	/* Miscellaneous stuff */
 	int			be_pid;			/* PID of backend --- needed for cancels */
-	char	   *be_cancel_key;	/* query cancellation key and its length */
-	uint16		be_cancel_key_len;
+	int			be_cancel_key_len;
+	uint8	   *be_cancel_key;	/* query cancellation key */
 	pgParameterStatus *pstatus; /* ParameterStatus data */
 	int			client_encoding;	/* encoding id */
 	bool		std_strings;	/* standard_conforming_strings */
@@ -787,9 +787,9 @@ extern int	pqPutc(char c, PGconn *conn);
 extern int	pqGets(PQExpBuffer buf, PGconn *conn);
 extern int	pqGets_append(PQExpBuffer buf, PGconn *conn);
 extern int	pqPuts(const char *s, PGconn *conn);
-extern int	pqGetnchar(char *s, size_t len, PGconn *conn);
+extern int	pqGetnchar(void *s, size_t len, PGconn *conn);
 extern int	pqSkipnchar(size_t len, PGconn *conn);
-extern int	pqPutnchar(const char *s, size_t len, PGconn *conn);
+extern int	pqPutnchar(const void *s, size_t len, PGconn *conn);
 extern int	pqGetInt(int *result, size_t bytes, PGconn *conn);
 extern int	pqPutInt(int value, size_t bytes, PGconn *conn);
 extern int	pqPutMsgStart(char msg_type, PGconn *conn);
-- 
2.39.5

---

From: Heikki Linnakangas <[email-masked]>
Date: Wed, 9 Apr 2025 13:46:57 +0300

> arguably those should be changed to use "uint8 *" too.

I went around looking a bit more anyway. Here's a patch to change more 
places to use 'uint8' for byte arrays, in SCRAM and MD5 salts and 
digests and such. It's a bit of code churn, but I think it improves 
readability. Especially the SCRAM code sometimes deals with 
base64-encoded string representations of digests and sometimes with 
decoded byte arrays, and it's helpful to use different datatypes for them.

-- 
Heikki Linnakangas
Neon (https://neon.tech)
From 2caf331cd4402d10440ea7cfad8875c192277273 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <[email-masked]>
Date: Wed, 9 Apr 2025 13:32:24 +0300
Subject: [PATCH v2 1/2] Use 'void *' for arbitrary buffers, 'uint8 *' for byte
 arrays

Discussion: https://www.postgresql.org/message-id/[email-masked]
---
 src/backend/libpq/auth-scram.c       |  2 +-
 src/backend/storage/ipc/procsignal.c |  6 +++---
 src/backend/utils/init/globals.c     |  2 +-
 src/common/base64.c                  | 16 ++++++++--------
 src/common/scram-common.c            | 10 +++++-----
 src/include/common/base64.h          |  4 ++--
 src/include/common/scram-common.h    |  4 ++--
 src/include/libpq/pqcomm.h           |  2 +-
 src/include/miscadmin.h              |  2 +-
 src/include/storage/procsignal.h     |  4 ++--
 src/interfaces/libpq/fe-auth-scram.c | 12 ++++++------
 src/interfaces/libpq/fe-cancel.c     |  2 +-
 src/interfaces/libpq/fe-misc.c       | 10 +++++-----
 src/interfaces/libpq/fe-protocol3.c  |  6 +++---
 src/interfaces/libpq/libpq-int.h     | 12 ++++++------
 15 files changed, 47 insertions(+), 47 deletions(-)

diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c
index 26dd241efa9..f80333bb533 100644
--- a/src/backend/libpq/auth-scram.c
+++ b/src/backend/libpq/auth-scram.c
@@ -484,7 +484,7 @@ pg_be_scram_build_secret(const char *password)
 {
 	char	   *prep_password;
 	pg_saslprep_rc rc;
-	char		saltbuf[SCRAM_DEFAULT_SALT_LEN];
+	uint8		saltbuf[SCRAM_DEFAULT_SALT_LEN];
 	char	   *result;
 	const char *errstr = NULL;

 
 
diff --git a/src/common/base64.c b/src/common/base64.c
index 6028f413472..240461fe1e6 100644
--- a/src/common/base64.c
+++ b/src/common/base64.c
@@ -41,15 +41,15 @@ static const int8 b64lookup[128] = {
 /*
  * pg_b64_encode
  *
- * Encode into base64 the given string.  Returns the length of the encoded
- * string, and -1 in the event of an error with the result buffer zeroed
- * for safety.
+ * Encode the 'src' byte array into base64.  Returns the length of the encoded
+ * string, and -1 in the event of an error with the result buffer zeroed for
+ * safety.
  */
 int
-pg_b64_encode(const char *src, int len, char *dst, int dstlen)
+pg_b64_encode(const uint8 *src, int len, char *dst, int dstlen)
 {
 	char	   *p;
-	const char *s,
+	const uint8 *s,
 			   *end = src + len;
 	int			pos = 2;
 	uint32		buf = 0;
@@ -59,7 +59,7 @@ pg_b64_encode(const char *src, int len, char *dst, int dstlen)
 
 	while (s < end)
 	{
-		buf |= (unsigned char) *s << (pos << 3);
+		buf |= *s << (pos << 3);
 		pos--;
 		s++;
 
@@ -113,11 +113,11 @@ error:
  * buffer zeroed for safety.
  */
 int
-pg_b64_decode(const char *src, int len, char *dst, int dstlen)
+pg_b64_decode(const char *src, int len, uint8 *dst, int dstlen)
 {
 	const char *srcend = src + len,
 			   *s = src;
-	char	   *p = dst;
+	uint8	   *p = dst;
 	char		c;
 	int			b = 0;
 	uint32		buf = 0;
diff --git a/src/common/scram-common.c b/src/common/scram-common.c
index bfd2a340016..e47a6ebbaab 100644
--- a/src/common/scram-common.c
+++ b/src/common/scram-common.c
@@ -37,7 +37,7 @@
 int
 scram_SaltedPassword(const char *password,
 					 pg_cryptohash_type hash_type, int key_length,
-					 const char *salt, int saltlen, int iterations,
+					 const uint8 *salt, int saltlen, int iterations,
 					 uint8 *result, const char **errstr)
 {
 	int			password_len = strlen(password);
@@ -62,7 +62,7 @@ scram_SaltedPassword(const char *password,
 
 	/* First iteration */
 	if (pg_hmac_init(hmac_ctx, (uint8 *) password, password_len) < 0 ||
-		pg_hmac_update(hmac_ctx, (uint8 *) salt, saltlen) < 0 ||
+		pg_hmac_update(hmac_ctx, salt, saltlen) < 0 ||
 		pg_hmac_update(hmac_ctx, (uint8 *) &one, sizeof(uint32)) < 0 ||
 		pg_hmac_final(hmac_ctx, Ui_prev, key_length) < 0)
 	{
@@ -207,7 +207,7 @@ scram_ServerKey(const uint8 *salted_password,
  */
 char *
 scram_build_secret(pg_cryptohash_type hash_type, int key_length,
-				   const char *salt, int saltlen, int iterations,
+				   const uint8 *salt, int saltlen, int iterations,
 				   const char *password, const char **errstr)
 {
 	uint8		salted_password[SCRAM_MAX_KEY_LEN];
@@ -290,7 +290,7 @@ scram_build_secret(pg_cryptohash_type hash_type, int key_length,
 	*(p++) = '$';
 
 	/* stored key */
-	encoded_result = pg_b64_encode((char *) stored_key, key_length, p,
+	encoded_result = pg_b64_encode(stored_key, key_length, p,
 								   encoded_stored_len);
 	if (encoded_result < 0)
 	{
@@ -307,7 +307,7 @@ scram_build_secret(pg_cryptohash_type hash_type, int key_length,
 	*(p++) = ':';
 
 	/* server key */
-	encoded_result = pg_b64_encode((char *) server_key, key_length, p,
+	encoded_result = pg_b64_encode(server_key, key_length, p,
 								   encoded_server_len);
 	if (encoded_result < 0)
 	{
diff --git a/src/include/common/base64.h b/src/include/common/base64.h
index 3f74aa301f0..66cb57b017f 100644
--- a/src/include/common/base64.h
+++ b/src/include/common/base64.h
@@ -11,8 +11,8 @@
 #define BASE64_H
 
 /* base 64 */
-pg_nodiscard extern int pg_b64_encode(const char *src, int len, char *dst, int dstlen);
-pg_nodiscard extern int pg_b64_decode(const char *src, int len, char *dst, int dstlen);
+pg_nodiscard extern int pg_b64_encode(const uint8 *src, int len, char *dst, int dstlen);
+pg_nodiscard extern int pg_b64_decode(const char *src, int len, uint8 *dst, int dstlen);
 extern int	pg_b64_enc_len(int srclen);
 extern int	pg_b64_dec_len(int srclen);
 
diff --git a/src/include/common/scram-common.h b/src/include/common/scram-common.h
index 46df94ef1d4..5ce055e0e27 100644
--- a/src/include/common/scram-common.h
+++ b/src/include/common/scram-common.h
@@ -51,7 +51,7 @@
 
 extern int	scram_SaltedPassword(const char *password,
 								 pg_cryptohash_type hash_type, int key_length,
-								 const char *salt, int saltlen, int iterations,
+								 const uint8 *salt, int saltlen, int iterations,
 								 uint8 *result, const char **errstr);
 extern int	scram_H(const uint8 *input, pg_cryptohash_type hash_type,
 					int key_length, uint8 *result,
@@ -64,7 +64,7 @@ extern int	scram_ServerKey(const uint8 *salted_password,
 							uint8 *result, const char **errstr);
 
 extern char *scram_build_secret(pg_cryptohash_type hash_type, int key_length,
-								const char *salt, int saltlen, int iterations,
+								const uint8 *salt, int saltlen, int iterations,
 								const char *password, const char **errstr);
 
 #endif							/* SCRAM_COMMON_H */

 
 
 extern uint64 EmitProcSignalBarrier(ProcSignalBarrierType type);
 extern void WaitForProcSignalBarrier(uint64 generation);
diff --git a/src/interfaces/libpq/fe-auth-scram.c b/src/interfaces/libpq/fe-auth-scram.c
index fe18615197f..3babbc8d522 100644
--- a/src/interfaces/libpq/fe-auth-scram.c
+++ b/src/interfaces/libpq/fe-auth-scram.c
@@ -70,7 +70,7 @@ typedef struct
 
 	/* These come from the server-first message */
 	char	   *server_first_message;
-	char	   *salt;
+	uint8	   *salt;
 	int			saltlen;
 	int			iterations;
 	char	   *nonce;
@@ -350,7 +350,7 @@ static char *
 build_client_first_message(fe_scram_state *state)
 {
 	PGconn	   *conn = state->conn;
-	char		raw_nonce[SCRAM_RAW_NONCE_LEN + 1];
+	uint8		raw_nonce[SCRAM_RAW_NONCE_LEN + 1];
 	char	   *result;
 	int			channel_info_len;
 	int			encoded_len;
@@ -477,7 +477,7 @@ build_client_final_message(fe_scram_state *state)
 		char	   *cbind_data = NULL;
 		size_t		cbind_data_len = 0;
 		size_t		cbind_header_len;
-		char	   *cbind_input;
+		uint8	   *cbind_input;
 		size_t		cbind_input_len;
 		int			encoded_cbind_len;
 
@@ -574,7 +574,7 @@ build_client_final_message(fe_scram_state *state)
 	encoded_len = pg_b64_enc_len(state->key_length);
 	if (!enlargePQExpBuffer(&buf, encoded_len))
 		goto oom_error;
-	encoded_len = pg_b64_encode((char *) client_proof,
+	encoded_len = pg_b64_encode(client_proof,
 								state->key_length,
 								buf.data + buf.len,
 								encoded_len);
@@ -694,7 +694,7 @@ read_server_final_message(fe_scram_state *state, char *input)
 {
 	PGconn	   *conn = state->conn;
 	char	   *encoded_server_signature;
-	char	   *decoded_server_signature;
+	uint8	   *decoded_server_signature;
 	int			server_signature_len;
 
 	state->server_final_message = strdup(input);
@@ -916,7 +916,7 @@ pg_fe_scram_build_secret(const char *password, int iterations, const char **errs
 {
 	char	   *prep_password;
 	pg_saslprep_rc rc;
-	char		saltbuf[SCRAM_DEFAULT_SALT_LEN];
+	uint8		saltbuf[SCRAM_DEFAULT_SALT_LEN];
 	char	   *result;
 
 	/*

 /*
 
 /*
  * pqGets[_append]:
 
 /*
  * pqGetnchar:
 
 	if (conn->be_cancel_key)
 	{

From e518b93e48fe9a14f5328e423e10e44b8c92b699 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <[email-masked]>
Date: Wed, 9 Apr 2025 13:45:10 +0300
Subject: [PATCH v2 2/2] WIP: use 'uint8 *' in more places for byte arrays

---
 src/backend/libpq/auth-scram.c       | 26 +++++++++++++-------------
 src/backend/libpq/auth.c             |  4 ++--
 src/backend/libpq/crypt.c            |  6 +++---
 src/common/md5_common.c              |  4 ++--
 src/include/common/md5.h             |  4 ++--
 src/include/libpq/auth.h             |  2 +-
 src/include/libpq/crypt.h            |  2 +-
 src/interfaces/libpq/fe-auth-scram.c |  2 +-
 src/interfaces/libpq/fe-auth.c       |  8 ++++----
 9 files changed, 29 insertions(+), 29 deletions(-)

diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c
index f80333bb533..6ba8212326d 100644
--- a/src/backend/libpq/auth-scram.c
+++ b/src/backend/libpq/auth-scram.c
@@ -158,7 +158,7 @@ typedef struct
 	/* Fields from the last message from client */
 	char	   *client_final_message_without_proof;
 	char	   *client_final_nonce;
-	char		ClientProof[SCRAM_MAX_KEY_LEN];
+	uint8		ClientProof[SCRAM_MAX_KEY_LEN];
 
 	/* Fields generated in the server */
 	char	   *server_first_message;
@@ -186,7 +186,7 @@ static void mock_scram_secret(const char *username, pg_cryptohash_type *hash_typ
 static bool is_scram_printable(char *p);
 static char *sanitize_char(char c);
 static char *sanitize_str(const char *s);
-static char *scram_mock_salt(const char *username,
+static uint8 *scram_mock_salt(const char *username,
 							 pg_cryptohash_type hash_type,
 							 int key_length);
 
@@ -524,7 +524,7 @@ scram_verify_plain_password(const char *username, const char *password,
 							const char *secret)
 {
 	char	   *encoded_salt;
-	char	   *salt;
+	uint8	   *salt;
 	int			saltlen;
 	int			iterations;
 	int			key_length = 0;
@@ -609,9 +609,9 @@ parse_scram_secret(const char *secret, int *iterations,
 	char	   *storedkey_str;
 	char	   *serverkey_str;
 	int			decoded_len;
-	char	   *decoded_salt_buf;
-	char	   *decoded_stored_buf;
-	char	   *decoded_server_buf;
+	uint8	   *decoded_salt_buf;
+	uint8	   *decoded_stored_buf;
+	uint8	   *decoded_server_buf;
 
 	/*
 	 * The secret is of form:
@@ -698,7 +698,7 @@ mock_scram_secret(const char *username, pg_cryptohash_type *hash_type,
 				  int *iterations, int *key_length, char **salt,
 				  uint8 *stored_key, uint8 *server_key)
 {
-	char	   *raw_salt;
+	uint8	   *raw_salt;
 	char	   *encoded_salt;
 	int			encoded_len;
 
@@ -1231,7 +1231,7 @@ build_server_first_message(scram_state *state)
 	 * For convenience, however, we don't use the whole range available,
 	 * rather, we generate some random bytes, and base64 encode them.
 	 */
-	char		raw_nonce[SCRAM_RAW_NONCE_LEN];
+	uint8		raw_nonce[SCRAM_RAW_NONCE_LEN];
 	int			encoded_len;
 
 	if (!pg_strong_random(raw_nonce, SCRAM_RAW_NONCE_LEN))
@@ -1271,7 +1271,7 @@ read_client_final_message(scram_state *state, const char *input)
 	char	   *begin,
 			   *proof;
 	char	   *p;
-	char	   *client_proof;
+	uint8	   *client_proof;
 	int			client_proof_len;
 
 	begin = p = pstrdup(input);
@@ -1340,7 +1340,7 @@ read_client_final_message(scram_state *state, const char *input)
 		b64_message_len = pg_b64_enc_len(cbind_input_len);
 		/* don't forget the zero-terminator */
 		b64_message = palloc(b64_message_len + 1);
-		b64_message_len = pg_b64_encode(cbind_input, cbind_input_len,
+		b64_message_len = pg_b64_encode((uint8 *) cbind_input, cbind_input_len,
 										b64_message, b64_message_len);
 		if (b64_message_len < 0)
 			elog(ERROR, "could not encode channel binding data");
@@ -1440,7 +1440,7 @@ build_server_final_message(scram_state *state)
 	siglen = pg_b64_enc_len(state->key_length);
 	/* don't forget the zero-terminator */
 	server_signature_base64 = palloc(siglen + 1);
-	siglen = pg_b64_encode((const char *) ServerSignature,
+	siglen = pg_b64_encode(ServerSignature,
 						   state->key_length, server_signature_base64,
 						   siglen);
 	if (siglen < 0)
@@ -1467,7 +1467,7 @@ build_server_final_message(scram_state *state)
  * hash based on the username and a cluster-level secret key.  Returns a
  * pointer to a static buffer of size SCRAM_DEFAULT_SALT_LEN, or NULL.
  */
-static char *
+static uint8 *
 scram_mock_salt(const char *username, pg_cryptohash_type hash_type,
 				int key_length)
 {
@@ -1501,5 +1501,5 @@ scram_mock_salt(const char *username, pg_cryptohash_type hash_type,
 	}
 	pg_cryptohash_free(ctx);
 
-	return (char *) sha_digest;
+	return sha_digest;
 }
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index e18683c47e7..9f4d05ffbd4 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -666,7 +666,7 @@ ClientAuthentication(Port *port)
  * Send an authentication request packet to the frontend.
  */
 void
-sendAuthRequest(Port *port, AuthRequest areq, const char *extradata, int extralen)
+sendAuthRequest(Port *port, AuthRequest areq, const void *extradata, int extralen)
 {
 	StringInfoData buf;
 
@@ -874,7 +874,7 @@ CheckPWChallengeAuth(Port *port, const char **logdetail)
 static int
 CheckMD5Auth(Port *port, char *shadow_pass, const char **logdetail)
 {
-	char		md5Salt[4];		/* Password salt */
+	uint8		md5Salt[4];		/* Password salt */
 	char	   *passwd;
 	int			result;
 
diff --git a/src/backend/libpq/crypt.c b/src/backend/libpq/crypt.c
index cbb85a27cc1..f6b641e726e 100644
--- a/src/backend/libpq/crypt.c
+++ b/src/backend/libpq/crypt.c
@@ -136,7 +136,7 @@ encrypt_password(PasswordType target_type, const char *role,
 			case PASSWORD_TYPE_MD5:
 				encrypted_password = palloc(MD5_PASSWD_LEN + 1);
 
-				if (!pg_md5_encrypt(password, role, strlen(role),
+				if (!pg_md5_encrypt(password, (uint8 *) role, strlen(role),
 									encrypted_password, &errstr))
 					elog(ERROR, "password encryption failed: %s", errstr);
 				break;
@@ -201,7 +201,7 @@ encrypt_password(PasswordType target_type, const char *role,
 int
 md5_crypt_verify(const char *role, const char *shadow_pass,
 				 const char *client_pass,
-				 const char *md5_salt, int md5_salt_len,
+				 const uint8 *md5_salt, int md5_salt_len,
 				 const char **logdetail)
 {
 	int			retval;
@@ -284,7 +284,7 @@ plain_crypt_verify(const char *role, const char *shadow_pass,
 
 		case PASSWORD_TYPE_MD5:
 			if (!pg_md5_encrypt(client_pass,
-								role,
+								(uint8 *) role,
 								strlen(role),
 								crypt_client_pass,
 								&errstr))
diff --git a/src/common/md5_common.c b/src/common/md5_common.c
index 61e396b0bbf..057ae7a449f 100644
--- a/src/common/md5_common.c
+++ b/src/common/md5_common.c
@@ -105,7 +105,7 @@ pg_md5_hash(const void *buff, size_t len, char *hexsum, const char **errstr)
  * (of size MD5_DIGEST_LENGTH) rather than being converted to ASCII hex.
  */
 bool
-pg_md5_binary(const void *buff, size_t len, void *outbuf, const char **errstr)
+pg_md5_binary(const void *buff, size_t len, uint8 *outbuf, const char **errstr)
 {
 	pg_cryptohash_ctx *ctx;
 
@@ -142,7 +142,7 @@ pg_md5_binary(const void *buff, size_t len, void *outbuf, const char **errstr)
  * error context.
  */
 bool
-pg_md5_encrypt(const char *passwd, const char *salt, size_t salt_len,
+pg_md5_encrypt(const char *passwd, const uint8 *salt, size_t salt_len,
 			   char *buf, const char **errstr)
 {
 	size_t		passwd_len = strlen(passwd);
diff --git a/src/include/common/md5.h b/src/include/common/md5.h
index 18ffd998453..0c9ae4888f2 100644
--- a/src/include/common/md5.h
+++ b/src/include/common/md5.h
@@ -28,9 +28,9 @@
 /* Utilities common to all the MD5 implementations, as of md5_common.c */
 extern bool pg_md5_hash(const void *buff, size_t len, char *hexsum,
 						const char **errstr);
-extern bool pg_md5_binary(const void *buff, size_t len, void *outbuf,
+extern bool pg_md5_binary(const void *buff, size_t len, uint8 *outbuf,
 						  const char **errstr);
-extern bool pg_md5_encrypt(const char *passwd, const char *salt,
+extern bool pg_md5_encrypt(const char *passwd, const uint8 *salt,
 						   size_t salt_len, char *buf,
 						   const char **errstr);
 
diff --git a/src/include/libpq/auth.h b/src/include/libpq/auth.h
index 25b5742068f..cc9643cce2f 100644
--- a/src/include/libpq/auth.h
+++ b/src/include/libpq/auth.h
@@ -37,7 +37,7 @@ extern PGDLLIMPORT bool pg_krb_caseins_users;
 extern PGDLLIMPORT bool pg_gss_accept_delegation;
 
 extern void ClientAuthentication(Port *port);
-extern void sendAuthRequest(Port *port, AuthRequest areq, const char *extradata,
+extern void sendAuthRequest(Port *port, AuthRequest areq, const void *extradata,
 							int extralen);
 extern void set_authn_id(Port *port, const char *id);
 
diff --git a/src/include/libpq/crypt.h b/src/include/libpq/crypt.h
index dee477428e4..a1b4b363143 100644
--- a/src/include/libpq/crypt.h
+++ b/src/include/libpq/crypt.h
@@ -51,7 +51,7 @@ extern char *encrypt_password(PasswordType target_type, const char *role,
 extern char *get_role_password(const char *role, const char **logdetail);
 
 extern int	md5_crypt_verify(const char *role, const char *shadow_pass,
-							 const char *client_pass, const char *md5_salt,
+							 const char *client_pass, const uint8 *md5_salt,
 							 int md5_salt_len, const char **logdetail);
 extern int	plain_crypt_verify(const char *role, const char *shadow_pass,
 							   const char *client_pass,
diff --git a/src/interfaces/libpq/fe-auth-scram.c b/src/interfaces/libpq/fe-auth-scram.c
index 3babbc8d522..807ee1f5d0d 100644
--- a/src/interfaces/libpq/fe-auth-scram.c
+++ b/src/interfaces/libpq/fe-auth-scram.c
@@ -77,7 +77,7 @@ typedef struct
 
 	/* These come from the server-final message */
 	char	   *server_final_message;
-	char		ServerSignature[SCRAM_MAX_KEY_LEN];
+	uint8		ServerSignature[SCRAM_MAX_KEY_LEN];
 } fe_scram_state;
 
 static bool read_server_first_message(fe_scram_state *state, char *input);
diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
index ec7a9236044..84a042269de 100644
--- a/src/interfaces/libpq/fe-auth.c
+++ b/src/interfaces/libpq/fe-auth.c
@@ -798,7 +798,7 @@ pg_password_sendauth(PGconn *conn, const char *password, AuthRequest areq)
 	int			ret;
 	char	   *crypt_pwd = NULL;
 	const char *pwd_to_send;
-	char		md5Salt[4];
+	uint8		md5Salt[4];
 
 	/* Read the salt from the AuthenticationMD5Password message. */
 	if (areq == AUTH_REQ_MD5)
@@ -829,7 +829,7 @@ pg_password_sendauth(PGconn *conn, const char *password, AuthRequest areq)
 				}
 
 				crypt_pwd2 = crypt_pwd + MD5_PASSWD_LEN + 1;
-				if (!pg_md5_encrypt(password, conn->pguser,
+				if (!pg_md5_encrypt(password, (uint8 *) conn->pguser,
 									strlen(conn->pguser), crypt_pwd2,
 									&errstr))
 				{
@@ -1369,7 +1369,7 @@ PQencryptPassword(const char *passwd, const char *user)
 	if (!crypt_pwd)
 		return NULL;
 
-	if (!pg_md5_encrypt(passwd, user, strlen(user), crypt_pwd, &errstr))
+	if (!pg_md5_encrypt(passwd, (uint8 *) user, strlen(user), crypt_pwd, &errstr))
 	{
 		free(crypt_pwd);
 		return NULL;
@@ -1482,7 +1482,7 @@ PQencryptPasswordConn(PGconn *conn, const char *passwd, const char *user,
 		{
 			const char *errstr = NULL;
 
-			if (!pg_md5_encrypt(passwd, user, strlen(user), crypt_pwd, &errstr))
+			if (!pg_md5_encrypt(passwd, (uint8 *) user, strlen(user), crypt_pwd, &errstr))
 			{
 				libpq_append_conn_error(conn, "could not encrypt password: %s", errstr);
 				free(crypt_pwd);
-- 
2.39.5

---

From: Heikki Linnakangas <[email-masked]>
Date: Thu, 8 May 2025 22:11:07 +0300

On 09/04/2025 13:46, Heikki Linnakangas wrote:
>>>> On second thoughts, -1 on that. 'void *' is appropriate for 
>>>> functions like libc's read() or pq_sendbytes(), where the buffer can 
>>>> point to anything. In other words, the caller is expected to have a 
>>>> pointer like 'foobar *', and it gets cast to 'void *' when you call 
>>>> the function. That's not the case with the cancellation key. The 
>>>> cancellation key is just an array of bytes, the caller is expected 
>>>> to pass an array of bytes, not a struct.
>>>>
>>>> I'll switch to "const uint *" everywhere that deals with cancel 
>>>> keys. There are a few more variables elsewhere in the backend and in 
>>>> libpq.
>>>
>> Here's a patch to change cancellation keys to "uint8 *". I did the 
>> same for a few other places, namely the new scram_client_key_binary 
>> and scram_server_key_binary fields in pg_conn, and a few libpq 
>> functions that started to give compiler warnings after that. There 
>> probably would be more code that could be changed to follow this 
>> convention, but I didn't look hard. What do you think?
>>
> readability. Especially the SCRAM code sometimes deals with base64- 
> encoded string representations of digests and sometimes with decoded 
> byte arrays, and it's helpful to use different datatypes for them.

Polished this up a tiny bit, and committed.

-- 
Heikki Linnakangas
Neon (https://neon.tech)

---

From: Jacob Champion <[email-masked]>
Date: Thu, 8 May 2025 15:28:24 -0700

> Polished this up a tiny bit, and committed.

Thanks! I think the uint8->int change for cancel_key_len is more than
just cosmetic; it most likely fixes a bug where a key size of 256
wrapped around to 0. I'll double-check that this fixes that later;
I've gotten side-tracked from the protocol stuff a bit.

While I have you, though, is the following just a really complicated
way to say `msgLength - 4`, or is there some other reason to do the
pointer math?

    cancel_key_len = 5 + msgLength - (conn->inCursor - conn->inStart);

--Jacob

---

From: Heikki Linnakangas <[email-masked]>
Date: Fri, 9 May 2025 09:37:58 +0300

> I've gotten side-tracked from the protocol stuff a bit.

True, although I'm pretty sure you'd fail the later cross-check that the 
whole message was consumed. ("message contents do not agree with length 
in message type"). But it's fixed now in any case.

>      cancel_key_len = 5 + msgLength - (conn->inCursor - conn->inStart);

Yes, it amounts to 'msgLength - 4'. I agree it looks pretty obscure. The 
way to read it is:

/* full length of the message, including the type code byte and the 
length field itself */
fullMsgLength = 5 + msgLength;

/* number of bytes consumed from the message so far */
lengthConsumed = (conn->inCursor - conn->inStart);

/* the cancel key consumes all the remaining bytes of the message */
cancel_key_len = fullMsgLength - lengthConsumed;

It didn't occur to me that you could write it simply as 'msgLength - 4'. 
That depends on knowing that the preceding fields are exactly 4 bytes 
long, but that's clear enough if we just add a comment on that, see 
attached.

-- 
Heikki Linnakangas
Neon (https://neon.tech)
diff --git a/src/interfaces/libpq/fe-protocol3.c b/src/interfaces/libpq/fe-protocol3.c
index beb1c889aad..551276e9011 100644
--- a/src/interfaces/libpq/fe-protocol3.c
+++ b/src/interfaces/libpq/fe-protocol3.c
@@ -1544,7 +1544,11 @@ getBackendKeyData(PGconn *conn, int msgLength)
 	if (pqGetInt(&(conn->be_pid), 4, conn))
 		return EOF;
 
-	cancel_key_len = 5 + msgLength - (conn->inCursor - conn->inStart);
+	/*
+	 * The pid consumed 4 bytes, and the cancellation key consumes the rest of
+	 * the message.
+	 */
+	cancel_key_len = msgLength - 4;
 
 	conn->be_cancel_key = malloc(cancel_key_len);
 	if (conn->be_cancel_key == NULL)

---

From: Jacob Champion <[email-masked]>
Date: Fri, 23 May 2025 14:59:44 -0700

> long, but that's clear enough if we just add a comment on that, see

Sorry for the conference delay; this looks fine to me.

One of the side effects of the uint8 change is that the client now
accepts cancel keys up to roughly 30kb. Is that intended?

--Jacob