pgsql: Make cancel request keys longer
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 <postgres@jeltef.nl>
Reviewed-by: Robert Haas <robertmhaas@gmail.com> (earlier versions)
Discussion: /messages/by-id/508d0505-8b7a-4864-a681-e7e5edfe32aa@iki.fi
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(-)
On 02.04.25 15:43, Heikki Linnakangas wrote:
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.
Attachments:
0001-WIP-Fix-cancel-key-stuff.patchtext/plain; charset=UTF-8; name=0001-WIP-Fix-cancel-key-stuff.patchDownload
From 8abe6e66521de7fe5d2132fd5471265ef1bf6b0e Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
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
On 08/04/2025 20:06, Peter Eisentraut wrote:
On 02.04.25 15:43, Heikki Linnakangas wrote:
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.
Good catch. I wonder why the cirrus CI didn't complain, it has a step to
check for warnings with clang.
While I was looking at this, I suggest to make the first argument void
*. This is consistent for passing binary data.
Ok, sure.
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.
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)
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.
Looks good to me. I can commit these tomorrow, or feel free to do it
yourself too.
I'm on this now.
--
Heikki Linnakangas
Neon (https://neon.tech)
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.
(moving to pgsql-hackers)
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 committed the other parts of your original patch, thanks!
--
Heikki Linnakangas
Neon (https://neon.tech)
Attachments:
0001-Use-void-for-arbitrary-buffers-uint8-for-byte-arrays.patchtext/x-patch; charset=UTF-8; name=0001-Use-void-for-arbitrary-buffers-uint8-for-byte-arrays.patchDownload
From 6d3a0ac89b76fe5f82aad5a3522e55fc165cd360 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
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/61be9e31-7b7d-49d5-bc11-721800d89d64@eisentraut.org
---
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
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)
Attachments:
v2-0001-Use-void-for-arbitrary-buffers-uint8-for-byte-arr.patchtext/x-patch; charset=UTF-8; name=v2-0001-Use-void-for-arbitrary-buffers-uint8-for-byte-arr.patchDownload
From 2caf331cd4402d10440ea7cfad8875c192277273 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
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/61be9e31-7b7d-49d5-bc11-721800d89d64@eisentraut.org
---
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
v2-0002-WIP-use-uint8-in-more-places-for-byte-arrays.patchtext/x-patch; charset=UTF-8; name=v2-0002-WIP-use-uint8-in-more-places-for-byte-arrays.patchDownload
From e518b93e48fe9a14f5328e423e10e44b8c92b699 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
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
On 09/04/2025 13:46, Heikki Linnakangas wrote:
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.
Polished this up a tiny bit, and committed.
--
Heikki Linnakangas
Neon (https://neon.tech)
On Thu, May 8, 2025 at 12:11 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
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
On 09/05/2025 01:28, Jacob Champion wrote:
On Thu, May 8, 2025 at 12:11 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
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.
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.
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);
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)
Attachments:
simplify-cancel_key_len-math.patchtext/x-patch; charset=UTF-8; name=simplify-cancel_key_len-math.patchDownload
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)
On Thu, May 8, 2025 at 11:38 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
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.
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