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

PDO_DBLIB: Quote LOB arguments as binary literals #10343

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

Conversation

NattyNarwhal
Copy link
Member

If the user is specifying a PDO::PARAM_LOB, there's a good chance that it's a binary file (i.e. an image), that is too fragile to survive in a string, as escaping it is insufficient. Because PDO_DBLIB relies on PDO to fill in a parameterized query and submits the filled string to the server, it ends up mangling the query.

This adds logic in the PDO_DBLIB quoter to handle binary parameters by using the SQL Server binary literal syntax (a hex string that begins with 0x). This resolves the issue because PDO consults the quoter for filling in the query string.


I've also added a unit test for this.

Questions not raised in the commit:

  • Does this impact anyone expecting it to be a string literal instead of a binary literal with a LOB parameter? Is SQL Server picky about the context in which they're used?
  • Is using PDO::PARAM_LOB the best option for this? I picked this because the end-user I was working with (as did I) expected binary behaviour out of it, but that might be too much of an assumption, especially since there's not really LOBs involved, just binaries. Would a PARAM_BINARY make sense? If so, as a modifier or its own type? Added to base PDO or just DBLIB?

Resolves GH-10312.

If the user is specifying a PDO::PARAM_LOB, there's a good chance
that it's a binary file (i.e. an image), that is too fragile to
survive in a string, as escaping it is insufficient. Because
PDO_DBLIB relies on PDO to fill in a parameterized query and
submits the filled string to the server, it ends up mangling the
query.

This adds logic in the PDO_DBLIB quoter to handle binary parameters
by using the SQL Server binary literal syntax (a hex string that
begins with `0x`). This resolves the issue because PDO consults the
quoter for filling in the query string.
This makes sure that a PDO::PARAM_LOB parameter gets converted to
a binary literal when doing the substitution for the query.
@NattyNarwhal
Copy link
Member Author

After discussing this with Derrick, we brought up the possibility of putting this behind a constant (PDO_DBLIB specific, but could be useful for other drivers?) or a feature flag (in the connection string, INI options should be avoided).

Also worth mentioning that the Postgres PDO driver does special-case binary strings in the quoter under PARAM_LOB: https://github.com/php/php-src/blob/PHP-8.2.2/ext/pdo_pgsql/pgsql_driver.c#L358-L368 - so perhaps this does make sense semantically, and is worth a possibly breaking change? Only PDO_OCI has (and a smaller degree PDO_ODBC) has any special LOB-based handling, it seems to be basically treated more as varbinary otherwise.

@NattyNarwhal
Copy link
Member Author

Someone emailed me about wanting a PARAM_BINARY in PDO_ODBC, to work around driver issues with strings as inputs/outputs for binary columns. I'm tempted to generalize this for other drivers.

NattyNarwhal added a commit to NattyNarwhal/php-src that referenced this pull request Jul 11, 2023
Original text:
>If the user is specifying a PDO::PARAM_LOB, there's a good chance
>that it's a binary file (i.e. an image), that is too fragile to
>survive in a string, as escaping it is insufficient. Because
>PDO_DBLIB relies on PDO to fill in a parameterized query and
>submits the filled string to the server, it ends up mangling the
>query.
>
>This adds logic in the PDO_DBLIB quoter to handle binary parameters
>by using the SQL Server binary literal syntax (a hex string that
>begins with `0x`). This resolves the issue because PDO consults the
>quoter for filling in the query string.
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.

PDO_DBLIB encodes passed binary data as a string, which is fragile
1 participant