pgsql: Make cancel request keys longer

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.

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)

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)

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)

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