Missing OOM checks in libpq (was Re: [HACKERS] Replication connection URI?)
On 11/24/2014 06:05 PM, Alex Shulgin wrote:
The first patch is not on topic, I just spotted this missing check.
*** a/src/interfaces/libpq/fe-connect.c
--- b/src/interfaces/libpq/fe-connect.c
*** conninfo_array_parse(const char *const *
*** 4402,4407
--- 4402,4415
if
(options[k].val)
free(options[k].val);
options[k].val =
strdup(str_option-val);
+ if
(!options[k].val)
+ {
+
printfPQExpBuffer(errorMessage,
+
libpq_gettext(out of memory\n));
+
PQconninfoFree(options);
+
PQconninfoFree(dbname_options);
+ return
NULL;
+ }
break;
}
}
Oh. There are actually many more places in connection option parsing
that don't check the return value of strdup(). The one in fillPGConn
even has an XXX comment saying it probably should check it. You can get
quite strange behavior if one of them fails. If for example the strdup()
on dbname fails, you might end up connecting to different database than
intended. And if the conn-sslmode = strdup(DefaultSSLMode); call in
connectOptions2 fails, you'll get a segfault later because at least
connectDBstart assumes that sslmode is not NULL.
I think we need to fix all of those, and backpatch. Per attached.
- Heikki
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 3fe8c21..d4d8c3b 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -333,7 +333,7 @@ static int connectDBStart(PGconn *conn);
static int connectDBComplete(PGconn *conn);
static PGPing internal_ping(PGconn *conn);
static PGconn *makeEmptyPGconn(void);
-static void fillPGconn(PGconn *conn, PQconninfoOption *connOptions);
+static bool fillPGconn(PGconn *conn, PQconninfoOption *connOptions);
static void freePGconn(PGconn *conn);
static void closePGconn(PGconn *conn);
static PQconninfoOption *conninfo_init(PQExpBuffer errorMessage);
@@ -585,7 +585,11 @@ PQconnectStartParams(const char *const * keywords,
/*
* Move option values into conn structure
*/
- fillPGconn(conn, connOptions);
+ if (!fillPGconn(conn, connOptions))
+ {
+ PQconninfoFree(connOptions);
+ return conn;
+ }
/*
* Free the option info - all is in conn now
@@ -665,19 +669,19 @@ PQconnectStart(const char *conninfo)
return conn;
}
-static void
+/*
+ * Move option values into conn structure
+ *
+ * Don't put anything cute here --- intelligence should be in
+ * connectOptions2 ...
+ *
+ * Returns true on success. On failure, returns false and sets error message.
+ */
+static bool
fillPGconn(PGconn *conn, PQconninfoOption *connOptions)
{
const internalPQconninfoOption *option;
- /*
- * Move option values into conn structure
- *
- * Don't put anything cute here --- intelligence should be in
- * connectOptions2 ...
- *
- * XXX: probably worth checking strdup() return value here...
- */
for (option = PQconninfoOptions; option-keyword; option++)
{
const char *tmp = conninfo_getval(connOptions, option-keyword);
@@ -688,9 +692,22 @@ fillPGconn(PGconn *conn, PQconninfoOption *connOptions)
if (*connmember)
free(*connmember);
- *connmember = tmp ? strdup(tmp) : NULL;
+ if (tmp)
+ {
+*connmember = strdup(tmp);
+if (*connmember == NULL)
+{
+ printfPQExpBuffer(conn-errorMessage,
+ libpq_gettext(out of memory\n));
+ return false;
+}
+ }
+ else
+*connmember = NULL;
}
}
+
+ return true;
}
/*
@@ -723,7 +740,12 @@ connectOptions1(PGconn *conn, const char *conninfo)
/*
* Move option values into conn structure
*/
- fillPGconn(conn, connOptions);
+ if (!fillPGconn(conn, connOptions))
+ {
+ conn-status = CONNECTION_BAD;
+ PQconninfoFree(connOptions);
+ return false;
+ }
/*
* Free the option info - all is in conn now
@@ -753,6 +775,8 @@ connectOptions2(PGconn *conn)
if (conn-dbName)
free(conn-dbName);
conn-dbName = strdup(conn-pguser);
+ if (!conn-dbName)
+ goto oom_error;
}
/*
@@ -765,7 +789,12 @@ connectOptions2(PGconn *conn)
conn-pgpass = PasswordFromFile(conn-pghost, conn-pgport,
Re: Missing OOM checks in libpq (was Re: [HACKERS] Replication connection URI?)
Heikki Linnakangas hlinnakan...@vmware.com writes:
On 11/24/2014 06:05 PM, Alex Shulgin wrote:
The first patch is not on topic, I just spotted this missing check.
*** a/src/interfaces/libpq/fe-connect.c
--- b/src/interfaces/libpq/fe-connect.c
*** conninfo_array_parse(const char *const *
*** 4402,4407
--- 4402,4415
if
(options[k].val)
free(options[k].val);
options[k].val
= strdup(str_option-val);
+if
(!options[k].val)
+{
+
printfPQExpBuffer(errorMessage,
+
libpq_gettext(out of memory\n));
+
PQconninfoFree(options);
+
PQconninfoFree(dbname_options);
+return
NULL;
+}
break;
}
}
Oh. There are actually many more places in connection option parsing
that don't check the return value of strdup(). The one in fillPGConn
even has an XXX comment saying it probably should check it. You can
get quite strange behavior if one of them fails. If for example the
strdup() on dbname fails, you might end up connecting to different
database than intended. And if the conn-sslmode =
strdup(DefaultSSLMode); call in connectOptions2 fails, you'll get a
segfault later because at least connectDBstart assumes that sslmode is
not NULL.
I think we need to fix all of those, and backpatch. Per attached.
Yikes! Looks sane to me.
I've checked that patches #2 and #3 can be applied on top of this, w/o
rebasing.
--
Alex
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: Missing OOM checks in libpq (was Re: [HACKERS] Replication connection URI?)
On 11/25/2014 01:37 PM, Alex Shulgin wrote:
Heikki Linnakangas hlinnakan...@vmware.com writes:
On 11/24/2014 06:05 PM, Alex Shulgin wrote:
The first patch is not on topic, I just spotted this missing check.
*** a/src/interfaces/libpq/fe-connect.c
--- b/src/interfaces/libpq/fe-connect.c
*** conninfo_array_parse(const char *const *
*** 4402,4407
--- 4402,4415
if
(options[k].val)
free(options[k].val);
options[k].val =
strdup(str_option-val);
+ if
(!options[k].val)
+ {
+
printfPQExpBuffer(errorMessage,
+
libpq_gettext(out of memory\n));
+
PQconninfoFree(options);
+
PQconninfoFree(dbname_options);
+ return
NULL;
+ }
break;
}
}
Oh. There are actually many more places in connection option parsing
that don't check the return value of strdup(). The one in fillPGConn
even has an XXX comment saying it probably should check it. You can
get quite strange behavior if one of them fails. If for example the
strdup() on dbname fails, you might end up connecting to different
database than intended. And if the conn-sslmode =
strdup(DefaultSSLMode); call in connectOptions2 fails, you'll get a
segfault later because at least connectDBstart assumes that sslmode is
not NULL.
I think we need to fix all of those, and backpatch. Per attached.
Yikes! Looks sane to me.
Ok thanks, committed. It didn't apply cleanly to 9.0, 9.1 and 9.2, so
the patch for those branches looks a bit different.
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: Missing OOM checks in libpq (was Re: [HACKERS] Replication connection URI?)
Heikki Linnakangas hlinnakan...@vmware.com writes: I think we need to fix all of those, and backpatch. Per attached. Yikes! Looks sane to me. Ok thanks, committed. It didn't apply cleanly to 9.0, 9.1 and 9.2, so the patch for those branches looks a bit different. Great. Are you looking at the actual replication URI patch? Or should I add it to commitfest so we don't lose track of it? -- Alex -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
