From 3553d71492bdf754472a2a12880ff4f36d00af96 Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Mon, 24 Feb 2020 11:49:49 +0100 Subject: [PATCH 1/2] Fix #64531: SQLite3Result::fetchArray runs the query again `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. --- ext/sqlite3/php_sqlite3_structs.h | 4 +++- ext/sqlite3/sqlite3.c | 27 ++++++++++++++++++------ ext/sqlite3/tests/bug64531_1.phpt | 26 +++++++++++++++++++++++ ext/sqlite3/tests/bug64531_2.phpt | 30 +++++++++++++++++++++++++++ ext/sqlite3/tests/bug79293.phpt | 34 +++++++++++++++++++++++++++++++ 5 files changed, 114 insertions(+), 7 deletions(-) create mode 100644 ext/sqlite3/tests/bug64531_1.phpt create mode 100644 ext/sqlite3/tests/bug64531_2.phpt create mode 100644 ext/sqlite3/tests/bug79293.phpt diff --git a/ext/sqlite3/php_sqlite3_structs.h b/ext/sqlite3/php_sqlite3_structs.h index 15cb8a8cb303d..ebc849b699648 100644 --- a/ext/sqlite3/php_sqlite3_structs.h +++ b/ext/sqlite3/php_sqlite3_structs.h @@ -128,7 +128,9 @@ struct _php_sqlite3_stmt_object { php_sqlite3_db_object *db_obj; zval db_obj_zval; - int initialised; + unsigned initialised:1; + unsigned has_stepped:1; + unsigned last_step_result:30; /* Keep track of the zvals for bound parameters */ HashTable *bound_params; diff --git a/ext/sqlite3/sqlite3.c b/ext/sqlite3/sqlite3.c index f06d373c56f09..2413b7a2cc0cd 100644 --- a/ext/sqlite3/sqlite3.c +++ b/ext/sqlite3/sqlite3.c @@ -591,6 +591,9 @@ PHP_METHOD(SQLite3, query) ZVAL_OBJ(&result->stmt_obj_zval, Z_OBJ(stmt)); return_code = sqlite3_step(result->stmt_obj->stmt); + result->stmt_obj->has_stepped = 1; + ZEND_ASSERT(return_code >= 0 && return_code < (1 << 30)); + result->stmt_obj->last_step_result = return_code; switch (return_code) { case SQLITE_ROW: /* Valid Row */ @@ -601,7 +604,6 @@ PHP_METHOD(SQLite3, query) free_item->stmt_obj = stmt_obj; free_item->stmt_obj_zval = stmt; zend_llist_add_element(&(db_obj->free_list), &free_item); - sqlite3_reset(result->stmt_obj->stmt); break; } default: @@ -610,6 +612,7 @@ PHP_METHOD(SQLite3, query) } sqlite3_finalize(stmt_obj->stmt); stmt_obj->initialised = 0; + stmt_obj->has_stepped = 0; zval_ptr_dtor(return_value); RETURN_FALSE; } @@ -1432,6 +1435,7 @@ PHP_METHOD(SQLite3Stmt, reset) SQLITE3_CHECK_INITIALIZED(stmt_obj->db_obj, stmt_obj->initialised, SQLite3); SQLITE3_CHECK_INITIALIZED_STMT(stmt_obj->stmt, SQLite3Stmt); + stmt_obj->has_stepped = 0; if (sqlite3_reset(stmt_obj->stmt) != SQLITE_OK) { php_sqlite3_error(stmt_obj->db_obj, "Unable to reset statement: %s", sqlite3_errmsg(sqlite3_db_handle(stmt_obj->stmt))); RETURN_FALSE; @@ -1782,12 +1786,14 @@ PHP_METHOD(SQLite3Stmt, execute) } return_code = sqlite3_step(stmt_obj->stmt); + stmt_obj->has_stepped = 1; + ZEND_ASSERT(return_code >= 0 && return_code < (1 << 30)); + stmt_obj->last_step_result = return_code; switch (return_code) { case SQLITE_ROW: /* Valid Row */ case SQLITE_DONE: /* Valid but no results */ { - sqlite3_reset(stmt_obj->stmt); object_init_ex(return_value, php_sqlite3_result_entry); result = Z_SQLITE3_RESULT_P(return_value); @@ -1800,9 +1806,6 @@ PHP_METHOD(SQLite3Stmt, execute) break; } - case SQLITE_ERROR: - sqlite3_reset(stmt_obj->stmt); - ZEND_FALLTHROUGH; default: if (!EG(exception)) { php_sqlite3_error(stmt_obj->db_obj, "Unable to execute statement: %s", sqlite3_errmsg(sqlite3_db_handle(stmt_obj->stmt))); @@ -1941,7 +1944,14 @@ PHP_METHOD(SQLite3Result, fetchArray) SQLITE3_CHECK_INITIALIZED(result_obj->db_obj, result_obj->stmt_obj->initialised, SQLite3Result) - ret = sqlite3_step(result_obj->stmt_obj->stmt); + if (result_obj->stmt_obj->has_stepped) { + ret = result_obj->stmt_obj->last_step_result; + } else { + ret = sqlite3_step(result_obj->stmt_obj->stmt); + result_obj->stmt_obj->has_stepped = 1; + ZEND_ASSERT(ret >= 0 && ret < (1 << 30)); + result_obj->stmt_obj->last_step_result = ret; + } switch (ret) { case SQLITE_ROW: /* If there was no return value then just skip fetching */ @@ -1985,6 +1995,7 @@ PHP_METHOD(SQLite3Result, fetchArray) zend_symtable_add_new(Z_ARR_P(return_value), result_obj->column_names[i], &data); } } + result_obj->stmt_obj->has_stepped = 0; break; case SQLITE_DONE: @@ -2021,6 +2032,7 @@ PHP_METHOD(SQLite3Result, reset) sqlite3result_clear_column_names_cache(result_obj); + result_obj->stmt_obj->has_stepped = 0; if (sqlite3_reset(result_obj->stmt_obj->stmt) != SQLITE_OK) { RETURN_FALSE; } @@ -2047,6 +2059,7 @@ PHP_METHOD(SQLite3Result, finalize) zend_llist_del_element(&(result_obj->db_obj->free_list), &result_obj->stmt_obj_zval, (int (*)(void *, void *)) php_sqlite3_compare_stmt_zval_free); } else { + result_obj->stmt_obj->has_stepped = 0; sqlite3_reset(result_obj->stmt_obj->stmt); } @@ -2273,6 +2286,7 @@ static void php_sqlite3_result_object_free_storage(zend_object *object) /* {{{ * if (!Z_ISNULL(intern->stmt_obj_zval)) { if (intern->stmt_obj && intern->stmt_obj->initialised) { sqlite3_reset(intern->stmt_obj->stmt); + intern->stmt_obj->has_stepped = 0; } zval_ptr_dtor(&intern->stmt_obj_zval); @@ -2312,6 +2326,7 @@ static zend_object *php_sqlite3_stmt_object_new(zend_class_entry *class_type) /* object_properties_init(&intern->zo, class_type); intern->zo.handlers = &sqlite3_stmt_object_handlers; + intern->has_stepped = 0; return &intern->zo; } diff --git a/ext/sqlite3/tests/bug64531_1.phpt b/ext/sqlite3/tests/bug64531_1.phpt new file mode 100644 index 0000000000000..02a944cf16ce8 --- /dev/null +++ b/ext/sqlite3/tests/bug64531_1.phpt @@ -0,0 +1,26 @@ +--TEST-- +Bug #64531 (SQLite3Result::fetchArray runs the query again) +--SKIPIF-- + +--FILE-- +exec("CREATE TABLE foo (id INT)"); + +$res = $conn->query("INSERT INTO foo VALUES (1)"); + +$res->fetchArray(); +$res->fetchArray(); + +$res = $conn->query("SELECT * FROM foo"); +while (($row = $res->fetchArray(SQLITE3_NUM))) { + var_dump($row); +} +?> +--EXPECT-- +array(1) { + [0]=> + int(1) +} diff --git a/ext/sqlite3/tests/bug64531_2.phpt b/ext/sqlite3/tests/bug64531_2.phpt new file mode 100644 index 0000000000000..ef943c6e8e5ab --- /dev/null +++ b/ext/sqlite3/tests/bug64531_2.phpt @@ -0,0 +1,30 @@ +--TEST-- +Bug #64531 (SQLite3Result::fetchArray runs the query again) +--SKIPIF-- + +--FILE-- +exec("CREATE TABLE foo (id INT)"); +for ($i = 1; $i <= 3; $i++) { + $conn->exec("INSERT INTO foo VALUES ($i)"); +} +$conn->createFunction('testing', 'testing', 1); + +$res = $conn->query('SELECT * FROM foo WHERE testing(id)'); +$arr = $res->fetchArray(); +$arr = $res->fetchArray(); +$arr = $res->fetchArray(); +$arr = $res->fetchArray(); +?> +--EXPECT-- +Testing with: 1 +Testing with: 2 +Testing with: 3 diff --git a/ext/sqlite3/tests/bug79293.phpt b/ext/sqlite3/tests/bug79293.phpt new file mode 100644 index 0000000000000..ce202e67edbeb --- /dev/null +++ b/ext/sqlite3/tests/bug79293.phpt @@ -0,0 +1,34 @@ +--TEST-- +Bug #79293 (SQLite3Result::fetchArray() may fetch rows after last row) +--SKIPIF-- + +--FILE-- +exec("CREATE TABLE foo (bar INT)"); +for ($i = 1; $i <= 3; $i++) { + $db->exec("INSERT INTO foo VALUES ($i)"); +} + +$res = $db->query("SELECT * FROM foo"); +while (($row = $res->fetchArray(SQLITE3_ASSOC))) { + var_dump($row); +} +var_dump($res->fetchArray(SQLITE3_ASSOC)); +?> +--EXPECT-- +array(1) { + ["bar"]=> + int(1) +} +array(1) { + ["bar"]=> + int(2) +} +array(1) { + ["bar"]=> + int(3) +} +bool(false) From a2ddc7cc4fdd7461685ce9079ea43f96ab6ca4b2 Mon Sep 17 00:00:00 2001 From: Mark Gallagher Date: Thu, 7 Jul 2022 02:00:46 +0100 Subject: [PATCH 2/2] sqlite3: Fix additional issues introduced by bug 64531 fix Fixes an existing issue tha was surfaced in #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. --- ext/sqlite3/sqlite3.c | 17 +++-- ext/sqlite3/tests/bug64531_1.phpt | 2 +- ext/sqlite3/tests/bug64531_2.phpt | 2 +- ext/sqlite3/tests/bug64531_3.phpt | 26 ++++++++ ext/sqlite3/tests/bug64531_4.phpt | 66 +++++++++++++++++++ ext/sqlite3/tests/bug64531_5.phpt | 31 +++++++++ .../tests/sqlite3_38_extended_error_2.phpt | 36 ++++++++++ 7 files changed, 172 insertions(+), 8 deletions(-) create mode 100644 ext/sqlite3/tests/bug64531_3.phpt create mode 100644 ext/sqlite3/tests/bug64531_4.phpt create mode 100644 ext/sqlite3/tests/bug64531_5.phpt create mode 100644 ext/sqlite3/tests/sqlite3_38_extended_error_2.phpt diff --git a/ext/sqlite3/sqlite3.c b/ext/sqlite3/sqlite3.c index 2413b7a2cc0cd..0c0fa82fc8db7 100644 --- a/ext/sqlite3/sqlite3.c +++ b/ext/sqlite3/sqlite3.c @@ -596,8 +596,10 @@ PHP_METHOD(SQLite3, query) result->stmt_obj->last_step_result = return_code; switch (return_code) { - case SQLITE_ROW: /* Valid Row */ case SQLITE_DONE: /* Valid but no results */ + sqlite3_reset(result->stmt_obj->stmt); + ZEND_FALLTHROUGH; + case SQLITE_ROW: /* Valid Row */ { php_sqlite3_free_list *free_item; free_item = emalloc(sizeof(php_sqlite3_free_list)); @@ -1791,8 +1793,10 @@ PHP_METHOD(SQLite3Stmt, execute) stmt_obj->last_step_result = return_code; switch (return_code) { - case SQLITE_ROW: /* Valid Row */ case SQLITE_DONE: /* Valid but no results */ + sqlite3_reset(stmt_obj->stmt); + ZEND_FALLTHROUGH; + case SQLITE_ROW: /* Valid Row */ { object_init_ex(return_value, php_sqlite3_result_entry); result = Z_SQLITE3_RESULT_P(return_value); @@ -2284,12 +2288,13 @@ static void php_sqlite3_result_object_free_storage(zend_object *object) /* {{{ * sqlite3result_clear_column_names_cache(intern); if (!Z_ISNULL(intern->stmt_obj_zval)) { + zval_ptr_dtor(&intern->stmt_obj_zval); if (intern->stmt_obj && intern->stmt_obj->initialised) { - sqlite3_reset(intern->stmt_obj->stmt); - intern->stmt_obj->has_stepped = 0; + if (GC_REFCOUNT(&intern->stmt_obj->zo) == 0) { + sqlite3_reset(intern->stmt_obj->stmt); + intern->stmt_obj->has_stepped = 0; + } } - - zval_ptr_dtor(&intern->stmt_obj_zval); } zend_object_std_dtor(&intern->zo); diff --git a/ext/sqlite3/tests/bug64531_1.phpt b/ext/sqlite3/tests/bug64531_1.phpt index 02a944cf16ce8..0cded308d6725 100644 --- a/ext/sqlite3/tests/bug64531_1.phpt +++ b/ext/sqlite3/tests/bug64531_1.phpt @@ -1,5 +1,5 @@ --TEST-- -Bug #64531 (SQLite3Result::fetchArray runs the query again) +Bug #64531 (SQLite3Result::fetchArray runs the query again) - SQLite3->query --SKIPIF-- query --SKIPIF-- execute +--SKIPIF-- + +--FILE-- +exec("CREATE TABLE foo (id INT)"); + +$stmt = $conn->prepare("INSERT INTO foo VALUES (1)"); +$res = $stmt->execute(); +$res->fetchArray(); +$res->fetchArray(); + +$res = $conn->query("SELECT * FROM foo"); +while (($row = $res->fetchArray(SQLITE3_NUM))) { + var_dump($row); +} +?> +--EXPECT-- +array(1) { + [0]=> + int(1) +} diff --git a/ext/sqlite3/tests/bug64531_4.phpt b/ext/sqlite3/tests/bug64531_4.phpt new file mode 100644 index 0000000000000..0660d4560f94d --- /dev/null +++ b/ext/sqlite3/tests/bug64531_4.phpt @@ -0,0 +1,66 @@ +--TEST-- +Bug #64531 (SQLite3Result::fetchArray runs the query again) - SQLite3Stmt->execute (reused) +--SKIPIF-- + +--FILE-- +exec("CREATE TABLE foo (id INT)"); + +function testing($val) { + echo $val; + debug_print_backtrace(); + return $val; +} + +$conn->createFunction('testing', 'testing', 1); + +$stmt = $conn->prepare("INSERT INTO foo VALUES (testing(?))"); + +$stmt->bindValue(1, 1); + +$res = $stmt->execute(); // Should run +$res->fetchArray(); // Should not run +$res->fetchArray(); // Should not run +$res->fetchArray(); // Should not run + +$stmt->bindValue(1, 2); + +$res = $stmt->execute(); // Should run +$res->fetchArray(); // Should not run +$res->fetchArray(); // Should not run +$res->fetchArray(); // Should not run + +$stmt->bindValue(1, 3); + +$res2 = $stmt->execute(); // Should run +$res2->fetchArray(); // Should not run +$res2->fetchArray(); // Should not run +$res2->fetchArray(); // Should not run + +$res = $conn->query("SELECT * FROM foo"); +while (($row = $res->fetchArray(SQLITE3_NUM))) { + var_dump($row); +} +?> +--EXPECTF-- +1#0 [internal function]: testing(1) +#1 %s(17): SQLite3Stmt->execute() +2#0 [internal function]: testing(2) +#1 %s(24): SQLite3Stmt->execute() +3#0 [internal function]: testing(3) +#1 %s(31): SQLite3Stmt->execute() +array(1) { + [0]=> + int(1) +} +array(1) { + [0]=> + int(2) +} +array(1) { + [0]=> + int(3) +} \ No newline at end of file diff --git a/ext/sqlite3/tests/bug64531_5.phpt b/ext/sqlite3/tests/bug64531_5.phpt new file mode 100644 index 0000000000000..27d8df7fd61f0 --- /dev/null +++ b/ext/sqlite3/tests/bug64531_5.phpt @@ -0,0 +1,31 @@ +--TEST-- +Bug #64531 (SQLite3Result::fetchArray runs the query again) - SQLite3Stmt->prepare +--SKIPIF-- + +--FILE-- +exec("CREATE TABLE foo (id INT)"); +for ($i = 1; $i <= 3; $i++) { + $conn->exec("INSERT INTO foo VALUES ($i)"); +} +$conn->createFunction('testing', 'testing', 1); + +$stmt = $conn->prepare("SELECT * FROM foo WHERE testing(id)"); +$res = $stmt->execute(); +$arr = $res->fetchArray(); +$arr = $res->fetchArray(); +$arr = $res->fetchArray(); +$arr = $res->fetchArray(); +?> +--EXPECT-- +Testing with: 1 +Testing with: 2 +Testing with: 3 diff --git a/ext/sqlite3/tests/sqlite3_38_extended_error_2.phpt b/ext/sqlite3/tests/sqlite3_38_extended_error_2.phpt new file mode 100644 index 0000000000000..b407afe59fad7 --- /dev/null +++ b/ext/sqlite3/tests/sqlite3_38_extended_error_2.phpt @@ -0,0 +1,36 @@ +--TEST-- +SQLite3 extended error code Function +--EXTENSIONS-- +sqlite3 +--FILE-- +query("CREATE TABLE dog ( id INTEGER PRIMARY KEY, name TEXT, annoying INTEGER )"); + +echo "Inserting first time which should succeed" . PHP_EOL; +$result = $db->prepare("INSERT INTO dog VALUES (1, 'Annoying Dog', 1)")->execute(); +echo "First Error Code: " . $db->lastErrorCode() . PHP_EOL; + +echo "Inserting second time which should fail" . PHP_EOL; +$result = $db->prepare("INSERT INTO dog VALUES (1, 'Annoying Dog', 1)")->execute(); +echo "Second Error Code: " . $db->lastErrorCode() . PHP_EOL; +echo "Second Extended Error Code: " . $db->lastExtendedErrorCode() . PHP_EOL; + +echo "Closing database\n"; +var_dump($db->close()); +echo "Done" . PHP_EOL; +?> +--EXPECTF-- +Inserting first time which should succeed +First Error Code: 0 +Inserting second time which should fail + +Warning: SQLite3Stmt::execute(): Unable to execute statement: UNIQUE constraint failed: dog.id in %s on line %d +Second Error Code: 19 +Second Extended Error Code: 1555 +Closing database +bool(true) +Done +