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.
From ba5dcae9311270e37e6181f5de0bb439a1b5b55b Mon Sep 17 00:00:00 2001 From: Aleksander Alekseev <aleksan...@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 <gorbanyo...@basealt.ru> Author: Aleksander Alekseev <aleksan...@tigerdata.com> Reviewed-by: Álvaro Herrera <alvhe...@kurilemu.de> Discussion: https://postgr.es/m/a6b193c1-6994-4d9c-9059-aca4aaf41...@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
From 919f3959e24ee86199978b6d20ff6bc43f80ce8f Mon Sep 17 00:00:00 2001 From: Aleksander Alekseev <aleksan...@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 <aleksan...@tigerdata.com> Reviewed-by: TODO FIXME Discussion: https://postgr.es/m/a6b193c1-6994-4d9c-9059-aca4aaf41...@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
