Thread: Re: pgsql: Make cancel request keys longer
(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)
Attachment
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)
Attachment
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)