Skip to content

Commit

Permalink
Bugfix#75419 Fix clearing of default link during pg_close()
Browse files Browse the repository at this point in the history
  • Loading branch information
@sgolemon
sgolemon committed Oct 23, 2017
1 parent b2dfcb3 commit a645af4
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 13 deletions.
20 changes: 7 additions & 13 deletions ext/pgsql/pgsql.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1562,32 +1562,26 @@ PHP_FUNCTION(pg_close)
{
zval *pgsql_link = NULL;
zend_resource *link;
int argc = ZEND_NUM_ARGS();
PGconn *pgsql;

if (zend_parse_parameters(argc, "|r", &pgsql_link) == FAILURE) {
if (zend_parse_parameters(ZEND_NUM_ARGS(), "|r", &pgsql_link) == FAILURE) {
return;
}

if (argc == 0) {
if (pgsql_link) {
link = Z_RES_P(pgsql_link);
} else {
link = FETCH_DEFAULT_LINK();
CHECK_DEFAULT_LINK(link);
} else {
link = Z_RES_P(pgsql_link);
}

if ((pgsql = (PGconn *)zend_fetch_resource2(link, "PostgreSQL link", le_link, le_plink)) == NULL) {
if (zend_fetch_resource2(link, "PostgreSQL link", le_link, le_plink) == NULL) {
RETURN_FALSE;
}

if (argc == 0) { /* explicit resource number */
zend_list_close(link);
}

if (argc || (pgsql_link && Z_RES_P(pgsql_link) == PGG(default_link))) {
zend_list_close(link);
if (link == PGG(default_link)) {
PGG(default_link) = NULL;
}
zend_list_close(link);

RETURN_TRUE;
}
Expand Down
14 changes: 14 additions & 0 deletions ext/pgsql/tests/bug75419.phpt
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
--TEST--
Bug #75419 Default link leaked via pg_close()
--SKIPIF--
<?php include("skipif.inc"); ?>
--FILE--
<?php
include('config.inc');

$db1 = pg_connect($conn_str, PGSQL_CONNECT_FORCE_NEW);
$db2 = pg_connect($conn_str, PGSQL_CONNECT_FORCE_NEW);
pg_close($db1);
var_dump(pg_ping());
--EXPECT--
bool(true)

5 comments on commit a645af4

@nikic
Copy link
Member

@nikic nikic commented on a645af4 Oct 23, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the expected behavior for connections that don't use PGSQL_CONNECT_FORCE_NEW? I think with this patch pg_close() is going to close shared connections.

@sgolemon
Copy link
Contributor Author

@sgolemon sgolemon commented on a645af4 Oct 23, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you don't pass PGSQL_CONNECT_FORCE_NEW then $db2 and $db1 will be referring to the same zend_resource, yes.

This diff maintains the existing behavior that the connection will close in response to pg_close() and both variables will refer to a dead resource. (Note that in the previous revision, lines 1583 and 1587 essentially resulted in a zend_list_close() regardless of the value of argc.

@nikic
Copy link
Member

@nikic nikic commented on a645af4 Oct 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sgolemon Yeah, you're right, I misread the diff. Another question: Shouldn't we be doing a decref in the default_link branch? We incref when we set the default link, but don't decref here.

@sgolemon
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I think you're right, will look closer in a bit.

@sgolemon
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. 68e27b0

Please sign in to comment.