Re: Fix calloc check if oom (PQcancelCreate) - Mailing list pgsql-hackers

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.