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

Reproduce MySQL warnings with and without emulated prepares #6522

Closed

Conversation

lcobucci
Copy link
Member

@lcobucci lcobucci commented Sep 9, 2024

This highlights MySQL warnings when dealing with binary data

@lcobucci lcobucci force-pushed the reproduce-warnings-with-binary-data branch from 6006dc1 to 42ee7bc Compare September 9, 2024 21:42
use function hex2bin;

/** @psalm-import-type WrapperParameterTypeArray from Connection */
final class MySQLWarningsWithBinaryTest extends FunctionalTestCase
Copy link
Member Author

Choose a reason for hiding this comment

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

After running things locally, this will need to be handled upstream (PDO MySQL), given it only happens when the prepared statement emulation is enabled.

@morozov
Copy link
Member

morozov commented Sep 9, 2024

So I see that there is a 2⨉2⨉2 matrix of parameters:

  1. Whether or not the emulation is used.
  2. Whether or not the _binary introducer is used.
  3. Parameter binding type: binary or string.

Observations:

  1. The test with any combination of the first two parameters passes or fails regardless of the binding type, so it can be ignored.
  2. With emulation but without _binary, the test will fail with:

    Invalid utf8mb4 character string: '91D886'

  3. Without emulation but with _binary, the test will fail with a syntax error.

This syntax error could be reduced to:

mysql> PREPARE s FROM 'SELECT _binary ?';
ERROR 1064 (42000): You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '?' at line 1

I didn't dig much into the documentation but I wouldn't expect this to be in invalid syntax.

Note that this works:

mysql> SELECT _binary '';
+----+
|    |
+----+
| 0x |
+----+

Which means that likely an introducer cannot be used in prepared statements. That's why it works only with emulation.

@lcobucci
Copy link
Member Author

Thank you for digging into this.

The introducer is just the workaround to avoid the warning when not using real prepared statements.

Given the removal of the emulation layer leads us to the usage of the binary protocol, things just work as expected.

This reinforces the feeling of something that needs to be addressed in the PDO driver. That way, the introducer can be automatically added when the right parameter hint is specified.

@morozov
Copy link
Member

morozov commented Sep 10, 2024

Got it. So in this case, we want to focus just on one of the 4 items from the data provider:

yield 'string and ParameterType::BINARY' => [
'INSERT INTO `binary_warnings_test` (`id`) VALUES (?)',
[ParameterType::BINARY],
];

We can configure MySQL to log all queries and query them:

SET GLOBAL general_log = 'ON';
SET GLOBAL log_output = 'table';

SELECT * FROM mysql.general_log;

The queries with and without emulation look like this:

# note that the value from the test (`0x0191D886E6DC73E7AF1FEE7F99EC6235`) is an exact substring of this query
➜ echo -n 494E5345525420494E544F206062696E6172795F7761726E696E67735F7465737460202860696460292056414C5545532028270191D886E6DC73E7AF1FEE7F99EC62352729 | xxd -r -p
INSERT INTO `binary_warnings_test` (`id`) VALUES ('�؆��s�����b5')

➜ echo -n 494E5345525420494E544F206062696E6172795F7761726E696E67735F7465737460202860696460292056414C5545532028270191D8865CE65CDC735CE7AF1F5CEE7F995CEC62352729 | xxd -r -p
INSERT INTO `binary_warnings_test` (`id`) VALUES ('�؆\�\�s\�\���\�b5')

I don't know how relevant this difference in string representation of binary data is, but it is in a few 0x5C characters (\) in the non-emulated query (which doesn't cause the warning) which are missing from the first one.

This is definitely a PDO/emulation problem. This test can be easily reworked to not use the DBAL at all and yield the same results.

@derrabus
Copy link
Member

This is definitely a PDO/emulation problem.

Shall we report this to PHP before we consider implementing workarounds on our side?

@morozov
Copy link
Member

morozov commented Sep 11, 2024

Shall we report this to PHP before we consider implementing workarounds on our side?

This is definitely to be reported to PHP. As for implementing a workaround, I'm not that eager (I don't mind if anybody wants to). However, historically, we deliberately didn't work around 3rd party issues in the DBAL.

I didn't check the correctness aspect of this issue, but if it's only a warning, then the DBAL/PDO users can ignore it until it's fixed.

@lcobucci
Copy link
Member Author

I fully agree that we shouldn't have a workaround for this in DBAL.

I'll do some investigation around PDO's emulation layer (it does trigger some mysql_real_escape_string() like behaviour).

I'll close this PR too.
Thank you again!

@lcobucci lcobucci closed this Sep 11, 2024
@lcobucci lcobucci deleted the reproduce-warnings-with-binary-data branch September 18, 2024 18:23
lcobucci added a commit to lcobucci/php-src that referenced this pull request Sep 18, 2024
The prepared statement emulation layer is handling binary content in a
way that creates warnings in MySQL.

When analysing the query logs, we saw that the content sent to the
server is missing `0x5C` characters when the using emulated prepares.

This introduces a minimal test case that reproduces the issue to aid the
solution.

More info: doctrine/dbal#6522 (comment)

Signed-off-by: Luís Cobucci <[email protected]>
lcobucci added a commit to lcobucci/php-src that referenced this pull request Sep 18, 2024
The prepared statement emulation layer is handling binary content in a
way that creates warnings in MySQL.

When analysing the query logs, we saw that the content sent to the
server is missing `0x5C` characters when the using emulated prepares.

This introduces a minimal test case that reproduces the issue to aid the
solution.

More info: doctrine/dbal#6522 (comment)

Signed-off-by: Luís Cobucci <[email protected]>
lcobucci added a commit to lcobucci/php-src that referenced this pull request Sep 18, 2024
The prepared statement emulation layer is handling binary content in a
way that creates warnings in MySQL.

When analysing the query logs, we saw that the content sent to the
server is missing `0x5C` characters when the using emulated prepares.

This introduces a minimal test case that reproduces the issue to aid the
solution.

More info: doctrine/dbal#6522 (comment)

Signed-off-by: Luís Cobucci <[email protected]>
mbeccati pushed a commit to lcobucci/php-src that referenced this pull request Sep 21, 2024
The prepared statement emulation layer is handling binary content in a
way that creates warnings in MySQL.

When analysing the query logs, we saw that the content sent to the
server is missing `0x5C` characters when the using emulated prepares.

This introduces a minimal test case that reproduces the issue to aid the
solution.

More info: doctrine/dbal#6522 (comment)

Signed-off-by: Luís Cobucci <[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