Bug #77051 [Asn->Csd]: Issue with re-binding on SQLite3

Edit report at https://bugs.php.net/bug.php?id=77051&edit=1 ID: 77051 Updated by: cmb@php.net Reported by: php at bohwaz dot net Summary: Issue with re-binding on SQLite3 -Status: Assigned +Status: Closed Type: Bug Package: SQLite related Operating System: All PHP Version: 5.6.38 Assigned To: cmb Block user comment: N Private report: N New Comment: Automatic comment on behalf of bohwaz@github.com Revision: http://git.php.net/?p=php-src.git;a=commit;h=94ec262fca2e832ab2e1c4f03bc68cbda6aa42ae Log: Fix #77051: Issue with re-binding on SQLite3 Previous Comments: ------------------------------------------------------------------------ [2018-11-29 00:25:16] cmb@php.net > Though calling clear_bindings is not an issue anyway as all the > values are stored in a list and re-binded every time execute is > called, so it wouldn't change anything. Indeed, you're right! Since we don't allow to unbind parameters (except via SQLite3Stmt::clear(), which calls sqlite3_clear_bindings() anyway), there is no need to temporarily bind the parameters to null values. > I also recommend applying @cmbs' patch to check the return value > of sqlite3_bind_ functions. I agree, but unless that would really fix a bug, I think this should target master only. ------------------------------------------------------------------------ [2018-11-22 16:05:20] php at bohwaz dot net Made a PR to fix this issue https://github.com/php/php-src/pull/3675 I also recommend applying @cmbs' patch to check the return value of sqlite3_bind_ functions. ------------------------------------------------------------------------ [2018-11-12 16:22:17] php at bohwaz dot net PDO_SQLite is not calling sqlite3_clear_bindings at any point so I'm not sure if it is necessary? I just tried and just doing a reset seems to be enough. Though calling clear_bindings is not an issue anyway as all the values are stored in a list and re-binded every time execute is called, so it wouldn't change anything. But ideally the binding should happen when the call to bindValue is done, and we shouldn't have to re-bind every value every time we execute (but it is currently required because of bindParam), in that context calling clear_bindings would not be adequate as it would set to NULL any value previously binded. (but as I said it would be "ideally", and not the current way it is handled) ------------------------------------------------------------------------ [2018-11-12 16:00:15] cmb@php.net > If you just call sqlite3_clear_bindings without calling > sqlite3_reset before, you will end up with all your binded params > with NULL values, even if you do call bindValue after. I don't > think that calling sqlite3_clear_bindings is actually useful. The > reset should be enough. Calling sqlite3_reset() without sqlite3_clear_bindings() is definitely insufficient[1]. The sole addition of sqlite3_clear_bindings() lets the supplied test script succeed, but additionally calling sqlite3_reset() might be more appropriate. > As for the memory issue, according to @cmb memcheck the issue > seem to happen inside the SQLite3 library. From what I can tell, the formerly bound parameter is written to by SQLite3 (because the second binding was rejected), although it has already been released in the meantime. [1] <https://www.sqlite.org/c3ref/clear_bindings.html> ------------------------------------------------------------------------ [2018-11-12 15:25:29] php at bohwaz dot net IMHO the best solution would be to reset the statement when needed, like PDO_SQLite is doing: bindValue/bindParam: https://github.com/php/php-src/blob/php-7.3.0RC5/ext/pdo_sqlite/sqlite_statement.c#L84-L87 and execute: https://github.com/php/php-src/blob/php-7.3.0RC5/ext/pdo_sqlite/sqlite_statement.c#L48-L50 We have discussed this option earlier while discussing issues with reset/clear behavior consistencies. It seems to be a good solution in that case as it will solve the issue and not produce an error message/exception if you happen to fail to know the correct order the execute/bindValue/reset methods should be called. However this doesn't change the fact that we still need to check the result of the sqlite3_bind_* functions, just in case. If you just call sqlite3_clear_bindings without calling sqlite3_reset before, you will end up with all your binded params with NULL values, even if you do call bindValue after. I don't think that calling sqlite3_clear_bindings is actually useful. The reset should be enough. As for the memory issue, according to @cmb memcheck the issue seem to happen inside the SQLite3 library. ------------------------------------------------------------------------ The remainder of the comments for this report are too long. To view the rest of the comments, please view the bug report online at https://bugs.php.net/bug.php?id=77051 -- Edit this bug report at https://bugs.php.net/bug.php?id=77051&edit=1