Re: JDBC Support for standard_conforming_strings

I'm still waiting for feedback. Kris? Dave?

Best Regards
Michael Paesold

---------------------------------------------------------------------

Michael Paesold wrote:
> Kris Jurka wrote:
>> Right. The parameters will either be sent out of line which won't
>> require escaping or for the V2 path they will be sent in the query,
>> but it isn't the array or bytea codes' responsibility to escape them
>> correctly. The code in core(dot)v2(dot)SimpleParameterList(at)setStringParameter
>> should handle that for us.
>
> Okay, thanks! Just wanted to have confirmation on that.
>
> I am sending a work-in-progress patch now, as I have some more questions
> on how to proceed.
>
> Progress so far:
> I have added a new method "getStandardConformingStrings" to
> ProtocolConnection, with tracking for standard_conforming_strings in the
> protocol connection implementation and the connection factory
> implementation classes (v2 + v3).
> BaseConnection has two new methods escapeString and getStringLiteral
> (escapeString in style of PQescapeStringConn, getStringLiteral for
> performance reasons to escape the string and add enclosing single-quotes
> in one pass). BaseConnection.escapeString is for example used in
> AbstractJdbc2DatabaseMetaData. The getStringLiteral method should be
> used in core(dot)v2(dot)SimpleParameterList(at)setStringParameter, but see below
> for that.
>
> I have also added support for standardConformingStrings to
> Parser.parseSingleQuotes.
>
> So far, the v3 Protocol path should work correctly with
> standard_conforming_strings = true (except for the query parsing code in
> AbstractJdbc2Statement).
>
> Open items:
> - Teach rest of parsing code about standard_conforming_strings
> - Support E'' escaped string syntax
> - Find a solution for the "V2 problem"
>
> Question 1:
> -----------
> The V2 code as well as the query-parsing code in
> AbstractJdbc2DatabaseMetaData is either mostly static or has otherwise
> no reference to the connection object. There are two ways to tackle
> this: either pass around standardConformingStrings (the variable name
> could be stdStrings for brevity), or give the code access to the current
> connection.
> For the parsing code, it's probably easier to just pass the variable to
> the two or three methods. Objections?
>
> For the V2 code, this leads me to question 2.
>
> Question 2:
> -----------
> The problem with the V2 protocol code is, that tracking the state of
> standard_conforming_strings is not really possible, AFAICS. Currently I
> just read standard_conforming_strings at startup, and that's it. So as
> soon as someone changes the variable, quoting will be incorrect, which
> is a security issue. Therefore my new suggestion is to used the E''
> string syntax in the V2 protocol path if the server version is >= 8.2.
> The version of the server cannot change during the connection, so we are
> save here.
>
> For question 1, I would then suggest to add an argument to the V2Query
> constructor that tells whether E'' should be used instead of '', or not.
>
> For parsing inside the JDBC driver, we would still rely on the initially
> reported value of standard_conforming_strings. We could also add an URL
> parameter to specify what should be set initially.
>
> Comments, ideas?
> Best Regards
> Michael Paesold
>
>
> ------------------------------------------------------------------------
>
> diff -crN pgjdbc.1cc1d2edf1a6/org/postgresql/core/BaseConnection.java pgjdbc.af39e026875b/org/postgresql/core/BaseConnection.java
> *** pgjdbc.1cc1d2edf1a6/org/postgresql/core/BaseConnection.java 2006-11-06 10:24:03.000000000 +0100
> --- pgjdbc.af39e026875b/org/postgresql/core/BaseConnection.java 2006-11-06 10:24:03.000000000 +0100
> ***************
> *** 143,148 ****
> --- 143,169 ----
> */
> public byte[] encodeString(String str) throws SQLException;
>
> + /**
> + * Escapes a string for use as string-literal within an SQL command. The
> + * method chooses the applicable escaping rules based on the value of
> + * {(at)link ProtocolConnection#getStandardConformingStrings()}.
> + *
> + * @param str a string value
> + * @return the escaped representation of the string
> + * @throws SQLException if the string contains a <tt>\0</tt> character
> + */
> + public String escapeString(String str) throws SQLException;
> +
> + /**
> + * Escapes a string for use as string-literal within an SQL command and
> + * returns the string-literal including beginning and ending single-quotes.
> + *
> + * @param str a string value
> + * @return the SQL string-literal for the given argument string
> + * @throws SQLException if the string contains a <tt>\0</tt> character
> + */
> + public String getStringLiteral(String str) throws SQLException;
> +
> // Ew. Quick hack to give access to the connection-specific utils implementation.
> public TimestampUtils getTimestampUtils();
>
> diff -crN pgjdbc.1cc1d2edf1a6/org/postgresql/core/Parser.java pgjdbc.af39e026875b/org/postgresql/core/Parser.java
> *** pgjdbc.1cc1d2edf1a6/org/postgresql/core/Parser.java 2006-11-06 10:24:03.000000000 +0100
> --- pgjdbc.af39e026875b/org/postgresql/core/Parser.java 2006-11-06 10:24:03.000000000 +0100
> ***************
> *** 24,43 ****
> * one. The caller must call the method a second time for the second
> * part of the quoted string.
> */
> ! public static int parseSingleQuotes(final char[] query, int offset) {
> ! while (++offset < query.length)
> {
> ! switch (query[offset])
> {
> ! case '\\':
> ! ++offset;
> ! break;
> ! case '\'':
> ! return offset;
> ! default:
> ! break;
> }
> }
> return query.length;
> }
>
> --- 24,63 ----
> * one. The caller must call the method a second time for the second
> * part of the quoted string.
> */
> ! public static int parseSingleQuotes(final char[] query, int offset,
> ! boolean standardConformingStrings) {
> ! if (standardConformingStrings)
> {
> ! // don NOT treat backslashes as escape characters
> ! while (++offset < query.length)
> ! {
> ! switch (query[offset])
> ! {
> ! case '\'':
> ! return offset;
> ! default:
> ! break;
> ! }
> ! }
> ! }
> ! else
> ! {
> ! // treat backslashes as escape characters
> ! while (++offset < query.length)
> {
> ! switch (query[offset])
> ! {
> ! case '\\':
> ! ++offset;
> ! break;
> ! case '\'':
> ! return offset;
> ! default:
> ! break;
> ! }
> }
> }
> +
> return query.length;
> }
>
> diff -crN pgjdbc.1cc1d2edf1a6/org/postgresql/core/ProtocolConnection.java pgjdbc.af39e026875b/org/postgresql/core/ProtocolConnection.java
> *** pgjdbc.1cc1d2edf1a6/org/postgresql/core/ProtocolConnection.java 2006-11-06 10:24:03.000000000 +0100
> --- pgjdbc.af39e026875b/org/postgresql/core/ProtocolConnection.java 2006-11-06 10:24:03.000000000 +0100
> ***************
> *** 67,72 ****
> --- 67,84 ----
> * @return the current encoding in use by this connection
> */
> Encoding getEncoding();
> +
> + /**
> + * Returns wether the server treats string-literals according to the SQL
> + * standard or if it uses traditional PostgreSQL escaping rules. Versions
> + * up to 8.1 always treated backslashes as escape characters in
> + * string-literals. Since 8.2, this depends on the value of the
> + * <tt>standard_conforming_strings<tt> server variable.
> + *
> + * @return true if the server treats string literals according to the SQL
> + * standard
> + */
> + boolean getStandardConformingStrings();
>
> /**
> * Get the current transaction state of this connection.
> diff -crN pgjdbc.1cc1d2edf1a6/org/postgresql/core/v2/ConnectionFactoryImpl.java pgjdbc.af39e026875b/org/postgresql/core/v2/ConnectionFactoryImpl.java
> *** pgjdbc.1cc1d2edf1a6/org/postgresql/core/v2/ConnectionFactoryImpl.java 2006-11-06 10:24:03.000000000 +0100
> --- pgjdbc.af39e026875b/org/postgresql/core/v2/ConnectionFactoryImpl.java 2006-11-06 10:24:03.000000000 +0100
> ***************
> *** 471,475 ****
> --- 471,487 ----
>
> if (logger.logDebug())
> logger.debug("Connection encoding (using JVM's nomenclature): " + protoConnection.getEncoding());
> +
> + if (dbVersion.compareTo("8.1") >= 0)
> + {
> + // Server versions since 8.1 report standard_conforming_strings
> + results = runSetupQuery(protoConnection, "select current_setting('standard_conforming_strings')", true);
> + String value = protoConnection.getEncoding().decode(results[0]);
> + protoConnection.setStandardConformingStrings(value.equalsIgnoreCase("on"));
> + }
> + else
> + {
> + protoConnection.setStandardConformingStrings(false);
> + }
> }
> }
> diff -crN pgjdbc.1cc1d2edf1a6/org/postgresql/core/v2/ProtocolConnectionImpl.java pgjdbc.af39e026875b/org/postgresql/core/v2/ProtocolConnectionImpl.java
> *** pgjdbc.1cc1d2edf1a6/org/postgresql/core/v2/ProtocolConnectionImpl.java 2006-11-06 10:24:03.000000000 +0100
> --- pgjdbc.af39e026875b/org/postgresql/core/v2/ProtocolConnectionImpl.java 2006-11-06 10:24:03.000000000 +0100
> ***************
> *** 51,56 ****
> --- 51,61 ----
> return serverVersion;
> }
>
> + public synchronized boolean getStandardConformingStrings()
> + {
> + return standardConformingStrings;
> + }
> +
> public synchronized int getTransactionState()
> {
> return transactionState;
> ***************
> *** 166,171 ****
> --- 171,180 ----
> this.cancelKey = cancelKey;
> }
>
> + synchronized void setStandardConformingStrings(boolean value) {
> + standardConformingStrings = value;
> + }
> +
> //
> // Package-private accessors called by the query executor
> //
> ***************
> *** 197,202 ****
> --- 206,212 ----
> private int cancelPid;
> private int cancelKey;
>
> + private boolean standardConformingStrings;
> private int transactionState;
> private SQLWarning warnings;
>
> diff -crN pgjdbc.1cc1d2edf1a6/org/postgresql/core/v2/QueryExecutorImpl.java pgjdbc.af39e026875b/org/postgresql/core/v2/QueryExecutorImpl.java
> *** pgjdbc.1cc1d2edf1a6/org/postgresql/core/v2/QueryExecutorImpl.java 2006-11-06 10:24:03.000000000 +0100
> --- pgjdbc.af39e026875b/org/postgresql/core/v2/QueryExecutorImpl.java 2006-11-06 10:24:03.000000000 +0100
> ***************
> *** 35,45 ****
> //
>
> public Query createSimpleQuery(String sql) {
> ! return new V2Query(sql, false);
> }
>
> public Query createParameterizedQuery(String sql) {
> ! return new V2Query(sql, true);
> }
>
> //
> --- 35,45 ----
> //
>
> public Query createSimpleQuery(String sql) {
> ! return new V2Query(sql, false, protoConnection.getStandardConformingStrings());
> }
>
> public Query createParameterizedQuery(String sql) {
> ! return new V2Query(sql, true, protoConnection.getStandardConformingStrings());
> }
>
> //
> diff -crN pgjdbc.1cc1d2edf1a6/org/postgresql/core/v2/SimpleParameterList.java pgjdbc.af39e026875b/org/postgresql/core/v2/SimpleParameterList.java
> *** pgjdbc.1cc1d2edf1a6/org/postgresql/core/v2/SimpleParameterList.java 2006-11-06 10:24:03.000000000 +0100
> --- pgjdbc.af39e026875b/org/postgresql/core/v2/SimpleParameterList.java 2006-11-06 10:24:03.000000000 +0100
> ***************
> *** 68,74 ****
> char ch = value.charAt(i);
> if (ch == '\0')
> throw new PSQLException(GT.tr("Zero bytes may not occur in string parameters."), PSQLState.INVALID_PARAMETER_VALUE);
> ! if (ch == '\\' || ch == '\'')
> sbuf.append('\\');
> sbuf.append(ch);
> }
> --- 68,74 ----
> char ch = value.charAt(i);
> if (ch == '\0')
> throw new PSQLException(GT.tr("Zero bytes may not occur in string parameters."), PSQLState.INVALID_PARAMETER_VALUE);
> ! if (ch == '\\' || ch == '\'') //HERE quoting
> sbuf.append('\\');
> sbuf.append(ch);
> }
> diff -crN pgjdbc.1cc1d2edf1a6/org/postgresql/core/v2/V2Query.java pgjdbc.af39e026875b/org/postgresql/core/v2/V2Query.java
> *** pgjdbc.1cc1d2edf1a6/org/postgresql/core/v2/V2Query.java 2006-11-06 10:24:03.000000000 +0100
> --- pgjdbc.af39e026875b/org/postgresql/core/v2/V2Query.java 2006-11-06 10:24:03.000000000 +0100
> ***************
> *** 17,23 ****
> * Query implementation for all queries via the V2 protocol.
> */
> class V2Query implements Query {
> ! V2Query(String query, boolean withParameters) {
> if (!withParameters)
> {
> fragments = new String[] { query };
> --- 17,23 ----
> * Query implementation for all queries via the V2 protocol.
> */
> class V2Query implements Query {
> ! V2Query(String query, boolean withParameters, boolean standardConformingStrings) {
> if (!withParameters)
> {
> fragments = new String[] { query };
> ***************
> *** 36,42 ****
> switch (aChars[i])
> {
> case '\'': // single-quotes
> ! i = Parser.parseSingleQuotes(aChars, i);
> break;
>
> case '"': // double-quotes
> --- 36,42 ----
> switch (aChars[i])
> {
> case '\'': // single-quotes
> ! i = Parser.parseSingleQuotes(aChars, i, standardConformingStrings);
> break;
>
> case '"': // double-quotes
> diff -crN pgjdbc.1cc1d2edf1a6/org/postgresql/core/v3/ConnectionFactoryImpl.java pgjdbc.af39e026875b/org/postgresql/core/v3/ConnectionFactoryImpl.java
> *** pgjdbc.1cc1d2edf1a6/org/postgresql/core/v3/ConnectionFactoryImpl.java 2006-11-06 10:24:03.000000000 +0100
> --- pgjdbc.af39e026875b/org/postgresql/core/v3/ConnectionFactoryImpl.java 2006-11-06 10:24:03.000000000 +0100
> ***************
> *** 470,475 ****
> --- 470,484 ----
> throw new PSQLException(GT.tr("Protocol error. Session setup failed."), PSQLState.CONNECTION_UNABLE_TO_CONNECT);
> pgStream.setEncoding(Encoding.getDatabaseEncoding("UNICODE"));
> }
> + else if (name.equals("standard_conforming_strings"))
> + {
> + if (value.equals("on"))
> + protoConnection.setStandardConformingStrings(true);
> + else if (value.equals("off"))
> + protoConnection.setStandardConformingStrings(false);
> + else
> + throw new PSQLException(GT.tr("Protocol error. Session setup failed."), PSQLState.CONNECTION_UNABLE_TO_CONNECT);
> + }
>
> break;
>
> diff -crN pgjdbc.1cc1d2edf1a6/org/postgresql/core/v3/ProtocolConnectionImpl.java pgjdbc.af39e026875b/org/postgresql/core/v3/ProtocolConnectionImpl.java
> *** pgjdbc.1cc1d2edf1a6/org/postgresql/core/v3/ProtocolConnectionImpl.java 2006-11-06 10:24:03.000000000 +0100
> --- pgjdbc.af39e026875b/org/postgresql/core/v3/ProtocolConnectionImpl.java 2006-11-06 10:24:03.000000000 +0100
> ***************
> *** 30,35 ****
> --- 30,37 ----
> this.database = database;
> this.logger = logger;
> this.executor = new QueryExecutorImpl(this, pgStream, info, logger);
> + // default value for server versions that don't report standard_conforming_strings
> + this.standardConformingStrings = false;
> }
>
> public String getHost() {
> ***************
> *** 52,57 ****
> --- 54,64 ----
> return serverVersion;
> }
>
> + public synchronized boolean getStandardConformingStrings()
> + {
> + return standardConformingStrings;
> + }
> +
> public synchronized int getTransactionState()
> {
> return transactionState;
> ***************
> *** 183,189 ****
> {
> transactionState = state;
> }
> !
> public int getProtocolVersion()
> {
> return 3;
> --- 190,201 ----
> {
> transactionState = state;
> }
> !
> ! synchronized void setStandardConformingStrings(boolean value)
> ! {
> ! standardConformingStrings = value;
> ! }
> !
> public int getProtocolVersion()
> {
> return 3;
> ***************
> *** 193,198 ****
> --- 205,211 ----
> private int cancelPid;
> private int cancelKey;
>
> + private boolean standardConformingStrings;
> private int transactionState;
> private SQLWarning warnings;
>
> diff -crN pgjdbc.1cc1d2edf1a6/org/postgresql/core/v3/QueryExecutorImpl.java pgjdbc.af39e026875b/org/postgresql/core/v3/QueryExecutorImpl.java
> *** pgjdbc.1cc1d2edf1a6/org/postgresql/core/v3/QueryExecutorImpl.java 2006-11-06 10:24:03.000000000 +0100
> --- pgjdbc.af39e026875b/org/postgresql/core/v3/QueryExecutorImpl.java 2006-11-06 10:24:03.000000000 +0100
> ***************
> *** 56,62 ****
> return parseQuery(sql, true);
> }
>
> ! private static Query parseQuery(String query, boolean withParameters) {
> // Parse query and find parameter placeholders;
> // also break the query into separate statements.
>
> --- 56,62 ----
> return parseQuery(sql, true);
> }
>
> ! private Query parseQuery(String query, boolean withParameters) {
> // Parse query and find parameter placeholders;
> // also break the query into separate statements.
>
> ***************
> *** 66,71 ****
> --- 66,73 ----
> int fragmentStart = 0;
> int inParen = 0;
>
> + boolean standardConformingStrings = protoConnection.getStandardConformingStrings();
> +
> char []aChars = query.toCharArray();
>
> for (int i = 0; i < aChars.length; ++i)
> ***************
> *** 73,79 ****
> switch (aChars[i])
> {
> case '\'': // single-quotes
> ! i = Parser.parseSingleQuotes(aChars, i);
> break;
>
> case '"': // double-quotes
> --- 75,81 ----
> switch (aChars[i])
> {
> case '\'': // single-quotes
> ! i = Parser.parseSingleQuotes(aChars, i, standardConformingStrings);
> break;
>
> case '"': // double-quotes
> ***************
> *** 1360,1365 ****
> --- 1362,1381 ----
> handler.handleError(new PSQLException(GT.tr("The server''s DateStyle parameter was changed to {0}. The JDBC driver requires DateStyle to begin with ISO for correct operation.", value), PSQLState.CONNECTION_FAILURE));
> endQuery = true;
> }
> +
> + if (name.equals("standard_conforming_strings"))
> + {
> + if (value.equals("on"))
> + protoConnection.setStandardConformingStrings(true);
> + else if (value.equals("off"))
> + protoConnection.setStandardConformingStrings(false);
> + else
> + {
> + protoConnection.close(); // we're screwed now; we don't know how to escape string literals
> + handler.handleError(new PSQLException(GT.tr("The server''s standard_conforming_strings parameter was reported as {0}. The JDBC driver expected on or off.", value), PSQLState.CONNECTION_FAILURE));
> + endQuery = true;
> + }
> + }
> }
> break;
>
> diff -crN pgjdbc.1cc1d2edf1a6/org/postgresql/jdbc2/AbstractJdbc2Connection.java pgjdbc.af39e026875b/org/postgresql/jdbc2/AbstractJdbc2Connection.java
> *** pgjdbc.1cc1d2edf1a6/org/postgresql/jdbc2/AbstractJdbc2Connection.java 2006-11-06 10:24:03.000000000 +0100
> --- pgjdbc.af39e026875b/org/postgresql/jdbc2/AbstractJdbc2Connection.java 2006-11-06 10:24:03.000000000 +0100
> ***************
> *** 938,943 ****
> --- 938,993 ----
> }
> }
>
> + public String escapeString(String str) throws SQLException {
> + return escapeStringInternal(str, false);
> + }
> +
> + public String getStringLiteral(String str) throws SQLException {
> + return escapeStringInternal(str, true);
> + }
> +
> + private String escapeStringInternal(String str, boolean addQuotes) throws SQLException {
> + StringBuffer sbuf = new StringBuffer(2 + str.length() * 11 / 10); // Add 10% for escaping.
> +
> + if (addQuotes)
> + sbuf.append('\'');
> +
> + if (protoConnection.getStandardConformingStrings())
> + {
> + // With standard_conforming_strings on, escape only single-quotes.
> + for (int i = 0; i < str.length(); ++i)
> + {
> + char ch = str.charAt(i);
> + if (ch == '\0')
> + throw new PSQLException(GT.tr("Zero bytes may not occur in string parameters."), PSQLState.INVALID_PARAMETER_VALUE);
> + if (ch == '\'')
> + sbuf.append('\'');
> + sbuf.append(ch);
> + }
> + }
> + else
> + {
> + // With standard_conforming_string off, escape backslashes and
> + // single-quotes, but still escape single-quotes by doubling, to
> + // avoid a security hazord if the reported value of
> + // standard_conforming_strings is incorrect.
> + for (int i = 0; i < str.length(); ++i)
> + {
> + char ch = str.charAt(i);
> + if (ch == '\0')
> + throw new PSQLException(GT.tr("Zero bytes may not occur in string parameters."), PSQLState.INVALID_PARAMETER_VALUE);
> + if (ch == '\\' || ch == '\'')
> + sbuf.append(ch);
> + sbuf.append(ch);
> + }
> + }
> +
> + if (addQuotes)
> + sbuf.append('\'');
> +
> + return sbuf.toString();
> + }
> +
> /*
> * This returns the java.sql.Types type for a PG type oid
> *
> diff -crN pgjdbc.1cc1d2edf1a6/org/postgresql/jdbc2/AbstractJdbc2DatabaseMetaData.java pgjdbc.af39e026875b/org/postgresql/jdbc2/AbstractJdbc2DatabaseMetaData.java
> *** pgjdbc.1cc1d2edf1a6/org/postgresql/jdbc2/AbstractJdbc2DatabaseMetaData.java 2006-11-06 10:24:03.000000000 +0100
> --- pgjdbc.af39e026875b/org/postgresql/jdbc2/AbstractJdbc2DatabaseMetaData.java 2006-11-06 10:24:03.000000000 +0100
> ***************
> *** 1566,1584 ****
> /**
> * Escape single quotes with another single quote.
> */
> ! protected static String escapeQuotes(String s) {
> ! StringBuffer sb = new StringBuffer();
> ! int length = s.length();
> ! for (int i = 0; i < length; i++)
> ! {
> ! char c = s.charAt(i);
> ! if (c == '\'' || c == '\\')
> ! {
> ! sb.append('\\');
> ! }
> ! sb.append(c);
> ! }
> ! return sb.toString();
> }
>
> /*
> --- 1566,1573 ----
> /**
> * Escape single quotes with another single quote.
> */
> ! protected String escapeQuotes(String s) throws SQLException {
> ! return connection.escapeString(s);
> }
>
> /*
> diff -crN pgjdbc.1cc1d2edf1a6/org/postgresql/jdbc2/AbstractJdbc2Statement.java pgjdbc.af39e026875b/org/postgresql/jdbc2/AbstractJdbc2Statement.java
> *** pgjdbc.1cc1d2edf1a6/org/postgresql/jdbc2/AbstractJdbc2Statement.java 2006-11-06 10:24:03.000000000 +0100
> --- pgjdbc.af39e026875b/org/postgresql/jdbc2/AbstractJdbc2Statement.java 2006-11-06 10:24:03.000000000 +0100
> ***************
> *** 878,884 ****
> if (c == '\'') // end of string?
> state = IN_SQLCODE;
> else if (c == '\\') // a backslash?
> ! state = BACKSLASH;
>
> newsql.append(c);
> break;
> --- 878,884 ----
> if (c == '\'') // end of string?
> state = IN_SQLCODE;
> else if (c == '\\') // a backslash?
> ! state = BACKSLASH; // HERE quoting
>
> newsql.append(c);
> break;
> ***************
> *** 2253,2259 ****
> inQuotes = !inQuotes;
> ++i;
> }
> ! else if (inQuotes && ch == '\\')
> {
> // Backslash in string constant, skip next character.
> i += 2;
> --- 2253,2259 ----
> inQuotes = !inQuotes;
> ++i;
> }
> ! else if (inQuotes && ch == '\\') //HERE quoting
> {
> // Backslash in string constant, skip next character.
> i += 2;