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.
From 47861dd766aca4abcbabd1805d073ae1167dbcb7 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@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 <gorbanyo...@basealt.ru>
Co-authored-by: Aleksander Alekseev <aleksan...@tigerdata.com>
Reviewed-by: Álvaro Herrera <alvhe...@kurilemu.de>
Reviewed-by: Me
Discussion: https://postgr.es/m/a6b193c1-6994-4d9c-9059-aca4aaf41...@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
