possible connection leak in dblink?

Started by Peter Eisentrautover 14 years ago9 messages

Peter Eisentraut

peter_e@gmx.net

over 14 years ago

With gcc 4.6, I get this warning:

dblink.c: In function ‘dblink_send_query’:
dblink.c:620:7: warning: variable ‘freeconn’ set but not used [-Wunused-but-set-variable]

I don't know much about the internals of dblink, but judging from the
surrounding code, I guess that this fix is necessary:

diff --git i/contrib/dblink/dblink.c w/contrib/dblink/dblink.c
index 19b98fb..e014c1a 100644
--- i/contrib/dblink/dblink.c
+++ w/contrib/dblink/dblink.c
@@ -634,6 +634,10 @@ dblink_send_query(PG_FUNCTION_ARGS)
        if (retval != 1)
                elog(NOTICE, "%s", PQerrorMessage(conn));

+       /* if needed, close the connection to the database and cleanup */
+       if (freeconn)
+               PQfinish(conn);
+
        PG_RETURN_INT32(retval);
 }

Otherwise the connection might not get freed. Could someone verify
that?

masao.fujii@gmail.com

over 14 years ago

In reply to: Peter Eisentraut (#1)

1 attachment(s)

Re: possible connection leak in dblink?

On Wed, Jun 15, 2011 at 5:34 AM, Peter Eisentraut <peter_e@gmx.net> wrote:

With gcc 4.6, I get this warning:

dblink.c: In function ‘dblink_send_query’:
dblink.c:620:7: warning: variable ‘freeconn’ set but not used [-Wunused-but-set-variable]

I don't know much about the internals of dblink, but judging from the
surrounding code, I guess that this fix is necessary:
diff --git i/contrib/dblink/dblink.c w/contrib/dblink/dblink.c
index 19b98fb..e014c1a 100644
--- i/contrib/dblink/dblink.c
+++ w/contrib/dblink/dblink.c
@@ -634,6 +634,10 @@ dblink_send_query(PG_FUNCTION_ARGS)
       if (retval != 1)
               elog(NOTICE, "%s", PQerrorMessage(conn));
+       /* if needed, close the connection to the database and cleanup */
+       if (freeconn)
+               PQfinish(conn);
+
       PG_RETURN_INT32(retval);
 }
Otherwise the connection might not get freed. Could someone verify
that?

ISTM that the root problem is that dblink_send_query calls DBLINK_GET_CONN
though it doesn't accept the connection string as an argument. Since the first
argument in dblink_send_query must be the connection name, dblink_send_query
should call DBLINK_GET_NAMED_CONN instead. The variable 'freeconn' is used
only when DBLINK_GET_CONN is called. So, if dblink_send_query uses
DBLINK_GET_NAMED_CONN instead, the variable 'freeconn' is no longer necessary.

The similar problem exists in dblink_get_result and dblink_record_internal.
Attached patch fixes those problems.

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Attachments:

dblink_bugfix_v1.patchtext/x-patch; charset=US-ASCII; name=dblink_bugfix_v1.patchDownload

*** a/contrib/dblink/dblink.c
--- b/contrib/dblink/dblink.c
***************
*** 613,628 **** Datum
  dblink_send_query(PG_FUNCTION_ARGS)
  {
  	PGconn	   *conn = NULL;
- 	char	   *connstr = NULL;
  	char	   *sql = NULL;
  	remoteConn *rconn = NULL;
- 	char	   *msg;
- 	bool		freeconn = false;
  	int			retval;
  
  	if (PG_NARGS() == 2)
  	{
! 		DBLINK_GET_CONN;
  		sql = text_to_cstring(PG_GETARG_TEXT_PP(1));
  	}
  	else
--- 613,625 ----
  dblink_send_query(PG_FUNCTION_ARGS)
  {
  	PGconn	   *conn = NULL;
  	char	   *sql = NULL;
  	remoteConn *rconn = NULL;
  	int			retval;
  
  	if (PG_NARGS() == 2)
  	{
! 		DBLINK_GET_NAMED_CONN;
  		sql = text_to_cstring(PG_GETARG_TEXT_PP(1));
  	}
  	else
***************
*** 711,723 **** dblink_record_internal(FunctionCallInfo fcinfo, bool is_async)
  		if (PG_NARGS() == 2)
  		{
  			/* text,bool */
! 			DBLINK_GET_CONN;
  			fail = PG_GETARG_BOOL(1);
  		}
  		else if (PG_NARGS() == 1)
  		{
  			/* text */
! 			DBLINK_GET_CONN;
  		}
  		else
  			/* shouldn't happen */
--- 708,720 ----
  		if (PG_NARGS() == 2)
  		{
  			/* text,bool */
! 			DBLINK_GET_NAMED_CONN;
  			fail = PG_GETARG_BOOL(1);
  		}
  		else if (PG_NARGS() == 1)
  		{
  			/* text */
! 			DBLINK_GET_NAMED_CONN;
  		}
  		else
  			/* shouldn't happen */

Itagaki Takahiro

itagaki.takahiro@gmail.com

over 14 years ago

In reply to: Fujii Masao (#2)

Re: possible connection leak in dblink?

On Wed, Jun 15, 2011 at 11:41, Fujii Masao <masao.fujii@gmail.com> wrote:

ISTM that the root problem is that dblink_send_query calls DBLINK_GET_CONN
though it doesn't accept the connection string as an argument.

+1 for the fix. I have the same conclusion at the first glance.

Since the first
argument in dblink_send_query must be the connection name, dblink_send_query
should call DBLINK_GET_NAMED_CONN instead. The variable 'freeconn' is used
only when DBLINK_GET_CONN is called. So, if dblink_send_query uses
DBLINK_GET_NAMED_CONN instead, the variable 'freeconn' is no longer necessary.

The similar problem exists in dblink_get_result and dblink_record_internal.
Attached patch fixes those problems.

--
Itagaki Takahiro

Peter Eisentraut

peter_e@gmx.net

over 14 years ago

In reply to: Fujii Masao (#2)

Re: possible connection leak in dblink?

On ons, 2011-06-15 at 11:41 +0900, Fujii Masao wrote:

ISTM that the root problem is that dblink_send_query calls DBLINK_GET_CONN
though it doesn't accept the connection string as an argument. Since the first
argument in dblink_send_query must be the connection name, dblink_send_query
should call DBLINK_GET_NAMED_CONN instead. The variable 'freeconn' is used
only when DBLINK_GET_CONN is called. So, if dblink_send_query uses
DBLINK_GET_NAMED_CONN instead, the variable 'freeconn' is no longer necessary.

The similar problem exists in dblink_get_result and dblink_record_internal.
Attached patch fixes those problems.

Is this a bug fix that should be backpatched?

tgl@sss.pgh.pa.us

over 14 years ago

In reply to: Peter Eisentraut (#4)

Re: possible connection leak in dblink?

Peter Eisentraut <peter_e@gmx.net> writes:

Is this a bug fix that should be backpatched?

I pinged Joe Conway about this. He is jetlagged from a trip to the Far
East but promised to take care of it soon. I think we can wait for his
review.

regards, tom lane

mail@joeconway.com

over 14 years ago

In reply to: Tom Lane (#5)

Re: possible connection leak in dblink?

On 06/17/2011 01:05 PM, Tom Lane wrote:

Peter Eisentraut <peter_e@gmx.net> writes:

Is this a bug fix that should be backpatched?

I pinged Joe Conway about this. He is jetlagged from a trip to the Far
East but promised to take care of it soon. I think we can wait for his
review.

Sorry for the delay. I'm finally caught up on most of my obligations,
and have plowed through the 1500 or so pgsql mailing list messages that
I was behind. But if everyone is OK with it I would like to aim to
commit a fix mid next week, because I still have to get through my
daughter's high school graduation tomorrow, and an out of state funeral
for my father-in-law Sunday/Monday.

That said, I really would like to commit this myself, as I have yet to
be brave enough to commit anything under git :-(

Joe

--
Joe Conway
credativ LLC: http://www.credativ.us
Linux, PostgreSQL, and general Open Source
Training, Service, Consulting, & 24x7 Support

bruce@momjian.us

over 14 years ago

In reply to: Joe Conway (#6)

Re: possible connection leak in dblink?

Joe Conway wrote:
-- Start of PGP signed section.

On 06/17/2011 01:05 PM, Tom Lane wrote:

Peter Eisentraut <peter_e@gmx.net> writes:

Is this a bug fix that should be backpatched?

I pinged Joe Conway about this. He is jetlagged from a trip to the Far
East but promised to take care of it soon. I think we can wait for his
review.

Sorry for the delay. I'm finally caught up on most of my obligations,
and have plowed through the 1500 or so pgsql mailing list messages that
I was behind. But if everyone is OK with it I would like to aim to
commit a fix mid next week, because I still have to get through my
daughter's high school graduation tomorrow, and an out of state funeral
for my father-in-law Sunday/Monday.

That said, I really would like to commit this myself, as I have yet to
be brave enough to commit anything under git :-(

Sounds good. We will be here to help.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

mail@joeconway.com

over 14 years ago

In reply to: Fujii Masao (#2)

Re: possible connection leak in dblink?

On 06/14/2011 07:41 PM, Fujii Masao wrote:

On Wed, Jun 15, 2011 at 5:34 AM, Peter Eisentraut <peter_e@gmx.net> wrote:

Otherwise the connection might not get freed. Could someone verify
that?

ISTM that the root problem is that dblink_send_query calls DBLINK_GET_CONN
though it doesn't accept the connection string as an argument. Since the first
argument in dblink_send_query must be the connection name, dblink_send_query
should call DBLINK_GET_NAMED_CONN instead. The variable 'freeconn' is used
only when DBLINK_GET_CONN is called. So, if dblink_send_query uses
DBLINK_GET_NAMED_CONN instead, the variable 'freeconn' is no longer necessary.

The similar problem exists in dblink_get_result and dblink_record_internal.
Attached patch fixes those problems.

Fujii's assessment looks correct, although arguably the change is
unnecessary for dblink_record_internal.

Looks like the issue with dblink_send_query goes back through 8.4, while
dblink_record_internal could be fixed as far back as 8.2.

However, since this is really just a case of unused variables and not a
leaked connection, I'm inclined to just fix git master -- comments on that?

Joe

--
Joe Conway
credativ LLC: http://www.credativ.us
Linux, PostgreSQL, and general Open Source
Training, Service, Consulting, & 24x7 Support

Peter Eisentraut

peter_e@gmx.net

over 14 years ago

In reply to: Joe Conway (#8)

Re: possible connection leak in dblink?

On lör, 2011-06-25 at 13:36 -0700, Joe Conway wrote:

However, since this is really just a case of unused variables and not
a leaked connection, I'm inclined to just fix git master -- comments
on that?

Please put it into 9.1 as well, so we can get a clean compile with gcc
4.6 there.