(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)
From 6d3a0ac89b76fe5f82aad5a3522e55fc165cd360 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@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-721800d89...@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
