Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SQLite pdo::quote use x'hex' syntax for null bytes #13962

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

divinity76
Copy link
Contributor

Implements x'hex' encoding in pdo::quote for handling strings with null bytes,
providing a reliable workaround for issue GH-13952.
An alternative fix is discussed in PR #13956

PR #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.

-> ''
x -> 'x'
\u0000 -> x'00'
a\u0000b -> x'610062'
Copy link
Contributor

@mvorisek mvorisek Apr 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
a\u0000b -> x'610062'
a\u0000b -> (''||x'610062')

quote function should not change type - https://dbfiddle.uk/ZodKR71s

Copy link
Contributor Author

@divinity76 divinity76 Apr 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually think datatype blob is perfectly reasonable for binary strings, but okay, 3e573cf

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One reason for "always text type" is as non-UTF-8 string like 0xff is currently "text type" and not "blob type".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm see #13956 (comment)
there are both pros and cons with retaining the "text type" 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"nul byte" is supported in "text type" as long as you use it from php :)

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 <[email protected]>
@SakiTakamachi
Copy link
Member

Thanks for working on this.

However, I'm a bit skeptical of such workarounds. I feel like it's a bit excessive to use PHP to do something that the original SQLite doesn't support.

For example, let's say that in the future SQLite supports this and we have to update these functions to match the changes in the SQLite specification. At that point, a BC Break may be unavoidable.
(Please let me know if I've misunderstood something.)

However, there is still some time until the release of 8.4.

I might leave this PR open for a little while, but I'll try to bring it upstream tonight, so give me some time.

@divinity76
Copy link
Contributor Author

I'll try to bring it upstream tonight

well the way i see it, there's 4 proposed fixes, all have their own pros and cons, and we haven't really reached a consensus,

the proposals are:
#1

Fatal error: Uncaught ValueError: PDO::quote(): Argument #1 ($string) must not contain any null bytes

or
#2

x'666f6f00626172'

or
#3

(''||x'666f6f00626172')

or
#4

('foo'||x'00'||'bar')

ping @nielsdos @mvorisek

divinity76 added a commit to divinity76/php-src that referenced this pull request Apr 15, 2024
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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants