Upgrade guava to 32.1.0-jre due CVE-2023-2976 by denodo-research-labs · Pull Request #23127 · prestodb/presto

denodo-research-labs · 2024-07-03T10:54:21Z

Description

Presto issue #22841

Motivation and Context

Solve CVE of severity HIGH.

Contributor checklist

Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
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 ==

General Changes
* Upgrade guava to 32.1.0-jre due CVE-2023-2976 :pr:`23127`

elharo · 2024-07-03T11:45:59Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveSplitSource.java

                        checkArgument(bucketNumber.isPresent(), "bucketNumber must be present");
                        if (!allSplitLoaded.isDone()) {
-                            return allSplitLoaded.transform(ignored -> ImmutableList.of(), executor);
+                            return FluentFuture.from(allSplitLoaded).transform(ignored -> ImmutableList.of(), executor);


How is this change related?

Fixes a compilation error due to this change introduced in Guava 27.0: AbstractFuture doesn't expose FluentFuture APIs anymore

elharo · 2024-07-03T11:47:36Z

presto-jdbc/pom.xml

                <exclusion>
-                    <groupId>*</groupId>
-                    <artifactId>*</artifactId>
+                    <artifactId>checker-qual</artifactId>


Why do these need to be excluded?

Also, groupId conventionally comes before artifactId. This made me look twice.

Done: groupid before artifactid.

Only failureaccess is needed, all others are excluded.

elharo · 2024-07-03T11:48:36Z

pom.xml

        <dep.jayway.version>2.6.0</dep.jayway.version>
        <dep.ratis.version>2.2.0</dep.ratis.version>
+        <dep.errorprone.version>2.18.0</dep.errorprone.version>
+        <dep.guava.version>32.1.0-jre</dep.guava.version>


Where was this set before? Was this pulled in via airlift? If so, we might want to update airlift instead.

guava version is pulled via airbase, but we see no movement on the forked airbase regarding to version upgrades.

errorprone version is introduced in this PR.

I personally don't know why we use airbase, but I assume there's a reason. Given that we do, I think the default approach is to fix that rather introducing the new guava version here. Otherwise it's still possible we'll pull in the wrong guava version somewhere and have dependency convergence issues.

It seems that in the past airbase was used to pull versions, but it does not appear to be the approach being used now:

the pom also overrides other versions declared in airbase such as slf4j, jackson, joda, etc...

airbase v102 is in use since May 2021

elharo · 2024-07-03T11:51:30Z

pom.xml

+        <dep.errorprone.version>2.18.0</dep.errorprone.version>
+        <dep.guava.version>32.1.0-jre</dep.guava.version>
        <dep.jackson.version>2.11.0</dep.jackson.version>
+        <dep.j2objc.version>2.8</dep.j2objc.version>


Do we need j2bjc? I see it in a few poms, but not in Java code anywhere. Can we instead remove it where it appears?

Yes we need it. It is often used in the guava code, e.g. in com.google.common.util.concurrent.AbstractFuture class, and this class is used via com.google.common.util.concurrent.SettableFuture by com.facebook.presto.hive.HiveSplitSource

but shouldn't guava supply that then? Does it need to be in our poms?

Also should we just use the guava BOM instead of setting these dependencies up manually?

We are pinning the version to avoid conflicts among different versions of the same dependency (Maven Enforcer plugin).

agrawaldevesh · 2024-07-26T06:10:47Z

presto-jdbc/pom.xml

                <exclusion>
-                    <groupId>*</groupId>
-                    <artifactId>*</artifactId>
+                    <groupId>com.google.errorprone</groupId>


I believe this is wrong and is causing shading issues in presto-jdbc: org/checkerframework is now creeping into presto-jdbc jar. The groupId should be org.checkerframework for the checker-qual

Please fix asap.

I am fixing this in #23304

agrawaldevesh

Please fix the highlighted issue asap since this is breaking projects that use presto-jdbc

denodo-research-labs requested a review from a team as a code owner July 3, 2024 10:54

denodo-research-labs requested a review from presto-oss July 3, 2024 10:54

elharo reviewed Jul 3, 2024

View reviewed changes

Upgrade guava to 32.1.0-jre due CVE-2023-2976

da1e753

denodo-research-labs force-pushed the cve_2023_2976_guava branch from 04f311c to da1e753 Compare July 3, 2024 12:26

tdcmeehan self-assigned this Jul 24, 2024

tdcmeehan approved these changes Jul 24, 2024

View reviewed changes

tdcmeehan merged commit 578aec4 into prestodb:master Jul 24, 2024

agrawaldevesh reviewed Jul 26, 2024

View reviewed changes

agrawaldevesh mentioned this pull request Jul 26, 2024

Fix for Pull request 23127 #23304

Merged

6 tasks

tdcmeehan mentioned this pull request Aug 23, 2024

Add release notes for 0.289 #23513

Merged

34 tasks

Conversation

denodo-research-labs commented Jul 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Contributor checklist

Release Notes

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agrawaldevesh Jul 26, 2024 • 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!

agrawaldevesh left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

denodo-research-labs commented Jul 3, 2024 •

edited

Loading

agrawaldevesh Jul 26, 2024 •

edited

Loading