From 272245632832d7f93e176634fee129e6eaba3ee9 Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Mon, 24 Feb 2020 11:49:49 +0100 Subject: [PATCH] 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 5a25af61e19bc..405dfebab8c67 100644 --- a/ext/sqlite3/php_sqlite3_structs.h +++ b/ext/sqlite3/php_sqlite3_structs.h @@ -124,7 +124,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 c2b4fa8bca54b..219bdf9ae6c14 100644 --- a/ext/sqlite3/sqlite3.c +++ b/ext/sqlite3/sqlite3.c @@ -560,6 +560,9 @@ PHP_METHOD(sqlite3, query) ZVAL_COPY_VALUE(&result->stmt_obj_zval, &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 */ @@ -570,7 +573,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: @@ -579,6 +581,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; } @@ -1346,6 +1349,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; @@ -1648,12 +1652,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); @@ -1664,9 +1670,6 @@ PHP_METHOD(sqlite3stmt, execute) break; } - case SQLITE_ERROR: - sqlite3_reset(stmt_obj->stmt); - default: if (!EG(exception)) { php_sqlite3_error(stmt_obj->db_obj, "Unable to execute statement: %s", sqlite3_errmsg(sqlite3_db_handle(stmt_obj->stmt))); @@ -1810,7 +1813,14 @@ PHP_METHOD(sqlite3result, fetchArray) return; } - 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 */ @@ -1838,6 +1848,7 @@ PHP_METHOD(sqlite3result, fetchArray) add_assoc_zval(return_value, (char*)sqlite3_column_name(result_obj->stmt_obj->stmt, i), &data); } } + result_obj->stmt_obj->has_stepped = 0; break; case SQLITE_DONE: @@ -1864,6 +1875,7 @@ PHP_METHOD(sqlite3result, reset) return; } + result_obj->stmt_obj->has_stepped = 0; if (sqlite3_reset(result_obj->stmt_obj->stmt) != SQLITE_OK) { RETURN_FALSE; } @@ -1891,6 +1903,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); } @@ -2204,6 +2217,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); @@ -2243,6 +2257,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)