[patch_done] Possible Memory Leakage

[klchin]

Hi,

Compiling version '6.6.5-stable' with C++B5

Warning Value assigned to 'Handle' never used under

[code]

unit ZDbcPostgreSqlUtils;

procedure CheckPostgreSQLError(Connection: IZConnection;
PlainDriver: IZPostgreSQLPlainDriver;
Handle: PZPostgreSQLConnect; LogCategory: TZLoggingCategory;
LogMessage: string;
ResultHandle: PZPostgreSQLResult);

if PlainDriver.GetStatus(Handle) = CONNECTION_BAD then
begin
PlainDriver.Finish(Handle);
Handle := nil; // Warning
end;

[/code]

Look like a memory leakage here.

[trupka]

IMO, its not leak but necessity as stated here: http://zeos.firmos.at/viewtopic.php?t=2376 and in postgresql docs (PQfinish function).

Since it's second time somebody recognized this as a bug, it might be good to:
- somebody disprove my opinion so we can fix this as a bug
or
- turn off hints where appropriate because its not bug but "feature" e.g.

Code: Select all

if PlainDriver.GetStatus(Handle) = CONNECTION_BAD then
begin
PlainDriver.Finish(Handle);
{$HINTS OFF}
Handle := nil; // Please ignore compiler hint for this line.
end;

[klchin]

Here some points

a) Will Finish/PQFinish really free it?

b) Since Handle in general was a POINTER, so setting to
NIL was redandunt, unless it was COM object

c) If set to NIL, then it should be declare as
"var Handle : PZPostgreSQLConnect;" to reflect the rest
of the code, otherwise it could be AV.

[trupka]

Here some points

a) Will Finish/PQFinish really free it?

PQFinish points to PGconn structure internali allocated by dll. The contents of this structure are not supposed to be known to applications. Postgres docs says:
---
PQfinish
Closes the connection to the server. Also frees memory used by the PGconn object.
void PQfinish(PGconn *conn);
Note that even if the server connection attempt fails (as indicated by PQstatus), the application should call PQfinish to free the memory used by the PGconn object. The PGconn pointer must not be used again after PQfinish has been called
---
So I must conclude: Yes, Finish will free the memory but, after that, Handle will point to invalid memory location. It's logical to set it to nil but...

b) Since Handle in general was a POINTER, so setting to
NIL was redandunt, unless it was COM object

c) If set to NIL, then it should be declare as
"var Handle : PZPostgreSQLConnect;" to reflect the rest
of the code, otherwise it could be AV.

...my mistake is that I overlooked missing var in param declaration. Obviously Handle := nil currently don't do anything - that line isn't of any use, but I don't see it as harmful either. I'll try to investigate this further - what really happens inside ZDBCPostgresqlUtils when connection becomes bad and what happens with application after that...

[mdaems]

@trupka,

You overlooked it. I just didn't really know. I had to check if pascal passes parameters by values when 'var' isn't in the parameter declaration.

Did a quick test run. Doesn't seem to break the test suite. And the warning is gone.

@klchin,

Thanks for reporting. I did commit the change right now. SVN Rev. 671.

Mark

[klchin]

Hi Mark,

I had downlaoded the SVN 671.

Under ZClasses.pas

const
ZEOS_VERSION = '7.0.0-dev';

Is this correct version?

Regards,
KL Chin

[mdaems]

Downloaded from the snapshots directory? Yes, that should be the right one. It's the 7.X testing branch version.
I'll do the changes to trunk and the 6.6-patches branch in a while. So it will be in the next maintenance release for the 6.6 series.

Mark