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

Fix #67495: pdo_dblib incorrectly escapes binary and unicode values #2017

Closed
wants to merge 3 commits into
base: PHP-7.0
from

Conversation

5 participants
@turboezh
Contributor

turboezh commented Jul 21, 2016

Fix quoting non-ascii and binary strings.
Also add more precise handling of BOOL and INT.

https://bugs.php.net/bug.php?id=67495

May ralates to:
https://bugs.php.net/bug.php?id=72414
https://bugs.php.net/bug.php?id=60818

Fix #67495: pdo_dblib incorrectly escapes binary and unicode values
Fix quoting non-ascii and binary strings. Add more precise handling of BOOL and INT.
@weltling

This comment has been minimized.

Show comment
Hide comment
@weltling

weltling Jul 25, 2016

Contributor

Does this one obsolete #2001 (if yes, the older one should be closed). @adambaratz, could you please check together with Alex?

Thanks.

Contributor

weltling commented Jul 25, 2016

Does this one obsolete #2001 (if yes, the older one should be closed). @adambaratz, could you please check together with Alex?

Thanks.

@turboezh

This comment has been minimized.

Show comment
Hide comment
@turboezh

turboezh Jul 25, 2016

Contributor

These are independent PRs. #2001 adds ability to stringify field type and doesn't relate to quoting.

Contributor

turboezh commented Jul 25, 2016

These are independent PRs. #2001 adds ability to stringify field type and doesn't relate to quoting.

};
if (is_ascii && (unquoted[i] < 32 || unquoted[i] > 126)) {
is_ascii = 0;

This comment has been minimized.

@adambaratz

adambaratz Jul 26, 2016

Contributor

Couple practical issues with doing this automatically:

  • While pdo_dblib is commonly used with MSSQL, that's not guaranteed. Some DBs might not like this syntax.
  • It could force undesirable casts. You care about the destination column type, not the data you're passing.

There are probably other scenarios where being clever isn't advantageous. I'd thought about creating syntax like this:
$db->quote('Съешь же ещё этих мягких французских булок, да выпей чаю.', PDO::PARAM_STR | PDO::PARAM_STR_UNICODE);

That way, it's totally explicit. Which is generally a desirable trait when talking to databases. You could also have some driver attributes that let you default into unicode mode if that's desired. You'd then want a PDO::PARAM_STR_ASCII type for overriding it.

@adambaratz

adambaratz Jul 26, 2016

Contributor

Couple practical issues with doing this automatically:

  • While pdo_dblib is commonly used with MSSQL, that's not guaranteed. Some DBs might not like this syntax.
  • It could force undesirable casts. You care about the destination column type, not the data you're passing.

There are probably other scenarios where being clever isn't advantageous. I'd thought about creating syntax like this:
$db->quote('Съешь же ещё этих мягких французских булок, да выпей чаю.', PDO::PARAM_STR | PDO::PARAM_STR_UNICODE);

That way, it's totally explicit. Which is generally a desirable trait when talking to databases. You could also have some driver attributes that let you default into unicode mode if that's desired. You'd then want a PDO::PARAM_STR_ASCII type for overriding it.

char is_int = 1;
switch (PDO_PARAM_TYPE(param_type)) {
case PDO_PARAM_LOB:

This comment has been minimized.

@adambaratz

adambaratz Jul 26, 2016

Contributor

My understanding is that this type is meant for streams:
http://php.net/manual/en/pdo.lobs.php

I agree that PDO's types are a bit limiting, but it'll potentially confuse things to overload this type for this use case. Do you know how other PDO drivers handle this content? I haven't had experience with something like this. Another approach I've seen, with a different API, involved a data type specific to non-stream binary data.

@adambaratz

adambaratz Jul 26, 2016

Contributor

My understanding is that this type is meant for streams:
http://php.net/manual/en/pdo.lobs.php

I agree that PDO's types are a bit limiting, but it'll potentially confuse things to overload this type for this use case. Do you know how other PDO drivers handle this content? I haven't had experience with something like this. Another approach I've seen, with a different API, involved a data type specific to non-stream binary data.

case PDO_PARAM_BOOL:
*quoted_len = 3;
if (!unquoted_len // empty strings treated as FALSE anyway
|| (unquoted_len == 1 && (*unquoted == '0' || *unquoted == ' ')) // one-char strings depends on a value

This comment has been minimized.

@adambaratz

adambaratz Jul 26, 2016

Contributor

Why does a space count as false but two spaces don't? It would be cleaner To say that empty strings and '0' are false, everything else true. But I wonder if there's a case here I'm not thinking of.

@adambaratz

adambaratz Jul 26, 2016

Contributor

Why does a space count as false but two spaces don't? It would be cleaner To say that empty strings and '0' are false, everything else true. But I wonder if there's a case here I'm not thinking of.

@adambaratz

This comment has been minimized.

Show comment
Hide comment
@adambaratz

adambaratz Jul 26, 2016

Contributor

I left some comments on specific points. I want to solve these problems with pdo_dblib, but I think there are some cases here you're not accounting for. I think this is one of those situations where the param types offered by PDO are limiting.

Contributor

adambaratz commented Jul 26, 2016

I left some comments on specific points. I want to solve these problems with pdo_dblib, but I think there are some cases here you're not accounting for. I think this is one of those situations where the param types offered by PDO are limiting.

@smalyshev smalyshev added the Bugfix label Sep 5, 2016

@krakjoe

This comment has been minimized.

Show comment
Hide comment
@krakjoe

krakjoe Jan 3, 2017

Member

@turboezh fix conflicts please :)

In addition, can you respond to the comments left by other contributors, as I am not sure of the current status of this PR.

If you consider this work abandoned, please close this PR.

Member

krakjoe commented Jan 3, 2017

@turboezh fix conflicts please :)

In addition, can you respond to the comments left by other contributors, as I am not sure of the current status of this PR.

If you consider this work abandoned, please close this PR.

@turboezh

This comment has been minimized.

Show comment
Hide comment
@turboezh

turboezh Jan 26, 2017

Contributor

Hi.
It is not abandoned. But I'm not sure yet how to solve all this the best way. And I think now that BOOL and LOB cases should be done and discussed in separate PRs, i.e. this one should be reworked.
I doesn't mind if anyone will present another solution.

Contributor

turboezh commented Jan 26, 2017

Hi.
It is not abandoned. But I'm not sure yet how to solve all this the best way. And I think now that BOOL and LOB cases should be done and discussed in separate PRs, i.e. this one should be reworked.
I doesn't mind if anyone will present another solution.

@krakjoe

This comment has been minimized.

Show comment
Hide comment
@krakjoe

krakjoe Apr 3, 2017

Member

Since there is no clear consensus on this fix, and since it still has unresolved conflicts, and having waited a month for those conflicts to be fixed, I'm closing this PR.

Member

krakjoe commented Apr 3, 2017

Since there is no clear consensus on this fix, and since it still has unresolved conflicts, and having waited a month for those conflicts to be fixed, I'm closing this PR.

@krakjoe krakjoe closed this Apr 3, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment