Copilot commented on code in PR #3555:
URL: https://github.com/apache/celeborn/pull/3555#discussion_r2870265729
##########
client-mr/mr-shaded/pom.xml:
##########
@@ -64,8 +64,8 @@
<shadedPattern>${shading.prefix}.org.scala-lang</shadedPattern>
</relocation>
<relocation>
- <pattern>org.lz4</pattern>
- <shadedPattern>${shading.prefix}.org.lz4</shadedPattern>
+ <pattern>${lz4-java.group}</pattern>
+
<shadedPattern>${shading.prefix}.${lz4-java.group}</shadedPattern>
Review Comment:
This shade relocation uses `${lz4-java.group}` (a Maven groupId like
`at.yawk.lz4` / `org.lz4`) as the relocation `<pattern>`. Maven Shade
relocation patterns match Java package names, so this relocation will not
affect the lz4-java classes (which live under `net.jpountz.*`). Either relocate
the actual lz4-java packages (e.g., `net.jpountz`) or remove this relocation
block to avoid a misleading no-op configuration.
```suggestion
<pattern>net.jpountz</pattern>
<shadedPattern>${shading.prefix}.net.jpountz</shadedPattern>
```
##########
client-tez/tez-shaded/pom.xml:
##########
@@ -94,7 +94,7 @@
<include>org.roaringbitmap:RoaringBitmap</include>
<include>org.scala-lang:scala-library</include>
<include>org.scala-lang:scala-reflect</include>
- <include>org.lz4:lz4-java</include>
+ <include>at.yawk.lz4:lz4-java</include>
Review Comment:
The shaded artifact set hard-codes `at.yawk.lz4:lz4-java` instead of using
the `${lz4-java.group}` property. This will break builds for profiles that
override `lz4-java.group` back to `org.lz4` (e.g., the `spark-*` profiles in
the parent pom). Use the shared property here for consistency and to keep
profile builds working.
```suggestion
<include>${lz4-java.group}:lz4-java</include>
```
##########
pom.xml:
##########
@@ -1530,6 +1531,7 @@
<module>tests/spark-it</module>
</modules>
<properties>
+ <lz4-java.group>org.lz4</lz4-java.group>
<lz4-java.version>1.4.0</lz4-java.version>
<scala.version>2.11.12</scala.version>
Review Comment:
PR description/title indicate bumping lz4-java to 1.10.3 to address CVEs,
but the `spark-*` Maven profiles still pin `lz4-java.version` to older
vulnerable versions (e.g., 1.4.0/1.7.1/1.8.0). If those profiles are still
supported/used in CI or releases, they likely need the same upgrade (or an
explicit rationale/mitigation documented) to actually eliminate the reported
CVE exposure for those builds.
##########
project/CelebornBuild.scala:
##########
@@ -38,7 +38,8 @@ import CelebornCommonSettings._
object Dependencies {
val zstdJniVersion =
sparkClientProjects.map(_.zstdJniVersion).getOrElse("1.5.7-1")
- val lz4JavaVersion =
sparkClientProjects.map(_.lz4JavaVersion).getOrElse("1.8.0")
+ val lz4JavaGroup =
sparkClientProjects.map(_.lz4JavaGroup).getOrElse("at.yawk.lz4")
+ val lz4JavaVersion =
sparkClientProjects.map(_.lz4JavaVersion).getOrElse("1.10.3")
Review Comment:
This only bumps lz4-java to `1.10.3` when no Spark profile is selected
(`sparkClientProjects` is `None`). When building with `spark-*` profiles,
`sparkClientProjects` supplies older lz4-java versions (currently
1.4.0/1.7.1/1.8.0), so the CVE remediation described in the PR won’t apply to
those builds. If Spark-profile artifacts are still published/supported,
consider upgrading their pinned lz4-java versions/group as well (or documenting
why they remain on older versions).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
