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

Error while binding short string with slash suffix #177

Closed
forrest79 opened this issue Jul 26, 2017 · 14 comments
Closed

Error while binding short string with slash suffix #177

forrest79 opened this issue Jul 26, 2017 · 14 comments

Comments

@forrest79
Copy link

@forrest79 forrest79 commented Jul 26, 2017

  • bug report? yes
  • feature request? nono
  • version: 2.4.3

Description

Get error syntax exception, when mixing short (with suffix ) and long string parameters binding to INSERT or UPDATE query (maybe there are probably more scenarios to get this error).

Steps To Reproduce

I have tested this with PostqreSQL. Let's create this simple table:

CREATE TABLE test
(
  col1 text,
  col2 text,
  col3 text
);

Then run this command:

/** @var \Nette\Database\Context $context */
$context->table('test')->insert([
	'col1' => 'some text \\', // shorter than 20
	'col2' => 'some text longer than 20 characters',
	'col3' => 'some text', // shorter than 20
]);

This exception is thrown:

Nette\Database\DriverException #42601

SQLSTATE[42601]: Syntax error: 7 ERROR:  syntax error at or near ","
LINE 1: ... ("col1", "col2", "col3") VALUES ('some text \', ?, 'some te...

This behavior is probably fixed in 3.0.0-alpha with commit 644b587. To fix this in 2.4 branch it is possible to change SqlPreprocessor->formatValue to bind all strings as variable, not only strings longer than 20 characters. Bud I don't why this logic is there, maybe this update could be BC?

@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Jul 26, 2017

According to the PostqreSQL documentation, everything is ok: https://www.postgresql.org/docs/8.2/static/sql-syntax-lexical.html#SQL-SYNTAX-STRINGS

A string constant in SQL is an arbitrary sequence of characters bounded by single quotes ('), for example 'This is a string'. To include a single-quote character within a string constant, write two adjacent single quotes, e.g. 'Dianne''s horse'. Note that this is not the same as a double-quote character (").

There are a mention switches standard_conforming_strings and backslash_quote. Can you find out what value they has? If it is true, it looks like a serious security issue in PDO.

@milo

This comment has been minimized.

Copy link
Member

@milo milo commented Jul 27, 2017

This is some bug in PDO parameters binding. The $pdo->quote() respects standard_conforming_strings setting.

# SET standard_conforming_strings TO on
$pdo->quote('BACK \\ SLASH')  =  BACK \ SLASH

# SET standard_conforming_strings TO off
$pdo->quote('BACK \\ SLASH')  =  BACK \\ SLASH

Query INSERT INTO test ("col1", "col2", "col3") VALUES ('some text \', 'abc', 'def') is valid PostgreSQL query with standard_conforming_strings set to on (which means, "Handle backslash as ordinary character").

IMHO the bug is in internal PDO values binding code when they are looking for ? in prepared query. The ? is not found, parameter is not binded correctly and PDO executes SQL which contains ?. The whole exception message is:

PHP Fatal error:  Uncaught PDOException: SQLSTATE[42601]: Syntax error: 7 ERROR:  syntax error at or near ","
LINE 1: INSERT INTO test (col1, col2, col3) VALUES ('\', ?, 'x')
                                                          ^ in D:\Web\temp\pdo\test2.php:14

Focus on the ^ and where it points.

Code to reproduce:

$pdo = new PDO('pgsql:host=localhost;dbname=test', 'test', 'test');
$pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
$pdo->query('SET standard_conforming_strings TO on');

$stmt = $pdo->prepare("INSERT INTO test (col1, col2, col3) VALUES ('\\', ?, '')");
$stmt->bindValue(1, 'x', PDO::PARAM_STR);
$stmt->execute();

If you don't, I'll report PHP bug in the evening.

@milo

This comment has been minimized.

Copy link
Member

@milo milo commented Jul 27, 2017

Workaround is SET standard_conforming_strings TO off if an application does not depends on it.
In that case, PDO internals code works.

@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Jul 27, 2017

That would be great if you could report it, I do not know the PostgreSQL at all. (I only develop its drivers :-) )

@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Jul 27, 2017

Can ? be replaced directly by the database instead of the PDO?

dg added a commit that referenced this issue Jul 27, 2017
@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Jul 27, 2017

We have solved it by workaround because sending all strings as parameters causes incompatibility in PostgreSQL:

SELECT 'string' vs SELECT ? + parameter 'string' leads to SQLSTATE[42P18]: Indeterminate datatype: 7 ERROR: could not determine data type of parameter $1.

On the other hand, this behavior relies on the 20-character limit, so maybe a BC break could be done in 2.4.

@milo

This comment has been minimized.

Copy link
Member

@milo milo commented Jul 27, 2017

Can ? be replaced directly by the database instead of the PDO?

There is a PREPARE statement, but I don't know syntax using ?.

sending all strings as parameters causes incompatibility

It is probably related to PHP #36652, workaround is mentioned in comment.

@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Jul 27, 2017

IMHO just PDO::ATTR_EMULATE_PREPARES causes this issue…

@milo

This comment has been minimized.

Copy link
Member

@milo milo commented Jul 27, 2017

I don't think so. With ATTR_EMULATE_PREPARES = false it uses PostgreSQL PREPARE statement. It translates ? into $1.

$pdo->setAttribute(PDO::ATTR_EMULATE_PREPARES, true);
# PG log
# LOG:  statement: INSERT INTO test (col1, col2, col3) VALUES ('a', 'x', ' ')

$pdo->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);
# PG log
# LOG:  execute pdo_stmt_00000001: INSERT INTO test (col1, col2, col3) VALUES ('a', $1, ' ')
# DETAIL:  parameters: $1 = 'x'

But with buggy query from this issue, it fails in the same way in both cases:

ERROR:  syntax error at or near "," at character 51
STATEMENT:  INSERT INTO test (col1, col2, col3) VALUES ('\', ?, ' ')
@milo

This comment has been minimized.

Copy link
Member

@milo milo commented Jul 27, 2017

It seems to me that issue appears only when some string ends with \.

$stmt = $pdo->prepare("INSERT INTO test (col1, col2, col3) VALUES ('\\', ?, ' ')");  # BUGGY
$stmt = $pdo->prepare("INSERT INTO test (col1, col2, col3) VALUES ('\\ ', ?, ' ')"); # OK
$stmt = $pdo->prepare("INSERT INTO test (col1, col2) VALUES ('\\', ?)"); # OK
@dg

This comment has been minimized.

Copy link
Member

@dg dg commented Jul 27, 2017

What about $stmt = $pdo->prepare("INSERT INTO test (col1, col2, col3) VALUES ('\\'' ', ?, ' ')");, ie doesn't end with, but contains \ before '

@milo

This comment has been minimized.

Copy link
Member

@milo milo commented Jul 27, 2017

It's buggy too.

@milo

This comment has been minimized.

Copy link
Member

@milo milo commented Jul 27, 2017

Reported PHP #74996.

dg added a commit that referenced this issue Jul 27, 2017
…n prepared query [Closes #177]
dg added a commit that referenced this issue Jul 27, 2017
…n prepared query [Closes #177]
dg added a commit that referenced this issue Jul 27, 2017
…n prepared query [Closes #177]
dg added a commit that referenced this issue Jul 27, 2017
…n prepared query [Closes #177]
dg added a commit that referenced this issue Jul 27, 2017
…n prepared query [Closes #177]
dg added a commit that referenced this issue Jul 27, 2017
…n prepared query [Closes #177]
@forrest79

This comment has been minimized.

Copy link
Author

@forrest79 forrest79 commented Aug 6, 2017

Hi, big thanks for really quick fix!

@dg dg closed this Aug 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.