pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #17186: In connect.c, the lock connections_mutex is not correctly released(Line 463) at the return(Line 522)
4+ messages / 2 participants
[nested] [flat]
* BUG #17186: In connect.c, the lock connections_mutex is not correctly released(Line 463) at the return(Line 522)
@ 2021-09-10 04:29 PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 4+ messages in thread
From: PG Bug reporting form @ 2021-09-10 04:29 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: ryancaicse@gmail.com
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/ec...
https://github.com/postgres/postgres/blob/c30f54ad732ca5c8762bb68bbe0f51de9137dd72/src/interfaces/ec...
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: BUG #17186: In connect.c, the lock connections_mutex is not correctly released(Line 463) at the return(Line 522)
@ 2021-09-10 07:07 Michael Paquier <michael@paquier.xyz>
parent: PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 4+ messages in thread
From: Michael Paquier @ 2021-09-10 07:07 UTC (permalink / raw)
To: ryancaicse@gmail.com; pgsql-bugs@lists.postgresql.org
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
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: BUG #17186: In connect.c, the lock connections_mutex is not correctly released(Line 463) at the return(Line 522)
@ 2021-09-11 05:38 Michael Paquier <michael@paquier.xyz>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 4+ messages in thread
From: Michael Paquier @ 2021-09-11 05:38 UTC (permalink / raw)
To: ryancaicse@gmail.com; pgsql-bugs@lists.postgresql.org
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
diff --git a/src/interfaces/ecpg/ecpglib/connect.c b/src/interfaces/ecpg/ecpglib/connect.c
index 60564b176c..a2166b1e12 100644
--- a/src/interfaces/ecpg/ecpglib/connect.c
+++ b/src/interfaces/ecpg/ecpglib/connect.c
@@ -458,6 +458,47 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
else
realname = NULL;
+ /*
+ * Count options for the allocation done below (this may produce an
+ * overestimate, it's ok).
+ */
+ if (options)
+ for (i = 0; options[i]; i++)
+ if (options[i] == '=')
+ connect_params++;
+
+ if (user && strlen(user) > 0)
+ connect_params++;
+ if (passwd && strlen(passwd) > 0)
+ connect_params++;
+
+ /*
+ * Allocate enough space for all connection parameters. These allocations
+ * are done before manipulating the list of connections to ease the error
+ * handling on failure.
+ */
+ conn_keywords = (const char **) ecpg_alloc((connect_params + 1) * sizeof(char *), lineno);
+ conn_values = (const char **) ecpg_alloc(connect_params * sizeof(char *), lineno);
+ if (conn_keywords == NULL || conn_values == NULL)
+ {
+ if (host)
+ ecpg_free(host);
+ if (port)
+ ecpg_free(port);
+ if (options)
+ ecpg_free(options);
+ if (realname)
+ ecpg_free(realname);
+ if (dbname)
+ ecpg_free(dbname);
+ if (conn_keywords)
+ ecpg_free(conn_keywords);
+ if (conn_values)
+ ecpg_free(conn_values);
+ free(this);
+ return false;
+ }
+
/* add connection to our list */
#ifdef ENABLE_THREAD_SAFETY
pthread_mutex_lock(&connections_mutex);
@@ -488,40 +529,6 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
options ? "with options " : "", options ? options : "",
(user && strlen(user) > 0) ? "for user " : "", user ? user : "");
- /* count options (this may produce an overestimate, it's ok) */
- if (options)
- for (i = 0; options[i]; i++)
- if (options[i] == '=')
- connect_params++;
-
- if (user && strlen(user) > 0)
- connect_params++;
- if (passwd && strlen(passwd) > 0)
- connect_params++;
-
- /* allocate enough space for all connection parameters */
- conn_keywords = (const char **) ecpg_alloc((connect_params + 1) * sizeof(char *), lineno);
- conn_values = (const char **) ecpg_alloc(connect_params * sizeof(char *), lineno);
- if (conn_keywords == NULL || conn_values == NULL)
- {
- if (host)
- ecpg_free(host);
- if (port)
- ecpg_free(port);
- if (options)
- ecpg_free(options);
- if (realname)
- ecpg_free(realname);
- if (dbname)
- ecpg_free(dbname);
- if (conn_keywords)
- ecpg_free(conn_keywords);
- if (conn_values)
- ecpg_free(conn_values);
- free(this);
- return false;
- }
-
i = 0;
if (realname)
{
Attachments:
ecpg-connect-threads.patch (text/plain, 2.6K) download
diff --git a/src/interfaces/ecpg/ecpglib/connect.c b/src/interfaces/ecpg/ecpglib/connect.c
index 60564b176c..a2166b1e12 100644
--- a/src/interfaces/ecpg/ecpglib/connect.c
+++ b/src/interfaces/ecpg/ecpglib/connect.c
@@ -458,6 +458,47 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
else
realname = NULL;
+ /*
+ * Count options for the allocation done below (this may produce an
+ * overestimate, it's ok).
+ */
+ if (options)
+ for (i = 0; options[i]; i++)
+ if (options[i] == '=')
+ connect_params++;
+
+ if (user && strlen(user) > 0)
+ connect_params++;
+ if (passwd && strlen(passwd) > 0)
+ connect_params++;
+
+ /*
+ * Allocate enough space for all connection parameters. These allocations
+ * are done before manipulating the list of connections to ease the error
+ * handling on failure.
+ */
+ conn_keywords = (const char **) ecpg_alloc((connect_params + 1) * sizeof(char *), lineno);
+ conn_values = (const char **) ecpg_alloc(connect_params * sizeof(char *), lineno);
+ if (conn_keywords == NULL || conn_values == NULL)
+ {
+ if (host)
+ ecpg_free(host);
+ if (port)
+ ecpg_free(port);
+ if (options)
+ ecpg_free(options);
+ if (realname)
+ ecpg_free(realname);
+ if (dbname)
+ ecpg_free(dbname);
+ if (conn_keywords)
+ ecpg_free(conn_keywords);
+ if (conn_values)
+ ecpg_free(conn_values);
+ free(this);
+ return false;
+ }
+
/* add connection to our list */
#ifdef ENABLE_THREAD_SAFETY
pthread_mutex_lock(&connections_mutex);
@@ -488,40 +529,6 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
options ? "with options " : "", options ? options : "",
(user && strlen(user) > 0) ? "for user " : "", user ? user : "");
- /* count options (this may produce an overestimate, it's ok) */
- if (options)
- for (i = 0; options[i]; i++)
- if (options[i] == '=')
- connect_params++;
-
- if (user && strlen(user) > 0)
- connect_params++;
- if (passwd && strlen(passwd) > 0)
- connect_params++;
-
- /* allocate enough space for all connection parameters */
- conn_keywords = (const char **) ecpg_alloc((connect_params + 1) * sizeof(char *), lineno);
- conn_values = (const char **) ecpg_alloc(connect_params * sizeof(char *), lineno);
- if (conn_keywords == NULL || conn_values == NULL)
- {
- if (host)
- ecpg_free(host);
- if (port)
- ecpg_free(port);
- if (options)
- ecpg_free(options);
- if (realname)
- ecpg_free(realname);
- if (dbname)
- ecpg_free(dbname);
- if (conn_keywords)
- ecpg_free(conn_keywords);
- if (conn_values)
- ecpg_free(conn_values);
- free(this);
- return false;
- }
-
i = 0;
if (realname)
{
* Re: BUG #17186: In connect.c, the lock connections_mutex is not correctly released(Line 463) at the return(Line 522)
@ 2021-09-13 04:37 Michael Paquier <michael@paquier.xyz>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 0 replies; 4+ messages in thread
From: Michael Paquier @ 2021-09-13 04:37 UTC (permalink / raw)
To: ryancaicse@gmail.com; pgsql-bugs@lists.postgresql.org
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
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2021-09-13 04:37 UTC | newest]
Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-09-10 04:29 BUG #17186: In connect.c, the lock connections_mutex is not correctly released(Line 463) at the return(Line 522) PG Bug reporting form <noreply@postgresql.org>
2021-09-10 07:07 ` Michael Paquier <michael@paquier.xyz>
2021-09-11 05:38 ` Michael Paquier <michael@paquier.xyz>
2021-09-13 04:37 ` Michael Paquier <michael@paquier.xyz>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox