leak in dbi->fetch_one and potentially other places

[GitMensch]

I don't know enough of the library (possibly also not about c++ either) and am primarily wondering here.
I've only checked ODBC and PG so far and the question applies to both.

Taking the following example -

gixsql/runtime/libgixsql-odbc/DbInterfaceODBC.cpp

Lines 353 to 388 in 6394c17

	int DbInterfaceODBC::_odbc_exec(ICursor* crsr, const std::string query, ODBCStatementData* prep_stmt_data)
	{
	int rc = 0;
	uint32_t nquery_cols = 0;
	std::string q = query;
	lib_logger->trace(FMT_FILE_FUNC "SQL: #{}#", __FILE__, __func__, q);
	ODBCStatementData* wk_rs = nullptr;
	if (!prep_stmt_data) {
	wk_rs = (ODBCStatementData*)((crsr != NULL) ? crsr->getPrivateData() : current_statement_data);
	if (wk_rs && wk_rs == current_statement_data) {
	delete current_statement_data;
	current_statement_data = nullptr;
	}
	wk_rs = new ODBCStatementData(conn_handle);
	if (!wk_rs->statement) {
	delete wk_rs;
	return DBERR_SQL_ERROR;
	}
	rc = SQLPrepare(wk_rs->statement, (SQLCHAR*)query.c_str(), SQL_NTS);
	if (odbcRetrieveError(rc, ErrorSource::Statement, wk_rs->statement) != SQL_SUCCESS) {
	return DBERR_SQL_ERROR;
	}
	}
	else {
	wk_rs = prep_stmt_data; // Already prepared
	}
	rc = SQLExecute(wk_rs->statement);
	if (odbcRetrieveError(rc, ErrorSource::Statement, wk_rs->statement) != SQL_SUCCESS) {
	return DBERR_SQL_ERROR;
	}

Would there be the need for delete wk_rs; before all of those return DBERR_SQL_ERROR;?

Should csr->getPrivateData be called and freed each time before it is set, like in

gixsql/runtime/libgixsql-odbc/DbInterfaceODBC.cpp

Lines 404 to 407 in 6394c17

	if (crsr->getPrivateData())
	delete (ODBCStatementData*)crsr->getPrivateData();
	crsr->setPrivateData(wk_rs);

or should setPrivateData do this itself?
The code for PG looks suspicious here, because the private data is not freed:

gixsql/runtime/libgixsql-pgsql/DbInterfacePGSQL.cpp

Lines 369 to 375 in 6394c17

	if (last_rc == PGRES_COMMAND_OK || last_rc == PGRES_TUPLES_OK) {
	if (crsr)
	crsr->setPrivateData(wk_rs);
	else
	current_resultset_data = wk_rs;
	return DBERR_NO_ERROR;

[mridoni]

Yes, you are right, wk_rs should be deleted in case of errors. I'll check the other places where this happen, then I'll fix it.

Thanks

[GitMensch]

That seems to be at least partially open in 1.0.18a (which is ok, I've just checked the lines referenced above to update this).

[mridoni]

That seems to be at least partially open in 1.0.18a (which is ok, I've just checked the lines referenced above to update this).

Yes, v1.0.18a is just a few hotfixes and a couple more things I could include "for free".

In more detail: the library initialization code was sometimes causing a hard crash in Gix-IDE (but had no apparent ill-effect on GixSQL itself). Since the two are connected, I had to update GixSQL to release a new version of Gix-IDE, and I threw in a couple more things (the 77 fields issue and a fix for the code generation in #88 which at this point still lacks the specific SQLCODE handling you pointed out in your last comments).

[mridoni]

I have some started some work here: this will probably best be implemented with a smart pointer and a custom deleter, but this will need some refactoring in all the drivers, so I am leaving it for post-1.0.19.

[GitMensch]

May I suggest to use milestones on GH?
This we we have "kind of a progress" view, some public visible planning, ...

[mridoni]

You are right. I am planning on releasing Gix-IDE 1.1.1 and GixSQL 1.0.19a in the first days of the next week. GixSQL v1.0.19a will be only a "re-alignment" version for some quick modifications that went into the version that will be shipping with Gix-IDE 1.1.1 (mostly internal stuff that Gix-IDE needs to generate metadata but that make no difference to the preprocessor itself).

I have just added a new milestone (v1.0.20) that for now hasn't a "due date". I want to perform some more triage before actually adding issues but I am planning to include #53, #109, #111, #120, #124, #125. Obviously feel free to make suggestions.

[GitMensch]

Obviously feel free to make suggestions.

#126, if done outside of automation (this can come later), is not really something to "directly" relate to code changes, but as the suite does exist I highly suggest to execute them (at least locally adding docs/help scripts to run them in the repo) to look out for issues. Fixing and implementing those are likely quite useful; if in question, then a "backlog" milestone helps, if issues in there are re-considered on each version planning for at least partial work in the next milestone

Three things that may not have an issue (not sure) that may be placed to a backlog, too:

• run the whole code through code analysis tools (cpp-check for offline/ci use and codecov for online checks come to mind)
• run the testsuite(s) automated via CI (this likely needs a docker setup to have the correct setup and reproducible state of DBs; an easy one may be sqlite with starting "DBs" that can be checked in directly [and be reverted at any time before starting a test run])
• run the (semi)-automated testsuite(s) with instrumentation; so far valgrind already found some issues and is not unlikely to find more; compiling with CPPFLAGS="-U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2" CXXFLAGS=-fstack-protector-strong LDFLAGS=-fstack-protector-strong would likely be a good start for gcc/clang, CXXFLAGS="-fsanitize=address -fsanitize=undefined -fno-omit-frame-pointer" LDFLAGS="-lasan -lubsan", too.

[mridoni]

I spotted a couple of places where this can be fixed, but since there are probably others, I am probably going to do what needed to be done anyway and start refactoring the interfaces and container objects to use smart pointers and C++-style allocation. This, by itself, should already solve several problems (I launched valgrind on some tests and it wasn't really happy) and make possible problems easier to spot.

By the way: I have (internally) modified the current test suite (still no NIST, but that will be coming too) to run under Linux: the main test runner is still written in C# under .Net Core, but, at least on Ubuntu, that's just a couple of "apt install" commands away and makes it possible to run tests with a GitHub Actions runner.

[GitMensch]

I spotted a couple of places where this can be fixed, but since there are probably others, I am probably going to do what needed to be done anyway and start refactoring the interfaces and container objects to use smart pointers and C++-style allocation. This, by itself, should already solve several problems (I launched valgrind on some tests and it wasn't really happy) and make possible problems easier to spot.

This sounds very promising and like the right approach! There's quite a lot in the 1.0.20 milestone already, do you have a rough ETA?
[BTW, more for GixIDE - GCs 3.2RC1 should be out this week, in case you want to possibly provide that]

I have (internally) modified the current test suite (still no NIST, but that will be coming too) to run under Linux: the main test runner is still written in C# under .Net Core, but, at least on Ubuntu, that's just a couple of "apt install" commands away and makes it possible to run tests with a GitHub Actions runner.

That sounds like good progress. Is executing the testsuite documented anywhere (I haven't looked hard)?

[mridoni]

This sounds very promising and like the right approach! There's quite a lot in the 1.0.20 milestone already, do you have a rough ETA?

No, not really, but I will begin working on it (actually I already have) and depending on how the work is progressing I'll try to establish one as soon as I can,

[BTW, more for GixIDE - GCs 3.2RC1 should be out this week, in case you want to possibly provide that]

Good, I'll try that

That sounds like good progress. Is executing the testsuite documented anywhere (I haven't looked hard)?

No, but now that I am going to use it in a CI setting and make its existence "official", I intend to document it. There were/are also a few things that were mainly thought for Visual Studio usage and with a certain DB setup (basically the one I use) and that I am trying to "generalize".

[mridoni]

The test suite can now be run with valgrind (and DrMemory on Windows). As soon as I have something more solid (and some docs) I will start to publish on GitHub the v1.0.20dev branch I have started working on. I have also done some preliminary work, replacing a lot of the C-style allocation stuff with smart pointers. For now this is only in the main runtime library and in the PostgreSQL driver, just to be able to run some tests but obviously I will have to extend this to all the drivers, since all tests are passing and valgrind now seems way happier (see below, this is a basic test run with a cursor).

==141030== Memcheck, a memory error detector
==141030== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==141030== Using Valgrind-3.18.1 and LibVEX; rerun with -h for copyright info
==141030== Command: TSQL001A
==141030== Parent PID: 140987
==141030==
==141030==
==141030== HEAP SUMMARY:
==141030==     in use at exit: 35,730 bytes in 93 blocks
==141030==   total heap usage: 10,186 allocs, 10,093 frees, 882,292 bytes allocated
==141030==
==141030== LEAK SUMMARY:
==141030==    definitely lost: 0 bytes in 0 blocks
==141030==    indirectly lost: 0 bytes in 0 blocks
==141030==      possibly lost: 0 bytes in 0 blocks
==141030==    still reachable: 35,730 bytes in 93 blocks
==141030==         suppressed: 0 bytes in 0 blocks
==141030== Reachable blocks (those to which a pointer was found) are not shown.
==141030== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==141030==
==141030== For lists of detected and suppressed errors, rerun with: -s
==141030== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

[mridoni]

I've pushed the v1.0.20 branch on GitHub: all the drivers now use smart pointers, so a lot of memory leaks should have been eradicated. The pgsql seems to be ok (see above), while I still have to check the other drivers. Since the test suite is being automated (see #122 (comment)) this work should move faster.

[mridoni]

This should be solved with the latest commit on the v1.0.20dev branch (dd46a20), now the tests all pass and show no definitely/possibly lost blocks in valgrind. I am marking this as ready. I need to do some triaging, then I will probably close this and #124 in preparation for v1.0.20.

marchetto@ubuntu-2204-lts-test-vm:/tmp/artifacts/1/gixsql-linux-test-1.0.20dev1$ cd /tmp/artifacts/1/gixsql-linux-test-1.0.20dev1/
marchetto@ubuntu-2204-lts-test-vm:/tmp/artifacts/1/gixsql-linux-test-1.0.20dev1$ grep 'definitely lost: 0 bytes' valgrind-TSQL0* -L
marchetto@ubuntu-2204-lts-test-vm:/tmp/artifacts/1/gixsql-linux-test-1.0.20dev1$ grep 'possibly lost: 0 bytes' valgrind-TSQL0* -L
marchetto@ubuntu-2204-lts-test-vm:/tmp/artifacts/1/gixsql-linux-test-1.0.20dev1$