Aosen Xiong created LANG-1837:
---------------------------------
Summary: 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
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)
