Memory leaks detected in psqlodbc library

[progmachine]

Hello, I am working on my C++ applications database connectivity using ODBC. Recently i began running my unit tests with address sanitizer enabled, and LeakSanitizer (used by default with ASAN) detected two points of memory leaks within psqlodbc.

1. The first leak is simple, detected with password field, but i suspect it relates to 3 points of memory leaks with the same principle (ci->password, ci->conn_settings, ci->pqopt): in file dlg_specific.c, function copyConnAttributes, lines 627, 673, 678. If these parameters have been set in data source config, and also provided with connection string, then previous values will be just lost, causing memory leak. It is better to use NULL_THE_NAME macro before assigning new values. I placed macro in correct places, and it fixed this first memory leak point.

2. The second leak is much more complex, leaked objects allocated at parse.c:908, when using SQLDescribeCol function after 'select' SQL-statement, SQLDescribeCol will call getColumnsInfo down the execution code path. This function will allocate COL_INFO objects and place them in ConnectionClass object as columns information cache. Next step to reproduce this bug, is to call 'drop table' SQL-statement in the same connection, it will detect that cache is obsolete and call CC_clear_col_info function on ConnectionClass object, to clear the colinfo cache, but because it is not final destroy, it will see to refcnt field of COL_INFO objects, and leave this objects not destroyed, just detaching them from ConnectionClass object.

The second leak is complex, because leaked objects use reference counting lifetime management, and it is not obvious to me where to look for another points in code, where this lifetime management should be done. May be it's simple refcnt decrement was forgotten in CC_clear_col_info, may be not. For example, StatementClass objects also have references to these COL_INFO objects, but it seems not doing any refcnt lifetime management of COL_INFO objects. If we simply add decrement in CC_clear_col_info function, it will leave zombie references in StatementClass objects. I suspect that this refcnt lifetime management of COL_INFO objects is completely broken...

[davecramer]

Can you provide a PR for these fixes ?

[progmachine]

I can easily provide PR to fix the first memory leak point, in a few days, may be Sunday or Monday.
But i can't provide fix for the second leak point, because i don't know how to fix it correctly. If no one can fix it, i can do this job, but it will not be fast, and potentially can introduce bugs :(
Because it's new codebase for me, I don't know it's design, and it's pure C code, without classes, constructors/destructors etc...
What we need to do in this situation?

[davecramer]

@apgrucza Any chance you can look at this. You have been in the code around allocating memory more than anyone lately ?

[apgrucza]

I feel much the same as @progmachine in that I don't really understand the relationships between the various structs and the intended design around ownership of allocated memory. The mimalloc changes I made were simply adding a drop-in replacement for malloc which did not require me to understand all this. Without modern language features such a destructors and smart pointers, I'm not surprised there are memory leaks.

When I added mimalloc and enabled MIMALLOC_SHOW_STATS, I did notice that it reported memory leaks (and the reports were more detailed in debug builds). But the problem does not lie with detecting memory leaks but knowing the correct way to fix them. I noticed the _MEMORY_DEBUG_ symbol was added by @hiinoue for the purposes of finding memory leaks, but that was 18 years ago so I doubt he'd be able to assist now.

If @progmachine were to attempt a fix, I wonder whether the current tests are comprehensive enough to guarantee that it does not introduce any bugs.

[davecramer]

@apgrucza OK, can you provide the report with all of the memory leaks ?
I think we need to try to fix this, and no I don't think @hiinoue is available any more.

[apgrucza]

Leaks were detected in the following tests:

11 - premature.txt
13 - param-conversions.txt
27 - cursor-name.txt
40 - diagnostic.txt
44 - odbc-escapes.txt
47 - fetch-refcursors.txt

As you can see, mimalloc doesn't tell you where in the code the memory leaks are. Although it can integrate with memory tracking tools, the readme recommends using the standard allocator to debug memory leaks. @progmachine are you able to provide a report from your tool whilst running the psqlODBC tests?

[davecramer]

so looking at the code

for (i = 0; i < self->ntables; i++)
		{
			if (coli = self->col_info[i], NULL != coli)
			{
				if (destroy || coli->refcnt == 0)
				{
					free_col_info_contents(coli);
					free(coli);
					self->col_info[i] = NULL;
				}
				else
					coli->acc_time = 0;
			}
		}
		self->ntables = 0;

The issue is that we set ntables to 0 regardless if the column info has been freed or not.

The naive solution to this is to re-order the column info as we delete them and use the correct value for ntables.

Thoughts ?

[progmachine]

That's incorrect. What this function doing, is completely "releasing" COL_INFO objects from ConnectionClass object. No mater if these objects actually destroyed or not, connection object will not have any one of them. So it's just zeroing self->ntables, and it is correct.
The problem is:

• if "destroy" is true (when connection is closing), COL_INFO objects will always be deleted. That's good.
• but if "destroy" is false (col_info cache is obsolete), COL_INFO objects will not be deleted, just detached from ConnectionClass object, without correct releasing using refcnt. That's bad :(

Today i finally had some spare time to begin working on psqlodbc project, in the near future i will post detailed memory leaks report from tests. And maybe will begin fixing these issues.

[davecramer]

That's incorrect. What this function doing, is completely "releasing" COL_INFO objects from ConnectionClass object. No mater if these objects actually destroyed or not, connection object will not have any one of them. So it's just zeroing self->ntables, and it is correct. The problem is:

• if "destroy" is true (when connection is closing), COL_INFO objects will always be deleted. That's good.
• but if "destroy" is false (col_info cache is obsolete), COL_INFO objects will not be deleted, just detached from ConnectionClass object, without correct releasing using refcnt. That's bad :(

Help me out here, where are they "released" ?

Today i finally had some spare time to begin working on psqlodbc project, in the near future i will post detailed memory leaks report from tests. And maybe will begin fixing these issues.

That would be excellent!

[progmachine]

Help me out here, where are they "released" ?

I took word "release" in quotes, with purpuse to indicate, that this action is done with deviations, and must be fixed. But overall meaning is, that COL_INFO objects must be released from ConnectionClass object with refcnt mechanizm, and anyhow ConnectionClass object will not have any of them, so operand self->ntables = 0; is correct.

[davecramer]

I'm trying to see where they are detached? All I see is that if refcount is not zero they are skipped and still self->ntables is set to 0 suggesting that there are no tables in the ConnectionClass object

[progmachine]

... and still self->ntables is set to 0 suggesting that there are no tables in the ConnectionClass object

CC_clear_col_info function makes exactly that: ConnectionClass object will not have any cached COL_INFO objects, after it's called.

All I see is that if refcount is not zero they are skipped

refcnt lifetime management works like that - if refcnt is not zero then somewhere also is reference to object exist, and object must be destroyed somewhere else, where refcnt will be 0. std::shared_ptr in C++ works with the same principle.

I strongly suspect, that this refcnt mechanize is broken, and destroy function parameter used in if statement alongside with refcnt, is a crutch, designed to avoid rude memory leaks, while working by standard way.

[davecramer]

see #18 for a possible fix. I agree the refcount is likely broken, but first we would need tests to make sure we don't make it workse

[progmachine]

First, partially successful run of tests, produced leaks. Some tests failed with messages AddressSanitizer:DEADLYSIGNAL and did not produced any results. I think, we need to debug all tests set with ASAN enabled, alongside with memleaks fixing :)
And i feel, it is a lot of work :-\

[davecramer]

So this shows where they were allocated, but doesn't show where the leak occurs ?