From 65480c05d2d62706fdcfe3015e29b0148ae86fe9 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Fri, 4 Oct 2024 16:26:07 -0400 Subject: [PATCH v5 8/9] Clean up some other assorted ecpg memory leaks. Avoid leaking the prior value when updating the "connection" state variable. Ditto for ECPGstruct_sizeof. (It seems like this one ought to be statement-local, but testing says it isn't, and I didn't feel like diving deeper.) The actual_type[] entries are statement-local, though, so no need to mm_strdup() strings stored in them. Likewise, sqlda variables are statement-local, so we can loc_alloc them. Also clean up sloppiness around management of the argsinsert and argsresult lists. progname changes are strictly to prevent valgrind from complaining about leaked allocations. With this, valgrind reports zero leakage in ecpg for all of our ecpg regression test cases. Discussion: https://postgr.es/m/2011420.1713493114@sss.pgh.pa.us --- src/interfaces/ecpg/preproc/descriptor.c | 14 +++++-- src/interfaces/ecpg/preproc/ecpg.addons | 47 +++++++++--------------- src/interfaces/ecpg/preproc/ecpg.c | 2 +- src/interfaces/ecpg/preproc/ecpg.header | 17 ++++++++- src/interfaces/ecpg/preproc/ecpg.trailer | 31 +++++++++------- src/interfaces/ecpg/preproc/output.c | 3 +- src/interfaces/ecpg/preproc/variable.c | 25 +++++++++++-- 7 files changed, 86 insertions(+), 53 deletions(-) diff --git a/src/interfaces/ecpg/preproc/descriptor.c b/src/interfaces/ecpg/preproc/descriptor.c index 9b87d07d09..e8c7016bdc 100644 --- a/src/interfaces/ecpg/preproc/descriptor.c +++ b/src/interfaces/ecpg/preproc/descriptor.c @@ -344,11 +344,17 @@ descriptor_variable(const char *name, int input) struct variable * sqlda_variable(const char *name) { - struct variable *p = (struct variable *) mm_alloc(sizeof(struct variable)); - - p->name = mm_strdup(name); - p->type = (struct ECPGtype *) mm_alloc(sizeof(struct ECPGtype)); + /* + * Presently, sqlda variables are only needed for the duration of the + * current statement. Rather than add infrastructure to manage them, + * let's just loc_alloc them. + */ + struct variable *p = (struct variable *) loc_alloc(sizeof(struct variable)); + + p->name = loc_strdup(name); + p->type = (struct ECPGtype *) loc_alloc(sizeof(struct ECPGtype)); p->type->type = ECPGt_sqlda; + p->type->type_name = NULL; p->type->size = NULL; p->type->struct_sizeof = NULL; p->type->u.element = NULL; diff --git a/src/interfaces/ecpg/preproc/ecpg.addons b/src/interfaces/ecpg/preproc/ecpg.addons index 9c120fead2..05de4ff1f1 100644 --- a/src/interfaces/ecpg/preproc/ecpg.addons +++ b/src/interfaces/ecpg/preproc/ecpg.addons @@ -135,6 +135,7 @@ ECPG: stmtViewStmt rule fprintf(base_yyout, "{ ECPGdescribe(__LINE__, %d, %d, %s, %s,", compat, $1.input, connection ? connection : "NULL", $1.stmt_name); dump_variables(argsresult, 1); + argsresult = NULL; fputs("ECPGt_EORT);", base_yyout); fprintf(base_yyout, "}"); output_line_number(); @@ -181,6 +182,7 @@ ECPG: stmtViewStmt rule if ((ptr = add_additional_variables(@1, true)) != NULL) { + free(connection); connection = ptr->connection ? mm_strdup(ptr->connection) : NULL; output_statement(ptr->command, 0, ECPGst_normal); ptr->opened = true; @@ -247,15 +249,13 @@ ECPG: var_valueNumericOnly addon ECPG: fetch_argscursor_name addon struct cursor *ptr = add_additional_variables(@1, false); - if (ptr->connection) - connection = mm_strdup(ptr->connection); + update_connection(ptr->connection); if (@1[0] == ':') @$ = "$0"; ECPG: fetch_argsfrom_incursor_name addon struct cursor *ptr = add_additional_variables(@2, false); - if (ptr->connection) - connection = mm_strdup(ptr->connection); + update_connection(ptr->connection); if (@2[0] == ':') @$ = cat2_str(@1, "$0"); ECPG: fetch_argsNEXTopt_from_incursor_name addon @@ -265,16 +265,14 @@ ECPG: fetch_argsLAST_Popt_from_incursor_name addon ECPG: fetch_argsALLopt_from_incursor_name addon struct cursor *ptr = add_additional_variables(@3, false); - if (ptr->connection) - connection = mm_strdup(ptr->connection); + update_connection(ptr->connection); if (@3[0] == ':') @$ = cat_str(3, @1, @2, "$0"); ECPG: fetch_argsSignedIconstopt_from_incursor_name addon struct cursor *ptr = add_additional_variables(@3, false); bool replace = false; - if (ptr->connection) - connection = mm_strdup(ptr->connection); + update_connection(ptr->connection); if (@3[0] == ':') { @3 = "$0"; @@ -291,8 +289,7 @@ ECPG: fetch_argsFORWARDALLopt_from_incursor_name addon ECPG: fetch_argsBACKWARDALLopt_from_incursor_name addon struct cursor *ptr = add_additional_variables(@4, false); - if (ptr->connection) - connection = mm_strdup(ptr->connection); + update_connection(ptr->connection); if (@4[0] == ':') @$ = cat_str(4, @1, @2, @3, "$0"); ECPG: fetch_argsABSOLUTE_PSignedIconstopt_from_incursor_name addon @@ -302,8 +299,7 @@ ECPG: fetch_argsBACKWARDSignedIconstopt_from_incursor_name addon struct cursor *ptr = add_additional_variables(@4, false); bool replace = false; - if (ptr->connection) - connection = mm_strdup(ptr->connection); + update_connection(ptr->connection); if (@4[0] == ':') { @4 = "$0"; @@ -412,8 +408,7 @@ ECPG: ClosePortalStmtCLOSEcursor_name block { if (strcmp(@2, ptr->name) == 0) { - if (ptr->connection) - connection = mm_strdup(ptr->connection); + update_connection(ptr->connection); break; } } @@ -483,8 +478,7 @@ ECPG: FetchStmtMOVEfetch_args rule const char *cursor_marker = @3[0] == ':' ? "$0" : @3; struct cursor *ptr = add_additional_variables(@3, false); - if (ptr->connection) - connection = mm_strdup(ptr->connection); + update_connection(ptr->connection); @$ = cat_str(2, "fetch forward", cursor_marker); } @@ -493,8 +487,7 @@ ECPG: FetchStmtMOVEfetch_args rule const char *cursor_marker = @4[0] == ':' ? "$0" : @4; struct cursor *ptr = add_additional_variables(@4, false); - if (ptr->connection) - connection = mm_strdup(ptr->connection); + update_connection(ptr->connection); @$ = cat_str(2, "fetch forward from", cursor_marker); } @@ -503,8 +496,7 @@ ECPG: FetchStmtMOVEfetch_args rule const char *cursor_marker = @3[0] == ':' ? "$0" : @3; struct cursor *ptr = add_additional_variables(@3, false); - if (ptr->connection) - connection = mm_strdup(ptr->connection); + update_connection(ptr->connection); @$ = cat_str(2, "fetch backward", cursor_marker); } @@ -513,8 +505,7 @@ ECPG: FetchStmtMOVEfetch_args rule const char *cursor_marker = @4[0] == ':' ? "$0" : @4; struct cursor *ptr = add_additional_variables(@4, false); - if (ptr->connection) - connection = mm_strdup(ptr->connection); + update_connection(ptr->connection); @$ = cat_str(2, "fetch backward from", cursor_marker); } @@ -523,8 +514,7 @@ ECPG: FetchStmtMOVEfetch_args rule const char *cursor_marker = @3[0] == ':' ? "$0" : @3; struct cursor *ptr = add_additional_variables(@3, false); - if (ptr->connection) - connection = mm_strdup(ptr->connection); + update_connection(ptr->connection); @$ = cat_str(2, "move forward", cursor_marker); } @@ -533,8 +523,7 @@ ECPG: FetchStmtMOVEfetch_args rule const char *cursor_marker = @4[0] == ':' ? "$0" : @4; struct cursor *ptr = add_additional_variables(@4, false); - if (ptr->connection) - connection = mm_strdup(ptr->connection); + update_connection(ptr->connection); @$ = cat_str(2, "move forward from", cursor_marker); } @@ -543,8 +532,7 @@ ECPG: FetchStmtMOVEfetch_args rule const char *cursor_marker = @3[0] == ':' ? "$0" : @3; struct cursor *ptr = add_additional_variables(@3, false); - if (ptr->connection) - connection = mm_strdup(ptr->connection); + update_connection(ptr->connection); @$ = cat_str(2, "move backward", cursor_marker); } @@ -553,8 +541,7 @@ ECPG: FetchStmtMOVEfetch_args rule const char *cursor_marker = @4[0] == ':' ? "$0" : @4; struct cursor *ptr = add_additional_variables(@4, false); - if (ptr->connection) - connection = mm_strdup(ptr->connection); + update_connection(ptr->connection); @$ = cat_str(2, "move backward from", cursor_marker); } diff --git a/src/interfaces/ecpg/preproc/ecpg.c b/src/interfaces/ecpg/preproc/ecpg.c index 73c37631ac..2fcc6f8f99 100644 --- a/src/interfaces/ecpg/preproc/ecpg.c +++ b/src/interfaces/ecpg/preproc/ecpg.c @@ -20,6 +20,7 @@ bool autocommit = false, regression_mode = false, auto_prepare = false; +static const char *progname; char *output_filename; enum COMPAT_MODE compat = ECPG_COMPAT_PGSQL; @@ -139,7 +140,6 @@ main(int argc, char *const argv[]) bool verbose = false, header_mode = false; struct _include_path *ip; - const char *progname; char my_exec_path[MAXPGPATH]; char include_path[MAXPGPATH]; diff --git a/src/interfaces/ecpg/preproc/ecpg.header b/src/interfaces/ecpg/preproc/ecpg.header index d54eca918d..a9a0ef9847 100644 --- a/src/interfaces/ecpg/preproc/ecpg.header +++ b/src/interfaces/ecpg/preproc/ecpg.header @@ -59,6 +59,7 @@ struct variable no_indicator = {"no_indicator", &ecpg_no_indicator, 0, NULL}; static struct ECPGtype ecpg_query = {ECPGt_char_variable, NULL, NULL, NULL, {NULL}, 0}; static bool check_declared_list(const char *name); +static void update_connection(const char *newconn); /* @@ -545,12 +546,26 @@ check_declared_list(const char *name) { if (connection && strcmp(ptr->connection, connection) != 0) mmerror(PARSE_ERROR, ET_WARNING, "connection %s is overwritten with %s by DECLARE statement %s", connection, ptr->connection, name); - connection = mm_strdup(ptr->connection); + update_connection(ptr->connection); return true; } } return false; } + +/* + * If newconn isn't NULL, update the global "connection" variable to that; + * otherwise do nothing. + */ +static void +update_connection(const char *newconn) +{ + if (newconn) + { + free(connection); + connection = mm_strdup(newconn); + } +} %} %expect 0 diff --git a/src/interfaces/ecpg/preproc/ecpg.trailer b/src/interfaces/ecpg/preproc/ecpg.trailer index 41029701fc..a90b1771fc 100644 --- a/src/interfaces/ecpg/preproc/ecpg.trailer +++ b/src/interfaces/ecpg/preproc/ecpg.trailer @@ -74,6 +74,8 @@ CreateAsStmt: CREATE OptTemp TABLE create_as_target AS at: AT connection_object { + if (connection) + free(connection); connection = mm_strdup(@2); /* @@ -556,13 +558,12 @@ type_declaration: S_TYPEDEF var_declaration: storage_declaration var_type { - actual_type[struct_level].type_storage = mm_strdup(@1); + actual_type[struct_level].type_storage = loc_strdup(@1); actual_type[struct_level].type_enum = $2.type_enum; - actual_type[struct_level].type_str = mm_strdup($2.type_str); - actual_type[struct_level].type_dimension = mm_strdup($2.type_dimension); - actual_type[struct_level].type_index = mm_strdup($2.type_index); - actual_type[struct_level].type_sizeof = - $2.type_sizeof ? mm_strdup($2.type_sizeof) : NULL; + actual_type[struct_level].type_str = $2.type_str; + actual_type[struct_level].type_dimension = $2.type_dimension; + actual_type[struct_level].type_index = $2.type_index; + actual_type[struct_level].type_sizeof = $2.type_sizeof; actual_startline[struct_level] = hashline_number(); } @@ -572,13 +573,12 @@ var_declaration: } | var_type { - actual_type[struct_level].type_storage = mm_strdup(""); + actual_type[struct_level].type_storage = loc_strdup(""); actual_type[struct_level].type_enum = $1.type_enum; - actual_type[struct_level].type_str = mm_strdup($1.type_str); - actual_type[struct_level].type_dimension = mm_strdup($1.type_dimension); - actual_type[struct_level].type_index = mm_strdup($1.type_index); - actual_type[struct_level].type_sizeof = - $1.type_sizeof ? mm_strdup($1.type_sizeof) : NULL; + actual_type[struct_level].type_str = $1.type_str; + actual_type[struct_level].type_dimension = $1.type_dimension; + actual_type[struct_level].type_index = $1.type_index; + actual_type[struct_level].type_sizeof = $1.type_sizeof; actual_startline[struct_level] = hashline_number(); } @@ -870,7 +870,7 @@ var_type: simple_type /* Otherwise, it must be a user-defined typedef name */ struct typedefs *this = get_typedef(@1, false); - $$.type_str = (this->type->type_enum == ECPGt_varchar || this->type->type_enum == ECPGt_bytea) ? mm_strdup("") : mm_strdup(this->name); + $$.type_str = (this->type->type_enum == ECPGt_varchar || this->type->type_enum == ECPGt_bytea) ? "" : this->name; $$.type_enum = this->type->type_enum; $$.type_dimension = this->type->type_dimension; $$.type_index = this->type->type_index; @@ -1004,6 +1004,7 @@ s_struct_union_symbol: SQL_STRUCT symbol { $$.su = "struct"; $$.symbol = @2; + free(ECPGstruct_sizeof); ECPGstruct_sizeof = mm_strdup(cat_str(3, "sizeof(", cat2_str($$.su, $$.symbol), ")")); @@ -1017,6 +1018,7 @@ s_struct_union_symbol: SQL_STRUCT symbol s_struct_union: SQL_STRUCT { + free(ECPGstruct_sizeof); ECPGstruct_sizeof = mm_strdup(""); /* This must not be NULL to * distinguish from simple types. */ @$ = "struct"; @@ -1696,18 +1698,21 @@ ECPGVar: SQL_VAR ECPGWhenever: SQL_WHENEVER SQL_SQLERROR action { when_error.code = $3.code; + free(when_error.command); when_error.command = $3.command ? mm_strdup($3.command) : NULL; @$ = cat_str(3, "/* exec sql whenever sqlerror ", $3.str, "; */"); } | SQL_WHENEVER NOT SQL_FOUND action { when_nf.code = $4.code; + free(when_nf.command); when_nf.command = $4.command ? mm_strdup($4.command) : NULL; @$ = cat_str(3, "/* exec sql whenever not found ", $4.str, "; */"); } | SQL_WHENEVER SQL_SQLWARNING action { when_warn.code = $3.code; + free(when_warn.command); when_warn.command = $3.command ? mm_strdup($3.command) : NULL; @$ = cat_str(3, "/* exec sql whenever sql_warning ", $3.str, "; */"); } diff --git a/src/interfaces/ecpg/preproc/output.c b/src/interfaces/ecpg/preproc/output.c index a18904f88b..b190e9f0ce 100644 --- a/src/interfaces/ecpg/preproc/output.c +++ b/src/interfaces/ecpg/preproc/output.c @@ -155,10 +155,11 @@ output_statement(const char *stmt, int whenever_mode, enum ECPG_statement_type s /* dump variables to C file */ dump_variables(argsinsert, 1); + argsinsert = NULL; fputs("ECPGt_EOIT, ", base_yyout); dump_variables(argsresult, 1); + argsresult = NULL; fputs("ECPGt_EORT);", base_yyout); - reset_variables(); whenever_action(whenever_mode | 2); } diff --git a/src/interfaces/ecpg/preproc/variable.c b/src/interfaces/ecpg/preproc/variable.c index 4831f56cba..8a2d0414ae 100644 --- a/src/interfaces/ecpg/preproc/variable.c +++ b/src/interfaces/ecpg/preproc/variable.c @@ -311,10 +311,12 @@ remove_variables(int brace_level) for (ptr = cur; ptr != NULL; ptr = ptr->next) { struct arguments *varptr, - *prevvar; + *prevvar, + *nextvar; - for (varptr = prevvar = ptr->argsinsert; varptr != NULL; varptr = varptr->next) + for (varptr = prevvar = ptr->argsinsert; varptr != NULL; varptr = nextvar) { + nextvar = varptr->next; if (p == varptr->variable) { /* remove from list */ @@ -322,10 +324,12 @@ remove_variables(int brace_level) ptr->argsinsert = varptr->next; else prevvar->next = varptr->next; + free(varptr); } } - for (varptr = prevvar = ptr->argsresult; varptr != NULL; varptr = varptr->next) + for (varptr = prevvar = ptr->argsresult; varptr != NULL; varptr = nextvar) { + nextvar = varptr->next; if (p == varptr->variable) { /* remove from list */ @@ -333,6 +337,7 @@ remove_variables(int brace_level) ptr->argsresult = varptr->next; else prevvar->next = varptr->next; + free(varptr); } } } @@ -372,7 +377,20 @@ struct arguments *argsresult = NULL; void reset_variables(void) { + struct arguments *p, + *next; + + for (p = argsinsert; p; p = next) + { + next = p->next; + free(p); + } argsinsert = NULL; + for (p = argsresult; p; p = next) + { + next = p->next; + free(p); + } argsresult = NULL; } @@ -431,6 +449,7 @@ remove_variable_from_list(struct arguments **list, struct variable *var) prev->next = p->next; else *list = p->next; + free(p); } } -- 2.43.5