Fix persistent procedural ODBC connections not getting closed

[NattyNarwhal]

Like oci8, procedural ODBC uses an apply function on the hash list to enumerate persistent connections and close the specific one. However, this function take zvals, not resources. However, it was getting casted as such, causing it to interpret the pointer incorrectly. This could have caused other issues, but mostly manifested as failing to close the connection even fi it matched.

The function now takes a zval and gets the resource from that. In addition, it also removes the cast of the function pointer and moves casting to the function body, to avoid possible confusion like this in refactors again. It also cleans up style and uses constants in the function body.

[NattyNarwhal]

FWIW it seems this was broken in f4cfaf3, which refactored apply_func_arg_t et al as well as callers, but not ODBC.

[kocsismate]

Can you please add a test as well?

[NattyNarwhal]

Looking at it, I'm not sure how to test it. When you do odbc_close, it calls the destructor for the resource successfully (even before this fix), and that's observable, but the change to the persistent connection list isn't. You'd have to change the state of the connection (be it wedge it or just simple registers for the connection), and that's tricky to do cross-driver. It's also hard to test a persist connection in the context of the test suite. The only tests involving odbc_pconnect are odbc_close_all_001 showcasing resource types, and odbc_setoption_002 pointing out odbc_setoption can't be used on a persistent connection.

[kocsismate]

And that's tricky to do cross-driver

I think it's enough to only restrict yourself to MS SQL, since that's the only driver we test in CI. I recently added many tests for ext/odbc, and I managed to come up with this skipif for MSSQL specific tests:

$conn = odbc_connect($dsn, $user, $pass);
$result = @odbc_exec($conn, "SELECT @@Version");
if ($result) {
    $array = odbc_fetch_array($result);
    $info = (string) reset($array);
    if (!str_contains($info, "Microsoft SQL Server")) {
       echo "skip MS SQL specific test";
    }
}

But of course odbc_setoption() not supporting persistent connections is a problem indeed. :)

However, I'm wondering if you could exploit the bug by inserting a new connection into the same hash key (https://github.com/php/php-src/blob/master/ext/odbc/php_odbc.c#L2205) as a closed one used to have, in which case, ODBC tries to reuse the connection. Is this behavior possible to make use of?

[NattyNarwhal]

I'm writing a test with SQL Server's CONTEXT_INFO, which does seem to work for proving this.

However, I'm wondering if you could exploit the bug by inserting a new connection into the same hash key (https://github.com/php/php-src/blob/master/ext/odbc/php_odbc.c#L2205) as a closed one used to have, in which case, ODBC tries to reuse the connection. Is this behavior possible to make use of?

I'm not sure here. odbc_pconnect is AFAIK the only way to insert an entry, and it'll fetch the connection again if you have the same hash (after poking it with some imperfect heuristics)?

FWIW finding this bug was motivated by if you have a wedged persistent connection, odbc_close followed by odbc_pconnect in an attempt to un-wedge it would simply pull the same resource again out of the persistent connection list - and probably had its resource destructor ran on it by odbc_close.

[Girgias]

Just note as this is a bug fix it should target the PHP-8.1 branch

[NattyNarwhal]

Incredibly great. I rebased and force pushed and now GitHub thinks there's no changes at all and automatically closed the PR.

[kocsismate]

I think you should set the target branch to PHP-8.1 too

[NattyNarwhal]

GitHub isn't letting me do that to this PR because it claims it's closed. I initially force pushed a branch rebased onto 8.1, which caused it to think there's no changes and automatically closed it. I'm trying to force push back my original master derived branch to see if it gives me the option to do so again, but nope.

I'm also trying to file a new PR with the branch that targets 8.1 directly instead, but GitHub also refuses to let me file one, pointing me to this PR.... which it still claims has no code in it:

[NattyNarwhal]

Never mind, the rebase didn't happen on my local 8.1 based branch, after manually cherry picking, GH-12132 should be this PR, but for 8.1.

I still have no idea why this PR is fouled up even after pushing the original branch back to it, but the new PR is targetting the right branch for a bug fix, so 🤷