Message-ID: <52d648b1-9aca-43d1-a9e4-a6a556410f41@iki.fi>
Date: Thu, 8 May 2025 22:11:07 +0300
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: pgsql: Make cancel request keys longer
From: Heikki Linnakangas <hlinnaka@iki.fi>
To: Peter Eisentraut <peter@eisentraut.org>,
 "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
References: <E1tzyNB-002IXa-0i@gemulon.postgresql.org>
 <61be9e31-7b7d-49d5-bc11-721800d89d64@eisentraut.org>
 <09323e6a-4743-4be2-9d7f-74b86e4dac64@iki.fi>
 <34d5d731-06cc-43af-88cd-b3a4c5c8d9df@iki.fi>
 <2afbd9c6-51b7-450f-9ae1-61e552368963@eisentraut.org>
 <4bd8421a-50ad-4169-a096-99247c2f563c@iki.fi>
 <9399dc5d-5575-483e-8e23-af6be79385c8@iki.fi>
Content-Language: en-US
In-Reply-To: <9399dc5d-5575-483e-8e23-af6be79385c8@iki.fi>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/52d648b1-9aca-43d1-a9e4-a6a556410f41%40iki.fi>
Precedence: bulk

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)