Fix GH-13952: sqlite PDO::quote silently corrupts strings with null bytes by ndossche · Pull Request #13956 · php/php-src

ndossche · 2024-04-13T11:30:44Z

The built-in way to quote string using sqlite3's custom printf does not support NULL bytes in a string. Reimplement it ourselves. This also gets rid of the integer-based length limit.

…l bytes The built-in way to quote string using sqlite3's custom printf does not support NULL bytes in a string. Reimplement it ourselves. This also gets rid of the integer-based length limit.

devnexen · 2024-04-13T12:18:32Z

+ state_in_nulls = true;
+ if (source == ZSTR_VAL(unquoted)) {
+ quoted_dest--; /* backup initial ' */
+ memcpy(quoted_dest, null_state_enter_at_start, sizeof(null_state_enter_at_start) - 1);


would zend_mempcpy be usable here ?

Yes, I can do that later today

SakiTakamachi

Looks good to me.

(I think you probably already know this, but I wrote it just in case.)

Although it's completely unnecessary to include it in this PR and is outside of its scope, SQLite3::escapeString has the same issue. The warning in the documentation explains that.

https://www.php.net/manual/en/sqlite3.escapestring.php

SakiTakamachi · 2024-04-13T13:23:45Z

Also, I have a SQLite community account, would you mind if I report this issue upstream?

ndossche · 2024-04-13T13:37:15Z

Although it's completely unnecessary to include it in this PR and is outside of its scope, SQLite3::escapeString has the same issue.

Ugh, right, and we can't really share code between the two extensions afaik. But I dislike having to copy paste code...

Also, I have a SQLite community account, would you mind if I report this issue upstream?

You could try, but the API used is a printf-like API which can't really support null-contained strings properly. I guess they would need to make a new API...

SakiTakamachi · 2024-04-13T13:58:32Z

Ugh, right, and we can't really share code between the two extensions afaik. But I dislike having to copy paste code...

Yeah, I was thinking about those too. Presumably this is a pdo specific issue and other drivers may have issues with code duplication in the future too. It would be nice to have a place to put code like shared...

You could try, but the API used is a printf-like API which can't really support null-contained strings properly. I guess they would need to make a new API...

Ah, I understand it. Doesn't seem like much worth reporting...

mvorisek · 2024-04-13T16:02:27Z

+-> ''
+x -> 'x'
+\u0000 -> x'00'
+a\u0000b -> 'a'||x'00'||'b'


Suggested change
a\u0000b -> 'a'||x'00'||'b'
a\u0000b ->('a'||x'00'||'b')
to make sure higher precedence operator (like COLLATE) applies to the whole string - https://www.sqlite.org/lang_expr.html#operators_and_parse_affecting_attributes

Good catch!

adsr · 2024-04-13T16:17:12Z

+
+	*quoted_dest++ = '\'';
+	*quoted_dest = '\0';
+


Not suggesting it's better but alternatively we could convert the entire string to hex if it contains null chars. Pros: faster, simpler impl. Cons (in the all-hex case): more string memory, unreadable return value. Curious what you thought of those trade-offs.

Yeah the extra memory / IO bandwidth necessary for that case was the main argument against that. In particular, if we don't expect many NULL bytes then the current approach is better.

divinity76 · 2024-04-13T17:59:01Z

I think this is a good fit for smart_str. A DOMXPath::quote()-inspired implementation looks something like

staticvoid*memchr_inverse(constchar*ptr, constsize_tlen, constcharc)
{
constchar*constend=ptr+len;
while (ptr<end) {
if (*ptr!=c) {
return (void*)ptr;
}
++ptr;
}
returnNULL;
}
/* NB: doesn't handle binary strings... use prepared stmts for that */staticzend_string*sqlite_handle_quoter(pdo_dbh_t*dbh, constzend_string*unquoted, enumpdo_param_typeparamtype)
{
constchar*ptr=ZSTR_VAL(unquoted);
constchar*constend=ZSTR_VAL(unquoted) +ZSTR_LEN(unquoted);
smart_stroutput= {0};
boolis_in_quote= false;
if(ptr==end) {
smart_str_appendl(&output, "''", 2);
returnsmart_str_extract(&output);
}
while (ptr<end) {
// \x00 and ' needs special handlingconstchar*constsingle_quote_ptr=memchr(ptr, '\'', end-ptr);
constchar*constnull_ptr=memchr(ptr, '\0', end-ptr);
constsize_tdistance_to_single_quote=single_quote_ptr ? single_quote_ptr-ptr : end-ptr;
constsize_tdistance_to_null=null_ptr ? null_ptr-ptr : end-ptr;
constsize_tbytes_until_special=MIN(distance_to_single_quote, distance_to_null);
if (bytes_until_special) {
if(!is_in_quote) {
if(ptr!=ZSTR_VAL(unquoted)) {
smart_str_appendl(&output, "||", 2);
}
smart_str_appendc(&output, '\'');
is_in_quote= true;
}
smart_str_appendl(&output, ptr, bytes_until_special);
ptr+=bytes_until_special;
ZEND_ASSERT(ptr <= end);
if(ptr==end) {
break;
}
}
if(*ptr=='\'') {
if(!is_in_quote) {
if(ptr!=ZSTR_VAL(unquoted)) {
smart_str_appendl(&output, "||", 2);
}
smart_str_appendc(&output, '\'');
is_in_quote= true;
}
constchar*constsingle_quotes_end=memchr_inverse(ptr, end-ptr, '\'');
constsize_tnumber_of_consecutive_single_quotes=single_quotes_end ? single_quotes_end-ptr : end-ptr;
smart_str_appendl(&output, ptr, number_of_consecutive_single_quotes);
smart_str_appendl(&output, ptr, number_of_consecutive_single_quotes);
ptr+=number_of_consecutive_single_quotes;
} else {
ZEND_ASSERT(*ptr=='\0');
if(is_in_quote) {
smart_str_appendl(&output, "'||", 3);
is_in_quote= false;
}
constchar*constnull_end=memchr_inverse(ptr, end-ptr, '\0');
constsize_tnumber_of_consecutive_nulls=null_end ? null_end-ptr : end-ptr;
smart_str_appendl(&output, "x'", 2);
for(size_ti=0; i<number_of_consecutive_nulls; ++i) {
smart_str_appendl(&output, "00", 2);
}
smart_str_appendc(&output, '\'');
ptr+=number_of_consecutive_nulls;
}
}
if(is_in_quote) {
smart_str_appendc(&output, '\'');
}
returnsmart_str_extract(&output);
}

edit: I thought a smart_str approach would be significantly smaller, but ... it isn't! well i'm surprised 😮

ndossche · 2024-04-13T18:20:43Z

@divinity76 I have thought of using smart_str, but it requires more reallocations when the string buffer needs to be extended, which is why I precomputed the length. Ultimately, it's quite difficult to tell which approach is truly better.
~~BTW, you can use memrchr to do a backwards memchr.~~ NVM I misread

divinity76 · 2024-04-13T18:56:23Z

true, smart_str isn't optimally performant, but in this case i think it reduces the possibility of bugs, and is probably fast enough

(Maybe some day it will be re-written in memory-safe Rust 🙏 )

ndossche · 2024-04-13T19:54:50Z

true, smart_str isn't optimally performant, but in this case i think it reduces the possibility of bugs, and is probably fast enough

Might very well be the case. We might try setting up benchmarking although a realistic test might be hard to do 🤷

(Maybe some day it will be re-written in memory-safe Rust 🙏 )

I wish, although I have a bad feeling that getting the policy RFC for Rust passing in internals will be difficult to do.

divinity76 · 2024-04-13T20:00:26Z

Something is fishy with this approach though, i can reproduce the odd || concat issue even in C:

$sqlite3 '' "SELECT LENGTH(x'666f6f00626172');"7$sqlite3 '' "SELECT LENGTH('foo'||x'00'||'bar');"
3$catpuck.c#include<stdio.h>#include<sqlite3.h>intmain()
{
sqlite3*db;
char*err_msg=NULL;
intrc=sqlite3_open(":memory:", &db);
constchar*sql="SELECT LENGTH('foo'||x'00'||'bar');";
sqlite3_stmt*stmt;
rc=sqlite3_prepare_v2(db, sql, -1, &stmt, NULL);
rc=sqlite3_step(stmt);
printf("Result: %d\n", sqlite3_column_int(stmt, 0));
sqlite3_finalize(stmt);
sqlite3_close(db);
return0;
}
$gccpuck.c-lsqlite3$ ./a.outResult: 3

and I wonder why we can not reproduce the || issue in PHP

divinity76 · 2024-04-13T20:16:44Z

umm... this doesn't look safe?

$ php -r "\$db=new PDO('sqlite::memory:');var_dump(\$db->query(\"SELECT LENGTH('foo'||x'00'||'bar')\")->fetch());"
Command line code:1:
array(2) {
'LENGTH(\'foo\'||x\'00\'||\'bar\')' =>
int(3)
[0] =>
int(3)
}

mvorisek · 2024-04-13T20:32:24Z

This is documented in https://www.sqlite.org/lang_corefunc.html#length

cast(...||...||... as blob) is more or less equivalent to x'...'

but be aware BLOBs are printed differently - ex. SELECT x'302030'

ndossche · 2024-04-13T20:37:27Z

What a damn mess, I wonder then if we even should support this given the surprises about the API stability wrt NUL bytes... So weird it works with SELECT and INSERT but there's a different behaviour for LENGTH.
In that case it might be better to switch to x'...' when we encounter a single NUL byte I guess... But I'm afraid of the issues that it might bring.
Sorry but I'm keeping my hands off this, this is a mess.

divinity76 · 2024-04-13T21:15:28Z

@nielsdos understandable, thanks for the effort thus far!

that case it might be better to switch to x'...' when we encounter a single NUL byte

Yeah that sounds good. Any objections anyone?

mvorisek · 2024-04-13T21:18:02Z

@nielsdos see https://www.db-fiddle.com/f/8ZSXSyCZyRM4QZR2cptbhC/0

I belive we do not want explicit BLOB cast nor implicit BLOB cast from x'...' syntax. The reason is the print Sqlite legacy behaviour is simply irrelevant and NUL byte presence should be change the type (it is not a BLOB now). All we want is to quote input string that can be retrived 1:1 back.

So this PR LGTM.

PS: for string consisting of only NUL bytes the output should be (''||x'00...') to always avoid BLOB

SakiTakamachi · 2024-04-13T23:12:50Z

For reference, for MySQL CLI

mysql> SELECT 'foo'||x'00'||'bar';
+---------------------+
| 'foo'||x'00'||'bar' |
+---------------------+
| 0 |
+---------------------+
1 row in set, 4 warnings (0.00 sec)
mysql> SELECT LENGTH('foo'||x'00'||'bar');
+-----------------------------+
| LENGTH('foo'||x'00'||'bar') |
+-----------------------------+
| 1 |
+-----------------------------+
1 row in set, 4 warnings (0.00 sec)

(edit)
ah, Mysql couldn't use this.

SakiTakamachi · 2024-04-13T23:41:30Z

PostgreSQL

<?php
$db = new PDO(/* pgsql */);
var_dump($db->query("SELECT LENGTH('foo'||x'00'||'bar')")->fetch());

Fatal error: Uncaught PDOException: SQLSTATE[22P02]: Invalid text representation: 7 ERROR: "f" is not a valid binary digit
LINE 1: SELECT LENGTH('foo'||x'00'||'bar')

(edit)
It might be worth reporting this SQLite behavior upstream to see if it's intended.

devnexen · 2024-04-14T05:33:32Z

I doubt PostgreSQL supports this feature :)

SakiTakamachi · 2024-04-14T06:54:23Z

Character concatenation using || is supported by PostgreSQL. (For reference, I have written the behavior when null bytes are mixed in.)

devnexen · 2024-04-14T07:17:05Z

I did not mean concat per say but mixing string and binary.
e.g. "SELECT LENGTH('foo'||'bar')" for sure works.

SakiTakamachi · 2024-04-14T07:30:35Z

Ah, yes it is. However, the following way of writing seems to be supported:

var_dump($db->query("SELECT CONCAT('aaa', x'00', 'bar')")->fetch());
array(2) {
["concat"]=>
string(14) "aaa00000000bar"
[0]=>
string(14) "aaa00000000bar"
}

var_dump($db->query("SELECT LENGTH(CONCAT('aaa', x'00', 'bar'))")->fetch());
array(2) {
["length"]=>
int(14)
[0]=>
int(14)
}

In any case, character concatenation in SQLite only supports ||, so I'm thinking of reporting this to upstream.

SakiTakamachi · 2024-04-14T07:38:28Z

Hmm, when I run it from the cli client, it looks like this...

sqlite> select 'aaa\0bbb';
aaa\0bbb
sqlite> select length('aaa\0bbb');
8

(edit) Ah, maybe the cli just interprets it as a string literal.

divinity76 · 2024-04-14T17:25:26Z

made a x'hex' alternative at #13962
if we agree on going that route.

divinity76 · 2024-04-15T10:50:05Z

If we want to do the (''||x'610062') thing to make sure quote() always generate a datatype string instead of blob,
then yeah, i think this PR and approach is better than #13962 and should be resumed:
This PR is faster (avoiding the overhead of copying to/from sqlite3_snprintf) and produce smaller queries too 🤔

Implements x'hex' encoding in pdo::quote for handling strings with null bytes, providing a reliable workaround for issue phpGH-13952. An alternative fix is discussed in PR php#13956 PR php#13956 does something interesting, it avoids the overhead of copying to/from sqlite3_snprintf, probably speeding up PDO::quote, but right now I just want to keep things simple. Co-authored-by: Niels Dossche <nielsdos@php.net>

ndossche · 2024-04-15T15:29:23Z

We'll see what Saki's question on the forum will receive as an answer I guess, I can always reopen this.

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>

SakiTakamachi · 2024-04-15T15:39:21Z

@nielsdos
I immediately received one reply. It's a very simple answer.

Emebdded NUL bytes are not supported for TEXT fields. Use BLOB type for things with embedded NUL characters.

Indeed, it seems to be working correctly with blobs.
https://gist.github.com/SakiTakamachi/8d994fc55defe287bb5cbe75e88614c4

ndossche · 2024-04-15T15:40:17Z

I guess we shouldn't try to hack around it then?

SakiTakamachi · 2024-04-15T15:44:44Z

Yes, I agree. In this case, I think it will either convert all strings to binary or raise an error.

divinity76 · 2024-04-15T15:53:33Z

Yes, I agree. In this case, I think it will either convert all strings to binary or raise an error.

i hope you meant "convert all strings containing null bytes to binary"

it would be crazy changing the output of $db->quote("foobar") from 'foobar' to x'666f6f626172'

SakiTakamachi · 2024-04-15T15:56:22Z

yes of course. Only if the string is emebdded with nulls.
I'm not a native English speaker, so please excuse me :)

ref php#13956 (comment)

SakiTakamachi · 2024-04-15T16:08:36Z

However, the behavior of returning a binary when a string is passed to quote() seems to be a major deviation from the method's original role.

Quotes a string for use in a query

PDO::quote() places quotes around the input string (if required) and escapes special characters within the input string, using a quoting style appropriate to the underlying driver.

It's unlikely that anyone could look at this description and predict that it would return binary.

I still don't have an idea of what is the rational thing to do.

divinity76 · 2024-04-15T16:09:02Z

🤷‍♂️ well PR #13962 seems like the simplest (but not the fastest) solution,
removed the datatype-text workaround in aa1cdef

FixphpGH-13952: sqlite PDO::quote silently corrupts strings with nul…
049c918
…l bytes The built-in way to quote string using sqlite3's custom printf does not support NULL bytes in a string. Reimplement it ourselves. This also gets rid of the integer-based length limit.

ndossche requested a review from kamil-tekiela April 13, 2024 11:30

ndossche requested a review from SakiTakamachi as a code owner April 13, 2024 11:30

ndossche linked an issue Apr 13, 2024 that may be closed by this pull request
sqlite PDO::quote silently corrupts strings with null bytes #13952
Closed

github-actionsBot added the Extension: pdo_sqlite label Apr 13, 2024

devnexen reviewed Apr 13, 2024
View reviewed changes

SakiTakamachi approved these changes Apr 13, 2024
View reviewed changes

mvorisek reviewed Apr 13, 2024
View reviewed changes

adsr reviewed Apr 13, 2024
View reviewed changes

ndossche closed this Apr 13, 2024

divinity76 mentioned this pull request Apr 14, 2024
SQLite pdo::quote use x'hex' syntax for null bytes #13962
Closed

divinity76 mentioned this pull request Apr 15, 2024
SQLite pdo::quote use ('foo'||x'0000'||'bar') for null bytes #13972
Closed

divinity76 added a commit to divinity76/php-src that referenced this pull request Apr 15, 2024
remove datatype text workaround
aa1cdef
ref php#13956 (comment)

Uh oh!

Conversation

ndossche commented Apr 13, 2024

Uh oh!

devnexenApr 13, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ndosscheApr 13, 2024

Choose a reason for hiding this comment

Uh oh!

SakiTakamachi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SakiTakamachi commented Apr 13, 2024

Uh oh!

ndossche commented Apr 13, 2024

Uh oh!

SakiTakamachi commented Apr 13, 2024

Uh oh!

mvorisekApr 13, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ndosscheApr 13, 2024

Choose a reason for hiding this comment

Uh oh!

adsrApr 13, 2024

Choose a reason for hiding this comment

Uh oh!

ndosscheApr 13, 2024

Choose a reason for hiding this comment

Uh oh!

divinity76 commented Apr 13, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ndossche commented Apr 13, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

divinity76 commented Apr 13, 2024

Uh oh!

ndossche commented Apr 13, 2024

Uh oh!

divinity76 commented Apr 13, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

divinity76 commented Apr 13, 2024

Uh oh!

mvorisek commented Apr 13, 2024

Uh oh!

ndossche commented Apr 13, 2024

Uh oh!

divinity76 commented Apr 13, 2024

Uh oh!

mvorisek commented Apr 13, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SakiTakamachi commented Apr 13, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SakiTakamachi commented Apr 13, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devnexen commented Apr 14, 2024

Uh oh!

SakiTakamachi commented Apr 14, 2024

Uh oh!

devnexen commented Apr 14, 2024

Uh oh!

SakiTakamachi commented Apr 14, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SakiTakamachi commented Apr 14, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

divinity76 commented Apr 14, 2024

Uh oh!

divinity76 commented Apr 15, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

devnexenApr 13, 2024•
edited
Loading

SakiTakamachi left a comment •
edited
Loading

mvorisekApr 13, 2024•
edited
Loading

divinity76 commented Apr 13, 2024•
edited
Loading

ndossche commented Apr 13, 2024•
edited
Loading

divinity76 commented Apr 13, 2024•
edited
Loading

mvorisek commented Apr 13, 2024•
edited
Loading

SakiTakamachi commented Apr 13, 2024•
edited
Loading

SakiTakamachi commented Apr 13, 2024•
edited
Loading

SakiTakamachi commented Apr 14, 2024•
edited
Loading

SakiTakamachi commented Apr 14, 2024•
edited
Loading

divinity76 commented Apr 15, 2024•
edited
Loading

SakiTakamachi commented Apr 15, 2024•
edited
Loading