Skip to content
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

Jump to bottom

Fix persistent procedural ODBC connections not getting closed (8.1) #12132

Closed
wants to merge 4 commits into from
Closed

Fix persistent procedural ODBC connections not getting closed (8.1) #12132

wants to merge 4 commits into from

Conversation

NattyNarwhal
Copy link
Member

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.

(This PR is GH-12096, but rebased onto PHP 8.1 instead.)

@NattyNarwhal
Fix persistent procedural ODBC connections not getting closed
9d67e47 
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
fix whitespace
095d7a5
@NattyNarwhal
Test fix for procedural ODBC pconn close
0f6c146 
While this issue affects any driver, getting a driver into a state that
persists across persistent connections is driver specific, be it wedged
or just some connection variables. As such, just make an SQL Server
specific test, since that's the driver used in CI.
@NattyNarwhal
clean remnant comment and expectf isn't needed
36cc9f1
@NattyNarwhal NattyNarwhal mentioned this pull request Sep 5, 2023
Closed
@Girgias Girgias closed this in 5a2b251 Sep 6, 2023
@iluuu1994
Copy link
Member

iluuu1994 commented Sep 7, 2023 •
edited
Loading

@NattyNarwhal This causes a leak in nightly (only PHP 8.3 and master):

https://github.com/php/php-src/actions/runs/6104241625/job/16566001830

004+ 
005+ =================================================================
006+ ==243082==ERROR: LeakSanitizer: detected memory leaks
007+ 
008+ Direct leak of 24 byte(s) in 1 object(s) allocated from:
009+     #0 0x7fb7d9a12587 in operator new(unsigned long) ../../../../src/libsanitizer/asan/asan_new_delete.cc:104
010+     #1 0x7fb7c9cc125a  (/opt/microsoft/msodbcsql17/lib64/libmsodbcsql-17.10.so.4.1+0x17e25a)
011+ 
012+ Indirect leak of 88 byte(s) in 1 object(s) allocated from:
013+     #0 0x7fb7d9a12587 in operator new(unsigned long) ../../../../src/libsanitizer/asan/asan_new_delete.cc:104
014+     #1 0x7fb7c9c4b8a9  (/opt/microsoft/msodbcsql17/lib64/libmsodbcsql-17.10.so.4.1+0x1088a9)
015+ 
016+ SUMMARY: AddressSanitizer: 112 byte(s) leaked in 2 allocation(s).
========DONE========
FAIL odbc_pconnect(): Make sure closing a persistent connection works [ext/odbc/tests/odbc_persistent_close.phpt]

I think the ODBC extension isn't tested in ASAN on push, which is why it's not caught there.

Can you have a look?

/cc @Girgias

@NattyNarwhal
Copy link
Member Author

My initial guess is the connection isn't being freed properly (since this code didn't work almost a decade, quite possible).

Another possibility, it might be on the database driver side (either as a false positive, or a driver bug). Last time I tried ASan, it didn't seem to like the DB2 driver. I can't remember if Valgrind was easier to deal with when I last tried that...

@NattyNarwhal
Copy link
Member Author

I can't seem to repro on macOS (noted that the test is using ZTS), I'll check out Linux.

@NattyNarwhal
Copy link
Member Author

Yup, can reproduce w/ driver 18 on Fedora 38/amd64:

=================================================================
==152782==ERROR: AddressSanitizer: alloc-dealloc-mismatch (operator new [] vs operator delete) on 0x60700000bc40
    #0 0x7ff9cdbb5868 in operator delete(void*) (/lib64/libasan.so.8+0xda868) (BuildId: e5f0a0d511a659fbc47bf41072869139cb2db47f)
    #1 0x7ff9bc34fdde  (/opt/microsoft/msodbcsql18/lib64/libmsodbcsql-18.3.so.1.1+0x14fdde) (BuildId: 15b322d5e20eae639fe78063c344fab0e4c7f885)
    #2 0x7ff9bc3622a7  (/opt/microsoft/msodbcsql18/lib64/libmsodbcsql-18.3.so.1.1+0x1622a7) (BuildId: 15b322d5e20eae639fe78063c344fab0e4c7f885)
    #3 0x7ff9bc3286ee  (/opt/microsoft/msodbcsql18/lib64/libmsodbcsql-18.3.so.1.1+0x1286ee) (BuildId: 15b322d5e20eae639fe78063c344fab0e4c7f885)
    #4 0x7ff9bc29481a  (/opt/microsoft/msodbcsql18/lib64/libmsodbcsql-18.3.so.1.1+0x9481a) (BuildId: 15b322d5e20eae639fe78063c344fab0e4c7f885)
    #5 0x7ff9bc294966  (/opt/microsoft/msodbcsql18/lib64/libmsodbcsql-18.3.so.1.1+0x94966) (BuildId: 15b322d5e20eae639fe78063c344fab0e4c7f885)
    #6 0x7ff9bc2949e7 in SQLFreeHandle (/opt/microsoft/msodbcsql18/lib64/libmsodbcsql-18.3.so.1.1+0x949e7) (BuildId: 15b322d5e20eae639fe78063c344fab0e4c7f885)
    #7 0x7ff9cd6d18ae in release_env.part.0 (/lib64/libodbc.so.2+0xa8ae) (BuildId: e9f0ec53432d1444dd704178dcc1551452f9b30a)
    #8 0x7ff9cd6d6591 in __disconnect_part_one (/lib64/libodbc.so.2+0xf591) (BuildId: e9f0ec53432d1444dd704178dcc1551452f9b30a)
    #9 0x7ff9cd6e2f75 in SQLDriverConnect (/lib64/libodbc.so.2+0x1bf75) (BuildId: e9f0ec53432d1444dd704178dcc1551452f9b30a)
    #10 0x9ba461 in odbc_sqlconnect /var/home/calvin/src/php-src/ext/odbc/php_odbc.c:2138
    #11 0x9baeb7 in odbc_do_connect /var/home/calvin/src/php-src/ext/odbc/php_odbc.c:2223
    #12 0x9b9b28 in zif_odbc_pconnect /var/home/calvin/src/php-src/ext/odbc/php_odbc.c:2047
    #13 0x10471f2 in ZEND_DO_ICALL_SPEC_RETVAL_USED_HANDLER /var/home/calvin/src/php-src/Zend/zend_vm_execute.h:1337
    #14 0x1195c03 in execute_ex /var/home/calvin/src/php-src/Zend/zend_vm_execute.h:57196
    #15 0x11a7d75 in zend_execute /var/home/calvin/src/php-src/Zend/zend_vm_execute.h:61584
    #16 0xfaa576 in zend_execute_scripts /var/home/calvin/src/php-src/Zend/zend.c:1876
    #17 0xe3405b in php_execute_script /var/home/calvin/src/php-src/main/main.c:2492
    #18 0x1375e5e in do_cli /var/home/calvin/src/php-src/sapi/cli/php_cli.c:966
    #19 0x1377b0a in main /var/home/calvin/src/php-src/sapi/cli/php_cli.c:1340
    #20 0x7ff9cd510b49 in __libc_start_call_main (/lib64/libc.so.6+0x27b49) (BuildId: 245240a31888ad5c11bbc55b18e02d87388f59a9)
    #21 0x7ff9cd510c0a in __libc_start_main_alias_2 (/lib64/libc.so.6+0x27c0a) (BuildId: 245240a31888ad5c11bbc55b18e02d87388f59a9)
    #22 0x6040d4 in _start (/var/home/calvin/src/php-src/sapi/cli/php+0x6040d4) (BuildId: f9cf5829bf26afebaba430597ec4e7ae7d0375a7)

0x60700000bc40 is located 0 bytes inside of 72-byte region [0x60700000bc40,0x60700000bc88)
allocated by thread T0 here:
    #0 0x7ff9cdbb4f88 in operator new[](unsigned long) (/lib64/libasan.so.8+0xd9f88) (BuildId: e5f0a0d511a659fbc47bf41072869139cb2db47f)
    #1 0x7ff9bc350d8c  (/opt/microsoft/msodbcsql18/lib64/libmsodbcsql-18.3.so.1.1+0x150d8c) (BuildId: 15b322d5e20eae639fe78063c344fab0e4c7f885)

SUMMARY: AddressSanitizer: alloc-dealloc-mismatch (/lib64/libasan.so.8+0xda868) (BuildId: e5f0a0d511a659fbc47bf41072869139cb2db47f) in operator delete(void*)
==152782==HINT: if you don't care about these errors you may set ASAN_OPTIONS=alloc_dealloc_mismatch=0
==152782==ABORTING

@NattyNarwhal
Copy link
Member Author

I'm personally leaning towards "driver issue", since all the IDs in the ODBC functions are IIRC opaque integers, not pointers. The truncated stack trace for the new though, I can't tell if that's a new thread's initial function or some issue with the ASan report?

@iluuu1994
Copy link
Member

@NattyNarwhal Looking up that error, alloc-dealloc-mismatch apparently occurs when using delete instead of delete[] in C++ on something that has been allocated with new[]. The difference is that delete[] calls the destructor for each object in the array. That could potentially explain the leak. If this is true, we should report this upstream.

@NattyNarwhal
Copy link
Member Author

I agree. I'm just not sure where to report the issue, is there a public facing bug tracker, or someone within MS that we can forward this to?

@iluuu1994
Copy link
Member

I've tried to see where it could be reported. I'm not sure which is the better place, as they both don't seem particularly fitting.

https://learn.microsoft.com/en-us/sql/connect/odbc/microsoft-odbc-driver-for-sql-server?view=sql-server-ver16
https://feedback.azure.com/d365community/forum/04fe6ee0-3b25-ec11-b6e6-000d3a4f0da0

The SQL Server ODBC driver is not open source, correct? This will make it hard to provide a proper stack trace. However, they should be able to reproduce the issue with a PHP build.

I will mark the test as XFAIL with ASAN for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@NattyNarwhal @iluuu1994