Upgrading mysql jar in presto #24754

namya28 · 2025-03-19T08:52:18Z

Description

This PR is for upgrading the mysql jar version to the latest version 9.2.0.
This fixes CVE-2023-22102.

Motivation and Context

Impact

Test Plan

Contributor checklist

Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

Security Changes

* Upgrade the MySQL to version 9.2.0 in response to `CVE-2023-22102 <https://github.com/advisories/GHSA-m6vm-37g8-gqvh>`_. :pr:`24754`

agrawalreetika · 2025-03-20T07:52:20Z

presto-mysql/src/main/java/com/facebook/presto/plugin/mysql/MySqlClientModule.java

        }
-        catch (SQLException e) {
+        catch (Exception e) {


Whats the specific exception here? Should we catch that here?

Hi @agrawalreetika , thank you for your feedback. Based on my research , The code is building fine without an exception as well. Since it is optional and if required , would you recommend me adding an IllegalArgumentException instead of Exception?

Earlier code was throwing SQLException which is why it was added now in your current implementation I don't think try-catch is even needed.
Please check and update the code accordingly.

agrawalreetika · 2025-03-20T07:54:16Z

Thanks for the changes, please squash your changes into 1 with the relevant commit message.

ZacBlanco · 2025-03-20T15:15:56Z

presto-product-tests/pom.xml

+                    <groupId>com.google.protobuf</groupId>
+                    <artifactId>protobuf-java</artifactId>
+                </exclusion>
+            </exclusions>


Do we need to exclude this or can we just pull up the dependency version through the dependencyManagement section? Or is it due to duplicate class conflicts?

@ZacBlanco, Excluding this as it is resulting in upper bound dependencies error.

Can't we try to just upgrade the dependency to the required version?

@agrawalreetika , thank you for your feedback! I tried removing the exclusions and tried upgrading the protobuf-java version to 4.29.0 using 2 approaches :

upgraded the version to 4.29.0 in root pom. It is resulting in error in presto-main

2. Kept the earlier version as is in the root pom (3.25.5) and tried upgrading the version to 4.29.0 using dependency management in the module pom (presto-function-namespace-managers) and it is resulting in the same error in presto-main . (same in the case of presto-product-tests module as well). 3. Also tried ignoring it under and under duplicate-finder-maven-plugin. Resulting in the same error.

If with 4.29.0 version upgrade we are hitting duplicate class error then I am ok with excluding it from <artifactId>mysql-connector-j</artifactId> if the CI tests are passing.

@ZacBlanco Do you have any concern with exclusion? Or have any other approach to handle this?

I am not a fan of excluding random dependencies. The conflicts aren't even on Java classes, so this could potentially lead to runtime issues if we're excluding core protobuf classes. In this case I would rather add exceptions to the duplicate finder for the .proto files

Upgraded the protobuf-java version in the root pom and ignored the dependency ratis-thirdparty-misc under duplicate-finder-maven-plugin since directly adding <ignoredResourcePattern>google/protobuf/*.proto</ignoredResourcePattern> was resulting in the same error.

ZacBlanco

Do we have a PR to upgrade the testing-mysql-server? I would prefer if we updated that + released a new version for Presto first before this gets merged in order to reduce the number of exclusions

pom.xml

ZacBlanco · 2025-03-27T04:51:09Z

presto-mysql/src/main/java/com/facebook/presto/plugin/mysql/MySqlClient.java

 import static java.lang.String.format;
 import static java.util.Locale.ENGLISH;

 public class MySqlClient
        extends BaseJdbcClient
 {
+    private static final String SQL_STATE_SYNTAX_ERROR = "42000";
+    private static final String SQL_STATE_ER_TABLE_EXISTS_ERROR = "42S01";


Are these not defined/available in the new mysql client? Where is this documented?

These are sql error codes corresponding to the mysql error code. https://dev.mysql.com/doc/connector-j/en/connector-j-reference-error-sqlstates.html.

I think for the syntax error, we should instead catch the SQLSyntaxError exception from the client rather than catching and comparing the code. For the table exists error, there isn't a corresponding exception type, so we can keep the current approach. Though we should leave a comment with a link to the documentation of error codes for future developers to know where the code came from

Added the SQLSyntaxError exception as well. Had to keep the SQLException as well as I was getting the error Unhandled exception: java.sql.SQLException .

ZacBlanco · 2025-03-27T04:52:25Z

presto-product-tests/pom.xml

+                    <groupId>com.google.protobuf</groupId>
+                    <artifactId>protobuf-java</artifactId>
+                </exclusion>
+            </exclusions>


I am not a fan of excluding random dependencies. The conflicts aren't even on Java classes, so this could potentially lead to runtime issues if we're excluding core protobuf classes. In this case I would rather add exceptions to the duplicate finder for the .proto files

linux-foundation-easycla · 2025-03-27T14:58:06Z

The committers listed above are authorized under a signed CLA.

✅ login: namya28 / name: Namya Sehgal (0348d9b)

namya28 · 2025-03-28T08:23:37Z

Do we have a PR to upgrade the testing-mysql-server? I would prefer if we updated that + released a new version for Presto first before this gets merged in order to reduce the number of exclusions

There already is a release for testing mysql-server 8 for the version being used. https://repo.maven.apache.org/maven2/com/facebook/presto/testing-mysql-server-8/ .

namya28 · 2025-04-01T11:47:31Z

@ZacBlanco @agrawalreetika , I have addressed the review comments. Could you please help review once again whenever convenient.

agrawalreetika

LGTM. Please squash all your commits into one.

ZacBlanco

one minor nit

presto-mysql/src/main/java/com/facebook/presto/plugin/mysql/MySqlClient.java

ZacBlanco

LGTM! Thanks for the upgrade!

prestodb-ci added the from:IBM PR from IBM label Mar 19, 2025

namya28 requested a review from agrawalreetika March 19, 2025 08:52

namya28 changed the title ~~Upgrading mysql in presto~~ Upgrading mysql jar in presto Mar 19, 2025

namya28 marked this pull request as ready for review March 19, 2025 14:35

namya28 requested a review from a team as a code owner March 19, 2025 14:35

namya28 requested a review from jaystarshot March 19, 2025 14:35

prestodb-ci requested review from a team and bibith4 and removed request for a team March 19, 2025 14:35

agrawalreetika requested changes Mar 20, 2025

View reviewed changes

agrawalreetika requested a review from ZacBlanco March 20, 2025 07:53

ZacBlanco reviewed Mar 20, 2025

View reviewed changes

namya28 force-pushed the mysql-upgradev2 branch from 7105659 to 99a8a11 Compare March 24, 2025 15:53

agrawalreetika previously approved these changes Mar 27, 2025

View reviewed changes

ZacBlanco requested changes Mar 27, 2025

View reviewed changes

namya28 dismissed agrawalreetika’s stale review via 3048031 March 27, 2025 14:58

namya28 force-pushed the mysql-upgradev2 branch from 3048031 to e11c91c Compare March 27, 2025 15:00

namya28 force-pushed the mysql-upgradev2 branch from 0e50de0 to 641eba5 Compare March 28, 2025 11:32

namya28 requested review from ZacBlanco and agrawalreetika April 1, 2025 11:47

agrawalreetika previously approved these changes Apr 1, 2025

View reviewed changes

namya28 force-pushed the mysql-upgradev2 branch from b6839be to c903617 Compare April 1, 2025 14:01

ZacBlanco reviewed Apr 2, 2025

View reviewed changes

presto-mysql/src/main/java/com/facebook/presto/plugin/mysql/MySqlClient.java Outdated Show resolved Hide resolved

Upgrade the MySQL to version 9.2.0

0348d9b

namya28 dismissed agrawalreetika’s stale review via 0348d9b April 2, 2025 07:09

namya28 force-pushed the mysql-upgradev2 branch from c903617 to 0348d9b Compare April 2, 2025 07:09

ZacBlanco approved these changes Apr 2, 2025

View reviewed changes

agrawalreetika approved these changes Apr 2, 2025

View reviewed changes

jaystarshot approved these changes Apr 2, 2025

View reviewed changes

agrawalreetika merged commit b0286c5 into prestodb:master Apr 2, 2025
97 checks passed

ethanyzhang added the Security label Apr 3, 2025

prestodb-ci requested review from a team and anandamideShakyan and removed request for a team April 3, 2025 04:02

ZacBlanco mentioned this pull request May 29, 2025

Add release notes for 0.293 #25222

Merged

21 tasks

This was referenced May 30, 2025

Add release notes for 0.293 unix280/presto#31

Closed

Add release notes for 0.293 unix280/presto#32

Closed

agrawalreetika mentioned this pull request Jul 15, 2025

Upgrade MySQL driver version to the latest version >=8.2.0 for both main and MySQL testing server #21845

Closed

Upgrading mysql jar in presto #24754

Upgrading mysql jar in presto #24754

Uh oh!

Conversation

namya28 commented Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agrawalreetika Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agrawalreetika commented Mar 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agrawalreetika Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

namya28 Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZacBlanco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

linux-foundation-easycla bot commented Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

namya28 commented Mar 28, 2025

Uh oh!

namya28 commented Apr 1, 2025

Uh oh!

agrawalreetika left a comment

Choose a reason for hiding this comment

Uh oh!

ZacBlanco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ZacBlanco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

namya28 commented Mar 19, 2025 •

edited

Loading

agrawalreetika Mar 24, 2025 •

edited

Loading

agrawalreetika Mar 26, 2025 •

edited

Loading

namya28 Mar 28, 2025 •

edited

Loading

linux-foundation-easycla bot commented Mar 27, 2025 •

edited

Loading