SQLite pdo::quote use ('foo'||x'0000'||'bar') for null bytes #13972
Conversation
resolves issue phpGH-13952, this is basically a smart_str() version of PR php#13956 per php#13962 (comment) this is 1 of the 4 proposed alternatives to the problem, and the pros of this solution is that it produces smaller queries than the alternatives, and retains the sqlite datatype 'string' (instead of changing it to blob), and should make PDO::quote faster as we now avoid the overhead of copying data to/from sqlite3_snprintf. The cons of this solution, that I can think of right now, is that the implementation is non-trivial, involves a bunch of php-allocator-reallocs() (PR php#13956 does not invovle reallocs, as it pre-computes the length. also worth noting that php allocator's reallocs() are faster than libc realloc, often avoiding talking to the OS), and SQLite's LENGTH(('foo'||x'00'||'bar')) returns 3 instead of 7, and binary strings gets the datatype 'string' instead of 'blob' (that can be considered both a pro and a con) Co-authored-by: Niels Dossche <nielsdos@php.net>
|
We're making a drastic change to the output format of this function. This may have breaks or implications we can't predict. Especially as sqlite developers have said that NULL bytes in text aren't supported by them, it seems risky to try to support something like this. |
Only for an edge case, seems it took 19 years (2006?) before someone noticed that SQLite's pdo::quote("\x00") is broken , which suggests that it is very rarely done in practice, otherwise someone would have noticed over 10 years ago |
|
As I wrote here, it is unlikely that anyone would be able to predict that this method will convert a string to binary without prior information. If the user wants binaries, they can achieve it like this: (this is hex, not bin...) It can also be solved with a prepare statement as mentioned by Niels. We should not accept values that SQLite does not support (strings containing null). Often such cases are the user's mistake and it seems more reasonable to raise an error and tell them what is wrong than to accept it. However, passing a string containing null to the SQLite CLI Client does not result in an error; the string after null is simply discarded. If we respect the SQLite spec, it's also possible that we shouldn't raise the error. |
|
This should not cause data corruption: <?php
$pdo = new PDO('sqlite::memory:', null, null, [PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC]);
$pdo->exec("CREATE TABLE tbl (data BLOB)");
$data = "foo\x00bar";
$pdo->exec("INSERT INTO tbl VALUES(" . $pdo->quote($data) .")");
var_dump($pdo->query("SELECT * FROM tbl")->fetchAll());but it fking does. it output that is #bullshit#. It should either throw (ValueError? InvalidArgumentException? UnexpectedValueException? idk) or it should generate get it fixed ffs. when I use quote(), I expect data to be safely handled. but PDO::quote() is not safely handling my data, This reminds me of that that mysql_escape_string() thing, before mysql_real_escape_string() 🤦♂️ |
|
Swearing and asking to get it fixed ffs won't motivate me to do anything. On the contrary. |
|
If you’re trying to achieve your goal without using prepared statements, it might make more sense to submit a feature request to the sqlite3 developers asking for support for null characters in What you're currently attempting is, in a sense, a hack that goes against the intended design of sqlite3. If However, personally, I think it's unlikely that such a request would be implemented, and I support the option of throwing an exception instead. |
fix a corruption issue where PDO::quote for SQLite would silently truncate strings with null bytes in them, by throwing. resolve phpGH-13952 close phpGH-13972
|
In MySQL and Postgres you can safely do $str=random_bytes(1000);
$pdo->query("SELECT " . $pdo->quote($str))->fetch(\PDO::FETCH_COLUMN)[0]
=== $str;but not in SQLite :( Makes life difficult for people who are used to code for MySQL. |
resolves issue GH-13952,
this is basically a smart_str() version of PR #13956
per #13962 (comment) this is 1 of the 4 proposed alternatives to the problem, and the pros of this solution is that it produces smaller queries than the alternatives, and retains the sqlite datatype 'string' (instead of changing it to blob), and should make PDO::quote faster as we now avoid the overhead of copying data to/from sqlite3_snprintf.
The cons of this solution, that I can think of right now,
is that the implementation is non-trivial,
involves a bunch of php-allocator-reallocs() (PR #13956 does not invovle reallocs, as it pre-computes the length.
also worth noting that php allocator's reallocs() are faster than libc realloc, often avoiding talking to the OS),
and SQLite's LENGTH(('foo'||x'00'||'bar')) returns 3 instead of 7,
and binary strings gets the datatype 'string' instead of 'blob' (that can be considered both a pro and a con)