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 <[email protected]> * Minor cosmetic changes. Signed-off-by: Alexandr Kuznetsov <[email protected]> * Forgot set col_info to NULL in TABLE_INFO object while clearing it. Signed-off-by: Alexandr Kuznetsov <[email protected]> * Fixing comments. Signed-off-by: Alexandr Kuznetsov <[email protected]> --------- Signed-off-by: Alexandr Kuznetsov <[email protected]>
postgresql-interfaces · May 24, 2024 · 6ca29b7 · 6ca29b7
1 parent 7ec2337
commit 6ca29b7
Show file tree

Hide file tree

Showing 12 changed files with 107 additions and 61 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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 */