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)