[
https://issues.apache.org/jira/browse/LANG-1837?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Gary D. Gregory resolved LANG-1837.
-----------------------------------
Fix Version/s: 3.21.1
Resolution: Fixed
[~aosenxiong]
This is now fixed in git master and snapshot builds from
[https://repository.apache.org/content/repositories/snapshots/org/apache/commons/commons-lang3/3.21.1-SNAPSHOT]
Please verify and close.
> Fraction.toString(), Fraction.toProperString(), Range.toString() and
> CharRange.toString() read a racily initialized field twice
> -------------------------------------------------------------------------------------------------------------------------------
>
> Key: LANG-1837
> URL: https://issues.apache.org/jira/browse/LANG-1837
> Project: Commons Lang
> Issue Type: Bug
> Components: lang.*, lang.math.*
> Affects Versions: 3.14.0
> Reporter: Aosen Xiong
> Priority: Minor
> Fix For: 3.21.1
>
>
> Four methods in classes that are documented as immutable cache their result
> in a non-volatile field that is written without synchronization, and read
> that field twice: once for the null check and once for the return.
> * {{{}org.apache.commons.lang3.math.Fraction.toString(){}}}, field
> {{toString}}
> * {{{}org.apache.commons.lang3.math.Fraction.toProperString(){}}}, field
> {{toProperString}}
> * {{{}org.apache.commons.lang3.Range.toString(){}}}, field {{toString}}
> * {{{}org.apache.commons.lang3.CharRange.toString(){}}}, field {{iToString}}
> Each has the form
> {code:java}
> if (toString == null) {
> toString = ...;
> }
> return toString;
> {code}
> {{Fraction}} says "This class is immutable", {{Range}} is "An immutable range
> of objects" and #ThreadSafe# if its elements are, and {{CharRange}} says
> "Instances are immutable" and #ThreadSafe#.
> If the first read observes a non-null value written by another thread, the
> Java Memory Model does not order the second read after that write, so the
> second read may still return null ([JLS
> 17.4|https://docs.oracle.com/javase/specs/jls/se21/html/jls-17.html#jls-17.4]).
> The method can then return null.
> h3. Precedent
> In this project, commit
> [a64817368d|https://github.com/apache/commons-lang/commit/a64817368d], "Fix
> race condition in Fraction.hashCode()", made the hash a final field. The two
> string caches in the same class were not changed.
> In OpenJDK the same double read was filed and fixed as a bug four times:
> * [JDK-8302822|https://bugs.openjdk.org/browse/JDK-8302822],
> "Method/Field/Constructor/RecordComponent::getGenericInfo() is not thread
> safe". Its description: "the genericInfo field is read twice, and the second
> read returned may be null under race conditions".
> * [JDK-8291061|https://bugs.openjdk.org/browse/JDK-8291061], "Improve thread
> safety of FileTime.toString and toInstant".
> * [JDK-8261404|https://bugs.openjdk.org/browse/JDK-8261404],
> "Class.getReflectionFactory() is not thread-safe".
> * [JDK-8166842|https://bugs.openjdk.org/browse/JDK-8166842],
> "String.hashCode() has a non-benign data race".
> h3. Proposed fix
> Read the field once into a local variable, then test and return the local:
> {code:java}
> @Override
> public String toString() {
> String result = toString;
> if (result == null) {
> result = getNumerator() + "/" + getDenominator();
> toString = result;
> }
> return result;
> }
> {code}
> and likewise for the other three methods. There is no functional change for
> single-threaded callers. I will open a pull request with this change.
> h3. How this was found
> By static analysis of this pattern, confirmed by reading the code. I have not
> observed a failure; the memory model permits the stale read, but it cannot be
> triggered on demand.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
