ICU-22322 Bump guava from 30.0-jre to 32.0.0-jre in /tools/cldr/cldr-to-icu

[dependabot]

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

• 32.0.0-jre.jar
• 32.0.0-android.jar

Guava requires one runtime dependency, which you can download here:

• failureaccess-1.0.1.jar

Javadoc

• 32.0.0-jre
• 32.0.0-android

JDiff

• 32.0.0-jre vs. 31.1-jre
• 32.0.0-android vs. 31.1-android
• 32.0.0-android vs. 32.0.0-jre

Changelog

Security fixes

• Reimplemented Files.createTempDir and FileBackedOutputStream to further address CVE-2020-8908 (#4011) and CVE-2023-2976 (#2575). (feb83a1c8f)

While CVE-2020-8908 was officially closed when we deprecated Files.createTempDir in 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 used FileBackedOutputStream class, 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 that common.io in 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 guava artifact.

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.createTempDir and FileBackedOutputStream throw an exception under Windows. This is fixed in 32.0.1. Sorry for the trouble.
• This release makes a binary-incompatible change to a @Beta API in the separate artifact guava-testlib. Specifically, we changed the return type of TestingExecutors.sameThreadScheduledExecutor to ListeningScheduledExecutorService. 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() and Parameter.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 like Stream. (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 rebase will rebase this PR
• @dependabot recreate will recreate this PR, overwriting any edits that have been made to it
• @dependabot merge will merge this PR after your CI passes on it
• @dependabot squash and merge will squash and merge this PR after your CI passes on it
• @dependabot cancel merge will cancel a previously requested merge and block automerging
• @dependabot reopen will reopen this PR if it is closed
• @dependabot close will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
• @dependabot ignore this major version will 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 version will 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 dependency will 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.

> **Note** > Automatic rebases have been disabled on this pull request as it has been open for over 30 days.

[jira-pull-request-webhook]

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

[markusicu]

looks fine
@pedberg-icu could you please run the CLDR-to-ICU converter unit tests on this PR?

FYI @mihnita @echeran

[pedberg-icu]

@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.

[mihnita]

@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.

[mihnita]

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?

[pedberg-icu]

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).

[pedberg-icu]

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.

[pedberg-icu]

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.

[pedberg-icu]

@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.

[mihnita]

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

[mihnita]

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();
     }

[mihnita]

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)

[srl295]

Maybe dump of CLDR-16622 already fixed in CLDR v44

[mihnita]

Created PR: unicode-org/cldr#3062

[mihnita]

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?

[srl295]

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.

[macchiati]

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.

[srl295]

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.

[srl295]

That said generally speaking CLDR needs fewer statics and not more.

[mihnita]

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 unicode-org/cldr#3062 PR? Or change it to point to CLDR-16622? Something else?

Mihai

[macchiati]

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 <#2502 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACJLEMAVSMOBUFJQ23Z3DGDXNOSKDANCNFSM6AAAAAAZHAEMXY> . You are receiving this because you commented.Message ID: ***@***.***>

[srl295]

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.

[markusicu]

Status? Blocked on a deadlock in CLDR code, and the CLDR PR is not merged yet?

@dependabot rebase

[dependabot]

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.