Subject: Re: pgsql: Make cancel request keys longer

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

On 09/04/2025 13:28, Heikki Linnakangas wrote:
> On 09/04/2025 12:39, Peter Eisentraut wrote:
>> On 09.04.25 10:53, Heikki Linnakangas wrote:
>>> On 08/04/2025 22:41, Heikki Linnakangas wrote:
>>>> On 08/04/2025 20:06, Peter Eisentraut wrote:
>>>>> While I was looking at this, I suggest to make the first argument 
>>>>> void *.  This is consistent for passing binary data.
>>>>
>>>> Ok, sure.
>>>
>>> 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.
>>
>> I was having the same second thoughts overnight.  I agree with your 
>> conclusion.
> 
> 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 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

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/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/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..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 */
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-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;
 
 	/*
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 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