Thread: Fix calloc check if oom (PQcancelCreate)

> On 27 May 2024, at 14:25, Ranier Vilela <ranier.vf@gmail.com> wrote:

> I think that commit 61461a3, left some oversight.
> The function *PQcancelCreate* fails in check,
> return of *calloc* function.
>
> Trivial fix is attached.

Agreed, this looks like a copy/paste from the calloc calls a few lines up.

> But, IMO, I think that has more problems.
> If any allocation fails, all allocations must be cleared.
> Or is the current behavior acceptable?

Since this is frontend library code I think we should free all the allocations
in case of OOM.

--
Daniel Gustafsson

On Mon, 27 May 2024 at 16:06, Ranier Vilela <ranier.vf@gmail.com> wrote:
> Em seg., 27 de mai. de 2024 às 10:23, Daniel Gustafsson <daniel@yesql.se> escreveu:
>> > On 27 May 2024, at 14:25, Ranier Vilela <ranier.vf@gmail.com> wrote:
>> > I think that commit 61461a3, left some oversight.
>> > The function *PQcancelCreate* fails in check,
>> > return of *calloc* function.
>> >
>> > Trivial fix is attached.
>>
>> Agreed, this looks like a copy/paste from the calloc calls a few lines up.
>
> Yeah.

Agreed, this was indeed a copy paste mistake


>> > But, IMO, I think that has more problems.
>> > If any allocation fails, all allocations must be cleared.
>> > Or is the current behavior acceptable?
>>
>> Since this is frontend library code I think we should free all the allocations
>> in case of OOM.
>
> Agreed.
>
> With v1 patch, it is handled.

I much prefer the original trivial patch to the v1. Even in case of
OOM users are expected to call PQcancelFinish on a non-NULL result,
which in turn calls freePGConn. And that function will free any
partially initialized PGconn correctly. This is also how
pqConnectOptions2 works.

On Mon, 27 May 2024 at 18:16, Ranier Vilela <ranier.vf@gmail.com> wrote:
> Is it mandatory to call PQcancelFinish in case PQcancelCreate fails?


Yes, see the following line in the docs:

Note that when PQcancelCreate returns a non-null pointer, you must
call PQcancelFinish when you are finished with it, in order to dispose
of the structure and any associated memory blocks. This must be done
even if the cancel request failed or was abandoned.

Source: https://www.postgresql.org/docs/17/libpq-cancel.html#LIBPQ-PQCANCELCREATE

> IMO, I would expect problems with users.

This pattern is taken from regular connection creation and is really
common in libpq code so I don't expect issues. And even if there were
an issue, your v1 patch would not be nearly enough. Because you're
only freeing the connhost and addr field now in case of OOM. But there
are many more fields that would need to be freed.

>> And that function will free any
>> partially initialized PGconn correctly. This is also how
>> pqConnectOptions2 works.
>
> Well, I think that function *pqReleaseConnHost*, is incomplete.
> 1. Don't free connhost field;

ehm... it does free that afaict? It only doesn't set it to NULL. Which
indeed would be good to do, but not doing so doesn't cause any issues
with it's current 2 usages afaict.

> 2. Don't free addr field;

That's what release_conn_addrinfo is for.

> 3. Leave nconnhost and naddr, non-zero.

I agree that it would be good to do that pqReleaseConnHost and
release_conn_addrinfo. But also here, I don't see any issues caused by
not doing that currently.

So overall I agree pqReleaseConnHost and release_conn_addrinfo can be
improved for easier safe usage in the future, but I don't think those
improvements should be grouped into the same commit with an actual
bugfix.

> On 27 May 2024, at 18:47, Jelte Fennema-Nio <postgres@jeltef.nl> wrote:

> So overall I agree pqReleaseConnHost and release_conn_addrinfo can be
> improved for easier safe usage in the future, but I don't think those
> improvements should be grouped into the same commit with an actual
> bugfix.

I have pushed the fix for the calloc check for now.

--
Daniel Gustafsson