From 6ca29b7042cec545cedbbc69d17ac769a9ead5fd Mon Sep 17 00:00:00 2001 From: Alexandr Kuznetsov Date: Fri, 24 May 2024 12:47:10 +0300 Subject: [PATCH] Fixed bugs, reported in issue #8. (#25) * Fixed bugs, reported in issue #8. Bunch of memory leaks, occuring in tests. One heap-buffer-overrun in read operation at convert.c:3162. Two memory leaks, detected by my unit tests: password, conn_settings, pqopt fields overwrite if their values provided by both, system config and connstring. Restored reference counting lifetime managment of COL_INFO objects. Some minor cosmetic changes. Signed-off-by: Alexandr Kuznetsov * Minor cosmetic changes. Signed-off-by: Alexandr Kuznetsov * Forgot set col_info to NULL in TABLE_INFO object while clearing it. Signed-off-by: Alexandr Kuznetsov * Fixing comments. Signed-off-by: Alexandr Kuznetsov --------- Signed-off-by: Alexandr Kuznetsov --- bind.c | 2 ++ connection.c | 15 ++++++++--- convert.c | 7 ++--- descriptor.c | 41 +++++++++++++++++++---------- descriptor.h | 1 + dlg_specific.c | 3 +++ parse.c | 27 ++++++++++++------- results.c | 1 + statement.c | 15 +++++++++-- test/src/cte-test.c | 14 +++++----- test/src/cursor-block-delete-test.c | 12 ++++----- test/src/premature-test.c | 30 ++++++++++----------- 12 files changed, 107 insertions(+), 61 deletions(-) diff --git a/bind.c b/bind.c index 59ddea1..be1018a 100644 --- a/bind.c +++ b/bind.c @@ -701,6 +701,8 @@ IPD_free_params(IPDFields *ipdopts, char option) return; if (option == STMT_FREE_PARAMS_ALL) { + for (int i = 0; i < ipdopts->allocated; ++i) + NULL_THE_NAME(ipdopts->parameters[i].paramName); free(ipdopts->parameters); ipdopts->parameters = NULL; ipdopts->allocated = 0; diff --git a/connection.c b/connection.c index 1c55af8..d83e9ac 100644 --- a/connection.c +++ b/connection.c @@ -553,21 +553,28 @@ CC_clear_col_info(ConnectionClass *self, BOOL destroy) for (i = 0; i < self->ntables; i++) { + /* Going through COL_INFO cache table and releasing coli objects. */ if (coli = self->col_info[i], NULL != coli) { - if (destroy || coli->refcnt == 0) + coli->refcnt--; + if (coli->refcnt <= 0) { + /* Last reference to coli object disappeared. Now destroying it. */ free_col_info_contents(coli); free(coli); self->col_info[i] = NULL; } else + { + /* coli object have another reference to it, so it will be destroyed somewhere else. */ coli->acc_time = 0; + } } } - self->ntables = 0; + self->ntables = 0; /* Now we have cleared COL_INFO cached objects table. */ if (destroy) { + /* We destroying COL_INFO cache completely. */ free(self->col_info); self->col_info = NULL; self->coli_allocated = 0; @@ -966,7 +973,7 @@ handle_pgres_error(ConnectionClass *self, const PGresult *pgres, CC_on_abort(self, CONN_DEAD); /* give up the connection */ } else if ((errseverity_nonloc && strcmp(errseverity_nonloc, "FATAL") == 0) || - (NULL == errseverity_nonloc && errseverity && strcmp(errseverity, "FATAL") == 0)) /* no */ + (NULL == errseverity_nonloc && errseverity && strcmp(errseverity, "FATAL") == 0)) /* no */ { CC_set_errornumber(self, CONNECTION_SERVER_REPORTED_SEVERITY_FATAL); CC_on_abort(self, CONN_DEAD); /* give up the connection */ @@ -2873,7 +2880,7 @@ LIBPQ_connect(ConnectionClass *self) QLOG(0, "PQconnectdbParams:"); for (popt = opts, pval = vals; *popt; popt++, pval++) QPRINTF(0, " %s='%s'", *popt, *pval); - QPRINTF(0, "\n"); + QPRINTF(0, "\n"); } pqconn = PQconnectdbParams(opts, vals, FALSE); if (!pqconn) diff --git a/convert.c b/convert.c index c831991..abb52d5 100644 --- a/convert.c +++ b/convert.c @@ -3056,7 +3056,7 @@ MYLOG(DETAIL_LOG_LEVEL, "type=" FORMAT_UINTEGER " concur=" FORMAT_UINTEGER "\n", } if (SC_is_fetchcursor(stmt)) { - snprintfcat(new_statement, qb->str_alsize, + snprintfcat(new_statement, qb->str_alsize, "declare \"%s\"%s cursor%s for ", SC_cursor_name(stmt), opt_scroll, opt_hold); qb->npos = strlen(new_statement); @@ -3159,7 +3159,7 @@ MYLOG(DETAIL_LOG_LEVEL, "type=" FORMAT_UINTEGER " concur=" FORMAT_UINTEGER "\n", CVT_APPEND_STR(qb, bestitem); } CVT_APPEND_STR(qb, "\" from "); - CVT_APPEND_DATA(qb, qp->statement + qp->from_pos + 5, npos - qp->from_pos - 5); + CVT_APPEND_DATA(qb, qp->statement + qp->from_pos + 5, qp->stmt_len - qp->from_pos - 5); } } npos -= qp->declare_pos; @@ -3744,7 +3744,7 @@ inner_process_tokens(QueryParse *qp, QueryBuild *qb) } if (converted) { - const char *column_def = (const char *) QR_get_value_backend_text(coli->result, i, COLUMNS_COLUMN_DEF); + const char *column_def = (const char *) QR_get_value_backend_text(coli->result, i, COLUMNS_COLUMN_DEF); if (NULL != column_def && strncmp(column_def, "nextval", 7) == 0) { @@ -3770,6 +3770,7 @@ inner_process_tokens(QueryParse *qp, QueryBuild *qb) } } } + TI_ClearObject(pti); } if (!converted) CVT_APPEND_STR(qb, "NULL"); diff --git a/descriptor.c b/descriptor.c index 407c1d6..b473a4d 100644 --- a/descriptor.c +++ b/descriptor.c @@ -41,26 +41,39 @@ MYLOG(DETAIL_LOG_LEVEL, "entering count=%d\n", count); { if (ti[i]) { - COL_INFO *coli = ti[i]->col_info; - if (coli) - { -MYLOG(0, "!!!refcnt %p:%d -> %d\n", coli, coli->refcnt, coli->refcnt - 1); - coli->refcnt--; - if (coli->refcnt <= 0 && 0 == coli->acc_time) /* acc_time == 0 means the table is dropped */ - free_col_info_contents(coli); - } - NULL_THE_NAME(ti[i]->schema_name); - NULL_THE_NAME(ti[i]->table_name); - NULL_THE_NAME(ti[i]->table_alias); - NULL_THE_NAME(ti[i]->bestitem); - NULL_THE_NAME(ti[i]->bestqual); - TI_Destroy_IH(ti[i]); + TI_ClearObject(ti[i]); free(ti[i]); ti[i] = NULL; } } } } +void TI_ClearObject(TABLE_INFO *ti) +{ + if (ti) + { + COL_INFO *coli = ti->col_info; + if (coli) + { +MYLOG(0, "!!!refcnt %p:%d -> %d\n", coli, coli->refcnt, coli->refcnt - 1); + coli->refcnt--; + if (coli->refcnt <= 1 && 0 == coli->acc_time) /* acc_time == 0 means the table is dropped */ + free_col_info_contents(coli); /* Now coli object is unused, and may be reused later. */ + if (coli->refcnt <= 0) + { + /* Last reference to coli object disappeared. Now destroying it. */ + free(coli); + ti->col_info = NULL; + } + } + NULL_THE_NAME(ti->schema_name); + NULL_THE_NAME(ti->table_name); + NULL_THE_NAME(ti->table_alias); + NULL_THE_NAME(ti->bestitem); + NULL_THE_NAME(ti->bestqual); + TI_Destroy_IH(ti); + } +} void FI_Constructor(FIELD_INFO *self, BOOL reuse) { MYLOG(DETAIL_LOG_LEVEL, "entering reuse=%d\n", reuse); diff --git a/descriptor.h b/descriptor.h index 4729d93..b15ee01 100644 --- a/descriptor.h +++ b/descriptor.h @@ -55,6 +55,7 @@ typedef struct #define TI_set_has_no_subclass(ti) (ti->flags &= (~TI_HASSUBCLASS)) void TI_Constructor(TABLE_INFO *, const ConnectionClass *); void TI_Destructor(TABLE_INFO **, int); +void TI_ClearObject(TABLE_INFO *ti); void TI_Create_IH(TABLE_INFO *ti); void TI_Destroy_IH(TABLE_INFO *ti); const pgNAME TI_From_IH(TABLE_INFO *ti, OID tableoid); diff --git a/dlg_specific.c b/dlg_specific.c index b15f7a4..a3db6d2 100644 --- a/dlg_specific.c +++ b/dlg_specific.c @@ -624,6 +624,7 @@ copyConnAttributes(ConnInfo *ci, const char *attribute, const char *value) STRCPY_FIXED(ci->username, value); else if (stricmp(attribute, INI_PASSWORD) == 0 || stricmp(attribute, "pwd") == 0) { + NULL_THE_NAME(ci->password); ci->password = decode_or_remove_braces(value); #ifndef FORCE_PASSWORDE_DISPLAY MYLOG(0, "key='%s' value='xxxxxxxx'\n", attribute); @@ -669,11 +670,13 @@ copyConnAttributes(ConnInfo *ci, const char *attribute, const char *value) else if (stricmp(attribute, INI_CONNSETTINGS) == 0 || stricmp(attribute, ABBR_CONNSETTINGS) == 0) { /* We can use the conn_settings directly when they are enclosed with braces */ + NULL_THE_NAME(ci->conn_settings); ci->conn_settings_in_str = TRUE; ci->conn_settings = decode_or_remove_braces(value); } else if (stricmp(attribute, INI_PQOPT) == 0 || stricmp(attribute, ABBR_PQOPT) == 0) { + NULL_THE_NAME(ci->pqopt); ci->pqopt_in_str = TRUE; ci->pqopt = decode_or_remove_braces(value); } diff --git a/parse.c b/parse.c index 366d514..f63f3fc 100644 --- a/parse.c +++ b/parse.c @@ -713,8 +713,7 @@ MYPRINTF(0, "->%d\n", updatable); } static BOOL -getCOLIfromTable(ConnectionClass *conn, pgNAME *schema_name, pgNAME table_name, -COL_INFO **coli) +getCOLIfromTable(ConnectionClass *conn, pgNAME *schema_name, pgNAME table_name, COL_INFO **coli) { int colidx; BOOL found = FALSE; @@ -836,11 +835,13 @@ getColumnsInfo(ConnectionClass *conn, TABLE_INFO *wti, OID greloid, StatementCla MYLOG(0, " Success\n"); if (greloid != 0) { + /* We have reloid. Try to find appropriate coli object from connection COL_INFO cache. */ for (k = 0; k < conn->ntables; k++) { tcoli = conn->col_info[k]; if (tcoli->table_oid == greloid) { + /* We found appropriate coli object, so we will use it. */ coli = tcoli; coli_exist = TRUE; break; @@ -849,42 +850,46 @@ getColumnsInfo(ConnectionClass *conn, TABLE_INFO *wti, OID greloid, StatementCla } if (!coli_exist) { + /* Not found, try to find unused coli or oldest (if overflow) in connection COL_INFO cache. */ for (k = 0; k < conn->ntables; k++) { tcoli = conn->col_info[k]; - if (0 < tcoli->refcnt) - continue; + if (1 < tcoli->refcnt) + continue; /* This coli object is used somewhere else, skipping it. */ if ((0 == tcoli->table_oid && NAME_IS_NULL(tcoli->table_name)) || strnicmp(SAFE_NAME(tcoli->schema_name), "pg_temp_", 8) == 0) { + /* Found unused coli object, taking it. */ coli = tcoli; coli_exist = TRUE; break; } - if (NULL == ccoli || - tcoli->acc_time < acctime) + if (NULL == ccoli || tcoli->acc_time < acctime) { + /* Not yet found. Alongside, searching least recently used coli object. */ ccoli = tcoli; acctime = tcoli->acc_time; } } - if (!coli_exist && - NULL != ccoli && - conn->ntables >= COLI_RECYCLE) + if (!coli_exist && NULL != ccoli && conn->ntables >= COLI_RECYCLE) { + /* Not found unsed object. Amount of them is on limit. Taking least recently used coli object. */ coli_exist = TRUE; coli = ccoli; } } if (coli_exist) { + /* We have ready to use coli object. Cleaning it. */ free_col_info_contents(coli); } else { + /* We have no coli object. Must create a new one. */ if (conn->ntables >= conn->coli_allocated) { + /* No place in connection COL_INFO cache table. Allocating or reallocating. */ Int2 new_alloc; COL_INFO **col_info; @@ -904,6 +909,7 @@ getColumnsInfo(ConnectionClass *conn, TABLE_INFO *wti, OID greloid, StatementCla conn->coli_allocated = new_alloc; } + /* Allocating new COL_INFO object. */ MYLOG(0, "PARSE: malloc at conn->col_info[%d]\n", conn->ntables); coli = conn->col_info[conn->ntables] = (COL_INFO *) malloc(sizeof(COL_INFO)); } @@ -915,6 +921,7 @@ getColumnsInfo(ConnectionClass *conn, TABLE_INFO *wti, OID greloid, StatementCla } col_info_initialize(coli); + coli->refcnt++; /* Counting one reference to coli object from connection COL_INFO cache table. */ coli->result = res; if (res && QR_get_num_cached_tuples(res) > 0) { @@ -969,7 +976,7 @@ MYLOG(DETAIL_LOG_LEVEL, "oid item == %s\n", (const char *) QR_get_value_backend_ MYLOG(0, "Created col_info table='%s', ntables=%d\n", PRINT_NAME(wti->table_name), conn->ntables); /* Associate a table from the statement with a SQLColumn info */ found = TRUE; - coli->refcnt++; + coli->refcnt++; /* Counting another one reference to coli object from TABLE_INFO wti object. */ wti->col_info = coli; } cleanup: diff --git a/results.c b/results.c index 37dd07a..934fce9 100644 --- a/results.c +++ b/results.c @@ -5007,6 +5007,7 @@ PGAPI_SetCursorName(HSTMT hstmt, return SQL_INVALID_HANDLE; } + NULL_THE_NAME(stmt->cursor_name); SET_NAME_DIRECTLY(stmt->cursor_name, make_string(szCursor, cbCursor, NULL, 0)); return SQL_SUCCESS; } diff --git a/statement.c b/statement.c index 0e04453..d2cc09f 100644 --- a/statement.c +++ b/statement.c @@ -277,6 +277,11 @@ PGAPI_FreeStmt(HSTMT hstmt, * before freeing the associated cursors. Otherwise * CC_cursor_count() would get wrong results. */ + if (stmt->parsed) + { + QR_Destructor(stmt->parsed); + stmt->parsed = NULL; + } res = SC_get_Result(stmt); QR_Destructor(res); SC_init_Result(stmt); @@ -495,6 +500,11 @@ SC_Destructor(StatementClass *self) QR_Destructor(res); } + if (self->parsed) + { + QR_Destructor(self->parsed); + self->parsed = NULL; + } SC_initialize_stmts(self, TRUE); @@ -1391,7 +1401,7 @@ SC_create_errorinfo(const StatementClass *self, PG_ErrorInfo *pgerror_fail_safe) pgerror = pgerror_fail_safe; pgerror->status = self->__error_number; pgerror->errorsize = sizeof(pgerror->__error_message); - STRCPY_FIXED(pgerror->__error_message, ermsg); + STRCPY_FIXED(pgerror->__error_message, ermsg); pgerror->recsize = -1; } else @@ -2047,7 +2057,7 @@ SC_execute(StatementClass *self) qflag &= (~READ_ONLY_QUERY); /* must be a SAVEPOINT after DECLARE */ } rhold = CC_send_query_append(conn, self->stmt_with_params, qryi, qflag, SC_get_ancestor(self), appendq); - first = rhold.first; + first = rhold.first; if (useCursor && QR_command_maybe_successful(first)) { /* @@ -2199,6 +2209,7 @@ MYLOG(DETAIL_LOG_LEVEL, "!!%p->miscinfo=%x res=%p\n", self, self->miscinfo, firs QR_set_fields(first, NULL); tres->num_fields = first->num_fields; QR_detach(first); + QR_Destructor(first); SC_set_Result(self, tres); rhold = self->rhold; } diff --git a/test/src/cte-test.c b/test/src/cte-test.c index a673ec0..97bad39 100644 --- a/test/src/cte-test.c +++ b/test/src/cte-test.c @@ -30,13 +30,13 @@ runTest(HSTMT hstmt) intparam = 3; cbParam1 = sizeof(intparam); rc = SQLBindParameter(hstmt, 1, SQL_PARAM_INPUT, - SQL_INTEGER, /* value type */ - SQL_INTEGER, /* param type */ - 0, /* column size (ignored for SQL_INTEGER) */ - 0, /* dec digits */ - &intparam, /* param value ptr */ - sizeof(intparam), /* buffer len (ignored for SQL_INTEGER) */ - &cbParam1 /* StrLen_or_IndPtr (ignored for SQL_INTEGER) */); + SQL_INTEGER, /* value type */ + SQL_INTEGER, /* param type */ + 0, /* column size (ignored for SQL_INTEGER) */ + 0, /* dec digits */ + &intparam, /* param value ptr */ + sizeof(intparam), /* buffer len (ignored for SQL_INTEGER) */ + &cbParam1 /* StrLen_or_IndPtr (ignored for SQL_INTEGER) */); CHECK_STMT_RESULT(rc, "SQLBindParameter failed", hstmt); /* Execute */ diff --git a/test/src/cursor-block-delete-test.c b/test/src/cursor-block-delete-test.c index 2069fe2..372b4c5 100644 --- a/test/src/cursor-block-delete-test.c +++ b/test/src/cursor-block-delete-test.c @@ -20,7 +20,7 @@ static SQLRETURN delete_loop(HSTMT hstmt) BOOL use_first_last = 0; int delcnt = 0, delsav, loopcnt = 0; SQLSMALLINT orientation = SQL_FETCH_FIRST; - + do { printf("\torientation=%d delete count=%d\n", orientation, delcnt); delsav = delcnt; @@ -51,7 +51,7 @@ int main(int argc, char **argv) int rc; HSTMT hstmt = SQL_NULL_HSTMT; int i, j, k; - int count = TOTAL; + int count = TOTAL; char query[100]; SQLLEN rowArraySize = BLOCK; SQLULEN rowsFetched; @@ -101,7 +101,7 @@ int main(int argc, char **argv) rc = SQLSetStmtAttr(hstmt, SQL_ATTR_CONCURRENCY, (SQLPOINTER) SQL_CONCUR_ROWVER, 0); CHECK_STMT_RESULT(rc, "SQLSetStmtAttr CONCURRENCY failed", hstmt); rc = SQLSetConnectAttr(conn, SQL_AUTOCOMMIT, (SQLPOINTER) SQL_AUTOCOMMIT_OFF, 0); - + rc = SQLSetStmtAttr(hstmt, SQL_ATTR_CURSOR_TYPE, (SQLPOINTER) SQL_CURSOR_KEYSET_DRIVEN, 0); CHECK_STMT_RESULT(rc, "SQLSetStmtAttr CURSOR_TYPE failed", hstmt); rc = SQLExecDirect(hstmt, (SQLCHAR *) "select * from tmptable", SQL_NTS); @@ -132,8 +132,8 @@ int main(int argc, char **argv) rc = SQLExecDirect(hstmte, (SQLCHAR *) "savepoint yuuki", SQL_NTS); CHECK_STMT_RESULT(rc, "savpoint failed", hstmte); } - } - + } + delete_loop(hstmt); /* the 2nd loop */ rc = SQLExecDirect(hstmte, (SQLCHAR *) "rollback to yuuki;release yuuki", SQL_NTS); @@ -150,7 +150,7 @@ int main(int argc, char **argv) CHECK_STMT_RESULT(rc, "SQLEndTran failed", hstmt); rc = SQLFreeStmt(hstmt, SQL_CLOSE); CHECK_STMT_RESULT(rc, "SQLFreeStmt failed", hstmt); - + /* Clean up */ test_disconnect(); diff --git a/test/src/premature-test.c b/test/src/premature-test.c index 9ef9ea1..e3a1218 100644 --- a/test/src/premature-test.c +++ b/test/src/premature-test.c @@ -34,13 +34,13 @@ runtest(const char *query, char *bind_before, char *bind_after, int execute) { cbParam1 = SQL_NTS; rc = SQLBindParameter(hstmt, 1, SQL_PARAM_INPUT, - SQL_C_CHAR, /* value type */ - SQL_CHAR, /* param type */ - 20, /* column size */ - 0, /* dec digits */ - bind_before, /* param value ptr */ - 0, /* buffer len */ - &cbParam1 /* StrLen_or_IndPtr */); + SQL_C_CHAR, /* value type */ + SQL_CHAR, /* param type */ + 20, /* column size */ + 0, /* dec digits */ + bind_before, /* param value ptr */ + 0, /* buffer len */ + &cbParam1 /* StrLen_or_IndPtr */); CHECK_STMT_RESULT(rc, "SQLBindParameter failed", hstmt); } @@ -53,16 +53,16 @@ runtest(const char *query, char *bind_before, char *bind_after, int execute) { cbParam1 = SQL_NTS; rc = SQLBindParameter(hstmt, 1, SQL_PARAM_INPUT, - SQL_C_CHAR, /* value type */ - SQL_CHAR, /* param type */ - 20, /* column size */ - 0, /* dec digits */ - bind_after, /* param value ptr */ - 0, /* buffer len */ - &cbParam1 /* StrLen_or_IndPtr */); + SQL_C_CHAR, /* value type */ + SQL_CHAR, /* param type */ + 20, /* column size */ + 0, /* dec digits */ + bind_after, /* param value ptr */ + 0, /* buffer len */ + &cbParam1 /* StrLen_or_IndPtr */); CHECK_STMT_RESULT(rc, "SQLBindParameter failed", hstmt); } - + /* Don't execute the statement! The row should not be inserted. */ /* And execute */ -- 2.39.5