MySQL Bugs: #115992: Contribution: Update StringUtils.java

Bug #115992	Contribution: Update StringUtils.java
Submitted:	3 Sep 16:34	Modified:	23 Nov 15:11
Reporter:	OCA Admin (OCA)	Email Updates:
Status:	Can't repeat	Impact on me:	None
Category:	Connector / J	Severity:	S2 (Serious)
Version:	9.0	OS:	Any
Assigned to:	Filipe Silva	CPU Architecture:	Any
Tags:	Contribution

Description:
This bug tracks a contribution by Artem Gordiienko (Github user: tema-mazy), as described in http://github.com/mysql/mysql-connector-j/pull/110

How to repeat:
See description

Suggested fix:
See contribution code attached

Contribution submitted via Github - Update StringUtils.java (*) Contribution by Artem Gordiienko (Github tema-mazy, mysql-connector-j/pull/110#issuecomment-2326124995): "I confirm the code being submitted is offered under the terms of the OCA, and that I am authorized to contribute it."

Contribution: git_patch_2049625248.txt (text/plain), 1.21 KiB.

Severity:	S3 (Non-critical)
it is critical for our production env
manticoresearch is not working with " ANSI quotation
so we can't upgrade the driver 
stuck on 8.0.17

Hello Artem Gordiienko,

Thank you for the report and contribution.

regards,
Umesh

Hi Artem Gordiienko,

Once again, thank for this contribution.

Doubling quotes is the preferred way of escaping a quote char in a MySQL string, regardless of ANSI_QUOTES mode being active or not. As such, this patch should have no impact in the Connector's behavior neither fixes any problem.

Unless you can come up with a test case that proves otherwise, I'm afraid this patch doesn't need to be merged.

Hi, Filipe!

The manticoresearch uses MySQL protocol to communicate, so we have to use MySQL driver in our Java application. But manticoreserch engine does not support ANSI quote escaping -> '', only backslash.
https://manual.manticoresearch.com/Searching/Full_text_matching/Escaping

After upgrading java mysql driver, we have got a thousand errors caused by double quotes. After debugging, we have issued that the driver does double quotation in all cases - despite setting ANSI SQL mode, which causes errors.  

The last working version is 8.0.17. But is has CVE and must be upgraded.

btw, public static byte[] quoteBytes(byte[] bytes) - also adds double quote, but this methon is used only in tests so does not affects.

example:
SQL [INSERT INTO table VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?),(?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?),(?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?),(?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?),(?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?),(?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?),(?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?),(?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?),(?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?),(?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?),(?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?),(?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?),(?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?),(?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?),(?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?),(?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?),(?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?),(?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?),(?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?),(?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?)]; P01: syntax error, unexpected string, expecting ',' or ')' near **''**s responsibilities ...
        at org.jooq_3.15.3.MYSQL.debug(Unknown Source)
        at org.jooq.impl.Tools.translate(Tools.java:2988)
        at org.jooq.impl.DefaultExecuteContext.sqlException(DefaultExecuteContext.java:639)
        at org.jooq.impl.BatchSingle.executePrepared(BatchSingle.java:251)
        at org.jooq.impl.BatchSingle.execute(BatchSingle.java:175)
        at com.atl.wdi2.cron.task.op.MctInsOp.call(MctInsOp.java:39)
        at com.atl.wdi2.cron.task.op.MctInsOp.call(MctInsOp.java:15)
        at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:317)
        at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144)
        at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642)
        at java.base/java.lang.Thread.run(Thread.java:1583)
Caused by: java.sql.BatchUpdateException: P01: syntax error, unexpected string, expecting ',' or ')' near ''s responsibilities...
        at com.mysql.cj.jdbc.exceptions.SQLError.createBatchUpdateException(SQLError.java:224)
        at com.mysql.cj.jdbc.ClientPreparedStatement.executeBatchSerially(ClientPreparedStatement.java:816)
        at com.mysql.cj.jdbc.ClientPreparedStatement.executeBatchInternal(ClientPreparedStatement.java:418)
        at com.mysql.cj.jdbc.StatementImpl.executeBatch(StatementImpl.java:795)
        at org.apache.tomcat.dbcp.dbcp2.DelegatingStatement.executeBatch(DelegatingStatement.java:241)
        at org.apache.tomcat.dbcp.dbcp2.DelegatingStatement.executeBatch(DelegatingStatement.java:241)
        at org.jooq.tools.jdbc.DefaultStatement.executeBatch(DefaultStatement.java:122)
        at org.jooq.impl.BatchSingle.executePrepared(BatchSingle.java:231)
        ... 7 common frames omitted

It seems to me this is a bug in manticorsearch, isn't it? Have you tried filing a bug/feature request there?

I do not think so. They use backslash quotation. No ANSI mode supported. it is not bug. 
From the other side - there is a bug in a driver, which uses double quotes all time, instead only in sql_mode = ANSI_QUOTES

proper code for non-ansi was in driver: 
https://github.com/mysql/mysql-connector-j/blob/16a712ddb3f826a1933ab42b0039f7fb9eebc6ec/s...

in all new versions 
https://github.com/mysql/mysql-connector-j/blob/e0e8e3461e5257ba4aa19e6b3614a2685b298947/s...

- just double quoted. 

yes. we've created bug report in manticore search also. 
https://github.com/manticoresoftware/manticoresearch/issues/2546

Doubling the quote character is the quotes escaping method defined in the SQL Standard. Only by chance MySQL supports both doubling the quote character and preceding it by a reverse solidus ('\'). This has nothing to do with ANSI_QUOTES.

As a general rule, Connector/J follows the standards as close as possible and this change would be a regression to a non-standard behavior. There are many other places in the code where Connector/J uses quote char doubling and, in the cases where it doesn't, most probably it will be changed to do so in the future. Additionally, using reverse solidus to escape characters can be highly problematic when used in conjunction with sql_mode=NO_BACKSLASH_ESCAPES. Note that there was a good reason to change this to current behavior, back in 8.0.18.

For all these reasons, I'm afraid I can't make the suggested change. It is my honest opinion that it should be "fixed" in manticorsearch. Especially because, AFAIU, manticorsearch technically sits between the driver (Connector/J) and the MySQL Server, and so, it's must adhere to MySQL's ways in all aspects. It would have been an entirely different issue if it were the MySQL Server itself rejecting the data, but this is not the case.

I hope you understand.

I do not understand. sorry.
There is a total mess in the driver's code - sometimes there is a check for ANSI, sometimes not. Sometimes it backslashing, sometimes - just doubling. 

https://github.com/mysql/mysql-connector-j/blob/release/9.x/src/main/core-api/java/com/mys...
- backslashed ( without attention to SQL_MODE )

https://github.com/mysql/mysql-connector-j/blob/e0e8e3461e5257ba4aa19e6b3614a2685b298947/s...
- doubled ( without attention to SQL_MODE )
"
case '\'':
   buf.append('\'');
   buf.append('\'');
break;
"
but in normal world this have to be 
"
case '\'':
   buf.append('\\'); /* quoted backlash */
   buf.append('\''); /* quoted apostrophe */
break;
"

So. You have the power to merge or not to merge.  

I just want to make behavior of the driver predictable. 
ANSI - doubled, not ANSI - backslashed. 

That's all.

P.S. Manticore - standalone engine, not a middleware with MySQL backend.
They just use binary MySQL protocol to communicate.

mysql server:
https://github.com/mysql/mysql-server/blob/trunk/mysys/charset.cc#L460

\' -> \' 

https://github.com/mysql/mysql-server/blob/trunk/mysys/charset.cc#L475
+ append \\ before
so \' quoted as \\\'

So, you are right. 
this PR is wrong. 

cat you just replace wrong \' to \\
here ? 
https://github.com/mysql/mysql-connector-j/blob/release/9.x/src/main/core-api/java/com/mys...

as in official MySQL C code

Blindly following MySQL Server code won't work here—actually, you might be looking at a bug in the MySQL server code itself. There are other reasons for having changed this behavior in Connector/J. I can come up with a simple test that fails with this patch, as well as the with other one you just submitted:

    @Test
    void testBug115992() throws Exception {
        String sqlMode = getMysqlVariable("sql_mode");
        sqlMode += ",NO_BACKSLASH_ESCAPES";
        System.out.println(sqlMode);

        Properties props = new Properties();
        props.setProperty("sslMode", "DISABLED");
        props.setProperty("allowPublicKeyRetrieval", "true");
        props.setProperty("characterEncoding", "latin1");
        props.setProperty("sessionVariables", "sql_mode='" + sqlMode + "'");

        try (Connection testConn = getConnectionWithProps(props)) {
            this.pstmt = testConn.prepareStatement("SELECT ?");
            String data = "MySQL'Connector/J";
            this.pstmt.setNString(1, data);
            this.rs = this.pstmt.executeQuery();
            this.rs.next();
            System.out.println(this.rs.getString(1));
        }
    }

This is just an example. There might be several other cases under the same situation.

What I'm saying is that, to the best of our knowledge, the safest and most correct way of escaping quotes in SQL strings is by doubling them. That's what the specification says, that's what is documented everywhere.

Assuming that maticoresearch intends to be SQL standard compliant, and compatible to main database systems out there, it should support double quotes. That's where the problem lies.

Not agreed. 
I can write a test that will work for 8.0.17 and fail in greater versions. 

Maybe my patch is not ideal, but the problem persists in driver's code — there should be exactly defined rules for escaping.

please look at 
https://github.com/mysql/mysql-connector-j/blob/release/9.x/src/main/core-api/java/com/mys...
and
https://github.com/mysql/mysql-connector-j/blob/release/9.x/src/main/core-api/java/com/mys...

why it is different ?

-- close --

BTW, there's a mechanism that allows you to rewrite queries before being executed. I don't know if it works for you, but where's rudimentary implementation of a query interceptor that replaces "''" by "\'":

    @Test
    void testBug115992() throws Exception {
        String sqlMode = getMysqlVariable("sql_mode");
        // sqlMode += ",ANSI_QUOTES,NO_BACKSLASH_ESCAPES";
        // sqlMode += ",NO_BACKSLASH_ESCAPES";
        System.out.println(sqlMode);

        Properties props = new Properties();
        props.setProperty("sslMode", "DISABLED");
        props.setProperty("allowPublicKeyRetrieval", "true");
        props.setProperty("characterEncoding", "latin1");
        props.setProperty("sessionVariables", "sql_mode='" + sqlMode + "'");
        props.setProperty("queryInterceptors", TestBug115992QueryInterceptor.class.getName());

        try (Connection testConn = getConnectionWithProps(props)) {
            this.pstmt = testConn.prepareStatement("SELECT ?");
            String data = "MySQL'Connector/J";
            this.pstmt.setNString(1, data);
            this.rs = this.pstmt.executeQuery();
            this.rs.next();
            System.out.println(this.rs.getString(1));
        }
    }

    public static class TestBug115992QueryInterceptor implements QueryInterceptor {

        @Override
        public QueryInterceptor init(MysqlConnection conn, Properties props, Log log) {
            return this;
        }

        @SuppressWarnings("unchecked")
        @Override
        public <T extends Resultset> T preProcess(Supplier<String> sql, Query interceptedQuery) {
            if (sql.get().contains("''")) {
                try {
                    return (T) ((Statement) interceptedQuery).executeQuery(sql.get().replace("''", "\\'"));
                } catch (SQLException ex) {
                    throw ExceptionFactory.createException(ex.getMessage(), ex);
                }
            }
            return null;
        }

        @Override
        public boolean executeTopLevelOnly() {
            return true;
        }

        @Override
        public void destroy() {
        }

        @Override
        public <T extends Resultset> T postProcess(Supplier<String> sql, Query interceptedQuery, T originalResultSet, ServerSession serverSession) {
            return originalResultSet;
        }

    }

Of course, like the previous example, it won't work if "NO_BACKSLASH_ESCAPES" is set, but it has the capability of also "fixing" queries that were originally written containing doubled quotes already, e.g., "SELECT 'MySQL ''rocks''!'"

Of course if you come up with a test that fails with Connector/J connecting directly to MySQL Server we'll gladly look at it and try to fix it the best we can.

No feedback was provided for this bug for over a month, so it is
being suspended automatically. If you are able to provide the
information that was originally requested, please do so and change
the status of the bug back to "Open".

Previously, you asked why StringUtils.escapeBytes() and StringUtils.escapeString() escapes single quotes differently. Note how StringUtils.escapeBytes() is only run if NO_BACKSLASH_ESCAPES is off, while StringUtils.escapeBytes() runs regardless of NO_BACKSLASH_ESCAPES setting.

Let me reinforce this: ANSI_QUOTES has nothing to do with escaping rules. What it does, is to define the behavior for the double-quotes character, whether it works as a string value delimiter or an identifier delimiter.

Since there was no test case showing the supposed problem, and the proposed fix is unacceptable, and there is a possible workaround, I'm forced to reject this contribution.

Once again, thank you so much for your contribution and for the opportunity to debate this issue with you.