Skip to content

Commit 6ca29b7

Browse files
progmachineprogmachine
authored
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]>
1 parent 7ec2337 commit 6ca29b7

File tree

12 files changed

+107
-61
lines changed

12 files changed

+107
-61
lines changed

‎bind.c

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -701,6 +701,8 @@ IPD_free_params(IPDFields *ipdopts, char option)
701701
return;
702702
if (option == STMT_FREE_PARAMS_ALL)
703703
{
704+
for (int i = 0; i < ipdopts->allocated; ++i)
705+
NULL_THE_NAME(ipdopts->parameters[i].paramName);
704706
free(ipdopts->parameters);
705707
ipdopts->parameters = NULL;
706708
ipdopts->allocated = 0;

‎connection.c

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -553,21 +553,28 @@ CC_clear_col_info(ConnectionClass *self, BOOL destroy)
553553

554554
for (i = 0; i < self->ntables; i++)
555555
{
556+
/* Going through COL_INFO cache table and releasing coli objects. */
556557
if (coli = self->col_info[i], NULL != coli)
557558
{
558-
if (destroy || coli->refcnt == 0)
559+
coli->refcnt--;
560+
if (coli->refcnt <= 0)
559561
{
562+
/* Last reference to coli object disappeared. Now destroying it. */
560563
free_col_info_contents(coli);
561564
free(coli);
562565
self->col_info[i] = NULL;
563566
}
564567
else
568+
{
569+
/* coli object have another reference to it, so it will be destroyed somewhere else. */
565570
coli->acc_time = 0;
571+
}
566572
}
567573
}
568-
self->ntables = 0;
574+
self->ntables = 0; /* Now we have cleared COL_INFO cached objects table. */
569575
if (destroy)
570576
{
577+
/* We destroying COL_INFO cache completely. */
571578
free(self->col_info);
572579
self->col_info = NULL;
573580
self->coli_allocated = 0;
@@ -966,7 +973,7 @@ handle_pgres_error(ConnectionClass *self, const PGresult *pgres,
966973
CC_on_abort(self, CONN_DEAD); /* give up the connection */
967974
}
968975
else if ((errseverity_nonloc && strcmp(errseverity_nonloc, "FATAL") == 0) ||
969-
(NULL == errseverity_nonloc && errseverity && strcmp(errseverity, "FATAL") == 0)) /* no */
976+
(NULL == errseverity_nonloc && errseverity && strcmp(errseverity, "FATAL") == 0)) /* no */
970977
{
971978
CC_set_errornumber(self, CONNECTION_SERVER_REPORTED_SEVERITY_FATAL);
972979
CC_on_abort(self, CONN_DEAD); /* give up the connection */
@@ -2873,7 +2880,7 @@ LIBPQ_connect(ConnectionClass *self)
28732880
QLOG(0, "PQconnectdbParams:");
28742881
for (popt = opts, pval = vals; *popt; popt++, pval++)
28752882
QPRINTF(0, " %s='%s'", *popt, *pval);
2876-
QPRINTF(0, "\n");
2883+
QPRINTF(0, "\n");
28772884
}
28782885
pqconn = PQconnectdbParams(opts, vals, FALSE);
28792886
if (!pqconn)

‎convert.c

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3056,7 +3056,7 @@ MYLOG(DETAIL_LOG_LEVEL, "type=" FORMAT_UINTEGER " concur=" FORMAT_UINTEGER "\n",
30563056
}
30573057
if (SC_is_fetchcursor(stmt))
30583058
{
3059-
snprintfcat(new_statement, qb->str_alsize,
3059+
snprintfcat(new_statement, qb->str_alsize,
30603060
"declare \"%s\"%s cursor%s for ",
30613061
SC_cursor_name(stmt), opt_scroll, opt_hold);
30623062
qb->npos = strlen(new_statement);
@@ -3159,7 +3159,7 @@ MYLOG(DETAIL_LOG_LEVEL, "type=" FORMAT_UINTEGER " concur=" FORMAT_UINTEGER "\n",
31593159
CVT_APPEND_STR(qb, bestitem);
31603160
}
31613161
CVT_APPEND_STR(qb, "\" from ");
3162-
CVT_APPEND_DATA(qb, qp->statement + qp->from_pos + 5, npos - qp->from_pos - 5);
3162+
CVT_APPEND_DATA(qb, qp->statement + qp->from_pos + 5, qp->stmt_len - qp->from_pos - 5);
31633163
}
31643164
}
31653165
npos -= qp->declare_pos;
@@ -3744,7 +3744,7 @@ inner_process_tokens(QueryParse *qp, QueryBuild *qb)
37443744
}
37453745
if (converted)
37463746
{
3747-
const char *column_def = (const char *) QR_get_value_backend_text(coli->result, i, COLUMNS_COLUMN_DEF);
3747+
const char *column_def = (const char *) QR_get_value_backend_text(coli->result, i, COLUMNS_COLUMN_DEF);
37483748
if (NULL != column_def &&
37493749
strncmp(column_def, "nextval", 7) == 0)
37503750
{
@@ -3770,6 +3770,7 @@ inner_process_tokens(QueryParse *qp, QueryBuild *qb)
37703770
}
37713771
}
37723772
}
3773+
TI_ClearObject(pti);
37733774
}
37743775
if (!converted)
37753776
CVT_APPEND_STR(qb, "NULL");

‎descriptor.c

Lines changed: 27 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -41,26 +41,39 @@ MYLOG(DETAIL_LOG_LEVEL, "entering count=%d\n", count);
4141
{
4242
if (ti[i])
4343
{
44-
COL_INFO *coli = ti[i]->col_info;
45-
if (coli)
46-
{
47-
MYLOG(0, "!!!refcnt %p:%d -> %d\n", coli, coli->refcnt, coli->refcnt - 1);
48-
coli->refcnt--;
49-
if (coli->refcnt <= 0 && 0 == coli->acc_time) /* acc_time == 0 means the table is dropped */
50-
free_col_info_contents(coli);
51-
}
52-
NULL_THE_NAME(ti[i]->schema_name);
53-
NULL_THE_NAME(ti[i]->table_name);
54-
NULL_THE_NAME(ti[i]->table_alias);
55-
NULL_THE_NAME(ti[i]->bestitem);
56-
NULL_THE_NAME(ti[i]->bestqual);
57-
TI_Destroy_IH(ti[i]);
44+
TI_ClearObject(ti[i]);
5845
free(ti[i]);
5946
ti[i] = NULL;
6047
}
6148
}
6249
}
6350
}
51+
void TI_ClearObject(TABLE_INFO *ti)
52+
{
53+
if (ti)
54+
{
55+
COL_INFO *coli = ti->col_info;
56+
if (coli)
57+
{
58+
MYLOG(0, "!!!refcnt %p:%d -> %d\n", coli, coli->refcnt, coli->refcnt - 1);
59+
coli->refcnt--;
60+
if (coli->refcnt <= 1 && 0 == coli->acc_time) /* acc_time == 0 means the table is dropped */
61+
free_col_info_contents(coli); /* Now coli object is unused, and may be reused later. */
62+
if (coli->refcnt <= 0)
63+
{
64+
/* Last reference to coli object disappeared. Now destroying it. */
65+
free(coli);
66+
ti->col_info = NULL;
67+
}
68+
}
69+
NULL_THE_NAME(ti->schema_name);
70+
NULL_THE_NAME(ti->table_name);
71+
NULL_THE_NAME(ti->table_alias);
72+
NULL_THE_NAME(ti->bestitem);
73+
NULL_THE_NAME(ti->bestqual);
74+
TI_Destroy_IH(ti);
75+
}
76+
}
6477
void FI_Constructor(FIELD_INFO *self, BOOL reuse)
6578
{
6679
MYLOG(DETAIL_LOG_LEVEL, "entering reuse=%d\n", reuse);

‎descriptor.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@ typedef struct
5555
#define TI_set_has_no_subclass(ti) (ti->flags &= (~TI_HASSUBCLASS))
5656
void TI_Constructor(TABLE_INFO *, const ConnectionClass *);
5757
void TI_Destructor(TABLE_INFO **, int);
58+
void TI_ClearObject(TABLE_INFO *ti);
5859
void TI_Create_IH(TABLE_INFO *ti);
5960
void TI_Destroy_IH(TABLE_INFO *ti);
6061
const pgNAME TI_From_IH(TABLE_INFO *ti, OID tableoid);

‎dlg_specific.c

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -624,6 +624,7 @@ copyConnAttributes(ConnInfo *ci, const char *attribute, const char *value)
624624
STRCPY_FIXED(ci->username, value);
625625
else if (stricmp(attribute, INI_PASSWORD) == 0 || stricmp(attribute, "pwd") == 0)
626626
{
627+
NULL_THE_NAME(ci->password);
627628
ci->password = decode_or_remove_braces(value);
628629
#ifndef FORCE_PASSWORDE_DISPLAY
629630
MYLOG(0, "key='%s' value='xxxxxxxx'\n", attribute);
@@ -669,11 +670,13 @@ copyConnAttributes(ConnInfo *ci, const char *attribute, const char *value)
669670
else if (stricmp(attribute, INI_CONNSETTINGS) == 0 || stricmp(attribute, ABBR_CONNSETTINGS) == 0)
670671
{
671672
/* We can use the conn_settings directly when they are enclosed with braces */
673+
NULL_THE_NAME(ci->conn_settings);
672674
ci->conn_settings_in_str = TRUE;
673675
ci->conn_settings = decode_or_remove_braces(value);
674676
}
675677
else if (stricmp(attribute, INI_PQOPT) == 0 || stricmp(attribute, ABBR_PQOPT) == 0)
676678
{
679+
NULL_THE_NAME(ci->pqopt);
677680
ci->pqopt_in_str = TRUE;
678681
ci->pqopt = decode_or_remove_braces(value);
679682
}

‎parse.c

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -713,8 +713,7 @@ MYPRINTF(0, "->%d\n", updatable);
713713
}
714714

715715
static BOOL
716-
getCOLIfromTable(ConnectionClass *conn, pgNAME *schema_name, pgNAME table_name,
717-
COL_INFO **coli)
716+
getCOLIfromTable(ConnectionClass *conn, pgNAME *schema_name, pgNAME table_name, COL_INFO **coli)
718717
{
719718
int colidx;
720719
BOOL found = FALSE;
@@ -836,11 +835,13 @@ getColumnsInfo(ConnectionClass *conn, TABLE_INFO *wti, OID greloid, StatementCla
836835
MYLOG(0, " Success\n");
837836
if (greloid != 0)
838837
{
838+
/* We have reloid. Try to find appropriate coli object from connection COL_INFO cache. */
839839
for (k = 0; k < conn->ntables; k++)
840840
{
841841
tcoli = conn->col_info[k];
842842
if (tcoli->table_oid == greloid)
843843
{
844+
/* We found appropriate coli object, so we will use it. */
844845
coli = tcoli;
845846
coli_exist = TRUE;
846847
break;
@@ -849,42 +850,46 @@ getColumnsInfo(ConnectionClass *conn, TABLE_INFO *wti, OID greloid, StatementCla
849850
}
850851
if (!coli_exist)
851852
{
853+
/* Not found, try to find unused coli or oldest (if overflow) in connection COL_INFO cache. */
852854
for (k = 0; k < conn->ntables; k++)
853855
{
854856
tcoli = conn->col_info[k];
855-
if (0 < tcoli->refcnt)
856-
continue;
857+
if (1 < tcoli->refcnt)
858+
continue; /* This coli object is used somewhere else, skipping it. */
857859
if ((0 == tcoli->table_oid &&
858860
NAME_IS_NULL(tcoli->table_name)) ||
859861
strnicmp(SAFE_NAME(tcoli->schema_name), "pg_temp_", 8) == 0)
860862
{
863+
/* Found unused coli object, taking it. */
861864
coli = tcoli;
862865
coli_exist = TRUE;
863866
break;
864867
}
865-
if (NULL == ccoli ||
866-
tcoli->acc_time < acctime)
868+
if (NULL == ccoli || tcoli->acc_time < acctime)
867869
{
870+
/* Not yet found. Alongside, searching least recently used coli object. */
868871
ccoli = tcoli;
869872
acctime = tcoli->acc_time;
870873
}
871874
}
872-
if (!coli_exist &&
873-
NULL != ccoli &&
874-
conn->ntables >= COLI_RECYCLE)
875+
if (!coli_exist && NULL != ccoli && conn->ntables >= COLI_RECYCLE)
875876
{
877+
/* Not found unsed object. Amount of them is on limit. Taking least recently used coli object. */
876878
coli_exist = TRUE;
877879
coli = ccoli;
878880
}
879881
}
880882
if (coli_exist)
881883
{
884+
/* We have ready to use coli object. Cleaning it. */
882885
free_col_info_contents(coli);
883886
}
884887
else
885888
{
889+
/* We have no coli object. Must create a new one. */
886890
if (conn->ntables >= conn->coli_allocated)
887891
{
892+
/* No place in connection COL_INFO cache table. Allocating or reallocating. */
888893
Int2 new_alloc;
889894
COL_INFO **col_info;
890895

@@ -904,6 +909,7 @@ getColumnsInfo(ConnectionClass *conn, TABLE_INFO *wti, OID greloid, StatementCla
904909
conn->coli_allocated = new_alloc;
905910
}
906911

912+
/* Allocating new COL_INFO object. */
907913
MYLOG(0, "PARSE: malloc at conn->col_info[%d]\n", conn->ntables);
908914
coli = conn->col_info[conn->ntables] = (COL_INFO *) malloc(sizeof(COL_INFO));
909915
}
@@ -915,6 +921,7 @@ getColumnsInfo(ConnectionClass *conn, TABLE_INFO *wti, OID greloid, StatementCla
915921
}
916922
col_info_initialize(coli);
917923

924+
coli->refcnt++; /* Counting one reference to coli object from connection COL_INFO cache table. */
918925
coli->result = res;
919926
if (res && QR_get_num_cached_tuples(res) > 0)
920927
{
@@ -969,7 +976,7 @@ MYLOG(DETAIL_LOG_LEVEL, "oid item == %s\n", (const char *) QR_get_value_backend_
969976
MYLOG(0, "Created col_info table='%s', ntables=%d\n", PRINT_NAME(wti->table_name), conn->ntables);
970977
/* Associate a table from the statement with a SQLColumn info */
971978
found = TRUE;
972-
coli->refcnt++;
979+
coli->refcnt++; /* Counting another one reference to coli object from TABLE_INFO wti object. */
973980
wti->col_info = coli;
974981
}
975982
cleanup:

‎results.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5007,6 +5007,7 @@ PGAPI_SetCursorName(HSTMT hstmt,
50075007
return SQL_INVALID_HANDLE;
50085008
}
50095009

5010+
NULL_THE_NAME(stmt->cursor_name);
50105011
SET_NAME_DIRECTLY(stmt->cursor_name, make_string(szCursor, cbCursor, NULL, 0));
50115012
return SQL_SUCCESS;
50125013
}

‎statement.c

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -277,6 +277,11 @@ PGAPI_FreeStmt(HSTMT hstmt,
277277
* before freeing the associated cursors. Otherwise
278278
* CC_cursor_count() would get wrong results.
279279
*/
280+
if (stmt->parsed)
281+
{
282+
QR_Destructor(stmt->parsed);
283+
stmt->parsed = NULL;
284+
}
280285
res = SC_get_Result(stmt);
281286
QR_Destructor(res);
282287
SC_init_Result(stmt);
@@ -495,6 +500,11 @@ SC_Destructor(StatementClass *self)
495500

496501
QR_Destructor(res);
497502
}
503+
if (self->parsed)
504+
{
505+
QR_Destructor(self->parsed);
506+
self->parsed = NULL;
507+
}
498508

499509
SC_initialize_stmts(self, TRUE);
500510

@@ -1391,7 +1401,7 @@ SC_create_errorinfo(const StatementClass *self, PG_ErrorInfo *pgerror_fail_safe)
13911401
pgerror = pgerror_fail_safe;
13921402
pgerror->status = self->__error_number;
13931403
pgerror->errorsize = sizeof(pgerror->__error_message);
1394-
STRCPY_FIXED(pgerror->__error_message, ermsg);
1404+
STRCPY_FIXED(pgerror->__error_message, ermsg);
13951405
pgerror->recsize = -1;
13961406
}
13971407
else
@@ -2047,7 +2057,7 @@ SC_execute(StatementClass *self)
20472057
qflag &= (~READ_ONLY_QUERY); /* must be a SAVEPOINT after DECLARE */
20482058
}
20492059
rhold = CC_send_query_append(conn, self->stmt_with_params, qryi, qflag, SC_get_ancestor(self), appendq);
2050-
first = rhold.first;
2060+
first = rhold.first;
20512061
if (useCursor && QR_command_maybe_successful(first))
20522062
{
20532063
/*
@@ -2199,6 +2209,7 @@ MYLOG(DETAIL_LOG_LEVEL, "!!%p->miscinfo=%x res=%p\n", self, self->miscinfo, firs
21992209
QR_set_fields(first, NULL);
22002210
tres->num_fields = first->num_fields;
22012211
QR_detach(first);
2212+
QR_Destructor(first);
22022213
SC_set_Result(self, tres);
22032214
rhold = self->rhold;
22042215
}

‎test/src/cte-test.c

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -30,13 +30,13 @@ runTest(HSTMT hstmt)
3030
intparam = 3;
3131
cbParam1 = sizeof(intparam);
3232
rc = SQLBindParameter(hstmt, 1, SQL_PARAM_INPUT,
33-
SQL_INTEGER, /* value type */
34-
SQL_INTEGER, /* param type */
35-
0, /* column size (ignored for SQL_INTEGER) */
36-
0, /* dec digits */
37-
&intparam, /* param value ptr */
38-
sizeof(intparam), /* buffer len (ignored for SQL_INTEGER) */
39-
&cbParam1 /* StrLen_or_IndPtr (ignored for SQL_INTEGER) */);
33+
SQL_INTEGER, /* value type */
34+
SQL_INTEGER, /* param type */
35+
0, /* column size (ignored for SQL_INTEGER) */
36+
0, /* dec digits */
37+
&intparam, /* param value ptr */
38+
sizeof(intparam), /* buffer len (ignored for SQL_INTEGER) */
39+
&cbParam1 /* StrLen_or_IndPtr (ignored for SQL_INTEGER) */);
4040
CHECK_STMT_RESULT(rc, "SQLBindParameter failed", hstmt);
4141

4242
/* Execute */

0 commit comments

Comments
 (0)