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

IDO PgSQL: always use regular string literals #8955

Merged
merged 1 commit into from
Aug 5, 2021
Merged

Conversation

julianbrost
Copy link
Contributor

IdoPgsqlConnection::Escape() internally uses PQescapeStringConn() and its documentation states the following:

Furthermore, PQescapeStringConn does not generate the single quotes that must surround PostgreSQL string literals; they should be provided in the SQL command that the result is inserted into.

So it's intended to use the result in 'string' literals, not in E'string' literals as Icinga did. This results in problems as the behavior of PQescapeStringConn() depends on how the current connection will interpret regular single quoted literals, namely on the value of the standard_conforming_strings variable.

The E'string' literals were initially introduced in ac6f3f8 to fix #1206 where PostgreSQL started warning about escape sequences in string literals not supported by the SQL standard (but by PostgreSQL depending on the value of standard_conforming_strings). In the meantime the oldest PostgreSQL version on any platform supported by Icinga increased to 9.2 (CentOS 7) and starting with 9.1, standard_conforming_strings is enabled by default, so there will be no warnings about escape sequences (as the warning is only issued if the escape sequence is actually interpreted by PostgreSQL).

refs #1206 #2811 #8123
fixes #8951

IdoPgsqlConnection::Escape() internally uses PQescapeStringConn() and its
documentation states the following:

  Furthermore, PQescapeStringConn does not generate the single quotes that must
  surround PostgreSQL string literals; they should be provided in the SQL
  command that the result is inserted into.

So it's intended to use the result in 'string' literals, not in E'string'
literals as Icinga did. This results in problems as the behavior of
PQescapeStringConn() depends on how the current connection will interpret
regular single quoted literals, namely on the value of the
standard_conforming_strings variable.

The E'string' literals were initially introduced in
ac6f3f8 to fix #1206 where PostgreSQL started
warning about escape sequences in string literals not supported by the SQL
standard (but by PostgreSQL depending on the value of
standard_conforming_strings). In the meantime the oldest PostgreSQL version on
any platform supported by Icinga increased to 9.2 (CentOS 7) and starting with
9.1, standard_conforming_strings is enabled by default, so there will be no
warnings about escape sequences (as the warning is only issued if the escape
sequence is actually interpreted by PostgreSQL).
@julianbrost julianbrost added bug Something isn't working area/db-ido Database output labels Aug 5, 2021
@cla-bot cla-bot bot added the cla/signed label Aug 5, 2021
@icinga-probot icinga-probot bot added this to the 2.14.0 milestone Aug 5, 2021
@icinga-probot icinga-probot bot removed the cla/signed label Aug 5, 2021
@Al2Klimov
Copy link
Member

@cla-bot check

@cla-bot cla-bot bot added the cla/signed label Aug 5, 2021
Copy link
Member

@Al2Klimov Al2Klimov left a comment

Choose a reason for hiding this comment

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

So it's intended to use the result in 'string' literals, not in E'string' literals as Icinga did.

Please verify this via an edge case (E'' vs. '').

@julianbrost
Copy link
Contributor Author

So it's intended to use the result in 'string' literals, not in E'string' literals as Icinga did.

Please verify this via an edge case (E'' vs. '').

Well, that's the very reason for #8951. Take the string C:\Users as an example. If standard_conforming_strings is set to off (as we did explicitly before #8123), PQescapeStringConn() escapes the \ whereas it won't when set to on (the default in PostgreSQL 9.1+) as the \ is not interpreted in single quotes on the connection.

Copy link
Member

@Al2Klimov Al2Klimov left a comment

Choose a reason for hiding this comment

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

LGTM

@Al2Klimov Al2Klimov merged commit a475755 into master Aug 5, 2021
@icinga-probot icinga-probot bot deleted the bugfix/pgsql-escape branch August 5, 2021 16:04
@julianbrost julianbrost restored the bugfix/pgsql-escape branch August 5, 2021 16:11
@icinga-probot icinga-probot bot deleted the bugfix/pgsql-escape branch August 12, 2021 12:25
@julianbrost julianbrost added the backported Fix was included in a bugfix release label Mar 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/db-ido Database output backported Fix was included in a bugfix release bug Something isn't working cla/signed
Projects
None yet
2 participants