sqlite3: Fix double execution of queries when fetchArray is called #8942
Conversation
|
pinging @cmb69 since you wrote a POC in #5204. The test I wrote is way more verbose than the one your POC - maybe needlessly so, I hadn't seen yours first. It sort of formalises the behaviour described in bug 79293 as it expects to see a call to sqlite being made when An aside on the 79293 bug - I noted you quote from the sqlite3_step docs before, but sqlite3_stmt says this: Technically, taking this into consideration, the PHP extension behaves in a manner consistent with the SQLite3 C API here - calling "step" (fetchArray) on a "done statement" (already returned false result set) automatically "resets the statement" (re-executes the query) in both scenarios. The test as written reflects this. I would support introducing an E_WARNING though which I suppose would require reintroducing the "complete" flag. I'm going to leave some comments on the code since I wasn't familiar with it at all when I jumped in and I'm not 100% sure on this despite the tests passing. cc: @ramsey since you commented on a previous PR for the same bug (#8415) recently. |
ext/sqlite3/sqlite3.c
Outdated
| case SQLITE_DONE: /* Valid but no results */ | ||
| sqlite3_reset(stmt_obj->stmt); |
There was a problem hiding this comment.
This fixes the lastErrorCode / Msg test failure - sqlite3_reset docs mentions something about copying the error code from the call into the database object. I suppose the same thing should be done in stmt execute, probably with another test written for errors occuring through prepared statements rather than queries like this.
Something slightly magical here, this doesn't cause duplicate query execution when fetchArray is called on the result set of an INSERT query because we still set the "pending step" flag and return false on the first call to fetchArray.
`SQLite3::query()` and `SQLite3Stmt::execute()` call `sqlite3_step()` to determine whether to return a `SQLite3Result` or `false`. Then they call `sqlite3_reset()`, so that `SQLite3Result::fetchArray()` can fetch values from the first row. This obviously leads to issues if the SQL query causes any side effects. We solve this by not calling `sqlite_reset()` anymore, but instead storing the last fetch result code on the statement object, and to use this in `::fetchArray()` when the first row is to be fetched. This also allows us to prevent an SQLite3 quirk which automatically resets a statement when `sqlite_fetch()` is called after the last fetch returned anything else than `SQLITE_ROW`; thus fixing bug #79293.
|
Forget the comment about matching the behaviour of the C API, I talked myself out of it - it wouldn't match anything else in PHP land and I don't imagine anyone would be expecting the extension to behave that way, even if they knew the C API did. After looking at #5204 I decided just to rebase it on to master and apply my fixes there, I think you've done a cleaner job overall. |
| if (intern->stmt_obj && intern->stmt_obj->initialised) { | ||
| sqlite3_reset(intern->stmt_obj->stmt); | ||
| if (GC_REFCOUNT(&intern->stmt_obj->zo) == 0) { | ||
| sqlite3_reset(intern->stmt_obj->stmt); |
There was a problem hiding this comment.
This is the bit I'm unsure about - to me it makes sense logically that we are only safe to reset the statement if the refcount on it is 0, but grepping the rest of the codebase I don't see much precedent for checking refcounts from extensions.
But, that said, the statement object is already inside free_list so we can be reasonably assured that it will be reset at some later point - so can we just remove this? if I remove this entire if (intern->stmt_obj block, I don't get any complaints from a debug build about detected memory leaks.
|
|
||
| $db->query("CREATE TABLE dog ( id INTEGER PRIMARY KEY, name TEXT, annoying INTEGER )"); | ||
|
|
||
| echo "Inserting first time which should succeed" . PHP_EOL; |
There was a problem hiding this comment.
I notice that these tests only test INSERT, so the "call _reset if _DONE" fix I've done covers it, but it's late and I'm drawing a blank on a SELECT which fails only on the second time you run it (short of using a file database and deleting a table from a second instance, maybe), but it feels like there could be something obvious that just isn't coming to mind.
If it is an issue, could we just run sqlite3_reset whenever lastErrorCode is called? It feels like a bit of a strange side effect to reset the fetch position but I can't really imagine a situation where you'd be checking error codes inside the fetchArray while loop.
Fixes an existing issue tha was surfaced in php#5204 where re-using a variable to get the result of a statement on the second round of execution would cause a spurious reset call to be made to SQLite between execute() and fetchArray() being called. This was previously not changing the behaviour, as reset was already being called too many times, but it did mean the bug was not fixed for all potential scenarios.
Introduces a flag which indicates to the fetch function that there is data from
a previous call to sqlite3_step that has not yet been surfaced to the running
script. When the flag is set, a round of calling sqlite3_step will be skipped.
Closes 64531. https://bugs.php.net/bug.php?id=64531