From c1b07f426902cb5450ea2334d6079dfd73d62d29 Mon Sep 17 00:00:00 2001 From: hanshenrik Date: Sun, 13 Apr 2025 18:45:51 +0200 Subject: [PATCH 1/8] throw on null bytes / resolve GH-13952 fix a corruption issue where PDO::quote for SQLite would silently truncate strings with null bytes in them, by throwing. resolve GH-13952 close GH-13972 --- ext/pdo_sqlite/sqlite_driver.c | 5 +++ ext/pdo_sqlite/tests/gh13952.phpt | 59 +++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+) create mode 100644 ext/pdo_sqlite/tests/gh13952.phpt diff --git a/ext/pdo_sqlite/sqlite_driver.c b/ext/pdo_sqlite/sqlite_driver.c index 645d7bc3333ed..aa564557b93d1 100644 --- a/ext/pdo_sqlite/sqlite_driver.c +++ b/ext/pdo_sqlite/sqlite_driver.c @@ -226,6 +226,11 @@ static zend_string* sqlite_handle_quoter(pdo_dbh_t *dbh, const zend_string *unqu if (ZSTR_LEN(unquoted) > (INT_MAX - 3) / 2) { return NULL; } + if(memchr(ZSTR_VAL(unquoted), '\0', ZSTR_LEN(unquoted)) != NULL) { + zend_throw_exception_ex(php_pdo_get_exception(), 0, + "SQLite PDO::quote does not support NULL bytes"); + return NULL; + } quoted = safe_emalloc(2, ZSTR_LEN(unquoted), 3); /* TODO use %Q format? */ sqlite3_snprintf(2*ZSTR_LEN(unquoted) + 3, quoted, "'%q'", ZSTR_VAL(unquoted)); diff --git a/ext/pdo_sqlite/tests/gh13952.phpt b/ext/pdo_sqlite/tests/gh13952.phpt new file mode 100644 index 0000000000000..2423b249e4c7f --- /dev/null +++ b/ext/pdo_sqlite/tests/gh13952.phpt @@ -0,0 +1,59 @@ +--TEST-- +GH-13952 (sqlite PDO::quote silently corrupts strings with null bytes) +--EXTENSIONS-- +pdo +pdo_sqlite +--FILE-- + \PDO::ERRMODE_EXCEPTION, + \PDO::ATTR_DEFAULT_FETCH_MODE => \PDO::FETCH_ASSOC, + \PDO::ATTR_EMULATE_PREPARES => false, +]); + +$test_cases = [ + "", + "x", + "\x00", + "a\x00b", + "\x00\x00\x00", + "foobar", + "foo'''bar", + "'foo'''bar'", + "foo\x00bar", + "'foo'\x00'bar'", + "foo\x00\x00\x00bar", + "\x00foo\x00\x00\x00bar\x00", + "\x00\x00\x00foo", + "foo\x00\x00\x00", + "\x80", // Invalid UTF-8 sequence + "\x00\x80\x00", // Invalid UTF-8 sequence with null bytes +]; + +foreach ($test_cases as $test) { + $should_throw = str_contains($test, "\x00"); + try { + $quoted = $db->quote($test); + if ($should_throw) { + $displayTest = var_export($test, true); + throw new LogicException("Failed for {$displayTest}: expected an exception but none was thrown."); + } + } catch (\PDOException $e) { + if (!$should_throw) { + $displayTest = var_export($test, true); + throw new LogicException("Failed for {$displayTest}: unexpected exception thrown.", 0, $e); + } + // Exception is expected + continue; + } + $fetched = $db->query("SELECT $quoted")->fetch($db::FETCH_NUM)[0]; + if ($fetched !== $test) { + $displayTest = var_export($test, true); + $displayFetched = var_export($fetched, true); + throw new LogicException("Round-trip data corruption for {$displayTest}: got {$displayFetched}."); + } +} +echo "ok\n"; +?> +--EXPECT-- +ok From 6d426fb9d7fab8cef71ba4c0600be8e2c0cc987d Mon Sep 17 00:00:00 2001 From: hanshenrik Date: Sun, 13 Apr 2025 19:36:13 +0200 Subject: [PATCH 2/8] forgot about PDO::ERRMODE_SILENT --- ext/pdo_sqlite/sqlite_driver.c | 31 +++++++++++-- ext/pdo_sqlite/tests/gh13952.phpt | 76 +++++++++++++++++++------------ 2 files changed, 75 insertions(+), 32 deletions(-) diff --git a/ext/pdo_sqlite/sqlite_driver.c b/ext/pdo_sqlite/sqlite_driver.c index aa564557b93d1..6b7caa38ba176 100644 --- a/ext/pdo_sqlite/sqlite_driver.c +++ b/ext/pdo_sqlite/sqlite_driver.c @@ -28,6 +28,14 @@ #include "zend_exceptions.h" #include "sqlite_driver_arginfo.h" +/* + * Updated to add support for PDO::ERRMODE_SILENT and PDO::ERRMODE_WARNING + * in addition to PDO::ERRMODE_EXCEPTION. The error mode is stored in + * dbh->error_mode (assumed to be part of pdo_dbh_t) and is set in the + * handle factory. Depending on the error mode, errors either throw an + * exception, emit a warning, or remain silent. + */ + int _pdo_sqlite_error(pdo_dbh_t *dbh, pdo_stmt_t *stmt, const char *file, int line) /* {{{ */ { pdo_sqlite_db_handle *H = (pdo_sqlite_db_handle *)dbh->driver_data; @@ -74,8 +82,11 @@ int _pdo_sqlite_error(pdo_dbh_t *dbh, pdo_stmt_t *stmt, const char *file, int li break; } - if (!dbh->methods) { - pdo_throw_exception(einfo->errcode, einfo->errmsg, pdo_err); + /* Handle error based on error mode */ + if (dbh->error_mode == PDO_ERRMODE_EXCEPTION) { + zend_throw_exception_ex(php_pdo_get_exception(), einfo->errcode, "%s", einfo->errmsg); + } else if (dbh->error_mode == PDO_ERRMODE_WARNING) { + php_error_docref(NULL, E_WARNING, "%s", einfo->errmsg); } return einfo->errcode; @@ -227,8 +238,12 @@ static zend_string* sqlite_handle_quoter(pdo_dbh_t *dbh, const zend_string *unqu return NULL; } if(memchr(ZSTR_VAL(unquoted), '\0', ZSTR_LEN(unquoted)) != NULL) { - zend_throw_exception_ex(php_pdo_get_exception(), 0, - "SQLite PDO::quote does not support NULL bytes"); + if (dbh->error_mode == PDO_ERRMODE_EXCEPTION) { + zend_throw_exception_ex(php_pdo_get_exception(), 0, + "SQLite PDO::quote does not support NULL bytes"); + } else if (dbh->error_mode == PDO_ERRMODE_WARNING) { + php_error_docref(NULL, E_WARNING, "SQLite PDO::quote does not support NULL bytes"); + } return NULL; } quoted = safe_emalloc(2, ZSTR_LEN(unquoted), 3); @@ -842,6 +857,14 @@ static int pdo_sqlite_handle_factory(pdo_dbh_t *dbh, zval *driver_options) /* {{ if (driver_options) { timeout = pdo_attr_lval(driver_options, PDO_ATTR_TIMEOUT, timeout); } + + /* Set the error mode attribute based on driver options, defaulting to ERRMODE_EXCEPTION */ + if (driver_options) { + dbh->error_mode = pdo_attr_lval(driver_options, PDO_ATTR_ERRMODE, PDO_ERRMODE_EXCEPTION); + } else { + dbh->error_mode = PDO_ERRMODE_EXCEPTION; + } + sqlite3_busy_timeout(H->db, timeout * 1000); dbh->alloc_own_columns = 1; diff --git a/ext/pdo_sqlite/tests/gh13952.phpt b/ext/pdo_sqlite/tests/gh13952.phpt index 2423b249e4c7f..b6a080cfa22a2 100644 --- a/ext/pdo_sqlite/tests/gh13952.phpt +++ b/ext/pdo_sqlite/tests/gh13952.phpt @@ -1,15 +1,16 @@ --TEST-- -GH-13952 (sqlite PDO::quote silently corrupts strings with null bytes) +GH-13952 (sqlite PDO::quote handles null bytes correctly) --EXTENSIONS-- pdo pdo_sqlite --FILE-- \PDO::ERRMODE_EXCEPTION, - \PDO::ATTR_DEFAULT_FETCH_MODE => \PDO::FETCH_ASSOC, - \PDO::ATTR_EMULATE_PREPARES => false, -]); + +$modes = [ + 'exception' => PDO::ERRMODE_EXCEPTION, + 'warning' => PDO::ERRMODE_WARNING, + 'silent' => PDO::ERRMODE_SILENT, +]; $test_cases = [ "", @@ -26,34 +27,53 @@ $test_cases = [ "\x00foo\x00\x00\x00bar\x00", "\x00\x00\x00foo", "foo\x00\x00\x00", - "\x80", // Invalid UTF-8 sequence - "\x00\x80\x00", // Invalid UTF-8 sequence with null bytes + "\x80", // << invalid UTF-8 + "\x00\x80\x00", // << invalid UTF-8 with null bytes ]; -foreach ($test_cases as $test) { - $should_throw = str_contains($test, "\x00"); - try { - $quoted = $db->quote($test); - if ($should_throw) { - $displayTest = var_export($test, true); - throw new LogicException("Failed for {$displayTest}: expected an exception but none was thrown."); - } - } catch (\PDOException $e) { - if (!$should_throw) { - $displayTest = var_export($test, true); - throw new LogicException("Failed for {$displayTest}: unexpected exception thrown.", 0, $e); +foreach ($modes as $mode_name => $mode) { + echo "Testing error mode: $mode_name\n"; + $db = new PDO('sqlite::memory:', null, null, [PDO::ATTR_ERRMODE => $mode]); + + foreach ($test_cases as $test) { + $contains_null = str_contains($test, "\x00"); + + if ($mode === PDO::ERRMODE_EXCEPTION && $contains_null) { + set_error_handler(fn() => throw new PDOException(), E_WARNING); + try { + $db->quote($test); + throw new LogicException("Expected exception not thrown."); + } catch (PDOException) { + // expected + } finally { + restore_error_handler(); + } + } else { + set_error_handler(fn() => null, E_WARNING); + $quoted = $db->quote($test); + restore_error_handler(); + + if ($contains_null) { + if ($quoted !== false) { + throw new LogicException("Expected false, got: " . var_export($quoted, true)); + } + } else { + if ($quoted === false) { + throw new LogicException("Unexpected false from quote()."); + } + $fetched = $db->query("SELECT $quoted")->fetchColumn(); + if ($fetched !== $test) { + throw new LogicException("Data corrupted: expected " . var_export($test, true) . " got " . var_export($fetched, true)); + } + } } - // Exception is expected - continue; - } - $fetched = $db->query("SELECT $quoted")->fetch($db::FETCH_NUM)[0]; - if ($fetched !== $test) { - $displayTest = var_export($test, true); - $displayFetched = var_export($fetched, true); - throw new LogicException("Round-trip data corruption for {$displayTest}: got {$displayFetched}."); } } + echo "ok\n"; ?> --EXPECT-- +Testing error mode: exception +Testing error mode: warning +Testing error mode: silent ok From 0d4f5519ebb161b6c1c8318f45f376ecbc35c1e2 Mon Sep 17 00:00:00 2001 From: hanshenrik Date: Sun, 13 Apr 2025 19:45:36 +0200 Subject: [PATCH 3/8] stray comment --- ext/pdo_sqlite/sqlite_driver.c | 7 ------- 1 file changed, 7 deletions(-) diff --git a/ext/pdo_sqlite/sqlite_driver.c b/ext/pdo_sqlite/sqlite_driver.c index 6b7caa38ba176..3da2124561142 100644 --- a/ext/pdo_sqlite/sqlite_driver.c +++ b/ext/pdo_sqlite/sqlite_driver.c @@ -28,13 +28,6 @@ #include "zend_exceptions.h" #include "sqlite_driver_arginfo.h" -/* - * Updated to add support for PDO::ERRMODE_SILENT and PDO::ERRMODE_WARNING - * in addition to PDO::ERRMODE_EXCEPTION. The error mode is stored in - * dbh->error_mode (assumed to be part of pdo_dbh_t) and is set in the - * handle factory. Depending on the error mode, errors either throw an - * exception, emit a warning, or remain silent. - */ int _pdo_sqlite_error(pdo_dbh_t *dbh, pdo_stmt_t *stmt, const char *file, int line) /* {{{ */ { From 097db319c160dd20987ebab89efda367b9829728 Mon Sep 17 00:00:00 2001 From: hanshenrik Date: Sun, 13 Apr 2025 19:52:07 +0200 Subject: [PATCH 4/8] wasn't supposed to be commited --- ext/pdo_sqlite/sqlite_driver.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/ext/pdo_sqlite/sqlite_driver.c b/ext/pdo_sqlite/sqlite_driver.c index 3da2124561142..57bef2d9f6ba6 100644 --- a/ext/pdo_sqlite/sqlite_driver.c +++ b/ext/pdo_sqlite/sqlite_driver.c @@ -851,12 +851,6 @@ static int pdo_sqlite_handle_factory(pdo_dbh_t *dbh, zval *driver_options) /* {{ timeout = pdo_attr_lval(driver_options, PDO_ATTR_TIMEOUT, timeout); } - /* Set the error mode attribute based on driver options, defaulting to ERRMODE_EXCEPTION */ - if (driver_options) { - dbh->error_mode = pdo_attr_lval(driver_options, PDO_ATTR_ERRMODE, PDO_ERRMODE_EXCEPTION); - } else { - dbh->error_mode = PDO_ERRMODE_EXCEPTION; - } sqlite3_busy_timeout(H->db, timeout * 1000); From 3f31caefba2e29eb990de093634d342c9a9516e5 Mon Sep 17 00:00:00 2001 From: hanshenrik Date: Sun, 13 Apr 2025 19:56:06 +0200 Subject: [PATCH 5/8] git problems sorry --- ext/pdo_sqlite/sqlite_driver.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/ext/pdo_sqlite/sqlite_driver.c b/ext/pdo_sqlite/sqlite_driver.c index 57bef2d9f6ba6..6cf6f71d58745 100644 --- a/ext/pdo_sqlite/sqlite_driver.c +++ b/ext/pdo_sqlite/sqlite_driver.c @@ -74,12 +74,8 @@ int _pdo_sqlite_error(pdo_dbh_t *dbh, pdo_stmt_t *stmt, const char *file, int li strncpy(*pdo_err, "HY000", sizeof(*pdo_err)); break; } - - /* Handle error based on error mode */ - if (dbh->error_mode == PDO_ERRMODE_EXCEPTION) { - zend_throw_exception_ex(php_pdo_get_exception(), einfo->errcode, "%s", einfo->errmsg); - } else if (dbh->error_mode == PDO_ERRMODE_WARNING) { - php_error_docref(NULL, E_WARNING, "%s", einfo->errmsg); + if (!dbh->methods) { + pdo_throw_exception(einfo->errcode, einfo->errmsg, pdo_err); } return einfo->errcode; From 18b9e1809068d6c3b4371c1829a659995c8c1268 Mon Sep 17 00:00:00 2001 From: hanshenrik Date: Sun, 13 Apr 2025 22:40:43 +0200 Subject: [PATCH 6/8] whitespace fixes / pr feedback --- ext/pdo_sqlite/sqlite_driver.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/ext/pdo_sqlite/sqlite_driver.c b/ext/pdo_sqlite/sqlite_driver.c index 6cf6f71d58745..e27f888f98a99 100644 --- a/ext/pdo_sqlite/sqlite_driver.c +++ b/ext/pdo_sqlite/sqlite_driver.c @@ -28,7 +28,6 @@ #include "zend_exceptions.h" #include "sqlite_driver_arginfo.h" - int _pdo_sqlite_error(pdo_dbh_t *dbh, pdo_stmt_t *stmt, const char *file, int line) /* {{{ */ { pdo_sqlite_db_handle *H = (pdo_sqlite_db_handle *)dbh->driver_data; @@ -74,6 +73,7 @@ int _pdo_sqlite_error(pdo_dbh_t *dbh, pdo_stmt_t *stmt, const char *file, int li strncpy(*pdo_err, "HY000", sizeof(*pdo_err)); break; } + if (!dbh->methods) { pdo_throw_exception(einfo->errcode, einfo->errmsg, pdo_err); } @@ -226,15 +226,19 @@ static zend_string* sqlite_handle_quoter(pdo_dbh_t *dbh, const zend_string *unqu if (ZSTR_LEN(unquoted) > (INT_MAX - 3) / 2) { return NULL; } - if(memchr(ZSTR_VAL(unquoted), '\0', ZSTR_LEN(unquoted)) != NULL) { - if (dbh->error_mode == PDO_ERRMODE_EXCEPTION) { - zend_throw_exception_ex(php_pdo_get_exception(), 0, - "SQLite PDO::quote does not support NULL bytes"); - } else if (dbh->error_mode == PDO_ERRMODE_WARNING) { - php_error_docref(NULL, E_WARNING, "SQLite PDO::quote does not support NULL bytes"); - } - return NULL; + + if (UNEXPECTED(zend_str_has_nul_byte(unquoted))) { + if (dbh->error_mode == PDO_ERRMODE_EXCEPTION) { + zend_throw_exception_ex( + php_pdo_get_exception(), 0, + "SQLite PDO::quote does not support null bytes"); + } else if (dbh->error_mode == PDO_ERRMODE_WARNING) { + php_error_docref(NULL, E_WARNING, + "SQLite PDO::quote does not support null bytes"); + } + return NULL; } + quoted = safe_emalloc(2, ZSTR_LEN(unquoted), 3); /* TODO use %Q format? */ sqlite3_snprintf(2*ZSTR_LEN(unquoted) + 3, quoted, "'%q'", ZSTR_VAL(unquoted)); @@ -750,7 +754,7 @@ static const struct pdo_dbh_methods sqlite_methods = { pdo_sqlite_request_shutdown, pdo_sqlite_in_transaction, pdo_sqlite_get_gc, - pdo_sqlite_scanner + pdo_sqlite_scanner }; static char *make_filename_safe(const char *filename) @@ -846,8 +850,6 @@ static int pdo_sqlite_handle_factory(pdo_dbh_t *dbh, zval *driver_options) /* {{ if (driver_options) { timeout = pdo_attr_lval(driver_options, PDO_ATTR_TIMEOUT, timeout); } - - sqlite3_busy_timeout(H->db, timeout * 1000); dbh->alloc_own_columns = 1; From c4192457edfad50c0b0a25bf7b98d9494b1ea3aa Mon Sep 17 00:00:00 2001 From: hanshenrik Date: Sun, 13 Apr 2025 22:43:34 +0200 Subject: [PATCH 7/8] whitespace... --- ext/pdo_sqlite/sqlite_driver.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/ext/pdo_sqlite/sqlite_driver.c b/ext/pdo_sqlite/sqlite_driver.c index e27f888f98a99..fdef9e1abe377 100644 --- a/ext/pdo_sqlite/sqlite_driver.c +++ b/ext/pdo_sqlite/sqlite_driver.c @@ -228,15 +228,15 @@ static zend_string* sqlite_handle_quoter(pdo_dbh_t *dbh, const zend_string *unqu } if (UNEXPECTED(zend_str_has_nul_byte(unquoted))) { - if (dbh->error_mode == PDO_ERRMODE_EXCEPTION) { + if (dbh->error_mode == PDO_ERRMODE_EXCEPTION) { zend_throw_exception_ex( php_pdo_get_exception(), 0, "SQLite PDO::quote does not support null bytes"); - } else if (dbh->error_mode == PDO_ERRMODE_WARNING) { + } else if (dbh->error_mode == PDO_ERRMODE_WARNING) { php_error_docref(NULL, E_WARNING, "SQLite PDO::quote does not support null bytes"); - } - return NULL; + } + return NULL; } quoted = safe_emalloc(2, ZSTR_LEN(unquoted), 3); From 5506b75039ef323cc11593aa2ef540cb96111cb0 Mon Sep 17 00:00:00 2001 From: hanshenrik Date: Sun, 13 Apr 2025 23:14:28 +0200 Subject: [PATCH 8/8] fsck whitespace --- ext/pdo_sqlite/sqlite_driver.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/ext/pdo_sqlite/sqlite_driver.c b/ext/pdo_sqlite/sqlite_driver.c index fdef9e1abe377..8a880a3425fba 100644 --- a/ext/pdo_sqlite/sqlite_driver.c +++ b/ext/pdo_sqlite/sqlite_driver.c @@ -229,12 +229,12 @@ static zend_string* sqlite_handle_quoter(pdo_dbh_t *dbh, const zend_string *unqu if (UNEXPECTED(zend_str_has_nul_byte(unquoted))) { if (dbh->error_mode == PDO_ERRMODE_EXCEPTION) { - zend_throw_exception_ex( - php_pdo_get_exception(), 0, - "SQLite PDO::quote does not support null bytes"); + zend_throw_exception_ex( + php_pdo_get_exception(), 0, + "SQLite PDO::quote does not support null bytes"); } else if (dbh->error_mode == PDO_ERRMODE_WARNING) { - php_error_docref(NULL, E_WARNING, - "SQLite PDO::quote does not support null bytes"); + php_error_docref(NULL, E_WARNING, + "SQLite PDO::quote does not support null bytes"); } return NULL; }