Thread: BUG #17186: In connect.c, the lock connections_mutex is not correctly released(Line 463) at the return(Line 522)

The following bug has been logged on the website:

Bug reference:      17186
Logged by:          An improper locking bug due to the unreleased lock connections_mutex in
/ecpg/ecpglib/connect.c
Email address:      ryancaicse@gmail.com
PostgreSQL version: 14beta3
Operating system:   All
Description:

This is a possible improper locking bug, which can lead to resource leaks
and even deadlock. I am not sure whether it can lead to a security
problem.
The problem is that the lock connections_mutex should be released at the end
of the function. But it not released when conn_keywords == NULL ||
conn_values == NULL (ecpg_alloc got errors and return NULL).
Thank you for your checking.

Locations:

https://github.com/postgres/postgres/blob/c30f54ad732ca5c8762bb68bbe0f51de9137dd72/src/interfaces/ecpg/ecpglib/connect.c#L463

https://github.com/postgres/postgres/blob/c30f54ad732ca5c8762bb68bbe0f51de9137dd72/src/interfaces/ecpg/ecpglib/connect.c#L522

On Fri, Sep 10, 2021 at 04:29:36AM +0000, PG Bug reporting form wrote:
> The problem is that the lock connections_mutex should be released at the end
> of the function. But it not released when conn_keywords == NULL ||
> conn_values == NULL (ecpg_alloc got errors and return NULL).

ecpg_alloc() is just a wrapper to calloc(), so errors are very
unlikely going to happen in this code path.  It does not change the
fact that it is wrong.  Nice catch, will fix.
--
Michael

On Fri, Sep 10, 2021 at 04:07:26PM +0900, Michael Paquier wrote:
> ecpg_alloc() is just a wrapper to calloc(), so errors are very
> unlikely going to happen in this code path.  It does not change the
> fact that it is wrong.  Nice catch, will fix.

After looking at the issue in depth, I have noticed that just
unlocking the pthread mutex is incorrect because we would still free a
connection object while is is *tracked* by the list of all the
connections.

In normal cases, we should just call ecpg_finish() to correctly clean
up the new connection from the list.  But I don't think that we need
to do any of that at this stage, and instead we had better move the
allocation for the connection parameters before doing the list
manipulation, saving from any cleanup action needed.  That also means
moving up a bit the calculations we do for connect_params when looking
at all the parameters.  And that means losing a log but as the
connection failed on OOM, it is not like we need to care anyway.

If a client is under memory pressure, that could really mess up
applications and the list of connections, so this had better be
backpatched.
--
Michael

On Sat, Sep 11, 2021 at 02:38:47PM +0900, Michael Paquier wrote:
> If a client is under memory pressure, that could really mess up
> applications and the list of connections, so this had better be
> backpatched.

Done as of 3768c46, down to 9.6.  Thanks for the report, Ryan.
--
Michael