Skip to content

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

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.

Sign up for GitHub

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

Merged
tdcmeehan merged 1 commit into from
Jul 24, 2024
+32 −15
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,10 @@
<dep.docker-java.version>3.3.0</dep.docker-java.version>
<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>
Copy link
Contributor

@elharo elharo Jul 3, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@denodo-research-labs denodo-research-labs Jul 3, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@elharo elharo Jul 3, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@denodo-research-labs denodo-research-labs Jul 3, 2024

Choose a reason for hiding this comment

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

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

<dep.jackson.version>2.11.0</dep.jackson.version>
<dep.j2objc.version>2.8</dep.j2objc.version>
Copy link
Contributor

@elharo elharo Jul 3, 2024

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@denodo-research-labs denodo-research-labs Jul 3, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@elharo elharo Jul 3, 2024

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@denodo-research-labs denodo-research-labs Jul 3, 2024

Choose a reason for hiding this comment

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

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

<!--
America/Bahia_Banderas has:
- offset change since 1970 (offset Jan 1970: -08:00, offset Jan 2018: -06:00)
Expand Down Expand Up @@ -2240,12 +2243,19 @@
<artifactId>datasketches-java</artifactId>
<version>5.0.1</version>
</dependency>


<dependency>
<groupId>com.google.errorprone</groupId>
<artifactId>error_prone_annotations</artifactId>
<version>${dep.errorprone.version}</version>
</dependency>

<dependency>
<groupId>com.google.j2objc</groupId>
<artifactId>j2objc-annotations</artifactId>
<version>1.3</version>
<version>${dep.j2objc.version}</version>
</dependency>

</dependencies>
</dependencyManagement>

Expand Down
10 changes: 0 additions & 10 deletions presto-cassandra/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,6 @@
<properties>
<air.main.basedir>${project.parent.basedir}</air.main.basedir>
</properties>

<dependencyManagement>
<dependencies>
<dependency>
<groupId>com.google.errorprone</groupId>
<artifactId>error_prone_annotations</artifactId>
<version>2.3.4</version>
</dependency>
</dependencies>
</dependencyManagement>

<dependencies>
<dependency>
Expand Down
3 changes: 2 additions & 1 deletion presto-hive/src/main/java/com/facebook/presto/hive/HiveSplitSource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import com.facebook.presto.spi.connector.ConnectorPartitionHandle;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableList;
import com.google.common.util.concurrent.FluentFuture;
import com.google.common.util.concurrent.Futures;
import com.google.common.util.concurrent.ListenableFuture;
import com.google.common.util.concurrent.SettableFuture;
Expand Down Expand Up @@ -328,7 +329,7 @@ public synchronized ListenableFuture<List<ConnectorSplit>> borrowBatchAsync(Opti
{
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);
Copy link
Contributor

@elharo elharo Jul 3, 2024

Choose a reason for hiding this comment

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

How is this change related?

Copy link
Contributor Author

@denodo-research-labs denodo-research-labs Jul 3, 2024

Choose a reason for hiding this comment

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

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

}
return immediateFuture(function.apply(getSplits(bucketNumber.getAsInt(), maxSize)).getResult());
}
Expand Down
20 changes: 18 additions & 2 deletions presto-jdbc/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,24 @@
<artifactId>guava</artifactId>
<exclusions>
<exclusion>
<groupId>*</groupId>
<artifactId>*</artifactId>
<groupId>com.google.errorprone</groupId>
Copy link
Contributor

@agrawaldevesh agrawaldevesh Jul 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@agrawaldevesh agrawaldevesh Jul 26, 2024

Choose a reason for hiding this comment

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

I am fixing this in #23304

<artifactId>checker-qual</artifactId>
Copy link
Contributor

@elharo elharo Jul 3, 2024

Choose a reason for hiding this comment

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

Why do these need to be excluded?

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

Copy link
Contributor Author

@denodo-research-labs denodo-research-labs Jul 3, 2024

Choose a reason for hiding this comment

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

Done: groupid before artifactid.

Only failureaccess is needed, all others are excluded.

</exclusion>
<exclusion>
<groupId>com.google.errorprone</groupId>
<artifactId>error_prone_annotations</artifactId>
</exclusion>
<exclusion>
<groupId>com.google.j2objc</groupId>
<artifactId>j2objc-annotations</artifactId>
</exclusion>
<exclusion>
<groupId>com.google.code.findbugs</groupId>
<artifactId>jsr305</artifactId>
</exclusion>
<exclusion>
<groupId>com.google.guava</groupId>
<artifactId>listenablefuture</artifactId>
</exclusion>
</exclusions>
</dependency>
Expand Down