Skip to content

Migrate ext/odbc resources to opaque objects #12040

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Apr 28, 2024
Merged

Migrate ext/odbc resources to opaque objects #12040

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
c32c501
Migrate ext/odbc resources to opaque objects
kocsismate May 19, 2021
cceabac
Address code review comments
kocsismate Sep 28, 2023
16c8b58
Fixes
kocsismate Apr 6, 2024
4a7b5f6
Fix some of the code review comments
kocsismate Apr 17, 2024
24696c0
New reviww round
kocsismate Apr 21, 2024
54ef55f
New fixes
kocsismate Apr 24, 2024
0483798
Fix one leak
nielsdos Apr 25, 2024
4282362
Move instanceof check inside ZEND_DEBUG guard
nielsdos Apr 25, 2024
887b793
Hopefully fix all memleaks
kocsismate Apr 26, 2024
f75d4b3
Fix one more address round and rename namespace to Odbc
kocsismate Apr 27, 2024
46d653d
Fix last code review comments
kocsismate Apr 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 33 additions & 21 deletions ext/odbc/php_odbc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ static int _close_pconn_with_res(zval *zv, void *p)
}

odbc_connection *list_conn = ((odbc_connection *)(le->ptr));
Copy link
Member

Choose a reason for hiding this comment

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

Style nit:

Suggested change
odbc_connection *list_conn = ((odbc_connection *)(le->ptr));
odbc_connection *list_conn = (odbc_connection *) le->ptr;

Copy link
Member

Choose a reason for hiding this comment

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

Actually, this entire function can be simplified into:

static int _close_pconn_with_res(zval *zv, void *p)
{
	zend_resource *le = Z_RES_P(zv);

	if (le->ptr == p) {
		return ZEND_HASH_APPLY_REMOVE;
	}

	return ZEND_HASH_APPLY_KEEP;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, initially I had a similar code which you suggested, but I wanted to debug it, that's why I created the local variables so that I can inspect them. Now that finally I don't have to do debugging, I'm fine to revert these changes

odbc_connection *obj_conn = odbc_link_from_obj((zend_object*)p)->connection;
odbc_connection *obj_conn = (odbc_connection *) p;
if (list_conn == obj_conn) {
return ZEND_HASH_APPLY_REMOVE;
}
Expand Down Expand Up @@ -138,10 +138,10 @@ static void odbc_link_free(odbc_link *link)
zend_hash_destroy(&link->connection->results);
efree(link->connection);
ODBCG(num_links)--;
}

if (link->hash) {
zend_hash_del(&ODBCG(non_persistent_connections), link->hash);
}
if (link->hash) {
Copy link
Member

Choose a reason for hiding this comment

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

Merge this if's body with the if's body at the bottom of this function.

zend_hash_del(&ODBCG(connections), link->hash);
}

link->connection = NULL;
Expand Down Expand Up @@ -317,6 +317,7 @@ static void _close_odbc_pconn(zend_resource *rsrc)
SQLFreeConnect(conn->hdbc);
SQLFreeEnv(conn->henv);
}
free(conn);

ODBCG(num_links)--;
ODBCG(num_persistent)--;
Expand Down Expand Up @@ -501,12 +502,12 @@ static PHP_GINIT_FUNCTION(odbc)
ZEND_TSRMLS_CACHE_UPDATE();
#endif
odbc_globals->num_persistent = 0;
zend_hash_init(&odbc_globals->non_persistent_connections, 0, NULL, NULL, 1);
zend_hash_init(&odbc_globals->connections, 0, NULL, NULL, 1);
Copy link
Member

Choose a reason for hiding this comment

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

Style nit:

Suggested change
zend_hash_init(&odbc_globals->connections, 0, NULL, NULL, 1);
zend_hash_init(&odbc_globals->connections, 0, NULL, NULL, true);

}

static PHP_GSHUTDOWN_FUNCTION(odbc)
{
zend_hash_destroy(&odbc_globals->non_persistent_connections);
zend_hash_destroy(&odbc_globals->connections);
}

/* {{{ PHP_MINIT_FUNCTION */
Expand Down Expand Up @@ -878,14 +879,14 @@ PHP_FUNCTION(odbc_close_all)
}

/* Loop through the non-persistent connection list, now close all non-persistent connections and their results */
Copy link
Member

Choose a reason for hiding this comment

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

Comment is inaccurate now: previously this looped over non-persistent connections but now it does both non-persistent and persistent.

ZEND_HASH_FOREACH_VAL(&ODBCG(non_persistent_connections), zv) {
ZEND_HASH_FOREACH_VAL(&ODBCG(connections), zv) {
odbc_link *link = Z_ODBC_LINK_P(zv);
if (link->connection) {
odbc_link_free(link);
}
} ZEND_HASH_FOREACH_END();

zend_hash_clean(&ODBCG(non_persistent_connections));
zend_hash_clean(&ODBCG(connections));

zend_hash_apply(&EG(persistent_list), _close_pconn);
}
Expand Down Expand Up @@ -2299,7 +2300,7 @@ void odbc_do_connect(INTERNAL_FUNCTION_PARAMETERS, int persistent)
}

char *hashed_details;
int hashed_details_len = spprintf(&hashed_details, 0, "%s_%s_%s_%s_%d", ODBC_TYPE, db, uid, pwd, cur_opt);
int hashed_details_len = spprintf(&hashed_details, 0, "%d_%s_%s_%s_%s_%d", persistent, ODBC_TYPE, db, uid, pwd, cur_opt);
Copy link
Member

Choose a reason for hiding this comment

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

I know this is pre-existing, but this should be size_t instead of int.


try_and_get_another_connection:

Expand Down Expand Up @@ -2333,6 +2334,8 @@ void odbc_do_connect(INTERNAL_FUNCTION_PARAMETERS, int persistent)
RETURN_FALSE;
}

zend_hash_str_add_new(&ODBCG(connections), hashed_details, hashed_details_len, return_value);

ODBCG(num_persistent)++;
ODBCG(num_links)++;
} else { /* found connection */
Expand Down Expand Up @@ -2381,15 +2384,22 @@ void odbc_do_connect(INTERNAL_FUNCTION_PARAMETERS, int persistent)
}
}

object_init_ex(return_value, odbc_connection_ce);
odbc_link *link = Z_ODBC_LINK_P(return_value);
link->connection = db_conn;
link->hash = zend_string_init(hashed_details, hashed_details_len, persistent);
link->persistent = true;
zval *tmp;
if ((tmp = zend_hash_str_find(&ODBCG(connections), hashed_details, hashed_details_len)) == NULL) {
object_init_ex(return_value, odbc_connection_ce);
odbc_link *link = Z_ODBC_LINK_P(return_value);
link->connection = db_conn;
link->hash = zend_string_init(hashed_details, hashed_details_len, persistent);
link->persistent = true;
} else {
ZVAL_COPY(return_value, tmp);

ZEND_ASSERT(Z_ODBC_CONNECTION_P(return_value) == db_conn && "Persistent connection has changed");
}
}
} else {
} else { /* non-persistent */
zval *tmp;
if ((tmp = zend_hash_str_find(&ODBCG(non_persistent_connections), hashed_details, hashed_details_len)) == NULL) { /* non-persistent, new */
if ((tmp = zend_hash_str_find(&ODBCG(connections), hashed_details, hashed_details_len)) == NULL) { /* non-persistent, new */
if (ODBCG(max_links) != -1 && ODBCG(num_links) >= ODBCG(max_links)) {
php_error_docref(NULL, E_WARNING, "Too many open connections (" ZEND_LONG_FMT ")", ODBCG(num_links));
efree(hashed_details);
Expand All @@ -2403,7 +2413,7 @@ void odbc_do_connect(INTERNAL_FUNCTION_PARAMETERS, int persistent)
}
ODBCG(num_links)++;

zend_hash_str_add_new(&ODBCG(non_persistent_connections), hashed_details, hashed_details_len, return_value);
zend_hash_str_add_new(&ODBCG(connections), hashed_details, hashed_details_len, return_value);
} else { /* non-persistent, pre-existing */
ZVAL_COPY(return_value, tmp);
}
Expand All @@ -2417,19 +2427,21 @@ PHP_FUNCTION(odbc_close)
{
zval *pv_conn;
odbc_link *link;
odbc_connection *connection;

if (zend_parse_parameters(ZEND_NUM_ARGS(), "O", &pv_conn, odbc_connection_ce) == FAILURE) {
RETURN_THROWS();
}

link = Z_ODBC_LINK_P(pv_conn);
CHECK_ODBC_CONNECTION(link->connection);
connection = Z_ODBC_CONNECTION_P(pv_conn);
Copy link
Member

Choose a reason for hiding this comment

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

Style nit: I prefer putting the declaration and assignment on the same line, i.e. odbc_connection *connection = Z_ODBC_CONNECTION_P(pv_conn);. This avoids bugs because it limits the scope of a variable such that it's not accidentally used uninitialized or in places it shouldn't be used.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I fixed this in quite some places

CHECK_ODBC_CONNECTION(connection);

odbc_link_free(link);

if (link->persistent) {
Copy link
Member Author

Choose a reason for hiding this comment

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

this block of code is not needed anymore since odbc_link_free() makes sure to remove any trace of the connection

zend_hash_apply_with_argument(&EG(persistent_list), _close_pconn_with_res, (void *) Z_OBJ_P(pv_conn));
zend_hash_apply_with_argument(&EG(persistent_list), _close_pconn_with_res, (void *) connection);
}

odbc_link_free(link);
}
/* }}} */

Expand Down
8 changes: 7 additions & 1 deletion ext/odbc/php_odbc_includes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,13 @@ ZEND_BEGIN_MODULE_GLOBALS(odbc)
zend_long default_cursortype;
char laststate[6];
char lasterrormsg[SQL_MAX_MESSAGE_LENGTH];
HashTable non_persistent_connections;
/* Stores ODBC links throughout the duration of a request. The connection member may be either persistent or
* non-persistent. In the former case, it is a pointer to an item in EG(persistent_list). This solution makes it
* possible to properly free links during RSHUTDOWN (or when they are explicitly closed), while persistent
* connections themselves are going to be freed later during the shutdown process (or when they are explicitly
* closed).
*/
HashTable connections;
Copy link
Member

Choose a reason for hiding this comment

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

ODBCG(connections) is used to store non-persistent connections, so they don't survive across requests, so they should be initialized in RINIT and cleaned up in RSHUTDOWN. That table should also be called non_persistent_connections btw, otherwise it's confusing for reviewers.
Creating a persistent table and then storing non-persistent stuff in it is icky.

Copy link
Member

Choose a reason for hiding this comment

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

As a side question, why do we hold such a reference at all? I know this is pre-existing, but I think it makes sense to clean up non-persistent connections as soon as the connection object becomes unreachable instead of waiting until the request ends, no? I do see that odbc_connect closes both persistent and non-persistent connections, but I find the fact that connections are held even if the connection object isn't reachable from userland very very weird.
The concept just makes everything more difficult than it has to be imho.

Copy link
Member Author

Choose a reason for hiding this comment

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

I originally put it there AFAIR but I changed for GSHUTDOWN after this review comment: #12040 (comment) I even looked up https://www.phpinternalsbook.com/php7/extensions_design/php_lifecycle.html#globals-termination-gshutdown and thought that GSHUTDOWN is called during RSHUTDOWN assuming that these globals should be freed there, however, I missed this sentence at the very end of the section: Remember that globals are not cleared after every request; aka GSHUTDOWN() is not called as part of RSHUTDOWN(). So you are surely right, I'll use RSHUTDOWN then.

Copy link
Member Author

@kocsismate kocsismate Apr 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

As a side question, why do we hold such a reference at all? I know this is pre-existing, but I think it makes sense to clean up non-persistent connections as soon as the connection object becomes unreachable instead of waiting until the request ends, no?

The connections hash table is needed because there's a odbc_close_all() function which closes all connections right away. The results hash table is needed because results are automatically closed as soon as a connection is closed... And I agree with you, I also think that odbc_close_all() is probably the main source of complexity in this extension... Also, by storing connections in a hash map, it's possible to implement connection reuse if we want to (it is already available for persistent resources).

ZEND_END_MODULE_GLOBALS(odbc)

int odbc_add_result(HashTable *list, odbc_result *result);
Expand Down
62 changes: 62 additions & 0 deletions ext/odbc/tests/odbc_close_001.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
--TEST--
odbc_close(): Basic test
--EXTENSIONS--
odbc
--SKIPIF--
<?php include 'skipif.inc'; ?>
--FILE--
<?php

include 'config.inc';

$conn1 = odbc_connect($dsn, $user, $pass);
$conn2 = odbc_pconnect($dsn, $user, $pass);
$result1 = odbc_columns($conn1, '', '', '', '');
$result2 = odbc_columns($conn2, '', '', '', '');

var_dump($conn1);
var_dump($conn2);
var_dump($result1);
var_dump($result2);

odbc_close($conn1);
odbc_close($conn2);

try {
odbc_columns($conn1, '', '', '', '');
} catch (Error $e) {
echo $e->getMessage() . "\n";
}

try {
odbc_columns($conn2, '', '', '', '');
} catch (Error $e) {
echo $e->getMessage() . "\n";
}

try {
odbc_num_rows($result1);
} catch (Error $e) {
echo $e->getMessage() . "\n";
}

try {
odbc_num_rows($result2);
} catch (Error $e) {
echo $e->getMessage() . "\n";
}

?>
--EXPECTF--
object(ODBC\Connection)#%d (%d) {
}
object(ODBC\Connection)#%d (%d) {
}
object(ODBC\Result)#%d (%d) {
}
object(ODBC\Result)#%d (%d) {
}
ODBC connection has already been closed
ODBC connection has already been closed
ODBC result has already been closed
ODBC result has already been closed
37 changes: 26 additions & 11 deletions ext/odbc/tests/odbc_close_all_001.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,21 +21,32 @@ var_dump($result2);

odbc_close_all();

var_dump($conn1);
var_dump($conn2);
var_dump($result1);
var_dump($result2);

?>
--EXPECTF--
object(ODBC\Connection)#%d (%d) {
try {
odbc_columns($conn1, '', '', '', '');
} catch (Error $e) {
echo $e->getMessage() . "\n";
}
object(ODBC\Connection)#%d (%d) {

try {
odbc_columns($conn2, '', '', '', '');
} catch (Error $e) {
echo $e->getMessage() . "\n";
}
object(ODBC\Result)#%d (%d) {

try {
odbc_num_rows($result1);
} catch (Error $e) {
echo $e->getMessage() . "\n";
}
object(ODBC\Result)#%d (%d) {

try {
odbc_num_rows($result2);
} catch (Error $e) {
echo $e->getMessage() . "\n";
}

?>
--EXPECTF--
object(ODBC\Connection)#%d (%d) {
}
object(ODBC\Connection)#%d (%d) {
Expand All @@ -44,3 +55,7 @@ object(ODBC\Result)#%d (%d) {
}
object(ODBC\Result)#%d (%d) {
}
ODBC connection has already been closed
ODBC connection has already been closed
ODBC result has already been closed
ODBC result has already been closed
Loading