pg.ddx.io pgsql-interfaces@postgresql.org mailing list archive
help / color / mirror / Atom feedlibpq++ tracing considered harmful (was Re: libpq++ memory problems)
6+ messages / 2 participants
[nested] [flat]
* libpq++ tracing considered harmful (was Re: libpq++ memory problems)
@ 2000-04-20 17:41 Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 6+ messages in thread
From: Tom Lane @ 2000-04-20 17:41 UTC (permalink / raw)
To: Lamar Owen <lamar.owen@wgcr.org>; +Cc: Tim Brookes <tim.brookes@mcmail.com>; pgsql-interfaces
Ah-hah, figured it out. There *was* a recent relevant change, but the
log message for it didn't say anything about leaks. The problem is
in the "debug" code for PgConnection, which in 6.5.* looks like:
// PgConnection::connect
// establish a connection to a backend
ConnStatusType PgConnection::Connect(const char* conninfo)
{
ConnStatusType cst;
// Turn the trace on
#if defined(DEBUG)
FILE *debug = fopen("/tmp/trace.out","w");
PQtrace(pgConn, debug);
#endif
// Connect to the database
pgConn = PQconnectdb(conninfo);
This is busted in at least five ways:
1. "DEBUG" is a symbol defined in the backend header files (it's a
severity level constant for elog()). So although libpq++'s Makefile
thinks it can turn the code on or off via -DDEBUG, it's mistaken;
that debug-tracing code always gets compiled.
Oliver Elphick fixed that a month or so ago by changing the
controlling symbol to "DEBUGFILE". So that's why you see the leak
in 6.5.3 and I don't see it in current sources.
2. The fopen'd file is never closed. (The destructor calls PQuntrace
but that routine isn't responsible for closing the file.) That's
the direct cause of the memory and file descriptor leak you see:
a stdio file is fopen'd and never fclose'd for each PgConnection
created.
3. The whole thing is a complete waste of time because the pgConn
object doesn't exist yet --- it's not created till the next line!
So PQtrace is being handed either a null pgConn pointer (which I
believe it will silently ignore) or a garbage pointer (which could
have who-knows-what unpleasant consequences). But in no case will
any tracing actually occur. Sheesh.
4. If any tracing did occur, all of the output from (perhaps many)
different PgConnection objects in different program runs would all
get dumped into the same temp file, leaving it of doubtful use to
anybody.
5. One could reasonably doubt that it's a good idea to have a compiled-in
option to dump the entire trace of a program's interaction with the
backend into a publicly readable temp file. Can you say "security
hole"? Seems to me that this function should require a specific
request from the application, not be something that could accidentally
get installed as the default behavior of a system library. (Think
what would happen if RPMs containing such behavior got distributed...)
Perhaps something can be salvaged from the wreckage, but for now the
right answer is just to make sure that this code is not compiled.
regards, tom lane
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: libpq++ tracing considered harmful (was Re: libpq++ memory problems)
@ 2000-04-20 18:06 Lamar Owen <lamar.owen@wgcr.org>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 6+ messages in thread
From: Lamar Owen @ 2000-04-20 18:06 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Tim Brookes <tim.brookes@mcmail.com>; pgsql-interfaces
Tom Lane wrote:
> Ah-hah, figured it out. There *was* a recent relevant change, but the
> log message for it didn't say anything about leaks. The problem is
> in the "debug" code for PgConnection, which in 6.5.* looks like:
[snip]
> 1. "DEBUG" is a symbol defined in the backend header files (it's a
> severity level constant for elog()). So although libpq++'s Makefile
> thinks it can turn the code on or off via -DDEBUG, it's mistaken;
> that debug-tracing code always gets compiled.
> Oliver Elphick fixed that a month or so ago by changing the
> controlling symbol to "DEBUGFILE". So that's why you see the leak
> in 6.5.3 and I don't see it in current sources.
> 4. If any tracing did occur, all of the output from (perhaps many)
> different PgConnection objects in different program runs would all
> get dumped into the same temp file, leaving it of doubtful use to
> anybody.
> 5. One could reasonably doubt that it's a good idea to have a compiled-in
> option to dump the entire trace of a program's interaction with the
> backend into a publicly readable temp file. Can you say "security
> hole"? Seems to me that this function should require a specific
Hmmmm..
> request from the application, not be something that could accidentally
> get installed as the default behavior of a system library. (Think
> what would happen if RPMs containing such behavior got distributed...)
Yes, think of it. How many other such 'surprises' lurk, I wonder?
> Perhaps something can be salvaged from the wreckage, but for now the
> right answer is just to make sure that this code is not compiled.
And to compile this code you supply _which_ configure option? Which is
the default (which is how the RPMset is built)? Or is this like one of
the many options in config.h.in (or config.h, depending on which side of
the configure divide you prefer to edit....), like so many other options
that aren't configure'able?
This is as good an argument as any for a single, canonical, RPMset as
I've seen in a while, Tom. I (and others in the development group who
can help watchdog the RPMset) just have to be extra-diligent to make
sure stuff like compiling this freakish code doesn't happen.
--
Lamar Owen
WGCR Internet Radio
1 Peter 4:11
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: libpq++ tracing considered harmful (was Re: libpq++ memory problems)
@ 2000-04-20 18:22 Tom Lane <tgl@sss.pgh.pa.us>
parent: Lamar Owen <lamar.owen@wgcr.org>
0 siblings, 1 reply; 6+ messages in thread
From: Tom Lane @ 2000-04-20 18:22 UTC (permalink / raw)
To: Lamar Owen <lamar.owen@wgcr.org>; +Cc: Tim Brookes <tim.brookes@mcmail.com>; pgsql-interfaces
Lamar Owen <lamar.owen@wgcr.org> writes:
>> Perhaps something can be salvaged from the wreckage, but for now the
>> right answer is just to make sure that this code is not compiled.
> And to compile this code you supply _which_ configure option?
It isn't a configure option --- you have to edit libpq++'s makefile
to enable it. Or, in the case of the 6.5.* code, it got turned on
anyway :-(. Fortunately there is no security hole in either 6.5.*
or current, since the code is too broken to actually produce any
trace output, compiled or not. My point was mainly that I thought
we should remove the code rather than fix it.
regards, tom lane
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: libpq++ tracing considered harmful (was Re: libpq++ memory problems)
@ 2000-04-20 18:28 Lamar Owen <lamar.owen@wgcr.org>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 6+ messages in thread
From: Lamar Owen @ 2000-04-20 18:28 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Tim Brookes <tim.brookes@mcmail.com>; pgsql-interfaces
Tom Lane wrote:
> Lamar Owen <lamar.owen@wgcr.org> writes:
> >> Perhaps something can be salvaged from the wreckage, but for now the
> >> right answer is just to make sure that this code is not compiled.
> > And to compile this code you supply _which_ configure option?
> It isn't a configure option --- you have to edit libpq++'s makefile
> to enable it. Or, in the case of the 6.5.* code, it got turned on
> anyway :-(.
Now that is just grand. It will be a happy day once all the various
configuration options, both runtime and compiletime, are brought under
the GrandReUnification umbrella that Peter E is weaving right now....
> Fortunately there is no security hole in either 6.5.*
> or current, since the code is too broken to actually produce any
> trace output, compiled or not.
That's what I thought you said. Which, in this case, a bug was a good
thing -- as if the code hadn't been buggy, we'd have had a security bug.
~:-]
> My point was mainly that I thought
> we should remove the code rather than fix it.
Who added the code in the first place? Would anyone _need_ it?
Otherwise, I agree -- _can_ it, if it's not too big of a change this
close to release (methinks not, but, you never know).
--
Lamar Owen
WGCR Internet Radio
1 Peter 4:11
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: libpq++ tracing considered harmful (was Re: libpq++ memory problems)
@ 2000-05-16 14:04 Tom Lane <tgl@sss.pgh.pa.us>
parent: Lamar Owen <lamar.owen@wgcr.org>
0 siblings, 1 reply; 6+ messages in thread
From: Tom Lane @ 2000-05-16 14:04 UTC (permalink / raw)
To: Tim Brookes <tim.brookes@mcmail.com>; +Cc: Lamar Owen <lamar.owen@wgcr.org>; pgsql-interfaces
Tim Brookes <tim.brookes@mcmail.com> writes:
> I have repeated my previous test, and found I am still leaking huge
> amounts of memory.
Hmph. Are you sure you are releasing query results when you're done
with them?
regards, tom lane
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: libpq++ tracing considered harmful (was Re: libpq++ memory problems)
@ 2000-05-16 15:11 Tom Lane <tgl@sss.pgh.pa.us>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 6+ messages in thread
From: Tom Lane @ 2000-05-16 15:11 UTC (permalink / raw)
To: Tim Brookes <tim.brookes@mcmail.com>; +Cc: Lamar Owen <lamar.owen@wgcr.org>; pgsql-interfaces
Tim Brookes <tim.brookes@mcmail.com> writes:
> The code is simply a constructor and destructor call of PgDatabase,
> called in succession, within an endless loop to emphasise the issue.
> But you're right, something isn't being released - within the
> destructor.
All I can say is, I see absolutely zero leakage with 7.0 code and the
attached test program. Are you sure you didn't link a 6.5 library
by mistake, or something like that?
regards, tom lane
#include <iostream.h>
#include <libpq++.h>
int main()
{
for (int i = 0; i < 10000; i++) {
// Open the connection to the database and make sure it's OK
PgDatabase data("dbname=template1 user=postgres");
if ( data.ConnectionBad() ) {
cout << "Connection was unsuccessful..." << endl
<< "Error message returned: " << data.ErrorMessage() << endl;
return 1;
}
}
return 0;
} // End main()
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2000-05-16 15:11 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2000-04-20 17:41 libpq++ tracing considered harmful (was Re: libpq++ memory problems) Tom Lane <tgl@sss.pgh.pa.us>
2000-04-20 18:06 ` Lamar Owen <lamar.owen@wgcr.org>
2000-04-20 18:22 ` Tom Lane <tgl@sss.pgh.pa.us>
2000-04-20 18:28 ` Lamar Owen <lamar.owen@wgcr.org>
2000-05-16 14:04 ` Tom Lane <tgl@sss.pgh.pa.us>
2000-05-16 15:11 ` Tom Lane <tgl@sss.pgh.pa.us>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox