Missing NULL check after calling ecpg_strdup
Started by Evgeniy Gorbanev6 months ago16 messages
Hi!
In case of out_of_memory, the ecpg_strdup function may return NULL.
Checks should be added in src/interfaces/ecpg/ecpglib/execute.c.
Patch attached.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
--
Best regards,
Evgeniy Gorbanev
Attachments:
ecpglib-execute-check-ecpg_strdup.patchtext/x-patch; charset=UTF-8; name=ecpglib-execute-check-ecpg_strdup.patchDownload
diff --git a/src/interfaces/ecpg/ecpglib/execute.c b/src/interfaces/ecpg/ecpglib/execute.c
index f52da06de9a..6524c646c13 100644
--- a/src/interfaces/ecpg/ecpglib/execute.c
+++ b/src/interfaces/ecpg/ecpglib/execute.c
@@ -2030,7 +2030,14 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator,
statement_type = ECPGst_execute;
}
else
+ {
stmt->command = ecpg_strdup(query, lineno);
+ if (!stmt->command)
+ {
+ ecpg_do_epilogue(stmt);
+ return false;
+ }
+ }
stmt->name = NULL;
@@ -2043,6 +2050,11 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator,
{
stmt->name = stmt->command;
stmt->command = ecpg_strdup(command, lineno);
+ if (!stmt->command)
+ {
+ ecpg_do_epilogue(stmt);
+ return false;
+ }
}
else
{
@@ -2176,6 +2188,11 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator,
if (!is_prepared_name_set && stmt->statement_type == ECPGst_prepare)
{
stmt->name = ecpg_strdup(var->value, lineno);
+ if (!stmt->name)
+ {
+ ecpg_do_epilogue(stmt);
+ return false;
+ }
is_prepared_name_set = true;
}
}
Re: Missing NULL check after calling ecpg_strdup
Hi Evgeniy,
In case of out_of_memory, the ecpg_strdup function may return NULL.
Checks should be added in src/interfaces/ecpg/ecpglib/execute.c.
Patch attached.Found by Linux Verification Center (linuxtesting.org) with SVACE.
The patch looks correct, but I believe it's incomplete. It misses
several other places where ecpg_strdup() is called without proper
checks. A correct patch would look like the one attached.
While working on it I noticed a potentially problematic strcmp call,
marked with XXX in the patch. I didn't address this issue in v2.
Thoughts?
Attachments:
v2-0001-Add-proper-checks-for-ecpg_strdup-return-value.patchtext/x-patch; charset=US-ASCII; name=v2-0001-Add-proper-checks-for-ecpg_strdup-return-value.patchDownload
From 4af966774361eac189e011d92e985883ee03ee5a Mon Sep 17 00:00:00 2001
From: Aleksander Alekseev <aleksander@timescale.com>
Date: Fri, 11 Jul 2025 17:59:50 +0300
Subject: [PATCH v2] Add proper checks for ecpg_strdup() return value
Evgeniy Gorbanev, Aleksander Alekseev. Reviewed by TODO FIXME.
---
src/interfaces/ecpg/ecpglib/connect.c | 26 ++++++++++++++++------
src/interfaces/ecpg/ecpglib/execute.c | 17 +++++++++++++++
src/interfaces/ecpg/ecpglib/prepare.c | 31 +++++++++++++++++++++++++++
3 files changed, 68 insertions(+), 6 deletions(-)
diff --git a/src/interfaces/ecpg/ecpglib/connect.c b/src/interfaces/ecpg/ecpglib/connect.c
index 2bbb70333dc..fc1034ccd7d 100644
--- a/src/interfaces/ecpg/ecpglib/connect.c
+++ b/src/interfaces/ecpg/ecpglib/connect.c
@@ -58,6 +58,7 @@ ecpg_get_connection_nr(const char *connection_name)
for (con = all_connections; con != NULL; con = con->next)
{
+ /* XXX strcmp() will segfault if con->name is NULL */
if (strcmp(connection_name, con->name) == 0)
break;
}
@@ -460,7 +461,21 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
*/
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)
+
+ /* Allocate memory for connection name */
+ if (connection_name != NULL)
+ this->name = ecpg_strdup(connection_name, lineno);
+ else
+ this->name = ecpg_strdup(realname, lineno);
+
+ /*
+ * Note that NULL is a correct value for realname and ecpg_strdup(NULL,
+ * ...) just returns NULL. For named reasons the ckecks for this->name are
+ * a bit complicated here.
+ */
+ if (conn_keywords == NULL || conn_values == NULL ||
+ (connection_name != NULL && this->name == NULL) || /* first call failed */
+ (connection_name == NULL && realname != NULL && this->name == NULL)) /* second call failed */
{
if (host)
ecpg_free(host);
@@ -476,6 +491,8 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
ecpg_free(conn_keywords);
if (conn_values)
ecpg_free(conn_values);
+ if (this->name)
+ ecpg_free(this->name);
free(this);
return false;
}
@@ -510,17 +527,14 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
ecpg_free(conn_keywords);
if (conn_values)
ecpg_free(conn_values);
+ if (this->name)
+ ecpg_free(this->name);
free(this);
return false;
}
}
#endif
- if (connection_name != NULL)
- this->name = ecpg_strdup(connection_name, lineno);
- else
- this->name = ecpg_strdup(realname, lineno);
-
this->cache_head = NULL;
this->prep_stmts = NULL;
diff --git a/src/interfaces/ecpg/ecpglib/execute.c b/src/interfaces/ecpg/ecpglib/execute.c
index f52da06de9a..6524c646c13 100644
--- a/src/interfaces/ecpg/ecpglib/execute.c
+++ b/src/interfaces/ecpg/ecpglib/execute.c
@@ -2030,7 +2030,14 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator,
statement_type = ECPGst_execute;
}
else
+ {
stmt->command = ecpg_strdup(query, lineno);
+ if (!stmt->command)
+ {
+ ecpg_do_epilogue(stmt);
+ return false;
+ }
+ }
stmt->name = NULL;
@@ -2043,6 +2050,11 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator,
{
stmt->name = stmt->command;
stmt->command = ecpg_strdup(command, lineno);
+ if (!stmt->command)
+ {
+ ecpg_do_epilogue(stmt);
+ return false;
+ }
}
else
{
@@ -2176,6 +2188,11 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator,
if (!is_prepared_name_set && stmt->statement_type == ECPGst_prepare)
{
stmt->name = ecpg_strdup(var->value, lineno);
+ if (!stmt->name)
+ {
+ ecpg_do_epilogue(stmt);
+ return false;
+ }
is_prepared_name_set = true;
}
}
diff --git a/src/interfaces/ecpg/ecpglib/prepare.c b/src/interfaces/ecpg/ecpglib/prepare.c
index ea1146f520f..7b6019deed7 100644
--- a/src/interfaces/ecpg/ecpglib/prepare.c
+++ b/src/interfaces/ecpg/ecpglib/prepare.c
@@ -86,8 +86,21 @@ ecpg_register_prepared_stmt(struct statement *stmt)
prep_stmt->lineno = lineno;
prep_stmt->connection = con;
prep_stmt->command = ecpg_strdup(stmt->command, lineno);
+ if (!prep_stmt->command)
+ {
+ ecpg_free(prep_stmt);
+ ecpg_free(this);
+ return false;
+ }
prep_stmt->inlist = prep_stmt->outlist = NULL;
this->name = ecpg_strdup(stmt->name, lineno);
+ if (!this->name)
+ {
+ ecpg_free(prep_stmt->command);
+ ecpg_free(prep_stmt);
+ ecpg_free(this);
+ return false;
+ }
this->stmt = prep_stmt;
this->prepared = true;
@@ -178,6 +191,12 @@ prepare_common(int lineno, struct connection *con, const char *name, const char
stmt->lineno = lineno;
stmt->connection = con;
stmt->command = ecpg_strdup(variable, lineno);
+ if (!stmt->command)
+ {
+ ecpg_free(stmt);
+ ecpg_free(this);
+ return false;
+ }
stmt->inlist = stmt->outlist = NULL;
/* if we have C variables in our statement replace them with '?' */
@@ -185,6 +204,13 @@ prepare_common(int lineno, struct connection *con, const char *name, const char
/* add prepared statement to our list */
this->name = ecpg_strdup(name, lineno);
+ if (!this->name)
+ {
+ ecpg_free(stmt->command);
+ ecpg_free(stmt);
+ ecpg_free(this);
+ return false;
+ }
this->stmt = stmt;
/* and finally really prepare the statement */
@@ -541,6 +567,8 @@ AddStmtToCache(int lineno, /* line # of statement */
entry = &stmtCacheEntries[entNo];
entry->lineno = lineno;
entry->ecpgQuery = ecpg_strdup(ecpgQuery, lineno);
+ if (!entry->ecpgQuery)
+ return -1;
entry->connection = connection;
entry->execs = 0;
memcpy(entry->stmtID, stmtID, sizeof(entry->stmtID));
@@ -595,6 +623,9 @@ ecpg_auto_prepare(int lineno, const char *connection_name, const int compat, cha
*name = ecpg_strdup(stmtID, lineno);
}
+ if (!*name)
+ return false;
+
/* increase usage counter */
stmtCacheEntries[entNo].execs++;
--
2.43.0
Re: Missing NULL check after calling ecpg_strdup
On Fri, Jul 11, 2025 at 07:22:36PM +0300, Aleksander Alekseev wrote:
The patch looks correct, but I believe it's incomplete. It misses
several other places where ecpg_strdup() is called without proper
checks. A correct patch would look like the one attached.While working on it I noticed a potentially problematic strcmp call,
marked with XXX in the patch. I didn't address this issue in v2.Thoughts?
The semantics that I'm finding really annoying is the fact that
ecpg_strdup() is OK to assume that a NULL input is valid to handle, so
there is no way to make the difference between what should be an
actual error and what should be valid, leading to more confusion
because "realname" can be NULL.
Should we actually check sqlca_t more seriously if failing one of the
strdup calls used for the port, host, etc. when attempting the
connection? The ecpg_log() assumes that a NULL value equals a
<DEFAULT>, which would be wrong if we failed one of these allocations
on OOM.
--
Michael
Re: Missing NULL check after calling ecpg_strdup
Hi Michael,
Should we actually check sqlca_t more seriously if failing one of the
strdup calls used for the port, host, etc. when attempting the
connection? The ecpg_log() assumes that a NULL value equals a
<DEFAULT>, which would be wrong if we failed one of these allocations
on OOM.
If I read this correctly, you propose to check if strdup returns an
error and if it does then somehow run extra checks on sqlca_t? Sorry,
I don't follow. Could you please elaborate what you are proposing?
Re: Missing NULL check after calling ecpg_strdup
Hi,
While working on it I noticed a potentially problematic strcmp call,
marked with XXX in the patch. I didn't address this issue in v2.
Here is the corrected patch v3. Changes since v2:
```
for (con = all_connections; con != NULL; con = con->next)
{
- /* XXX strcmp() will segfault if con->name is NULL */
- if (strcmp(connection_name, con->name) == 0)
+ /* Check for NULL to prevent segfault */
+ if (con->name != NULL &&
strcmp(connection_name, con->name) == 0)
break;
}
ret = con;
```I was tired or something and didn't think of this trivial fix.
As a side note it looks like ecpg could use some refactoring, but this
is subject for another patch IMO.
Re: Missing NULL check after calling ecpg_strdup
Hi,
Here is the corrected patch v3. Changes since v2:
``` for (con = all_connections; con != NULL; con = con->next) { - /* XXX strcmp() will segfault if con->name is NULL */ - if (strcmp(connection_name, con->name) == 0) + /* Check for NULL to prevent segfault */ + if (con->name != NULL && strcmp(connection_name, con->name) == 0) break; } ret = con; ```I was tired or something and didn't think of this trivial fix.
As a side note it looks like ecpg could use some refactoring, but this
is subject for another patch IMO.
Forgot the attachment. Sorry for the noise.
Attachments:
v3-0001-Add-proper-checks-for-ecpg_strdup-return-value.patchtext/x-patch; charset=US-ASCII; name=v3-0001-Add-proper-checks-for-ecpg_strdup-return-value.patchDownload
From 0ef7a6ba96dd8900ed5d34e3d3e8aa6dec53213b Mon Sep 17 00:00:00 2001
From: Aleksander Alekseev <aleksander@timescale.com>
Date: Fri, 11 Jul 2025 17:59:50 +0300
Subject: [PATCH v3] Add proper checks for ecpg_strdup() return value
Evgeniy Gorbanev, Aleksander Alekseev. Reviewed by TODO FIXME.
Discussion: https://postgr.es/m/a6b193c1-6994-4d9c-9059-aca4aaf41ddd@basealt.ru
---
src/interfaces/ecpg/ecpglib/connect.c | 28 ++++++++++++++++++------
src/interfaces/ecpg/ecpglib/execute.c | 17 +++++++++++++++
src/interfaces/ecpg/ecpglib/prepare.c | 31 +++++++++++++++++++++++++++
3 files changed, 69 insertions(+), 7 deletions(-)
diff --git a/src/interfaces/ecpg/ecpglib/connect.c b/src/interfaces/ecpg/ecpglib/connect.c
index 2bbb70333dc..a7b84f2d239 100644
--- a/src/interfaces/ecpg/ecpglib/connect.c
+++ b/src/interfaces/ecpg/ecpglib/connect.c
@@ -58,7 +58,8 @@ ecpg_get_connection_nr(const char *connection_name)
for (con = all_connections; con != NULL; con = con->next)
{
- if (strcmp(connection_name, con->name) == 0)
+ /* Check for NULL to prevent segfault */
+ if (con->name != NULL && strcmp(connection_name, con->name) == 0)
break;
}
ret = con;
@@ -460,7 +461,21 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
*/
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)
+
+ /* Allocate memory for connection name */
+ if (connection_name != NULL)
+ this->name = ecpg_strdup(connection_name, lineno);
+ else
+ this->name = ecpg_strdup(realname, lineno);
+
+ /*
+ * Note that NULL is a correct value for realname and ecpg_strdup(NULL,
+ * ...) just returns NULL. For named reasons the ckecks for this->name are
+ * a bit complicated here.
+ */
+ if (conn_keywords == NULL || conn_values == NULL ||
+ (connection_name != NULL && this->name == NULL) || /* first call failed */
+ (connection_name == NULL && realname != NULL && this->name == NULL)) /* second call failed */
{
if (host)
ecpg_free(host);
@@ -476,6 +491,8 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
ecpg_free(conn_keywords);
if (conn_values)
ecpg_free(conn_values);
+ if (this->name)
+ ecpg_free(this->name);
free(this);
return false;
}
@@ -510,17 +527,14 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
ecpg_free(conn_keywords);
if (conn_values)
ecpg_free(conn_values);
+ if (this->name)
+ ecpg_free(this->name);
free(this);
return false;
}
}
#endif
- if (connection_name != NULL)
- this->name = ecpg_strdup(connection_name, lineno);
- else
- this->name = ecpg_strdup(realname, lineno);
-
this->cache_head = NULL;
this->prep_stmts = NULL;
diff --git a/src/interfaces/ecpg/ecpglib/execute.c b/src/interfaces/ecpg/ecpglib/execute.c
index f52da06de9a..6524c646c13 100644
--- a/src/interfaces/ecpg/ecpglib/execute.c
+++ b/src/interfaces/ecpg/ecpglib/execute.c
@@ -2030,7 +2030,14 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator,
statement_type = ECPGst_execute;
}
else
+ {
stmt->command = ecpg_strdup(query, lineno);
+ if (!stmt->command)
+ {
+ ecpg_do_epilogue(stmt);
+ return false;
+ }
+ }
stmt->name = NULL;
@@ -2043,6 +2050,11 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator,
{
stmt->name = stmt->command;
stmt->command = ecpg_strdup(command, lineno);
+ if (!stmt->command)
+ {
+ ecpg_do_epilogue(stmt);
+ return false;
+ }
}
else
{
@@ -2176,6 +2188,11 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator,
if (!is_prepared_name_set && stmt->statement_type == ECPGst_prepare)
{
stmt->name = ecpg_strdup(var->value, lineno);
+ if (!stmt->name)
+ {
+ ecpg_do_epilogue(stmt);
+ return false;
+ }
is_prepared_name_set = true;
}
}
diff --git a/src/interfaces/ecpg/ecpglib/prepare.c b/src/interfaces/ecpg/ecpglib/prepare.c
index ea1146f520f..7b6019deed7 100644
--- a/src/interfaces/ecpg/ecpglib/prepare.c
+++ b/src/interfaces/ecpg/ecpglib/prepare.c
@@ -86,8 +86,21 @@ ecpg_register_prepared_stmt(struct statement *stmt)
prep_stmt->lineno = lineno;
prep_stmt->connection = con;
prep_stmt->command = ecpg_strdup(stmt->command, lineno);
+ if (!prep_stmt->command)
+ {
+ ecpg_free(prep_stmt);
+ ecpg_free(this);
+ return false;
+ }
prep_stmt->inlist = prep_stmt->outlist = NULL;
this->name = ecpg_strdup(stmt->name, lineno);
+ if (!this->name)
+ {
+ ecpg_free(prep_stmt->command);
+ ecpg_free(prep_stmt);
+ ecpg_free(this);
+ return false;
+ }
this->stmt = prep_stmt;
this->prepared = true;
@@ -178,6 +191,12 @@ prepare_common(int lineno, struct connection *con, const char *name, const char
stmt->lineno = lineno;
stmt->connection = con;
stmt->command = ecpg_strdup(variable, lineno);
+ if (!stmt->command)
+ {
+ ecpg_free(stmt);
+ ecpg_free(this);
+ return false;
+ }
stmt->inlist = stmt->outlist = NULL;
/* if we have C variables in our statement replace them with '?' */
@@ -185,6 +204,13 @@ prepare_common(int lineno, struct connection *con, const char *name, const char
/* add prepared statement to our list */
this->name = ecpg_strdup(name, lineno);
+ if (!this->name)
+ {
+ ecpg_free(stmt->command);
+ ecpg_free(stmt);
+ ecpg_free(this);
+ return false;
+ }
this->stmt = stmt;
/* and finally really prepare the statement */
@@ -541,6 +567,8 @@ AddStmtToCache(int lineno, /* line # of statement */
entry = &stmtCacheEntries[entNo];
entry->lineno = lineno;
entry->ecpgQuery = ecpg_strdup(ecpgQuery, lineno);
+ if (!entry->ecpgQuery)
+ return -1;
entry->connection = connection;
entry->execs = 0;
memcpy(entry->stmtID, stmtID, sizeof(entry->stmtID));
@@ -595,6 +623,9 @@ ecpg_auto_prepare(int lineno, const char *connection_name, const int compat, cha
*name = ecpg_strdup(stmtID, lineno);
}
+ if (!*name)
+ return false;
+
/* increase usage counter */
stmtCacheEntries[entNo].execs++;
--
2.43.0
Re: Missing NULL check after calling ecpg_strdup
On 2025-Jul-14, Aleksander Alekseev wrote:
@@ -460,7 +461,21 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p */ 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) + + /* Allocate memory for connection name */ + if (connection_name != NULL) + this->name = ecpg_strdup(connection_name, lineno); + else + this->name = ecpg_strdup(realname, lineno); + + /* + * Note that NULL is a correct value for realname and ecpg_strdup(NULL, + * ...) just returns NULL. For named reasons the ckecks for this->name are + * a bit complicated here. + */ + if (conn_keywords == NULL || conn_values == NULL || + (connection_name != NULL && this->name == NULL) || /* first call failed */ + (connection_name == NULL && realname != NULL && this->name == NULL)) /* second call failed */ { if (host) ecpg_free(host);
This looks super baroque. Why not simplify a bit instead? Maybe
something like
@@ -267,6 +268,7 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
*options = NULL;
const char **conn_keywords;
const char **conn_values;
+ bool strdup_failed;
if (sqlca == NULL)
{
@@ -460,7 +462,21 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
*/
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)
+
+ /* Decide on a connection name */
+ strdup_failed = false;
+ if (connection_name != NULL || realname != NULL)
+ {
+ this->name = ecpg_strdup(connection_name ? connection_name : realname,
+ lineno);
+ if (this->name == NULL)
+ strdup_failed = true;
+ }
+ else
+ this->name = NULL;
+
+ /* Deal with any failed allocations above */
+ if (conn_keywords == NULL || conn_values == NULL || strdup_failed)
{
if (host)
ecpg_free(host);--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
Re: Missing NULL check after calling ecpg_strdup
Hi Alvaro,
This looks super baroque. Why not simplify a bit instead? Maybe
something like[...]
Fair point. Here is the corrected patch.
Attachments:
v4-0001-Add-proper-checks-for-ecpg_strdup-return-value.patchtext/x-patch; charset=UTF-8; name=v4-0001-Add-proper-checks-for-ecpg_strdup-return-value.patchDownload
From 35cbf8fa22d0a1e9d1b46784a83adfdfd5c675fb Mon Sep 17 00:00:00 2001
From: Aleksander Alekseev <aleksander@timescale.com>
Date: Fri, 11 Jul 2025 17:59:50 +0300
Subject: [PATCH v4] Add proper checks for ecpg_strdup() return value
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Author: Evgeniy Gorbanev <gorbanyoves@basealt.ru>
Author: Aleksander Alekseev <aleksander@tigerdata.com>
Reviewed-by: Álvaro Herrera <alvherre@kurilemu.de>
Discussion: https://postgr.es/m/a6b193c1-6994-4d9c-9059-aca4aaf41ddd@basealt.ru
---
src/interfaces/ecpg/ecpglib/connect.c | 29 +++++++++++++++++++------
src/interfaces/ecpg/ecpglib/execute.c | 17 +++++++++++++++
src/interfaces/ecpg/ecpglib/prepare.c | 31 +++++++++++++++++++++++++++
3 files changed, 70 insertions(+), 7 deletions(-)
diff --git a/src/interfaces/ecpg/ecpglib/connect.c b/src/interfaces/ecpg/ecpglib/connect.c
index 2bbb70333dc..af25582abbd 100644
--- a/src/interfaces/ecpg/ecpglib/connect.c
+++ b/src/interfaces/ecpg/ecpglib/connect.c
@@ -58,7 +58,8 @@ ecpg_get_connection_nr(const char *connection_name)
for (con = all_connections; con != NULL; con = con->next)
{
- if (strcmp(connection_name, con->name) == 0)
+ /* Check for NULL to prevent segfault */
+ if (con->name != NULL && strcmp(connection_name, con->name) == 0)
break;
}
ret = con;
@@ -267,6 +268,7 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
*options = NULL;
const char **conn_keywords;
const char **conn_values;
+ bool strdup_failed;
if (sqlca == NULL)
{
@@ -460,7 +462,21 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
*/
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)
+
+ /* Decide on a connection name */
+ strdup_failed = false;
+ if (connection_name != NULL || realname != NULL)
+ {
+ this->name = ecpg_strdup(connection_name ? connection_name : realname,
+ lineno);
+ if (this->name == NULL)
+ strdup_failed = true;
+ }
+ else
+ this->name = NULL;
+
+ /* Deal with any failed allocations above */
+ if (conn_keywords == NULL || conn_values == NULL || strdup_failed)
{
if (host)
ecpg_free(host);
@@ -476,6 +492,8 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
ecpg_free(conn_keywords);
if (conn_values)
ecpg_free(conn_values);
+ if (this->name)
+ ecpg_free(this->name);
free(this);
return false;
}
@@ -510,17 +528,14 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
ecpg_free(conn_keywords);
if (conn_values)
ecpg_free(conn_values);
+ if (this->name)
+ ecpg_free(this->name);
free(this);
return false;
}
}
#endif
- if (connection_name != NULL)
- this->name = ecpg_strdup(connection_name, lineno);
- else
- this->name = ecpg_strdup(realname, lineno);
-
this->cache_head = NULL;
this->prep_stmts = NULL;
diff --git a/src/interfaces/ecpg/ecpglib/execute.c b/src/interfaces/ecpg/ecpglib/execute.c
index f52da06de9a..6524c646c13 100644
--- a/src/interfaces/ecpg/ecpglib/execute.c
+++ b/src/interfaces/ecpg/ecpglib/execute.c
@@ -2030,7 +2030,14 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator,
statement_type = ECPGst_execute;
}
else
+ {
stmt->command = ecpg_strdup(query, lineno);
+ if (!stmt->command)
+ {
+ ecpg_do_epilogue(stmt);
+ return false;
+ }
+ }
stmt->name = NULL;
@@ -2043,6 +2050,11 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator,
{
stmt->name = stmt->command;
stmt->command = ecpg_strdup(command, lineno);
+ if (!stmt->command)
+ {
+ ecpg_do_epilogue(stmt);
+ return false;
+ }
}
else
{
@@ -2176,6 +2188,11 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator,
if (!is_prepared_name_set && stmt->statement_type == ECPGst_prepare)
{
stmt->name = ecpg_strdup(var->value, lineno);
+ if (!stmt->name)
+ {
+ ecpg_do_epilogue(stmt);
+ return false;
+ }
is_prepared_name_set = true;
}
}
diff --git a/src/interfaces/ecpg/ecpglib/prepare.c b/src/interfaces/ecpg/ecpglib/prepare.c
index ea1146f520f..7b6019deed7 100644
--- a/src/interfaces/ecpg/ecpglib/prepare.c
+++ b/src/interfaces/ecpg/ecpglib/prepare.c
@@ -86,8 +86,21 @@ ecpg_register_prepared_stmt(struct statement *stmt)
prep_stmt->lineno = lineno;
prep_stmt->connection = con;
prep_stmt->command = ecpg_strdup(stmt->command, lineno);
+ if (!prep_stmt->command)
+ {
+ ecpg_free(prep_stmt);
+ ecpg_free(this);
+ return false;
+ }
prep_stmt->inlist = prep_stmt->outlist = NULL;
this->name = ecpg_strdup(stmt->name, lineno);
+ if (!this->name)
+ {
+ ecpg_free(prep_stmt->command);
+ ecpg_free(prep_stmt);
+ ecpg_free(this);
+ return false;
+ }
this->stmt = prep_stmt;
this->prepared = true;
@@ -178,6 +191,12 @@ prepare_common(int lineno, struct connection *con, const char *name, const char
stmt->lineno = lineno;
stmt->connection = con;
stmt->command = ecpg_strdup(variable, lineno);
+ if (!stmt->command)
+ {
+ ecpg_free(stmt);
+ ecpg_free(this);
+ return false;
+ }
stmt->inlist = stmt->outlist = NULL;
/* if we have C variables in our statement replace them with '?' */
@@ -185,6 +204,13 @@ prepare_common(int lineno, struct connection *con, const char *name, const char
/* add prepared statement to our list */
this->name = ecpg_strdup(name, lineno);
+ if (!this->name)
+ {
+ ecpg_free(stmt->command);
+ ecpg_free(stmt);
+ ecpg_free(this);
+ return false;
+ }
this->stmt = stmt;
/* and finally really prepare the statement */
@@ -541,6 +567,8 @@ AddStmtToCache(int lineno, /* line # of statement */
entry = &stmtCacheEntries[entNo];
entry->lineno = lineno;
entry->ecpgQuery = ecpg_strdup(ecpgQuery, lineno);
+ if (!entry->ecpgQuery)
+ return -1;
entry->connection = connection;
entry->execs = 0;
memcpy(entry->stmtID, stmtID, sizeof(entry->stmtID));
@@ -595,6 +623,9 @@ ecpg_auto_prepare(int lineno, const char *connection_name, const int compat, cha
*name = ecpg_strdup(stmtID, lineno);
}
+ if (!*name)
+ return false;
+
/* increase usage counter */
stmtCacheEntries[entNo].execs++;
--
2.43.0
Re: Missing NULL check after calling ecpg_strdup
On Mon, Jul 14, 2025 at 04:03:42PM +0300, Aleksander Alekseev wrote:
Hi Michael,
Should we actually check sqlca_t more seriously if failing one of the
strdup calls used for the port, host, etc. when attempting the
connection? The ecpg_log() assumes that a NULL value equals a
<DEFAULT>, which would be wrong if we failed one of these allocations
on OOM.If I read this correctly, you propose to check if strdup returns an
error and if it does then somehow run extra checks on sqlca_t? Sorry,
I don't follow. Could you please elaborate what you are proposing?
What I mean is that we need to be smarter with the error handling done
by the result returned by ecpg_strdup(), and that while your patch is
an improvement, we have much more problems that we should address.
For example, even if I look at your v4 posted at [1]/messages/by-id/CAJ7c6TPn+eO4cVH7urFr+G8d5TmMm0svsVQXjhY_C2LuBm8a7g@mail.gmail.com -- Michael, I am still
seeing such code block for the various fields when filling in the data
of a connection:
char *dbname = name ? ecpg_strdup(name, lineno) : NULL,
[...]
if (tmp[1]/messages/by-id/CAJ7c6TPn+eO4cVH7urFr+G8d5TmMm0svsVQXjhY_C2LuBm8a7g@mail.gmail.com -- Michael != '\0') /* non-empty database name */
{
realname = ecpg_strdup(tmp + 1, lineno);
connect_params++;
}
*tmp = '\0';
[...]
if (tmp != NULL) /* port number given */
{
*tmp = '\0';
port = ecpg_strdup(tmp + 1, lineno);
connect_params++;
}
[...]
etc.
So it happens that if there is a value to allocate and that we fail to
allocate it it, we log an OOM error with ecpg_raise(), but we don't
return immediately from ECPGconnect().
At the end the current coding means, if I am reading that right, that
we could create a connection data structure where we
think there are default values because these are NULL, but the caller
may have included a completely different value (note: I've also
confirmed that by forcing an error, and we happily try to open a
connection). This means this stuff could connect to a completely
unrelated server if some of the ecpg_strdup() calls fail on OOM, while
the next ones work. connect_params is used to count the number of
connection parameters, but it's not really used as in sanity checks
depending on what's set in a URI. I think that we need to redesign a
bit ecpg_strdup(), perhaps by providing an extra input argument so as
we can detect hard failures on OOM and let ECPGconnect() return early
if we find a problem. We should also force callers to take decisions
if they always have a non-NULL input, which is what we expect from
most of the fields extracted from the URI with strrchr().
[1]: /messages/by-id/CAJ7c6TPn+eO4cVH7urFr+G8d5TmMm0svsVQXjhY_C2LuBm8a7g@mail.gmail.com -- Michael
--
Michael
Re: Missing NULL check after calling ecpg_strdup
Hi Michael,
depending on what's set in a URI. I think that we need to redesign a
bit ecpg_strdup(), perhaps by providing an extra input argument so as
we can detect hard failures on OOM and let ECPGconnect() return early
if we find a problem.
Makes sense. In this case however I believe we should refactor the
rest of the functions in src/interfaces/ecpg/ecpglib/memory.c for
consistency. E.g. we will remove `lineno` argument and pass `bool*
alloc_failed` and let the callers make decisions.
Since this is going to be a fairly large refactoring I would like to
propose it as a separate patch after we agree on this one, if it works
for you. I believe change like this deserves a separate thread for
better visibility and also separate discussion.
We should also force callers to take decisions
if they always have a non-NULL input, which is what we expect from
most of the fields extracted from the URI with strrchr().
Not 100% sure if I fully understand this part. Are you proposing to move:
```
if (string == NULL)
return NULL;
```
... check from ecpg_strdup() on the calling side, or something else?
Re: Missing NULL check after calling ecpg_strdup
On Wed, Jul 16, 2025 at 02:04:12PM +0300, Aleksander Alekseev wrote:
Hi Michael,
depending on what's set in a URI. I think that we need to redesign a
bit ecpg_strdup(), perhaps by providing an extra input argument so as
we can detect hard failures on OOM and let ECPGconnect() return early
if we find a problem.Makes sense. In this case however I believe we should refactor the
rest of the functions in src/interfaces/ecpg/ecpglib/memory.c for
consistency. E.g. we will remove `lineno` argument and pass `bool*
alloc_failed` and let the callers make decisions.
Or you could keep the "lineno" and the error generated in memory.c as
this gives enough details about the location where the problem
happens. We are going to need the extra "alloc_failed".
Since this is going to be a fairly large refactoring I would like to
propose it as a separate patch after we agree on this one, if it works
for you. I believe change like this deserves a separate thread for
better visibility and also separate discussion.
Hmm. But both are related and the same problem, no? This is touching
the same code paths..
Not 100% sure if I fully understand this part. Are you proposing to move:
if (string == NULL)
return NULL;... check from ecpg_strdup() on the calling side, or something else?
Nope, we could keep this shortcut in ecpg_strdup() as long as there is
an extra field to track if an error has happened.
--
Michael
Re: Missing NULL check after calling ecpg_strdup
Hi Michael,
Thanks for all your great feedback.
Or you could keep the "lineno" and the error generated in memory.c as
this gives enough details about the location where the problem
happens. We are going to need the extra "alloc_failed".[...]
Hmm. But both are related and the same problem, no? This is touching
the same code paths..[...]
Nope, we could keep this shortcut in ecpg_strdup() as long as there is
an extra field to track if an error has happened.
OK, patch 0002 implements this idea with minimal changes to the existing logic.
Attachments:
v5-0001-Add-proper-checks-for-ecpg_strdup-return-value.patchtext/x-patch; charset=UTF-8; name=v5-0001-Add-proper-checks-for-ecpg_strdup-return-value.patchDownload
From ba5dcae9311270e37e6181f5de0bb439a1b5b55b Mon Sep 17 00:00:00 2001
From: Aleksander Alekseev <aleksander@timescale.com>
Date: Fri, 11 Jul 2025 17:59:50 +0300
Subject: [PATCH v5 1/2] Add proper checks for ecpg_strdup() return value
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Author: Evgeniy Gorbanev <gorbanyoves@basealt.ru>
Author: Aleksander Alekseev <aleksander@tigerdata.com>
Reviewed-by: Álvaro Herrera <alvherre@kurilemu.de>
Discussion: https://postgr.es/m/a6b193c1-6994-4d9c-9059-aca4aaf41ddd@basealt.ru
---
src/interfaces/ecpg/ecpglib/connect.c | 29 +++++++++++++++++++------
src/interfaces/ecpg/ecpglib/execute.c | 17 +++++++++++++++
src/interfaces/ecpg/ecpglib/prepare.c | 31 +++++++++++++++++++++++++++
3 files changed, 70 insertions(+), 7 deletions(-)
diff --git a/src/interfaces/ecpg/ecpglib/connect.c b/src/interfaces/ecpg/ecpglib/connect.c
index 2bbb70333dc..af25582abbd 100644
--- a/src/interfaces/ecpg/ecpglib/connect.c
+++ b/src/interfaces/ecpg/ecpglib/connect.c
@@ -58,7 +58,8 @@ ecpg_get_connection_nr(const char *connection_name)
for (con = all_connections; con != NULL; con = con->next)
{
- if (strcmp(connection_name, con->name) == 0)
+ /* Check for NULL to prevent segfault */
+ if (con->name != NULL && strcmp(connection_name, con->name) == 0)
break;
}
ret = con;
@@ -267,6 +268,7 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
*options = NULL;
const char **conn_keywords;
const char **conn_values;
+ bool strdup_failed;
if (sqlca == NULL)
{
@@ -460,7 +462,21 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
*/
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)
+
+ /* Decide on a connection name */
+ strdup_failed = false;
+ if (connection_name != NULL || realname != NULL)
+ {
+ this->name = ecpg_strdup(connection_name ? connection_name : realname,
+ lineno);
+ if (this->name == NULL)
+ strdup_failed = true;
+ }
+ else
+ this->name = NULL;
+
+ /* Deal with any failed allocations above */
+ if (conn_keywords == NULL || conn_values == NULL || strdup_failed)
{
if (host)
ecpg_free(host);
@@ -476,6 +492,8 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
ecpg_free(conn_keywords);
if (conn_values)
ecpg_free(conn_values);
+ if (this->name)
+ ecpg_free(this->name);
free(this);
return false;
}
@@ -510,17 +528,14 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
ecpg_free(conn_keywords);
if (conn_values)
ecpg_free(conn_values);
+ if (this->name)
+ ecpg_free(this->name);
free(this);
return false;
}
}
#endif
- if (connection_name != NULL)
- this->name = ecpg_strdup(connection_name, lineno);
- else
- this->name = ecpg_strdup(realname, lineno);
-
this->cache_head = NULL;
this->prep_stmts = NULL;
diff --git a/src/interfaces/ecpg/ecpglib/execute.c b/src/interfaces/ecpg/ecpglib/execute.c
index f52da06de9a..6524c646c13 100644
--- a/src/interfaces/ecpg/ecpglib/execute.c
+++ b/src/interfaces/ecpg/ecpglib/execute.c
@@ -2030,7 +2030,14 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator,
statement_type = ECPGst_execute;
}
else
+ {
stmt->command = ecpg_strdup(query, lineno);
+ if (!stmt->command)
+ {
+ ecpg_do_epilogue(stmt);
+ return false;
+ }
+ }
stmt->name = NULL;
@@ -2043,6 +2050,11 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator,
{
stmt->name = stmt->command;
stmt->command = ecpg_strdup(command, lineno);
+ if (!stmt->command)
+ {
+ ecpg_do_epilogue(stmt);
+ return false;
+ }
}
else
{
@@ -2176,6 +2188,11 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator,
if (!is_prepared_name_set && stmt->statement_type == ECPGst_prepare)
{
stmt->name = ecpg_strdup(var->value, lineno);
+ if (!stmt->name)
+ {
+ ecpg_do_epilogue(stmt);
+ return false;
+ }
is_prepared_name_set = true;
}
}
diff --git a/src/interfaces/ecpg/ecpglib/prepare.c b/src/interfaces/ecpg/ecpglib/prepare.c
index ea1146f520f..7b6019deed7 100644
--- a/src/interfaces/ecpg/ecpglib/prepare.c
+++ b/src/interfaces/ecpg/ecpglib/prepare.c
@@ -86,8 +86,21 @@ ecpg_register_prepared_stmt(struct statement *stmt)
prep_stmt->lineno = lineno;
prep_stmt->connection = con;
prep_stmt->command = ecpg_strdup(stmt->command, lineno);
+ if (!prep_stmt->command)
+ {
+ ecpg_free(prep_stmt);
+ ecpg_free(this);
+ return false;
+ }
prep_stmt->inlist = prep_stmt->outlist = NULL;
this->name = ecpg_strdup(stmt->name, lineno);
+ if (!this->name)
+ {
+ ecpg_free(prep_stmt->command);
+ ecpg_free(prep_stmt);
+ ecpg_free(this);
+ return false;
+ }
this->stmt = prep_stmt;
this->prepared = true;
@@ -178,6 +191,12 @@ prepare_common(int lineno, struct connection *con, const char *name, const char
stmt->lineno = lineno;
stmt->connection = con;
stmt->command = ecpg_strdup(variable, lineno);
+ if (!stmt->command)
+ {
+ ecpg_free(stmt);
+ ecpg_free(this);
+ return false;
+ }
stmt->inlist = stmt->outlist = NULL;
/* if we have C variables in our statement replace them with '?' */
@@ -185,6 +204,13 @@ prepare_common(int lineno, struct connection *con, const char *name, const char
/* add prepared statement to our list */
this->name = ecpg_strdup(name, lineno);
+ if (!this->name)
+ {
+ ecpg_free(stmt->command);
+ ecpg_free(stmt);
+ ecpg_free(this);
+ return false;
+ }
this->stmt = stmt;
/* and finally really prepare the statement */
@@ -541,6 +567,8 @@ AddStmtToCache(int lineno, /* line # of statement */
entry = &stmtCacheEntries[entNo];
entry->lineno = lineno;
entry->ecpgQuery = ecpg_strdup(ecpgQuery, lineno);
+ if (!entry->ecpgQuery)
+ return -1;
entry->connection = connection;
entry->execs = 0;
memcpy(entry->stmtID, stmtID, sizeof(entry->stmtID));
@@ -595,6 +623,9 @@ ecpg_auto_prepare(int lineno, const char *connection_name, const int compat, cha
*name = ecpg_strdup(stmtID, lineno);
}
+ if (!*name)
+ return false;
+
/* increase usage counter */
stmtCacheEntries[entNo].execs++;
--
2.43.0
v5-0002-Add-alloc_failed-argument-to-ecpg_strdup.patchtext/x-patch; charset=US-ASCII; name=v5-0002-Add-alloc_failed-argument-to-ecpg_strdup.patchDownload
From 919f3959e24ee86199978b6d20ff6bc43f80ce8f Mon Sep 17 00:00:00 2001
From: Aleksander Alekseev <aleksander@timescale.com>
Date: Fri, 18 Jul 2025 14:50:36 +0300
Subject: [PATCH v5 2/2] Add alloc_failed argument to ecpg_strdup()
The new bool* argument allows to distinguish cases when ecpg_strdup() was
called with NULL argument and when internal call of strdup() failed. This
simplifies error handling on the calling side.
Author: Aleksander Alekseev <aleksander@tigerdata.com>
Reviewed-by: TODO FIXME
Discussion: https://postgr.es/m/a6b193c1-6994-4d9c-9059-aca4aaf41ddd@basealt.ru
---
src/interfaces/ecpg/ecpglib/connect.c | 29 +++++++--------
src/interfaces/ecpg/ecpglib/descriptor.c | 8 +++--
src/interfaces/ecpg/ecpglib/ecpglib_extern.h | 2 +-
src/interfaces/ecpg/ecpglib/execute.c | 38 ++++++++++----------
src/interfaces/ecpg/ecpglib/memory.c | 3 +-
src/interfaces/ecpg/ecpglib/prepare.c | 20 ++++++-----
6 files changed, 54 insertions(+), 46 deletions(-)
diff --git a/src/interfaces/ecpg/ecpglib/connect.c b/src/interfaces/ecpg/ecpglib/connect.c
index af25582abbd..9827751f331 100644
--- a/src/interfaces/ecpg/ecpglib/connect.c
+++ b/src/interfaces/ecpg/ecpglib/connect.c
@@ -260,7 +260,8 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
struct connection *this;
int i,
connect_params = 0;
- char *dbname = name ? ecpg_strdup(name, lineno) : NULL,
+ bool alloc_failed = (sqlca == NULL);
+ char *dbname = name ? ecpg_strdup(name, lineno, &alloc_failed) : NULL,
*host = NULL,
*tmp,
*port = NULL,
@@ -268,9 +269,8 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
*options = NULL;
const char **conn_keywords;
const char **conn_values;
- bool strdup_failed;
- if (sqlca == NULL)
+ if (alloc_failed)
{
ecpg_raise(lineno, ECPG_OUT_OF_MEMORY,
ECPG_SQLSTATE_ECPG_OUT_OF_MEMORY, NULL);
@@ -299,7 +299,7 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
if (envname)
{
ecpg_free(dbname);
- dbname = ecpg_strdup(envname, lineno);
+ dbname = ecpg_strdup(envname, lineno, &alloc_failed);
}
}
@@ -351,7 +351,7 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
tmp = strrchr(dbname + offset, '?');
if (tmp != NULL) /* options given */
{
- options = ecpg_strdup(tmp + 1, lineno);
+ options = ecpg_strdup(tmp + 1, lineno, &alloc_failed);
*tmp = '\0';
}
@@ -360,7 +360,7 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
{
if (tmp[1] != '\0') /* non-empty database name */
{
- realname = ecpg_strdup(tmp + 1, lineno);
+ realname = ecpg_strdup(tmp + 1, lineno, &alloc_failed);
connect_params++;
}
*tmp = '\0';
@@ -370,7 +370,7 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
if (tmp != NULL) /* port number given */
{
*tmp = '\0';
- port = ecpg_strdup(tmp + 1, lineno);
+ port = ecpg_strdup(tmp + 1, lineno, &alloc_failed);
connect_params++;
}
@@ -404,7 +404,7 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
{
if (*(dbname + offset) != '\0')
{
- host = ecpg_strdup(dbname + offset, lineno);
+ host = ecpg_strdup(dbname + offset, lineno, &alloc_failed);
connect_params++;
}
}
@@ -416,7 +416,7 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
tmp = strrchr(dbname, ':');
if (tmp != NULL) /* port number given */
{
- port = ecpg_strdup(tmp + 1, lineno);
+ port = ecpg_strdup(tmp + 1, lineno, &alloc_failed);
connect_params++;
*tmp = '\0';
}
@@ -424,14 +424,14 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
tmp = strrchr(dbname, '@');
if (tmp != NULL) /* host name given */
{
- host = ecpg_strdup(tmp + 1, lineno);
+ host = ecpg_strdup(tmp + 1, lineno, &alloc_failed);
connect_params++;
*tmp = '\0';
}
if (strlen(dbname) > 0)
{
- realname = ecpg_strdup(dbname, lineno);
+ realname = ecpg_strdup(dbname, lineno, &alloc_failed);
connect_params++;
}
else
@@ -464,19 +464,16 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
conn_values = (const char **) ecpg_alloc(connect_params * sizeof(char *), lineno);
/* Decide on a connection name */
- strdup_failed = false;
if (connection_name != NULL || realname != NULL)
{
this->name = ecpg_strdup(connection_name ? connection_name : realname,
- lineno);
- if (this->name == NULL)
- strdup_failed = true;
+ lineno, &alloc_failed);
}
else
this->name = NULL;
/* Deal with any failed allocations above */
- if (conn_keywords == NULL || conn_values == NULL || strdup_failed)
+ if (conn_keywords == NULL || conn_values == NULL || alloc_failed)
{
if (host)
ecpg_free(host);
diff --git a/src/interfaces/ecpg/ecpglib/descriptor.c b/src/interfaces/ecpg/ecpglib/descriptor.c
index 651d5c8b2ed..0480ba23ffc 100644
--- a/src/interfaces/ecpg/ecpglib/descriptor.c
+++ b/src/interfaces/ecpg/ecpglib/descriptor.c
@@ -240,8 +240,9 @@ ECPGget_desc(int lineno, const char *desc_name, int index,...)
act_tuple;
struct variable data_var;
struct sqlca_t *sqlca = ECPGget_sqlca();
+ bool alloc_failed = (sqlca == NULL);
- if (sqlca == NULL)
+ if (alloc_failed)
{
ecpg_raise(lineno, ECPG_OUT_OF_MEMORY,
ECPG_SQLSTATE_ECPG_OUT_OF_MEMORY, NULL);
@@ -493,7 +494,10 @@ ECPGget_desc(int lineno, const char *desc_name, int index,...)
#ifdef WIN32
stmt.oldthreadlocale = _configthreadlocale(_ENABLE_PER_THREAD_LOCALE);
#endif
- stmt.oldlocale = ecpg_strdup(setlocale(LC_NUMERIC, NULL), lineno);
+ stmt.oldlocale = ecpg_strdup(setlocale(LC_NUMERIC, NULL), lineno, &alloc_failed);
+ if (alloc_failed)
+ return false;
+
setlocale(LC_NUMERIC, "C");
#endif
diff --git a/src/interfaces/ecpg/ecpglib/ecpglib_extern.h b/src/interfaces/ecpg/ecpglib/ecpglib_extern.h
index 75cc68275bd..949ff66cefc 100644
--- a/src/interfaces/ecpg/ecpglib/ecpglib_extern.h
+++ b/src/interfaces/ecpg/ecpglib/ecpglib_extern.h
@@ -175,7 +175,7 @@ void ecpg_free(void *ptr);
bool ecpg_init(const struct connection *con,
const char *connection_name,
const int lineno);
-char *ecpg_strdup(const char *string, int lineno);
+char *ecpg_strdup(const char *string, int lineno, bool *alloc_failed);
const char *ecpg_type_name(enum ECPGttype typ);
int ecpg_dynamic_type(Oid type);
int sqlda_dynamic_type(Oid type, enum COMPAT_MODE compat);
diff --git a/src/interfaces/ecpg/ecpglib/execute.c b/src/interfaces/ecpg/ecpglib/execute.c
index 6524c646c13..186be720e1a 100644
--- a/src/interfaces/ecpg/ecpglib/execute.c
+++ b/src/interfaces/ecpg/ecpglib/execute.c
@@ -508,6 +508,7 @@ ecpg_store_input(const int lineno, const bool force_indicator, const struct vari
{
char *mallocedval = NULL;
char *newcopy = NULL;
+ bool alloc_failed = false;
/*
* arrays are not possible unless the column is an array, too FIXME: we do
@@ -860,11 +861,11 @@ ecpg_store_input(const int lineno, const bool force_indicator, const struct vari
numeric *nval;
if (var->arrsize > 1)
- mallocedval = ecpg_strdup("{", lineno);
+ mallocedval = ecpg_strdup("{", lineno, &alloc_failed);
else
- mallocedval = ecpg_strdup("", lineno);
+ mallocedval = ecpg_strdup("", lineno, &alloc_failed);
- if (!mallocedval)
+ if (alloc_failed)
return false;
for (element = 0; element < asize; element++)
@@ -923,11 +924,11 @@ ecpg_store_input(const int lineno, const bool force_indicator, const struct vari
int slen;
if (var->arrsize > 1)
- mallocedval = ecpg_strdup("{", lineno);
+ mallocedval = ecpg_strdup("{", lineno, &alloc_failed);
else
- mallocedval = ecpg_strdup("", lineno);
+ mallocedval = ecpg_strdup("", lineno, &alloc_failed);
- if (!mallocedval)
+ if (alloc_failed)
return false;
for (element = 0; element < asize; element++)
@@ -970,11 +971,11 @@ ecpg_store_input(const int lineno, const bool force_indicator, const struct vari
int slen;
if (var->arrsize > 1)
- mallocedval = ecpg_strdup("{", lineno);
+ mallocedval = ecpg_strdup("{", lineno, &alloc_failed);
else
- mallocedval = ecpg_strdup("", lineno);
+ mallocedval = ecpg_strdup("", lineno, &alloc_failed);
- if (!mallocedval)
+ if (alloc_failed)
return false;
for (element = 0; element < asize; element++)
@@ -1017,11 +1018,11 @@ ecpg_store_input(const int lineno, const bool force_indicator, const struct vari
int slen;
if (var->arrsize > 1)
- mallocedval = ecpg_strdup("{", lineno);
+ mallocedval = ecpg_strdup("{", lineno, &alloc_failed);
else
- mallocedval = ecpg_strdup("", lineno);
+ mallocedval = ecpg_strdup("", lineno, &alloc_failed);
- if (!mallocedval)
+ if (alloc_failed)
return false;
for (element = 0; element < asize; element++)
@@ -1952,6 +1953,7 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator,
struct variable **list;
char *prepname;
bool is_prepared_name_set;
+ bool alloc_failed = false;
*stmt_out = NULL;
@@ -2001,7 +2003,7 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator,
return false;
}
#endif
- stmt->oldlocale = ecpg_strdup(setlocale(LC_NUMERIC, NULL), lineno);
+ stmt->oldlocale = ecpg_strdup(setlocale(LC_NUMERIC, NULL), lineno, &alloc_failed);
if (stmt->oldlocale == NULL)
{
ecpg_do_epilogue(stmt);
@@ -2031,8 +2033,8 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator,
}
else
{
- stmt->command = ecpg_strdup(query, lineno);
- if (!stmt->command)
+ stmt->command = ecpg_strdup(query, lineno, &alloc_failed);
+ if (alloc_failed)
{
ecpg_do_epilogue(stmt);
return false;
@@ -2049,8 +2051,8 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator,
if (command)
{
stmt->name = stmt->command;
- stmt->command = ecpg_strdup(command, lineno);
- if (!stmt->command)
+ stmt->command = ecpg_strdup(command, lineno, &alloc_failed);
+ if (alloc_failed)
{
ecpg_do_epilogue(stmt);
return false;
@@ -2187,7 +2189,7 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator,
if (!is_prepared_name_set && stmt->statement_type == ECPGst_prepare)
{
- stmt->name = ecpg_strdup(var->value, lineno);
+ stmt->name = ecpg_strdup(var->value, lineno, &alloc_failed);
if (!stmt->name)
{
ecpg_do_epilogue(stmt);
diff --git a/src/interfaces/ecpg/ecpglib/memory.c b/src/interfaces/ecpg/ecpglib/memory.c
index 6979be2c988..ee71dd72f52 100644
--- a/src/interfaces/ecpg/ecpglib/memory.c
+++ b/src/interfaces/ecpg/ecpglib/memory.c
@@ -44,7 +44,7 @@ ecpg_realloc(void *ptr, long size, int lineno)
}
char *
-ecpg_strdup(const char *string, int lineno)
+ecpg_strdup(const char *string, int lineno, bool *alloc_failed)
{
char *new;
@@ -54,6 +54,7 @@ ecpg_strdup(const char *string, int lineno)
new = strdup(string);
if (!new)
{
+ *alloc_failed = true;
ecpg_raise(lineno, ECPG_OUT_OF_MEMORY, ECPG_SQLSTATE_ECPG_OUT_OF_MEMORY, NULL);
return NULL;
}
diff --git a/src/interfaces/ecpg/ecpglib/prepare.c b/src/interfaces/ecpg/ecpglib/prepare.c
index 7b6019deed7..1eda837e3e7 100644
--- a/src/interfaces/ecpg/ecpglib/prepare.c
+++ b/src/interfaces/ecpg/ecpglib/prepare.c
@@ -63,6 +63,7 @@ ecpg_register_prepared_stmt(struct statement *stmt)
struct connection *con = stmt->connection;
struct prepared_statement *prev = NULL;
int lineno = stmt->lineno;
+ bool alloc_failed = false;
/* check if we already have prepared this statement */
this = ecpg_find_prepared_statement(stmt->name, con, &prev);
@@ -85,7 +86,7 @@ ecpg_register_prepared_stmt(struct statement *stmt)
/* create statement */
prep_stmt->lineno = lineno;
prep_stmt->connection = con;
- prep_stmt->command = ecpg_strdup(stmt->command, lineno);
+ prep_stmt->command = ecpg_strdup(stmt->command, lineno, &alloc_failed);
if (!prep_stmt->command)
{
ecpg_free(prep_stmt);
@@ -93,7 +94,7 @@ ecpg_register_prepared_stmt(struct statement *stmt)
return false;
}
prep_stmt->inlist = prep_stmt->outlist = NULL;
- this->name = ecpg_strdup(stmt->name, lineno);
+ this->name = ecpg_strdup(stmt->name, lineno, &alloc_failed);
if (!this->name)
{
ecpg_free(prep_stmt->command);
@@ -174,6 +175,7 @@ prepare_common(int lineno, struct connection *con, const char *name, const char
struct statement *stmt;
struct prepared_statement *this;
PGresult *query;
+ bool alloc_failed = false;
/* allocate new statement */
this = (struct prepared_statement *) ecpg_alloc(sizeof(struct prepared_statement), lineno);
@@ -190,7 +192,7 @@ prepare_common(int lineno, struct connection *con, const char *name, const char
/* create statement */
stmt->lineno = lineno;
stmt->connection = con;
- stmt->command = ecpg_strdup(variable, lineno);
+ stmt->command = ecpg_strdup(variable, lineno, &alloc_failed);
if (!stmt->command)
{
ecpg_free(stmt);
@@ -203,7 +205,7 @@ prepare_common(int lineno, struct connection *con, const char *name, const char
replace_variables(&(stmt->command), lineno);
/* add prepared statement to our list */
- this->name = ecpg_strdup(name, lineno);
+ this->name = ecpg_strdup(name, lineno, &alloc_failed);
if (!this->name)
{
ecpg_free(stmt->command);
@@ -525,6 +527,7 @@ AddStmtToCache(int lineno, /* line # of statement */
luEntNo,
entNo;
stmtCacheEntry *entry;
+ bool alloc_failed = false;
/* allocate and zero cache array if we haven't already */
if (stmtCacheEntries == NULL)
@@ -566,7 +569,7 @@ AddStmtToCache(int lineno, /* line # of statement */
/* add the query to the entry */
entry = &stmtCacheEntries[entNo];
entry->lineno = lineno;
- entry->ecpgQuery = ecpg_strdup(ecpgQuery, lineno);
+ entry->ecpgQuery = ecpg_strdup(ecpgQuery, lineno, &alloc_failed);
if (!entry->ecpgQuery)
return -1;
entry->connection = connection;
@@ -581,6 +584,7 @@ bool
ecpg_auto_prepare(int lineno, const char *connection_name, const int compat, char **name, const char *query)
{
int entNo;
+ bool alloc_failed = false;
/* search the statement cache for this statement */
entNo = SearchStmtCache(query);
@@ -602,7 +606,7 @@ ecpg_auto_prepare(int lineno, const char *connection_name, const int compat, cha
if (!prep && !prepare_common(lineno, con, stmtID, query))
return false;
- *name = ecpg_strdup(stmtID, lineno);
+ *name = ecpg_strdup(stmtID, lineno, &alloc_failed);
}
else
{
@@ -620,10 +624,10 @@ ecpg_auto_prepare(int lineno, const char *connection_name, const int compat, cha
if (entNo < 0)
return false;
- *name = ecpg_strdup(stmtID, lineno);
+ *name = ecpg_strdup(stmtID, lineno, &alloc_failed);
}
- if (!*name)
+ if (alloc_failed)
return false;
/* increase usage counter */
--
2.43.0
Re: Missing NULL check after calling ecpg_strdup
Hi,
OK, patch 0002 implements this idea with minimal changes to the existing logic.
Here is a slightly modified version:
``
--- a/src/interfaces/ecpg/ecpglib/prepare.c
+++ b/src/interfaces/ecpg/ecpglib/prepare.c
@@ -570,7 +570,7 @@ AddStmtToCache(int lineno, /* line # of
statement */
entry = &stmtCacheEntries[entNo];
entry->lineno = lineno;
entry->ecpgQuery = ecpg_strdup(ecpgQuery, lineno, &alloc_failed);
- if (!entry->ecpgQuery)
+ if (alloc_failed)
return -1;
entry->connection = connection;
entry->execs = 0;
```We know that ecpgQuery can't be NULL because we hash its value above.
Thus ecpg_strdup can fail only if strdup() fails.
Attachments:
v6-0002-Add-alloc_failed-argument-to-ecpg_strdup.patchtext/x-patch; charset=US-ASCII; name=v6-0002-Add-alloc_failed-argument-to-ecpg_strdup.patchDownload
From 7f9c42897bcc7032e4bfe4c6bb6bae7b3883e8ac Mon Sep 17 00:00:00 2001
From: Aleksander Alekseev <aleksander@timescale.com>
Date: Fri, 18 Jul 2025 14:50:36 +0300
Subject: [PATCH v6 2/2] Add alloc_failed argument to ecpg_strdup()
The new bool* argument allows to distinguish cases when ecpg_strdup() was
called with NULL argument and when internal call of strdup() failed. This
simplifies error handling on the calling side.
Author: Aleksander Alekseev <aleksander@tigerdata.com>
Reviewed-by: TODO FIXME
Discussion: https://postgr.es/m/a6b193c1-6994-4d9c-9059-aca4aaf41ddd@basealt.ru
---
src/interfaces/ecpg/ecpglib/connect.c | 29 +++++++--------
src/interfaces/ecpg/ecpglib/descriptor.c | 8 +++--
src/interfaces/ecpg/ecpglib/ecpglib_extern.h | 2 +-
src/interfaces/ecpg/ecpglib/execute.c | 38 ++++++++++----------
src/interfaces/ecpg/ecpglib/memory.c | 3 +-
src/interfaces/ecpg/ecpglib/prepare.c | 22 +++++++-----
6 files changed, 55 insertions(+), 47 deletions(-)
diff --git a/src/interfaces/ecpg/ecpglib/connect.c b/src/interfaces/ecpg/ecpglib/connect.c
index af25582abbd..9827751f331 100644
--- a/src/interfaces/ecpg/ecpglib/connect.c
+++ b/src/interfaces/ecpg/ecpglib/connect.c
@@ -260,7 +260,8 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
struct connection *this;
int i,
connect_params = 0;
- char *dbname = name ? ecpg_strdup(name, lineno) : NULL,
+ bool alloc_failed = (sqlca == NULL);
+ char *dbname = name ? ecpg_strdup(name, lineno, &alloc_failed) : NULL,
*host = NULL,
*tmp,
*port = NULL,
@@ -268,9 +269,8 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
*options = NULL;
const char **conn_keywords;
const char **conn_values;
- bool strdup_failed;
- if (sqlca == NULL)
+ if (alloc_failed)
{
ecpg_raise(lineno, ECPG_OUT_OF_MEMORY,
ECPG_SQLSTATE_ECPG_OUT_OF_MEMORY, NULL);
@@ -299,7 +299,7 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
if (envname)
{
ecpg_free(dbname);
- dbname = ecpg_strdup(envname, lineno);
+ dbname = ecpg_strdup(envname, lineno, &alloc_failed);
}
}
@@ -351,7 +351,7 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
tmp = strrchr(dbname + offset, '?');
if (tmp != NULL) /* options given */
{
- options = ecpg_strdup(tmp + 1, lineno);
+ options = ecpg_strdup(tmp + 1, lineno, &alloc_failed);
*tmp = '\0';
}
@@ -360,7 +360,7 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
{
if (tmp[1] != '\0') /* non-empty database name */
{
- realname = ecpg_strdup(tmp + 1, lineno);
+ realname = ecpg_strdup(tmp + 1, lineno, &alloc_failed);
connect_params++;
}
*tmp = '\0';
@@ -370,7 +370,7 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
if (tmp != NULL) /* port number given */
{
*tmp = '\0';
- port = ecpg_strdup(tmp + 1, lineno);
+ port = ecpg_strdup(tmp + 1, lineno, &alloc_failed);
connect_params++;
}
@@ -404,7 +404,7 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
{
if (*(dbname + offset) != '\0')
{
- host = ecpg_strdup(dbname + offset, lineno);
+ host = ecpg_strdup(dbname + offset, lineno, &alloc_failed);
connect_params++;
}
}
@@ -416,7 +416,7 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
tmp = strrchr(dbname, ':');
if (tmp != NULL) /* port number given */
{
- port = ecpg_strdup(tmp + 1, lineno);
+ port = ecpg_strdup(tmp + 1, lineno, &alloc_failed);
connect_params++;
*tmp = '\0';
}
@@ -424,14 +424,14 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
tmp = strrchr(dbname, '@');
if (tmp != NULL) /* host name given */
{
- host = ecpg_strdup(tmp + 1, lineno);
+ host = ecpg_strdup(tmp + 1, lineno, &alloc_failed);
connect_params++;
*tmp = '\0';
}
if (strlen(dbname) > 0)
{
- realname = ecpg_strdup(dbname, lineno);
+ realname = ecpg_strdup(dbname, lineno, &alloc_failed);
connect_params++;
}
else
@@ -464,19 +464,16 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
conn_values = (const char **) ecpg_alloc(connect_params * sizeof(char *), lineno);
/* Decide on a connection name */
- strdup_failed = false;
if (connection_name != NULL || realname != NULL)
{
this->name = ecpg_strdup(connection_name ? connection_name : realname,
- lineno);
- if (this->name == NULL)
- strdup_failed = true;
+ lineno, &alloc_failed);
}
else
this->name = NULL;
/* Deal with any failed allocations above */
- if (conn_keywords == NULL || conn_values == NULL || strdup_failed)
+ if (conn_keywords == NULL || conn_values == NULL || alloc_failed)
{
if (host)
ecpg_free(host);
diff --git a/src/interfaces/ecpg/ecpglib/descriptor.c b/src/interfaces/ecpg/ecpglib/descriptor.c
index 651d5c8b2ed..0480ba23ffc 100644
--- a/src/interfaces/ecpg/ecpglib/descriptor.c
+++ b/src/interfaces/ecpg/ecpglib/descriptor.c
@@ -240,8 +240,9 @@ ECPGget_desc(int lineno, const char *desc_name, int index,...)
act_tuple;
struct variable data_var;
struct sqlca_t *sqlca = ECPGget_sqlca();
+ bool alloc_failed = (sqlca == NULL);
- if (sqlca == NULL)
+ if (alloc_failed)
{
ecpg_raise(lineno, ECPG_OUT_OF_MEMORY,
ECPG_SQLSTATE_ECPG_OUT_OF_MEMORY, NULL);
@@ -493,7 +494,10 @@ ECPGget_desc(int lineno, const char *desc_name, int index,...)
#ifdef WIN32
stmt.oldthreadlocale = _configthreadlocale(_ENABLE_PER_THREAD_LOCALE);
#endif
- stmt.oldlocale = ecpg_strdup(setlocale(LC_NUMERIC, NULL), lineno);
+ stmt.oldlocale = ecpg_strdup(setlocale(LC_NUMERIC, NULL), lineno, &alloc_failed);
+ if (alloc_failed)
+ return false;
+
setlocale(LC_NUMERIC, "C");
#endif
diff --git a/src/interfaces/ecpg/ecpglib/ecpglib_extern.h b/src/interfaces/ecpg/ecpglib/ecpglib_extern.h
index 75cc68275bd..949ff66cefc 100644
--- a/src/interfaces/ecpg/ecpglib/ecpglib_extern.h
+++ b/src/interfaces/ecpg/ecpglib/ecpglib_extern.h
@@ -175,7 +175,7 @@ void ecpg_free(void *ptr);
bool ecpg_init(const struct connection *con,
const char *connection_name,
const int lineno);
-char *ecpg_strdup(const char *string, int lineno);
+char *ecpg_strdup(const char *string, int lineno, bool *alloc_failed);
const char *ecpg_type_name(enum ECPGttype typ);
int ecpg_dynamic_type(Oid type);
int sqlda_dynamic_type(Oid type, enum COMPAT_MODE compat);
diff --git a/src/interfaces/ecpg/ecpglib/execute.c b/src/interfaces/ecpg/ecpglib/execute.c
index 6524c646c13..186be720e1a 100644
--- a/src/interfaces/ecpg/ecpglib/execute.c
+++ b/src/interfaces/ecpg/ecpglib/execute.c
@@ -508,6 +508,7 @@ ecpg_store_input(const int lineno, const bool force_indicator, const struct vari
{
char *mallocedval = NULL;
char *newcopy = NULL;
+ bool alloc_failed = false;
/*
* arrays are not possible unless the column is an array, too FIXME: we do
@@ -860,11 +861,11 @@ ecpg_store_input(const int lineno, const bool force_indicator, const struct vari
numeric *nval;
if (var->arrsize > 1)
- mallocedval = ecpg_strdup("{", lineno);
+ mallocedval = ecpg_strdup("{", lineno, &alloc_failed);
else
- mallocedval = ecpg_strdup("", lineno);
+ mallocedval = ecpg_strdup("", lineno, &alloc_failed);
- if (!mallocedval)
+ if (alloc_failed)
return false;
for (element = 0; element < asize; element++)
@@ -923,11 +924,11 @@ ecpg_store_input(const int lineno, const bool force_indicator, const struct vari
int slen;
if (var->arrsize > 1)
- mallocedval = ecpg_strdup("{", lineno);
+ mallocedval = ecpg_strdup("{", lineno, &alloc_failed);
else
- mallocedval = ecpg_strdup("", lineno);
+ mallocedval = ecpg_strdup("", lineno, &alloc_failed);
- if (!mallocedval)
+ if (alloc_failed)
return false;
for (element = 0; element < asize; element++)
@@ -970,11 +971,11 @@ ecpg_store_input(const int lineno, const bool force_indicator, const struct vari
int slen;
if (var->arrsize > 1)
- mallocedval = ecpg_strdup("{", lineno);
+ mallocedval = ecpg_strdup("{", lineno, &alloc_failed);
else
- mallocedval = ecpg_strdup("", lineno);
+ mallocedval = ecpg_strdup("", lineno, &alloc_failed);
- if (!mallocedval)
+ if (alloc_failed)
return false;
for (element = 0; element < asize; element++)
@@ -1017,11 +1018,11 @@ ecpg_store_input(const int lineno, const bool force_indicator, const struct vari
int slen;
if (var->arrsize > 1)
- mallocedval = ecpg_strdup("{", lineno);
+ mallocedval = ecpg_strdup("{", lineno, &alloc_failed);
else
- mallocedval = ecpg_strdup("", lineno);
+ mallocedval = ecpg_strdup("", lineno, &alloc_failed);
- if (!mallocedval)
+ if (alloc_failed)
return false;
for (element = 0; element < asize; element++)
@@ -1952,6 +1953,7 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator,
struct variable **list;
char *prepname;
bool is_prepared_name_set;
+ bool alloc_failed = false;
*stmt_out = NULL;
@@ -2001,7 +2003,7 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator,
return false;
}
#endif
- stmt->oldlocale = ecpg_strdup(setlocale(LC_NUMERIC, NULL), lineno);
+ stmt->oldlocale = ecpg_strdup(setlocale(LC_NUMERIC, NULL), lineno, &alloc_failed);
if (stmt->oldlocale == NULL)
{
ecpg_do_epilogue(stmt);
@@ -2031,8 +2033,8 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator,
}
else
{
- stmt->command = ecpg_strdup(query, lineno);
- if (!stmt->command)
+ stmt->command = ecpg_strdup(query, lineno, &alloc_failed);
+ if (alloc_failed)
{
ecpg_do_epilogue(stmt);
return false;
@@ -2049,8 +2051,8 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator,
if (command)
{
stmt->name = stmt->command;
- stmt->command = ecpg_strdup(command, lineno);
- if (!stmt->command)
+ stmt->command = ecpg_strdup(command, lineno, &alloc_failed);
+ if (alloc_failed)
{
ecpg_do_epilogue(stmt);
return false;
@@ -2187,7 +2189,7 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator,
if (!is_prepared_name_set && stmt->statement_type == ECPGst_prepare)
{
- stmt->name = ecpg_strdup(var->value, lineno);
+ stmt->name = ecpg_strdup(var->value, lineno, &alloc_failed);
if (!stmt->name)
{
ecpg_do_epilogue(stmt);
diff --git a/src/interfaces/ecpg/ecpglib/memory.c b/src/interfaces/ecpg/ecpglib/memory.c
index 6979be2c988..ee71dd72f52 100644
--- a/src/interfaces/ecpg/ecpglib/memory.c
+++ b/src/interfaces/ecpg/ecpglib/memory.c
@@ -44,7 +44,7 @@ ecpg_realloc(void *ptr, long size, int lineno)
}
char *
-ecpg_strdup(const char *string, int lineno)
+ecpg_strdup(const char *string, int lineno, bool *alloc_failed)
{
char *new;
@@ -54,6 +54,7 @@ ecpg_strdup(const char *string, int lineno)
new = strdup(string);
if (!new)
{
+ *alloc_failed = true;
ecpg_raise(lineno, ECPG_OUT_OF_MEMORY, ECPG_SQLSTATE_ECPG_OUT_OF_MEMORY, NULL);
return NULL;
}
diff --git a/src/interfaces/ecpg/ecpglib/prepare.c b/src/interfaces/ecpg/ecpglib/prepare.c
index 7b6019deed7..fb281fb6594 100644
--- a/src/interfaces/ecpg/ecpglib/prepare.c
+++ b/src/interfaces/ecpg/ecpglib/prepare.c
@@ -63,6 +63,7 @@ ecpg_register_prepared_stmt(struct statement *stmt)
struct connection *con = stmt->connection;
struct prepared_statement *prev = NULL;
int lineno = stmt->lineno;
+ bool alloc_failed = false;
/* check if we already have prepared this statement */
this = ecpg_find_prepared_statement(stmt->name, con, &prev);
@@ -85,7 +86,7 @@ ecpg_register_prepared_stmt(struct statement *stmt)
/* create statement */
prep_stmt->lineno = lineno;
prep_stmt->connection = con;
- prep_stmt->command = ecpg_strdup(stmt->command, lineno);
+ prep_stmt->command = ecpg_strdup(stmt->command, lineno, &alloc_failed);
if (!prep_stmt->command)
{
ecpg_free(prep_stmt);
@@ -93,7 +94,7 @@ ecpg_register_prepared_stmt(struct statement *stmt)
return false;
}
prep_stmt->inlist = prep_stmt->outlist = NULL;
- this->name = ecpg_strdup(stmt->name, lineno);
+ this->name = ecpg_strdup(stmt->name, lineno, &alloc_failed);
if (!this->name)
{
ecpg_free(prep_stmt->command);
@@ -174,6 +175,7 @@ prepare_common(int lineno, struct connection *con, const char *name, const char
struct statement *stmt;
struct prepared_statement *this;
PGresult *query;
+ bool alloc_failed = false;
/* allocate new statement */
this = (struct prepared_statement *) ecpg_alloc(sizeof(struct prepared_statement), lineno);
@@ -190,7 +192,7 @@ prepare_common(int lineno, struct connection *con, const char *name, const char
/* create statement */
stmt->lineno = lineno;
stmt->connection = con;
- stmt->command = ecpg_strdup(variable, lineno);
+ stmt->command = ecpg_strdup(variable, lineno, &alloc_failed);
if (!stmt->command)
{
ecpg_free(stmt);
@@ -203,7 +205,7 @@ prepare_common(int lineno, struct connection *con, const char *name, const char
replace_variables(&(stmt->command), lineno);
/* add prepared statement to our list */
- this->name = ecpg_strdup(name, lineno);
+ this->name = ecpg_strdup(name, lineno, &alloc_failed);
if (!this->name)
{
ecpg_free(stmt->command);
@@ -525,6 +527,7 @@ AddStmtToCache(int lineno, /* line # of statement */
luEntNo,
entNo;
stmtCacheEntry *entry;
+ bool alloc_failed = false;
/* allocate and zero cache array if we haven't already */
if (stmtCacheEntries == NULL)
@@ -566,8 +569,8 @@ AddStmtToCache(int lineno, /* line # of statement */
/* add the query to the entry */
entry = &stmtCacheEntries[entNo];
entry->lineno = lineno;
- entry->ecpgQuery = ecpg_strdup(ecpgQuery, lineno);
- if (!entry->ecpgQuery)
+ entry->ecpgQuery = ecpg_strdup(ecpgQuery, lineno, &alloc_failed);
+ if (alloc_failed)
return -1;
entry->connection = connection;
entry->execs = 0;
@@ -581,6 +584,7 @@ bool
ecpg_auto_prepare(int lineno, const char *connection_name, const int compat, char **name, const char *query)
{
int entNo;
+ bool alloc_failed = false;
/* search the statement cache for this statement */
entNo = SearchStmtCache(query);
@@ -602,7 +606,7 @@ ecpg_auto_prepare(int lineno, const char *connection_name, const int compat, cha
if (!prep && !prepare_common(lineno, con, stmtID, query))
return false;
- *name = ecpg_strdup(stmtID, lineno);
+ *name = ecpg_strdup(stmtID, lineno, &alloc_failed);
}
else
{
@@ -620,10 +624,10 @@ ecpg_auto_prepare(int lineno, const char *connection_name, const int compat, cha
if (entNo < 0)
return false;
- *name = ecpg_strdup(stmtID, lineno);
+ *name = ecpg_strdup(stmtID, lineno, &alloc_failed);
}
- if (!*name)
+ if (alloc_failed)
return false;
/* increase usage counter */
--
2.43.0
v6-0001-Add-proper-checks-for-ecpg_strdup-return-value.patchtext/x-patch; charset=UTF-8; name=v6-0001-Add-proper-checks-for-ecpg_strdup-return-value.patchDownload
From ca47bb6e04dfc6fd5cfee534e6aeb4abdcc3ea2e Mon Sep 17 00:00:00 2001
From: Aleksander Alekseev <aleksander@timescale.com>
Date: Fri, 11 Jul 2025 17:59:50 +0300
Subject: [PATCH v6 1/2] Add proper checks for ecpg_strdup() return value
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Author: Evgeniy Gorbanev <gorbanyoves@basealt.ru>
Author: Aleksander Alekseev <aleksander@tigerdata.com>
Reviewed-by: Álvaro Herrera <alvherre@kurilemu.de>
Discussion: https://postgr.es/m/a6b193c1-6994-4d9c-9059-aca4aaf41ddd@basealt.ru
---
src/interfaces/ecpg/ecpglib/connect.c | 29 +++++++++++++++++++------
src/interfaces/ecpg/ecpglib/execute.c | 17 +++++++++++++++
src/interfaces/ecpg/ecpglib/prepare.c | 31 +++++++++++++++++++++++++++
3 files changed, 70 insertions(+), 7 deletions(-)
diff --git a/src/interfaces/ecpg/ecpglib/connect.c b/src/interfaces/ecpg/ecpglib/connect.c
index 2bbb70333dc..af25582abbd 100644
--- a/src/interfaces/ecpg/ecpglib/connect.c
+++ b/src/interfaces/ecpg/ecpglib/connect.c
@@ -58,7 +58,8 @@ ecpg_get_connection_nr(const char *connection_name)
for (con = all_connections; con != NULL; con = con->next)
{
- if (strcmp(connection_name, con->name) == 0)
+ /* Check for NULL to prevent segfault */
+ if (con->name != NULL && strcmp(connection_name, con->name) == 0)
break;
}
ret = con;
@@ -267,6 +268,7 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
*options = NULL;
const char **conn_keywords;
const char **conn_values;
+ bool strdup_failed;
if (sqlca == NULL)
{
@@ -460,7 +462,21 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
*/
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)
+
+ /* Decide on a connection name */
+ strdup_failed = false;
+ if (connection_name != NULL || realname != NULL)
+ {
+ this->name = ecpg_strdup(connection_name ? connection_name : realname,
+ lineno);
+ if (this->name == NULL)
+ strdup_failed = true;
+ }
+ else
+ this->name = NULL;
+
+ /* Deal with any failed allocations above */
+ if (conn_keywords == NULL || conn_values == NULL || strdup_failed)
{
if (host)
ecpg_free(host);
@@ -476,6 +492,8 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
ecpg_free(conn_keywords);
if (conn_values)
ecpg_free(conn_values);
+ if (this->name)
+ ecpg_free(this->name);
free(this);
return false;
}
@@ -510,17 +528,14 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
ecpg_free(conn_keywords);
if (conn_values)
ecpg_free(conn_values);
+ if (this->name)
+ ecpg_free(this->name);
free(this);
return false;
}
}
#endif
- if (connection_name != NULL)
- this->name = ecpg_strdup(connection_name, lineno);
- else
- this->name = ecpg_strdup(realname, lineno);
-
this->cache_head = NULL;
this->prep_stmts = NULL;
diff --git a/src/interfaces/ecpg/ecpglib/execute.c b/src/interfaces/ecpg/ecpglib/execute.c
index f52da06de9a..6524c646c13 100644
--- a/src/interfaces/ecpg/ecpglib/execute.c
+++ b/src/interfaces/ecpg/ecpglib/execute.c
@@ -2030,7 +2030,14 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator,
statement_type = ECPGst_execute;
}
else
+ {
stmt->command = ecpg_strdup(query, lineno);
+ if (!stmt->command)
+ {
+ ecpg_do_epilogue(stmt);
+ return false;
+ }
+ }
stmt->name = NULL;
@@ -2043,6 +2050,11 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator,
{
stmt->name = stmt->command;
stmt->command = ecpg_strdup(command, lineno);
+ if (!stmt->command)
+ {
+ ecpg_do_epilogue(stmt);
+ return false;
+ }
}
else
{
@@ -2176,6 +2188,11 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator,
if (!is_prepared_name_set && stmt->statement_type == ECPGst_prepare)
{
stmt->name = ecpg_strdup(var->value, lineno);
+ if (!stmt->name)
+ {
+ ecpg_do_epilogue(stmt);
+ return false;
+ }
is_prepared_name_set = true;
}
}
diff --git a/src/interfaces/ecpg/ecpglib/prepare.c b/src/interfaces/ecpg/ecpglib/prepare.c
index ea1146f520f..7b6019deed7 100644
--- a/src/interfaces/ecpg/ecpglib/prepare.c
+++ b/src/interfaces/ecpg/ecpglib/prepare.c
@@ -86,8 +86,21 @@ ecpg_register_prepared_stmt(struct statement *stmt)
prep_stmt->lineno = lineno;
prep_stmt->connection = con;
prep_stmt->command = ecpg_strdup(stmt->command, lineno);
+ if (!prep_stmt->command)
+ {
+ ecpg_free(prep_stmt);
+ ecpg_free(this);
+ return false;
+ }
prep_stmt->inlist = prep_stmt->outlist = NULL;
this->name = ecpg_strdup(stmt->name, lineno);
+ if (!this->name)
+ {
+ ecpg_free(prep_stmt->command);
+ ecpg_free(prep_stmt);
+ ecpg_free(this);
+ return false;
+ }
this->stmt = prep_stmt;
this->prepared = true;
@@ -178,6 +191,12 @@ prepare_common(int lineno, struct connection *con, const char *name, const char
stmt->lineno = lineno;
stmt->connection = con;
stmt->command = ecpg_strdup(variable, lineno);
+ if (!stmt->command)
+ {
+ ecpg_free(stmt);
+ ecpg_free(this);
+ return false;
+ }
stmt->inlist = stmt->outlist = NULL;
/* if we have C variables in our statement replace them with '?' */
@@ -185,6 +204,13 @@ prepare_common(int lineno, struct connection *con, const char *name, const char
/* add prepared statement to our list */
this->name = ecpg_strdup(name, lineno);
+ if (!this->name)
+ {
+ ecpg_free(stmt->command);
+ ecpg_free(stmt);
+ ecpg_free(this);
+ return false;
+ }
this->stmt = stmt;
/* and finally really prepare the statement */
@@ -541,6 +567,8 @@ AddStmtToCache(int lineno, /* line # of statement */
entry = &stmtCacheEntries[entNo];
entry->lineno = lineno;
entry->ecpgQuery = ecpg_strdup(ecpgQuery, lineno);
+ if (!entry->ecpgQuery)
+ return -1;
entry->connection = connection;
entry->execs = 0;
memcpy(entry->stmtID, stmtID, sizeof(entry->stmtID));
@@ -595,6 +623,9 @@ ecpg_auto_prepare(int lineno, const char *connection_name, const int compat, cha
*name = ecpg_strdup(stmtID, lineno);
}
+ if (!*name)
+ return false;
+
/* increase usage counter */
stmtCacheEntries[entNo].execs++;
--
2.43.0
Re: Missing NULL check after calling ecpg_strdup
On Mon, Jul 21, 2025 at 11:48:27AM +0300, Aleksander Alekseev wrote:
+ /* Check for NULL to prevent segfault */ + if (con->name != NULL && strcmp(connection_name, con->name) == 0) break;
I have spent some time rechecking the whole code, and I have
backpatched this part. One pattern where I've easily been able to
trigger the problem is by specifying "\0" as the database name. The
preprocessor rejects empty strings, but it fails to reject the case of
a CONNECT TO "\0". Perhaps it should actually, found that funny..
Anyway, it does not change the fact that if we do a named connection
lookup in a stack where at least one NULL connection is set, we would
crash during the lookup.
I have also hacked on the rest, spotted a couple of bugs and
inconsistencies.
- stmt.oldlocale = ecpg_strdup(setlocale(LC_NUMERIC, NULL), lineno);
+ stmt.oldlocale = ecpg_strdup(setlocale(LC_NUMERIC, NULL), lineno, &alloc_failed);
+ if (alloc_failed)
+ return false;Hmm.. Aren't you missing a va_end(args) in the early exit you are
adding here?
In ecpg_store_input() and ecpg_do_prologue(), we can do without any
alloc_failed. All the inputs can never be NULL.
I think that the order of the operations in ecpg_auto_prepare() is
incorrect: we would add statements to the cache even if the strdup()
fails. We have only one caller of ecpg_auto_prepare(), where the
*name we set does not matter.
At the end, I finish with the attached, where alloc_failed matters for
the failure checks with repeated calls of strdup() in ECPGconnect()
and also the setlocale() case. This is for HEAD due to how unlikely
these issues would occur in practice.
--
Michael
Attachments:
v7-0001-ecpg-Improve-error-detection-of-ecpg_strdup.patchtext/x-diff; charset=iso-8859-1Download
From a848f832e772fb9b66baf9e48765d6912c14103b Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Tue, 22 Jul 2025 16:03:00 +0900
Subject: [PATCH v7] ecpg: Improve error detection of ecpg_strdup()
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
This routine gains a new argument, giving its callers the possibility to
differentiate allocation failures vs valid cases where the caller is
giving a NULL value in input. The rest of the code is adapted to that.
Author: Evgeniy Gorbanev <gorbanyoves@basealt.ru>
Co-authored-by: Aleksander Alekseev <aleksander@tigerdata.com>
Reviewed-by: Álvaro Herrera <alvherre@kurilemu.de>
Reviewed-by: Me
Discussion: https://postgr.es/m/a6b193c1-6994-4d9c-9059-aca4aaf41ddd@basealt.ru
---
src/interfaces/ecpg/ecpglib/connect.c | 43 +++++++++++-------
src/interfaces/ecpg/ecpglib/descriptor.c | 10 ++++-
src/interfaces/ecpg/ecpglib/ecpglib_extern.h | 2 +-
src/interfaces/ecpg/ecpglib/execute.c | 42 ++++++++++++-----
src/interfaces/ecpg/ecpglib/memory.c | 11 ++++-
src/interfaces/ecpg/ecpglib/prepare.c | 47 ++++++++++++++++----
6 files changed, 116 insertions(+), 39 deletions(-)
diff --git a/src/interfaces/ecpg/ecpglib/connect.c b/src/interfaces/ecpg/ecpglib/connect.c
index 713cbbf6360b..1cf1ac2cf8e6 100644
--- a/src/interfaces/ecpg/ecpglib/connect.c
+++ b/src/interfaces/ecpg/ecpglib/connect.c
@@ -264,7 +264,8 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
struct connection *this;
int i,
connect_params = 0;
- char *dbname = name ? ecpg_strdup(name, lineno) : NULL,
+ bool alloc_failed = (sqlca == NULL);
+ char *dbname = name ? ecpg_strdup(name, lineno, &alloc_failed) : NULL,
*host = NULL,
*tmp,
*port = NULL,
@@ -273,7 +274,7 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
const char **conn_keywords;
const char **conn_values;
- if (sqlca == NULL)
+ if (alloc_failed)
{
ecpg_raise(lineno, ECPG_OUT_OF_MEMORY,
ECPG_SQLSTATE_ECPG_OUT_OF_MEMORY, NULL);
@@ -302,7 +303,7 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
if (envname)
{
ecpg_free(dbname);
- dbname = ecpg_strdup(envname, lineno);
+ dbname = ecpg_strdup(envname, lineno, &alloc_failed);
}
}
@@ -354,7 +355,7 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
tmp = strrchr(dbname + offset, '?');
if (tmp != NULL) /* options given */
{
- options = ecpg_strdup(tmp + 1, lineno);
+ options = ecpg_strdup(tmp + 1, lineno, &alloc_failed);
*tmp = '\0';
}
@@ -363,7 +364,7 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
{
if (tmp[1] != '\0') /* non-empty database name */
{
- realname = ecpg_strdup(tmp + 1, lineno);
+ realname = ecpg_strdup(tmp + 1, lineno, &alloc_failed);
connect_params++;
}
*tmp = '\0';
@@ -373,7 +374,7 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
if (tmp != NULL) /* port number given */
{
*tmp = '\0';
- port = ecpg_strdup(tmp + 1, lineno);
+ port = ecpg_strdup(tmp + 1, lineno, &alloc_failed);
connect_params++;
}
@@ -407,7 +408,7 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
{
if (*(dbname + offset) != '\0')
{
- host = ecpg_strdup(dbname + offset, lineno);
+ host = ecpg_strdup(dbname + offset, lineno, &alloc_failed);
connect_params++;
}
}
@@ -419,7 +420,7 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
tmp = strrchr(dbname, ':');
if (tmp != NULL) /* port number given */
{
- port = ecpg_strdup(tmp + 1, lineno);
+ port = ecpg_strdup(tmp + 1, lineno, &alloc_failed);
connect_params++;
*tmp = '\0';
}
@@ -427,14 +428,14 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
tmp = strrchr(dbname, '@');
if (tmp != NULL) /* host name given */
{
- host = ecpg_strdup(tmp + 1, lineno);
+ host = ecpg_strdup(tmp + 1, lineno, &alloc_failed);
connect_params++;
*tmp = '\0';
}
if (strlen(dbname) > 0)
{
- realname = ecpg_strdup(dbname, lineno);
+ realname = ecpg_strdup(dbname, lineno, &alloc_failed);
connect_params++;
}
else
@@ -465,7 +466,18 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
*/
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)
+
+ /* Decide on a connection name */
+ if (connection_name != NULL || realname != NULL)
+ {
+ this->name = ecpg_strdup(connection_name ? connection_name : realname,
+ lineno, &alloc_failed);
+ }
+ else
+ this->name = NULL;
+
+ /* Deal with any failed allocations above */
+ if (conn_keywords == NULL || conn_values == NULL || alloc_failed)
{
if (host)
ecpg_free(host);
@@ -481,6 +493,8 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
ecpg_free(conn_keywords);
if (conn_values)
ecpg_free(conn_values);
+ if (this->name)
+ ecpg_free(this->name);
free(this);
return false;
}
@@ -515,17 +529,14 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
ecpg_free(conn_keywords);
if (conn_values)
ecpg_free(conn_values);
+ if (this->name)
+ ecpg_free(this->name);
free(this);
return false;
}
}
#endif
- if (connection_name != NULL)
- this->name = ecpg_strdup(connection_name, lineno);
- else
- this->name = ecpg_strdup(realname, lineno);
-
this->cache_head = NULL;
this->prep_stmts = NULL;
diff --git a/src/interfaces/ecpg/ecpglib/descriptor.c b/src/interfaces/ecpg/ecpglib/descriptor.c
index 651d5c8b2ed3..4c60fa0d7fb0 100644
--- a/src/interfaces/ecpg/ecpglib/descriptor.c
+++ b/src/interfaces/ecpg/ecpglib/descriptor.c
@@ -240,6 +240,7 @@ ECPGget_desc(int lineno, const char *desc_name, int index,...)
act_tuple;
struct variable data_var;
struct sqlca_t *sqlca = ECPGget_sqlca();
+ bool alloc_failed = false;
if (sqlca == NULL)
{
@@ -493,7 +494,14 @@ ECPGget_desc(int lineno, const char *desc_name, int index,...)
#ifdef WIN32
stmt.oldthreadlocale = _configthreadlocale(_ENABLE_PER_THREAD_LOCALE);
#endif
- stmt.oldlocale = ecpg_strdup(setlocale(LC_NUMERIC, NULL), lineno);
+ stmt.oldlocale = ecpg_strdup(setlocale(LC_NUMERIC, NULL),
+ lineno, &alloc_failed);
+ if (alloc_failed)
+ {
+ va_end(args);
+ return false;
+ }
+
setlocale(LC_NUMERIC, "C");
#endif
diff --git a/src/interfaces/ecpg/ecpglib/ecpglib_extern.h b/src/interfaces/ecpg/ecpglib/ecpglib_extern.h
index 75cc68275bda..949ff66cefc9 100644
--- a/src/interfaces/ecpg/ecpglib/ecpglib_extern.h
+++ b/src/interfaces/ecpg/ecpglib/ecpglib_extern.h
@@ -175,7 +175,7 @@ void ecpg_free(void *ptr);
bool ecpg_init(const struct connection *con,
const char *connection_name,
const int lineno);
-char *ecpg_strdup(const char *string, int lineno);
+char *ecpg_strdup(const char *string, int lineno, bool *alloc_failed);
const char *ecpg_type_name(enum ECPGttype typ);
int ecpg_dynamic_type(Oid type);
int sqlda_dynamic_type(Oid type, enum COMPAT_MODE compat);
diff --git a/src/interfaces/ecpg/ecpglib/execute.c b/src/interfaces/ecpg/ecpglib/execute.c
index f52da06de9a1..84a4a9fc5781 100644
--- a/src/interfaces/ecpg/ecpglib/execute.c
+++ b/src/interfaces/ecpg/ecpglib/execute.c
@@ -860,9 +860,9 @@ ecpg_store_input(const int lineno, const bool force_indicator, const struct vari
numeric *nval;
if (var->arrsize > 1)
- mallocedval = ecpg_strdup("{", lineno);
+ mallocedval = ecpg_strdup("{", lineno, NULL);
else
- mallocedval = ecpg_strdup("", lineno);
+ mallocedval = ecpg_strdup("", lineno, NULL);
if (!mallocedval)
return false;
@@ -923,9 +923,9 @@ ecpg_store_input(const int lineno, const bool force_indicator, const struct vari
int slen;
if (var->arrsize > 1)
- mallocedval = ecpg_strdup("{", lineno);
+ mallocedval = ecpg_strdup("{", lineno, NULL);
else
- mallocedval = ecpg_strdup("", lineno);
+ mallocedval = ecpg_strdup("", lineno, NULL);
if (!mallocedval)
return false;
@@ -970,9 +970,9 @@ ecpg_store_input(const int lineno, const bool force_indicator, const struct vari
int slen;
if (var->arrsize > 1)
- mallocedval = ecpg_strdup("{", lineno);
+ mallocedval = ecpg_strdup("{", lineno, NULL);
else
- mallocedval = ecpg_strdup("", lineno);
+ mallocedval = ecpg_strdup("", lineno, NULL);
if (!mallocedval)
return false;
@@ -1017,9 +1017,9 @@ ecpg_store_input(const int lineno, const bool force_indicator, const struct vari
int slen;
if (var->arrsize > 1)
- mallocedval = ecpg_strdup("{", lineno);
+ mallocedval = ecpg_strdup("{", lineno, NULL);
else
- mallocedval = ecpg_strdup("", lineno);
+ mallocedval = ecpg_strdup("", lineno, NULL);
if (!mallocedval)
return false;
@@ -2001,7 +2001,8 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator,
return false;
}
#endif
- stmt->oldlocale = ecpg_strdup(setlocale(LC_NUMERIC, NULL), lineno);
+ stmt->oldlocale = ecpg_strdup(setlocale(LC_NUMERIC, NULL), lineno,
+ NULL);
if (stmt->oldlocale == NULL)
{
ecpg_do_epilogue(stmt);
@@ -2030,7 +2031,14 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator,
statement_type = ECPGst_execute;
}
else
- stmt->command = ecpg_strdup(query, lineno);
+ {
+ stmt->command = ecpg_strdup(query, lineno, NULL);
+ if (!stmt->command)
+ {
+ ecpg_do_epilogue(stmt);
+ return false;
+ }
+ }
stmt->name = NULL;
@@ -2042,7 +2050,12 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator,
if (command)
{
stmt->name = stmt->command;
- stmt->command = ecpg_strdup(command, lineno);
+ stmt->command = ecpg_strdup(command, lineno, NULL);
+ if (!stmt->command)
+ {
+ ecpg_do_epilogue(stmt);
+ return false;
+ }
}
else
{
@@ -2175,7 +2188,12 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator,
if (!is_prepared_name_set && stmt->statement_type == ECPGst_prepare)
{
- stmt->name = ecpg_strdup(var->value, lineno);
+ stmt->name = ecpg_strdup(var->value, lineno, NULL);
+ if (!stmt->name)
+ {
+ ecpg_do_epilogue(stmt);
+ return false;
+ }
is_prepared_name_set = true;
}
}
diff --git a/src/interfaces/ecpg/ecpglib/memory.c b/src/interfaces/ecpg/ecpglib/memory.c
index 6979be2c988a..2112e55b6e42 100644
--- a/src/interfaces/ecpg/ecpglib/memory.c
+++ b/src/interfaces/ecpg/ecpglib/memory.c
@@ -43,8 +43,15 @@ ecpg_realloc(void *ptr, long size, int lineno)
return new;
}
+/*
+ * Wrapper for strdup(), with NULL in input treated as a correct case.
+ *
+ * "alloc_failed" can be optionally specified by the caller to check for
+ * allocation failures. The caller is responsible for its initialization,
+ * as ecpg_strdup() may be called repeatedly across multiple allocations.
+ */
char *
-ecpg_strdup(const char *string, int lineno)
+ecpg_strdup(const char *string, int lineno, bool *alloc_failed)
{
char *new;
@@ -54,6 +61,8 @@ ecpg_strdup(const char *string, int lineno)
new = strdup(string);
if (!new)
{
+ if (alloc_failed)
+ *alloc_failed = true;
ecpg_raise(lineno, ECPG_OUT_OF_MEMORY, ECPG_SQLSTATE_ECPG_OUT_OF_MEMORY, NULL);
return NULL;
}
diff --git a/src/interfaces/ecpg/ecpglib/prepare.c b/src/interfaces/ecpg/ecpglib/prepare.c
index ea1146f520f3..dd6fd1fe7f40 100644
--- a/src/interfaces/ecpg/ecpglib/prepare.c
+++ b/src/interfaces/ecpg/ecpglib/prepare.c
@@ -85,9 +85,22 @@ ecpg_register_prepared_stmt(struct statement *stmt)
/* create statement */
prep_stmt->lineno = lineno;
prep_stmt->connection = con;
- prep_stmt->command = ecpg_strdup(stmt->command, lineno);
+ prep_stmt->command = ecpg_strdup(stmt->command, lineno, NULL);
+ if (!prep_stmt->command)
+ {
+ ecpg_free(prep_stmt);
+ ecpg_free(this);
+ return false;
+ }
prep_stmt->inlist = prep_stmt->outlist = NULL;
- this->name = ecpg_strdup(stmt->name, lineno);
+ this->name = ecpg_strdup(stmt->name, lineno, NULL);
+ if (!this->name)
+ {
+ ecpg_free(prep_stmt->command);
+ ecpg_free(prep_stmt);
+ ecpg_free(this);
+ return false;
+ }
this->stmt = prep_stmt;
this->prepared = true;
@@ -177,14 +190,27 @@ prepare_common(int lineno, struct connection *con, const char *name, const char
/* create statement */
stmt->lineno = lineno;
stmt->connection = con;
- stmt->command = ecpg_strdup(variable, lineno);
+ stmt->command = ecpg_strdup(variable, lineno, NULL);
+ if (!stmt->command)
+ {
+ ecpg_free(stmt);
+ ecpg_free(this);
+ return false;
+ }
stmt->inlist = stmt->outlist = NULL;
/* if we have C variables in our statement replace them with '?' */
replace_variables(&(stmt->command), lineno);
/* add prepared statement to our list */
- this->name = ecpg_strdup(name, lineno);
+ this->name = ecpg_strdup(name, lineno, NULL);
+ if (!this->name)
+ {
+ ecpg_free(stmt->command);
+ ecpg_free(stmt);
+ ecpg_free(this);
+ return false;
+ }
this->stmt = stmt;
/* and finally really prepare the statement */
@@ -540,7 +566,9 @@ AddStmtToCache(int lineno, /* line # of statement */
/* add the query to the entry */
entry = &stmtCacheEntries[entNo];
entry->lineno = lineno;
- entry->ecpgQuery = ecpg_strdup(ecpgQuery, lineno);
+ entry->ecpgQuery = ecpg_strdup(ecpgQuery, lineno, NULL);
+ if (!entry->ecpgQuery)
+ return -1;
entry->connection = connection;
entry->execs = 0;
memcpy(entry->stmtID, stmtID, sizeof(entry->stmtID));
@@ -567,6 +595,9 @@ ecpg_auto_prepare(int lineno, const char *connection_name, const int compat, cha
ecpg_log("ecpg_auto_prepare on line %d: statement found in cache; entry %d\n", lineno, entNo);
stmtID = stmtCacheEntries[entNo].stmtID;
+ *name = ecpg_strdup(stmtID, lineno, NULL);
+ if (*name == NULL)
+ return false;
con = ecpg_get_connection(connection_name);
prep = ecpg_find_prepared_statement(stmtID, con, NULL);
@@ -574,7 +605,6 @@ ecpg_auto_prepare(int lineno, const char *connection_name, const int compat, cha
if (!prep && !prepare_common(lineno, con, stmtID, query))
return false;
- *name = ecpg_strdup(stmtID, lineno);
}
else
{
@@ -584,6 +614,9 @@ ecpg_auto_prepare(int lineno, const char *connection_name, const int compat, cha
/* generate a statement ID */
sprintf(stmtID, "ecpg%d", nextStmtID++);
+ *name = ecpg_strdup(stmtID, lineno, NULL);
+ if (*name == NULL)
+ return false;
if (!ECPGprepare(lineno, connection_name, 0, stmtID, query))
return false;
@@ -591,8 +624,6 @@ ecpg_auto_prepare(int lineno, const char *connection_name, const int compat, cha
entNo = AddStmtToCache(lineno, stmtID, connection_name, compat, query);
if (entNo < 0)
return false;
-
- *name = ecpg_strdup(stmtID, lineno);
}
/* increase usage counter */
--
2.50.0
Re: Missing NULL check after calling ecpg_strdup
Hi,
I have spent some time rechecking the whole code, and I have
backpatched this part. [...]
Many thanks!
Hmm.. Aren't you missing a va_end(args) in the early exit you are
adding here?
I do, and it's rather stupid of me. Thanks.
[...]
At the end, I finish with the attached, where alloc_failed matters for
the failure checks with repeated calls of strdup() in ECPGconnect()
and also the setlocale() case. This is for HEAD due to how unlikely
these issues would occur in practice.
v7 may have a compilation warning on Linux:
```
warning: unused variable ‘alloc_failed’ [-Wunused-variable]
```
... because the only use of the variable is hidden under #ifdef's.
Fixed in v8:
```
--- a/src/interfaces/ecpg/ecpglib/descriptor.c
+++ b/src/interfaces/ecpg/ecpglib/descriptor.c
@@ -240,9 +240,9 @@ ECPGget_desc(int lineno, const char *desc_name,
int index,...)
act_tuple;
struct variable data_var;
struct sqlca_t *sqlca = ECPGget_sqlca();
- bool alloc_failed = false;
+ bool alloc_failed = (sqlca == NULL);- if (sqlca == NULL)
+ if (alloc_failed)
{
ecpg_raise(lineno, ECPG_OUT_OF_MEMORY,
ECPG_SQLSTATE_ECPG_OUT_OF_MEMORY, NULL);
```
This code is also more consistent with what we ended up having in connect.c.
Other than that the patch looks OK to me.
Attachments:
v8-0001-ecpg-Improve-error-detection-of-ecpg_strdup.patchtext/x-patch; charset=UTF-8; name=v8-0001-ecpg-Improve-error-detection-of-ecpg_strdup.patchDownload
From 47861dd766aca4abcbabd1805d073ae1167dbcb7 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Tue, 22 Jul 2025 16:03:00 +0900
Subject: [PATCH v8] ecpg: Improve error detection of ecpg_strdup()
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
This routine gains a new argument, giving its callers the possibility to
differentiate allocation failures vs valid cases where the caller is
giving a NULL value in input. The rest of the code is adapted to that.
Author: Evgeniy Gorbanev <gorbanyoves@basealt.ru>
Co-authored-by: Aleksander Alekseev <aleksander@tigerdata.com>
Reviewed-by: Álvaro Herrera <alvherre@kurilemu.de>
Reviewed-by: Me
Discussion: https://postgr.es/m/a6b193c1-6994-4d9c-9059-aca4aaf41ddd@basealt.ru
---
src/interfaces/ecpg/ecpglib/connect.c | 43 +++++++++++-------
src/interfaces/ecpg/ecpglib/descriptor.c | 12 ++++-
src/interfaces/ecpg/ecpglib/ecpglib_extern.h | 2 +-
src/interfaces/ecpg/ecpglib/execute.c | 42 ++++++++++++-----
src/interfaces/ecpg/ecpglib/memory.c | 11 ++++-
src/interfaces/ecpg/ecpglib/prepare.c | 47 ++++++++++++++++----
6 files changed, 117 insertions(+), 40 deletions(-)
diff --git a/src/interfaces/ecpg/ecpglib/connect.c b/src/interfaces/ecpg/ecpglib/connect.c
index 713cbbf6360..1cf1ac2cf8e 100644
--- a/src/interfaces/ecpg/ecpglib/connect.c
+++ b/src/interfaces/ecpg/ecpglib/connect.c
@@ -264,7 +264,8 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
struct connection *this;
int i,
connect_params = 0;
- char *dbname = name ? ecpg_strdup(name, lineno) : NULL,
+ bool alloc_failed = (sqlca == NULL);
+ char *dbname = name ? ecpg_strdup(name, lineno, &alloc_failed) : NULL,
*host = NULL,
*tmp,
*port = NULL,
@@ -273,7 +274,7 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
const char **conn_keywords;
const char **conn_values;
- if (sqlca == NULL)
+ if (alloc_failed)
{
ecpg_raise(lineno, ECPG_OUT_OF_MEMORY,
ECPG_SQLSTATE_ECPG_OUT_OF_MEMORY, NULL);
@@ -302,7 +303,7 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
if (envname)
{
ecpg_free(dbname);
- dbname = ecpg_strdup(envname, lineno);
+ dbname = ecpg_strdup(envname, lineno, &alloc_failed);
}
}
@@ -354,7 +355,7 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
tmp = strrchr(dbname + offset, '?');
if (tmp != NULL) /* options given */
{
- options = ecpg_strdup(tmp + 1, lineno);
+ options = ecpg_strdup(tmp + 1, lineno, &alloc_failed);
*tmp = '\0';
}
@@ -363,7 +364,7 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
{
if (tmp[1] != '\0') /* non-empty database name */
{
- realname = ecpg_strdup(tmp + 1, lineno);
+ realname = ecpg_strdup(tmp + 1, lineno, &alloc_failed);
connect_params++;
}
*tmp = '\0';
@@ -373,7 +374,7 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
if (tmp != NULL) /* port number given */
{
*tmp = '\0';
- port = ecpg_strdup(tmp + 1, lineno);
+ port = ecpg_strdup(tmp + 1, lineno, &alloc_failed);
connect_params++;
}
@@ -407,7 +408,7 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
{
if (*(dbname + offset) != '\0')
{
- host = ecpg_strdup(dbname + offset, lineno);
+ host = ecpg_strdup(dbname + offset, lineno, &alloc_failed);
connect_params++;
}
}
@@ -419,7 +420,7 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
tmp = strrchr(dbname, ':');
if (tmp != NULL) /* port number given */
{
- port = ecpg_strdup(tmp + 1, lineno);
+ port = ecpg_strdup(tmp + 1, lineno, &alloc_failed);
connect_params++;
*tmp = '\0';
}
@@ -427,14 +428,14 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
tmp = strrchr(dbname, '@');
if (tmp != NULL) /* host name given */
{
- host = ecpg_strdup(tmp + 1, lineno);
+ host = ecpg_strdup(tmp + 1, lineno, &alloc_failed);
connect_params++;
*tmp = '\0';
}
if (strlen(dbname) > 0)
{
- realname = ecpg_strdup(dbname, lineno);
+ realname = ecpg_strdup(dbname, lineno, &alloc_failed);
connect_params++;
}
else
@@ -465,7 +466,18 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
*/
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)
+
+ /* Decide on a connection name */
+ if (connection_name != NULL || realname != NULL)
+ {
+ this->name = ecpg_strdup(connection_name ? connection_name : realname,
+ lineno, &alloc_failed);
+ }
+ else
+ this->name = NULL;
+
+ /* Deal with any failed allocations above */
+ if (conn_keywords == NULL || conn_values == NULL || alloc_failed)
{
if (host)
ecpg_free(host);
@@ -481,6 +493,8 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
ecpg_free(conn_keywords);
if (conn_values)
ecpg_free(conn_values);
+ if (this->name)
+ ecpg_free(this->name);
free(this);
return false;
}
@@ -515,17 +529,14 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
ecpg_free(conn_keywords);
if (conn_values)
ecpg_free(conn_values);
+ if (this->name)
+ ecpg_free(this->name);
free(this);
return false;
}
}
#endif
- if (connection_name != NULL)
- this->name = ecpg_strdup(connection_name, lineno);
- else
- this->name = ecpg_strdup(realname, lineno);
-
this->cache_head = NULL;
this->prep_stmts = NULL;
diff --git a/src/interfaces/ecpg/ecpglib/descriptor.c b/src/interfaces/ecpg/ecpglib/descriptor.c
index 651d5c8b2ed..466428edfeb 100644
--- a/src/interfaces/ecpg/ecpglib/descriptor.c
+++ b/src/interfaces/ecpg/ecpglib/descriptor.c
@@ -240,8 +240,9 @@ ECPGget_desc(int lineno, const char *desc_name, int index,...)
act_tuple;
struct variable data_var;
struct sqlca_t *sqlca = ECPGget_sqlca();
+ bool alloc_failed = (sqlca == NULL);
- if (sqlca == NULL)
+ if (alloc_failed)
{
ecpg_raise(lineno, ECPG_OUT_OF_MEMORY,
ECPG_SQLSTATE_ECPG_OUT_OF_MEMORY, NULL);
@@ -493,7 +494,14 @@ ECPGget_desc(int lineno, const char *desc_name, int index,...)
#ifdef WIN32
stmt.oldthreadlocale = _configthreadlocale(_ENABLE_PER_THREAD_LOCALE);
#endif
- stmt.oldlocale = ecpg_strdup(setlocale(LC_NUMERIC, NULL), lineno);
+ stmt.oldlocale = ecpg_strdup(setlocale(LC_NUMERIC, NULL),
+ lineno, &alloc_failed);
+ if (alloc_failed)
+ {
+ va_end(args);
+ return false;
+ }
+
setlocale(LC_NUMERIC, "C");
#endif
diff --git a/src/interfaces/ecpg/ecpglib/ecpglib_extern.h b/src/interfaces/ecpg/ecpglib/ecpglib_extern.h
index 75cc68275bd..949ff66cefc 100644
--- a/src/interfaces/ecpg/ecpglib/ecpglib_extern.h
+++ b/src/interfaces/ecpg/ecpglib/ecpglib_extern.h
@@ -175,7 +175,7 @@ void ecpg_free(void *ptr);
bool ecpg_init(const struct connection *con,
const char *connection_name,
const int lineno);
-char *ecpg_strdup(const char *string, int lineno);
+char *ecpg_strdup(const char *string, int lineno, bool *alloc_failed);
const char *ecpg_type_name(enum ECPGttype typ);
int ecpg_dynamic_type(Oid type);
int sqlda_dynamic_type(Oid type, enum COMPAT_MODE compat);
diff --git a/src/interfaces/ecpg/ecpglib/execute.c b/src/interfaces/ecpg/ecpglib/execute.c
index f52da06de9a..84a4a9fc578 100644
--- a/src/interfaces/ecpg/ecpglib/execute.c
+++ b/src/interfaces/ecpg/ecpglib/execute.c
@@ -860,9 +860,9 @@ ecpg_store_input(const int lineno, const bool force_indicator, const struct vari
numeric *nval;
if (var->arrsize > 1)
- mallocedval = ecpg_strdup("{", lineno);
+ mallocedval = ecpg_strdup("{", lineno, NULL);
else
- mallocedval = ecpg_strdup("", lineno);
+ mallocedval = ecpg_strdup("", lineno, NULL);
if (!mallocedval)
return false;
@@ -923,9 +923,9 @@ ecpg_store_input(const int lineno, const bool force_indicator, const struct vari
int slen;
if (var->arrsize > 1)
- mallocedval = ecpg_strdup("{", lineno);
+ mallocedval = ecpg_strdup("{", lineno, NULL);
else
- mallocedval = ecpg_strdup("", lineno);
+ mallocedval = ecpg_strdup("", lineno, NULL);
if (!mallocedval)
return false;
@@ -970,9 +970,9 @@ ecpg_store_input(const int lineno, const bool force_indicator, const struct vari
int slen;
if (var->arrsize > 1)
- mallocedval = ecpg_strdup("{", lineno);
+ mallocedval = ecpg_strdup("{", lineno, NULL);
else
- mallocedval = ecpg_strdup("", lineno);
+ mallocedval = ecpg_strdup("", lineno, NULL);
if (!mallocedval)
return false;
@@ -1017,9 +1017,9 @@ ecpg_store_input(const int lineno, const bool force_indicator, const struct vari
int slen;
if (var->arrsize > 1)
- mallocedval = ecpg_strdup("{", lineno);
+ mallocedval = ecpg_strdup("{", lineno, NULL);
else
- mallocedval = ecpg_strdup("", lineno);
+ mallocedval = ecpg_strdup("", lineno, NULL);
if (!mallocedval)
return false;
@@ -2001,7 +2001,8 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator,
return false;
}
#endif
- stmt->oldlocale = ecpg_strdup(setlocale(LC_NUMERIC, NULL), lineno);
+ stmt->oldlocale = ecpg_strdup(setlocale(LC_NUMERIC, NULL), lineno,
+ NULL);
if (stmt->oldlocale == NULL)
{
ecpg_do_epilogue(stmt);
@@ -2030,7 +2031,14 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator,
statement_type = ECPGst_execute;
}
else
- stmt->command = ecpg_strdup(query, lineno);
+ {
+ stmt->command = ecpg_strdup(query, lineno, NULL);
+ if (!stmt->command)
+ {
+ ecpg_do_epilogue(stmt);
+ return false;
+ }
+ }
stmt->name = NULL;
@@ -2042,7 +2050,12 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator,
if (command)
{
stmt->name = stmt->command;
- stmt->command = ecpg_strdup(command, lineno);
+ stmt->command = ecpg_strdup(command, lineno, NULL);
+ if (!stmt->command)
+ {
+ ecpg_do_epilogue(stmt);
+ return false;
+ }
}
else
{
@@ -2175,7 +2188,12 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator,
if (!is_prepared_name_set && stmt->statement_type == ECPGst_prepare)
{
- stmt->name = ecpg_strdup(var->value, lineno);
+ stmt->name = ecpg_strdup(var->value, lineno, NULL);
+ if (!stmt->name)
+ {
+ ecpg_do_epilogue(stmt);
+ return false;
+ }
is_prepared_name_set = true;
}
}
diff --git a/src/interfaces/ecpg/ecpglib/memory.c b/src/interfaces/ecpg/ecpglib/memory.c
index 6979be2c988..2112e55b6e4 100644
--- a/src/interfaces/ecpg/ecpglib/memory.c
+++ b/src/interfaces/ecpg/ecpglib/memory.c
@@ -43,8 +43,15 @@ ecpg_realloc(void *ptr, long size, int lineno)
return new;
}
+/*
+ * Wrapper for strdup(), with NULL in input treated as a correct case.
+ *
+ * "alloc_failed" can be optionally specified by the caller to check for
+ * allocation failures. The caller is responsible for its initialization,
+ * as ecpg_strdup() may be called repeatedly across multiple allocations.
+ */
char *
-ecpg_strdup(const char *string, int lineno)
+ecpg_strdup(const char *string, int lineno, bool *alloc_failed)
{
char *new;
@@ -54,6 +61,8 @@ ecpg_strdup(const char *string, int lineno)
new = strdup(string);
if (!new)
{
+ if (alloc_failed)
+ *alloc_failed = true;
ecpg_raise(lineno, ECPG_OUT_OF_MEMORY, ECPG_SQLSTATE_ECPG_OUT_OF_MEMORY, NULL);
return NULL;
}
diff --git a/src/interfaces/ecpg/ecpglib/prepare.c b/src/interfaces/ecpg/ecpglib/prepare.c
index ea1146f520f..dd6fd1fe7f4 100644
--- a/src/interfaces/ecpg/ecpglib/prepare.c
+++ b/src/interfaces/ecpg/ecpglib/prepare.c
@@ -85,9 +85,22 @@ ecpg_register_prepared_stmt(struct statement *stmt)
/* create statement */
prep_stmt->lineno = lineno;
prep_stmt->connection = con;
- prep_stmt->command = ecpg_strdup(stmt->command, lineno);
+ prep_stmt->command = ecpg_strdup(stmt->command, lineno, NULL);
+ if (!prep_stmt->command)
+ {
+ ecpg_free(prep_stmt);
+ ecpg_free(this);
+ return false;
+ }
prep_stmt->inlist = prep_stmt->outlist = NULL;
- this->name = ecpg_strdup(stmt->name, lineno);
+ this->name = ecpg_strdup(stmt->name, lineno, NULL);
+ if (!this->name)
+ {
+ ecpg_free(prep_stmt->command);
+ ecpg_free(prep_stmt);
+ ecpg_free(this);
+ return false;
+ }
this->stmt = prep_stmt;
this->prepared = true;
@@ -177,14 +190,27 @@ prepare_common(int lineno, struct connection *con, const char *name, const char
/* create statement */
stmt->lineno = lineno;
stmt->connection = con;
- stmt->command = ecpg_strdup(variable, lineno);
+ stmt->command = ecpg_strdup(variable, lineno, NULL);
+ if (!stmt->command)
+ {
+ ecpg_free(stmt);
+ ecpg_free(this);
+ return false;
+ }
stmt->inlist = stmt->outlist = NULL;
/* if we have C variables in our statement replace them with '?' */
replace_variables(&(stmt->command), lineno);
/* add prepared statement to our list */
- this->name = ecpg_strdup(name, lineno);
+ this->name = ecpg_strdup(name, lineno, NULL);
+ if (!this->name)
+ {
+ ecpg_free(stmt->command);
+ ecpg_free(stmt);
+ ecpg_free(this);
+ return false;
+ }
this->stmt = stmt;
/* and finally really prepare the statement */
@@ -540,7 +566,9 @@ AddStmtToCache(int lineno, /* line # of statement */
/* add the query to the entry */
entry = &stmtCacheEntries[entNo];
entry->lineno = lineno;
- entry->ecpgQuery = ecpg_strdup(ecpgQuery, lineno);
+ entry->ecpgQuery = ecpg_strdup(ecpgQuery, lineno, NULL);
+ if (!entry->ecpgQuery)
+ return -1;
entry->connection = connection;
entry->execs = 0;
memcpy(entry->stmtID, stmtID, sizeof(entry->stmtID));
@@ -567,6 +595,9 @@ ecpg_auto_prepare(int lineno, const char *connection_name, const int compat, cha
ecpg_log("ecpg_auto_prepare on line %d: statement found in cache; entry %d\n", lineno, entNo);
stmtID = stmtCacheEntries[entNo].stmtID;
+ *name = ecpg_strdup(stmtID, lineno, NULL);
+ if (*name == NULL)
+ return false;
con = ecpg_get_connection(connection_name);
prep = ecpg_find_prepared_statement(stmtID, con, NULL);
@@ -574,7 +605,6 @@ ecpg_auto_prepare(int lineno, const char *connection_name, const int compat, cha
if (!prep && !prepare_common(lineno, con, stmtID, query))
return false;
- *name = ecpg_strdup(stmtID, lineno);
}
else
{
@@ -584,6 +614,9 @@ ecpg_auto_prepare(int lineno, const char *connection_name, const int compat, cha
/* generate a statement ID */
sprintf(stmtID, "ecpg%d", nextStmtID++);
+ *name = ecpg_strdup(stmtID, lineno, NULL);
+ if (*name == NULL)
+ return false;
if (!ECPGprepare(lineno, connection_name, 0, stmtID, query))
return false;
@@ -591,8 +624,6 @@ ecpg_auto_prepare(int lineno, const char *connection_name, const int compat, cha
entNo = AddStmtToCache(lineno, stmtID, connection_name, compat, query);
if (entNo < 0)
return false;
-
- *name = ecpg_strdup(stmtID, lineno);
}
/* increase usage counter */
--
2.43.0
Re: Missing NULL check after calling ecpg_strdup
On Tue, Jul 22, 2025 at 04:20:53PM +0300, Aleksander Alekseev wrote:
v7 may have a compilation warning on Linux:
```
warning: unused variable ‘alloc_failed’ [-Wunused-variable]
```... because the only use of the variable is hidden under #ifdef's.
Yep, thanks, didn't see this one coming. Doing a CI run was still on
my list of things still to do. I have just done one after a second
lookup and I think that we should be OK.
What you are suggesting for ECPGget_desc() is fine by me at the end.
Both are immediate failure checks and the first one is never expected
to be NULL, but the second with setlocale() needs that, so...
Applied v8 on HEAD.
--
Michael