Fix GH-15432: Heap corruption when querying a vector #15445
Conversation
Since the mysqlnd result set is arena allocated, we must not simply free it, but rather call the appropriate `free_result` method.
github-actions
bot
added
Extension: mysqli
Extension: mysqlnd
Extension: pdo_mysql
labels
|
Note that the skip conditions are not correct; we probably can't check for the server version to detect vector datatype support (this might not be available for other databases supporting the MySQL protocol), and we likely get different results with libmysql-client (although I wouldn't know which ones exactly; particularly wrt. the libmysql client version in use). And the SKIPIF sections are already very ugly. Maybe @kamil-tekiela can help a bit with this? And of course we need to decide whether we want to implement (basic) vector database support right away (i.e. for PHP-8.2). See PR #15431 for a basic implementation. Anyhow, I've tested this with the Windows debug heap enabled, and couldn't find any issues. Some additional ASan/MSan or valgrind testing might be in order nonetheless. |
Co-authored-by: Kamil Tekiela <tekiela246@gmail.com>
|
See 80ece39 |
|
Ahh yes, but now we run into a problem. My PR fixes the bug so now your test case will not reproduce it. Do we have a different way of reproducing it? |
First, having a regression test is good, but not absolutely mandatory. Still, to test this issue we would need to have a server that sends an unknown datatype code (something else instead of that 242). We have something like that for pdo_firebird, but such tests are brittle, complex to create and maintain, and in this case it might not be worth it. |
|
Is this good to be applied (this heap corruption needs to be addressed)? Afterwards, we could address PR #15431. |
As it's now, your PR would overwrite the test (after having resolved the merge conflict), and I still think that this is okay (not perfect, but good enough). |
|
Ok, then let's merge this one. |
|
Applied as b1211c1. |
Since the mysqlnd result set is arena allocated, we must not simply free it, but rather call the appropriate
free_resultmethod.