unicode-org
ICU-22322 Bump guava from 30.0-jre to 32.0.0-jre in /tools/cldr/cldr-to-icu
Bumps guava from 30.0-jre to 32.0.0-jre.
Release notes
Sourced from guava's releases.
32.0.0
Maven
<dependency> <groupId>com.google.guava</groupId> <artifactId>guava</artifactId> <version>32.0.0-jre</version> <!-- or, for Android: --> <version>32.0.0-android</version> </dependency>Jar files
Guava requires one runtime dependency, which you can download here:
Javadoc
JDiff
Changelog
Security fixes
- Reimplemented
Files.createTempDirandFileBackedOutputStreamto further address CVE-2020-8908 (#4011) and CVE-2023-2976 (#2575). (feb83a1c8f)While CVE-2020-8908 was officially closed when we deprecated
Files.createTempDirin Guava 30.0, we've heard from users that even recent versions of Guava have been listed as vulnerable in other databases of security vulnerabilities. In response, we've reimplemented the method (and the very rarely usedFileBackedOutputStreamclass, which had a similar issue) to eliminate the insecure behavior entirely. This change could technically affect users in a number of different ways (discussed under "Incompatible changes" below), but in practice, the only problem users are likely to encounter is with Windows. If you are using those APIs under Windows, you should skip 32.0.0 and go straight to 32.0.1 which fixes the problem. (Unfortunately, we didn't think of the Windows problem until after the release. And while we warn thatcommon.ioin particular may not work under Windows, we didn't intend to regress support.) Sorry for the trouble.Incompatible changes
Although this release bumps Guava's major version number, it makes no binary-incompatible changes to the
guavaartifact.One change could cause issues for Widows users, and a few other changes could cause issues for users in more usual situations:
- The new implementations of
Files.createTempDirandFileBackedOutputStreamthrow an exception under Windows. This is fixed in 32.0.1. Sorry for the trouble.- This release makes a binary-incompatible change to a
@BetaAPI in the separate artifactguava-testlib. Specifically, we changed the return type ofTestingExecutors.sameThreadScheduledExecutortoListeningScheduledExecutorService. The old return type was a package-private class, which caused the Kotlin compiler to produce warnings. (dafaa3e435)- This release adds two methods to the Android flavor of Guava:
Invokable.getAnnotatedReturnType()andParameter.getAnnotatedType(). Those methods do not work under an Android VM; we added them only to help our tests of the Android flavor (since we also run those tests under a JRE). Android VMs tolerate such methods as long as the app does not call them or perform reflection on them, and builds tolerate them because of our new Proguard configurations (discussed below). Thus, we expect no impact to most users. However, we could imagine build problems for users who have set up their own build system for the Android flavor of Guava. Please report any problems so that we can judge how safely we might be able to add other methods to the Android flavor in the future, such as APIs that use Java 8 classes likeStream. (b30e73cfa81ad15c1023c17cfd083255a3df0105)
... (truncated)
Commits
- See full diff in compare view
You can trigger a rebase of this PR by commenting @dependabot rebase.
Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR:
@dependabot rebasewill rebase this PR@dependabot recreatewill recreate this PR, overwriting any edits that have been made to it@dependabot mergewill merge this PR after your CI passes on it@dependabot squash and mergewill squash and merge this PR after your CI passes on it@dependabot cancel mergewill cancel a previously requested merge and block automerging@dependabot reopenwill reopen this PR if it is closed@dependabot closewill close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually@dependabot ignore this major versionwill close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this minor versionwill close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this dependencywill close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) You can disable automated security fix PRs for this repo from the Security Alerts page.
![dependabot[bot] avatar](https://avatars.githubusercontent.com/in/29110?v=4 "Published by a pub.dev verified publisher"){kind=small}
Hooray! The files in the branch are the same across the force-push. 😃
~ Your Friendly Jira-GitHub PR Checker Bot
![jira-pull-request-webhook[bot] avatar](https://avatars.githubusercontent.com/u/13873561?v=4 "Published by a pub.dev verified publisher"){kind=small}
@pedberg-icu could you please run the CLDR-to-ICU converter unit tests on this PR? FYI @mihnita @echeran
I get a hang in CldrValue.parseValue when running the tests on the branch. Not sure whether it is related to the new libraries, investigating.
@pedberg-icu could you please run the CLDR-to-ICU converter unit tests on this PR? FYI @mihnita @echeran
I get a hang in CldrValue.parseValue when running the tests on the branch. Not sure whether it is related to the new libraries, investigating.
Sorry, I've only seen this now. Starting on in.
Everything seems to be working fine for me.
But I am not directly involved in the CLDR / ICU dev / release. I might patch something here and there, by I am not familiar with the processes uses for releasing, and I normally I use whatever branch people tell me to.
So I don't know what "the branch" means in "running the tests on the branch" All the tests I did were top of main repos (icu, cldr, and cldr-staging). The only change as this PR. And all I did was on Linux.
But I've seen something interesting.
This PR updates to com.google.guava:guava to 32.0.0-jre
But in the meantime there is a 32.0.1-jre release: https://mvnrepository.com/artifact/com.google.guava/guava/32.0.1-jre
And the Changelog for that version says: "io: Fixed Files.createTempDir and FileBackedOutputStream under Windows, which broke as part of the security fix in release 32.0.0. Sorry for the trouble." https://github.com/google/guava/releases/tag/v32.0.1
If the errors you see are on Windows, then maybe that's the cause?
If the errors you see are on Windows, then maybe that's the cause?
I am running on macOS, not Windows 🙃. By running tests on "this branch" I mean this dependabot/maven/tools/cldr/cldr-to-icu/com.google.guava-guava-32.0.0-jre](https://github.com/unicode-org/icu/tree/dependabot/maven/tools/cldr/cldr-to-icu/com.google.guava-guava-32.0.0-jre branch that includes the guava update (and I am running with the latest CLDR tools on the cldr main branch, and the production data generated from the latest CLDR data on the main branch, in case that is relevant).
I am running the tests using
mvn test -DCLDR_DIR="$CLDR_DATA_DIR"
in the icu/tools/cldr/cldr-to-icu directory (with CLDR_DATA_DIR pointing to the cldr production data in cldr-staging/production).
I am running the tests using
mvn test -DCLDR_DIR="$CLDR_DATA_DIR"in the icu/tools/cldr/cldr-to-icu directory (with CLDR_DATA_DIR pointing to the cldr production data in cldr-staging/production).
Hmm, the problems I am seeing could easily be with the latest cldr 44 production data, which we have not yet tried integrating to ICU. The hang I am seeing is when XPathParser.handleParse starts to process the path //ldml/numbers/currencies/currency[@type="USD"]/displayName, it then goes off to process supplementalData (presumably to check the USD currency code?), and eventually hangs in that. I will re-try using the production data from CLDR 43.1 to see if that works.
I will re-try using the production data from CLDR 43.1 to see if that works.
No, the FilteredDataTest hangs using CLDR 43.1 tools and production data too.
@markusicu @mihnita I tried running the CLDR-to-ICU converter unit tests on the ICU main branch (i.e. without the guava lib update) using the CLDR 43.1 data (which was the last CLDR data integrated to ICU) and the tests hung in the same place: Running org.unicode.icu.tool.cldrtoicu.FilteredDataTest
There are not tests I usually run (I had never run them until requested by Markus for this PR) and I have no idea how long they have been failing, but the failure does not seem related to the guava lib update in this PR.
I've managed to track down the cause.
It is a lock in org.unicode.cldr.util.XPathParts (in the tools folder of the cldr repo)
XPathParts caches using computeIfAbsent on a ConcurrentHashMap.
But the computation done in the lambda ends up creating some frozen DtdData.Element,
also heavily cached in a ConcurrentHashMap and using computeIfAbsent
Here is a stack dump of the frozen test:
And here is a patch with a fix.
diff --git a/tools/cldr-code/src/main/java/org/unicode/cldr/util/XPathParts.java b/tools/cldr-code/src/main/java/org/unicode/cldr/util/XPathParts.java
index 86513ca009..a28a530433 100644
--- a/tools/cldr-code/src/main/java/org/unicode/cldr/util/XPathParts.java
+++ b/tools/cldr-code/src/main/java/org/unicode/cldr/util/XPathParts.java
@@ -1211,10 +1211,17 @@ public final class XPathParts extends XPathParser
}
public static XPathParts getFrozenInstance(String path) {
- XPathParts result =
- cache.computeIfAbsent(
- path,
- (String forPath) -> new XPathParts().addInternal(forPath, true).freeze());
+ XPathParts result = cache.get(path);
+ if (result == null) {
+ // If we create the value in the lambda inside computeIfAbsent we lock
+ // The DtdData also caches aggressively, in a ConcurrentHashMap and
+ // also calling computeIfAbsent.
+ // With this change we might create a throw-away value (temp) ia something
+ // else won a race and added already added a value for path.
+ // But should be rare enough (we already tested if the key has a value)
+ final XPathParts temp = new XPathParts().addInternal(path, true).freeze();
+ result = cache.computeIfAbsent(path, (forPath) -> temp);
+ }
return result;
}
It belongs in the cldr repo, not here, but just in case you want to check.
But it shows that this PR is not not at fault.
With that patch applied the tests still fail somewhere else with:
[ERROR] org.unicode.icu.tool.cldrtoicu.SupplementalDataTest Time elapsed: 0.006 s <<< ERROR!
java.lang.NullPointerException
at java.base/sun.nio.fs.UnixPath.normalizeAndCheck(UnixPath.java:75)
at java.base/sun.nio.fs.UnixPath.<init>(UnixPath.java:69)
at java.base/sun.nio.fs.UnixFileSystem.getPath(UnixFileSystem.java:279)
at java.base/java.nio.file.Path.of(Path.java:147)
at java.base/java.nio.file.Paths.get(Paths.java:69)
at org.unicode.icu.tool.cldrtoicu.SupplementalDataTest.loadRegressionData
This was probably broken for such a long time that the failures accumulated, but the first one managed to hide the others.
Let me know how you want to proceed.
Cheers, Mihai
My bad about the null path, false alert.
The tests need to run with
mvn test -DCLDR_DIR=$CLDR_DIR
It is documented in the README.
I didn't pay attention and I though that having CLDR_DIR in the environment is enough.
With that everything passes.
But it would be nice to change the code to try properties, and if not defined the environment
(so -D would win, if present)
Patch:
diff --git a/tools/cldr/cldr-to-icu/src/test/java/org/unicode/icu/tool/cldrtoicu/SupplementalDataTest.java b/tools/cldr/cldr-to-icu/src/test/java/org/unicode/icu/tool/cldrtoicu/SupplementalDataTest.java
index 6d53170769..7f8ebbafd8 100644
--- a/tools/cldr/cldr-to-icu/src/test/java/org/unicode/icu/tool/cldrtoicu/SupplementalDataTest.java
+++ b/tools/cldr/cldr-to-icu/src/test/java/org/unicode/icu/tool/cldrtoicu/SupplementalDataTest.java
@@ -39,7 +39,14 @@ public class SupplementalDataTest {
@BeforeClass
public static void loadRegressionData() {
- Path cldrRoot = Paths.get(System.getProperty("CLDR_DIR"));
+ String cldrDir = System.getProperty("CLDR_DIR");
+ if (cldrDir == null) {
+ cldrDir = System.getenv("CLDR_DIR");
+ }
+ assertWithMessage("CLDR_DIR should be defined either with -DCLDR_DIR=<value> or as environment variable")
+ .that(cldrDir)
+ .isNotNull();
+ Path cldrRoot = Paths.get(cldrDir);
regressionData = SupplementalData.create(CldrDataSupplier.forCldrFilesIn(cldrRoot));
likelySubtags = new LikelySubtags();
}
I've created https://unicode-org.atlassian.net/jira/software/c/projects/CLDR/issues/CLDR-16797 Please assign it to me (I am already working on a PR, with the fix described above)
Maybe dump of CLDR-16622 already fixed in CLDR v44
It looks indeed like the same issue. But for me this still happens at the top of the main repos (icu + cldr). Is CLDR v44 in a branch, not main?
Maybe dump of CLDR-16622 already fixed in CLDR v44
It looks indeed like the same issue. But for me this still happens at the top of the main repos (icu + cldr). Is CLDR v44 in a branch, not main?
actually it may be the same issue but needing a different fix site, something like:
diff --git a/tools/cldr-code/src/main/java/org/unicode/cldr/util/SupplementalDataInfo.java b/tools/cldr-code/src/main/java/org/unicode/cldr/util/SupplementalDataInfo.java
index 071ae0ef70..d60212759b 100644
--- a/tools/cldr-code/src/main/java/org/unicode/cldr/util/SupplementalDataInfo.java
+++ b/tools/cldr-code/src/main/java/org/unicode/cldr/util/SupplementalDataInfo.java
@@ -1282,7 +1282,7 @@ public class SupplementalDataInfo {
@Override
public void handlePathValue(String path, String value) {
try {
- XPathParts parts = XPathParts.getFrozenInstance(path);
+ XPathValue parts = SimpleXPathParts.getFrozenInstance(path);
String level0 = parts.getElement(0);
String level1 = parts.size() < 2 ? null : parts.getElement(1);
String level2 = parts.size() < 3 ? null : parts.getElement(2);
XPathParts can enforce attribute ordering, which is needed for writing LDML. However, for reading such as here — at initialization time — SimpleXPathParts was added to break the concurrency issue.
ICU data build is obviously initializing in a different order.
I'm thinking we test for this by having a special test static final environment variable. If that variable is true, then XPathParts throws an exception until supplemental data is loaded. If we run the tests, we can find out if there are any remaining exposures.
I'm thinking we test for this by having a special test static final environment variable. If that variable is true, then XPathParts throws an exception until supplemental data is loaded. If we run the tests, we can find out if there are any remaining exposures.
We should replace all XPathParts with XPathValue / SimplexPathParser where we are only reading and not writing. Simple doesn't have any dependencies.
I'd almost say never, not even in production, use xpath parts until SDI/CldrConfig is loaded. And test for it always with a message.
actually it may be the same issue but needing a different fix site, something like: ... - XPathParts parts = XPathParts.getFrozenInstance(path); + XPathValue parts = SimpleXPathParts.getFrozenInstance(path);
That changes seems to change the call site to avoid using XPathParts.getFrozenInstance
I think my fix avoids the problem by fixing it closer to the root, directly in XPathParts.getFrozenInstance, instead of hunting all instances where it's used.
But it is your decision, I will do whatever you say. Drop the https://github.com/unicode-org/cldr/pull/3062 PR? Or change it to point to CLDR-16622? Something else?
Mihai
We should replace all XPathParts with XPathValue / SimplexPathParser where we are only reading and not writing. Simple doesn't have any dependencies.
I think we need a slightly different tack; we should use SimplePathParser in anything that needs initialization (accessing supplemental data: SupplementalDataInfo, Validity, etc).
We don't want to always use SimplePathParser, because (a) XPathParts always checks a level of validity that SimplePathParser doesn't (b) if people get the toString value for it, and then use that, it will cause failures in matching, because of the order.
On Tue, Jun 27, 2023 at 8:46 PM Steven R. Loomis @.***> wrote:
I'm thinking we test for this by having a special test static final environment variable. If that variable is true, then XPathParts throws an exception until supplemental data is loaded. If we run the tests, we can find out if there are any remaining exposures.
We should replace all XPathParts with XPathValue / SimplexPathParser where we are only reading and not writing. Simple doesn't have any dependencies.
I'd almost say never, not even in production, use xpath parts until SDI/CldrConfig is loaded. And test for it always with a message.
— Reply to this email directly, view it on GitHub https://github.com/unicode-org/icu/pull/2502#issuecomment-1610643092, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACJLEMAVSMOBUFJQ23Z3DGDXNOSKDANCNFSM6AAAAAAZHAEMXY . You are receiving this because you commented.Message ID: @.***>
actually it may be the same issue but needing a different fix site, something like: ...
XPathParts parts = XPathParts.getFrozenInstance(path);
XPathValue parts = SimpleXPathParts.getFrozenInstance(path);That changes seems to change the call site to avoid using
XPathParts.getFrozenInstance
Yes, because it's startup time. XPathParts isn't usable at startup time.
I think my fix avoids the problem by fixing it closer to the root, directly in
XPathParts.getFrozenInstance, instead of hunting all instances where it's used.But it is your decision, I will do whatever you say. Drop the unicode-org/cldr#3062 PR? Or change it to point to CLDR-16622? Something else?
Keep it targeted there, I'll take a look.
Status? Blocked on a deadlock in CLDR code, and the CLDR PR is not merged yet?
@dependabot rebase
Looks like this PR has been edited by someone other than Dependabot. That means Dependabot can't rebase it - sorry!
If you're happy for Dependabot to recreate it from scratch, overwriting any edits, you can request @dependabot recreate.