RFR: 8166842: String.hashCode() has a non-benign data race

[David Holmes]

On 29/09/2016 10:49 AM, Carsten Varming wrote:
> Dear David,
>
> See inline.
>
> On Wed, Sep 28, 2016 at 7:47 PM, David Holmes <david.holmes at oracle.com
> <mailto:david.holmes at oracle.com>> wrote:
>
>     On 29/09/2016 3:44 AM, Carsten Varming wrote:
>
>         Dear Vitaly and David,
>
>         Looking at your webrev the original code is:
>
>         public int hashCode() {
>           if (hash == 0 && value.length > 0) {
>             hash = isLatin1() ? StringLatin1.hashCode(value)
>           }
>           return hash;
>         }
>
>         There is a constructor:
>
>         public String(String original) {
>           this.value = original.value;
>           this.coder = original.coder;
>           this.hash = original.hash;
>         }
>
>         that might write zero to the mutable field "hash".
>
>         The object created by this constructor might be shared using
>         plain reads
>         and writes between two threads[1] and the write of 0 in the
>         constructor
>         might be interleaved with the reads and write in hashCode. Does this
>         capture the problem?
>
>
>     Because String has final fields there is a freeze action at the end
>     of construction so that String instances are always safely published
>     even if not "safely published".
>
>
> I always thought that the freeze action only freezes final fields. The
> hash field in String is not final and example 17.5-1 is applicable as
> far as I can see
> (https://docs.oracle.com/javase/specs/jls/se8/html/jls-17.html#jls-17.5). Has
> the memory model changed in JDK9 to invalidate example 17.5-1 or I am
> missing something about String.

Sorry - I was confusing what the spec says versus what the VM actually 
does - as Vitaly pointed out.

David

>
> Carsten
>
>
>     David
>
>
>         [1]: Meaning the is no happens-before relationship established
>         between
>         object construction and another thread calling hashCode on the
>         object.
>
>         Carsten
>
>         On Wed, Sep 28, 2016 at 10:13 AM, Vitaly Davidovich
>         <vitalyd at gmail.com <mailto:vitalyd at gmail.com>
>         <mailto:vitalyd at gmail.com <mailto:vitalyd at gmail.com>>> wrote:
>
>             On Wednesday, September 28, 2016, David Holmes
>             <david.holmes at oracle.com <mailto:david.holmes at oracle.com>
>         <mailto:david.holmes at oracle.com <mailto:david.holmes at oracle.com>>>
>             wrote:
>
>             > On 28/09/2016 10:44 PM, Peter Levart wrote:
>             >
>             >> Hi,
>             >>
>             >> According to discussion here:
>             >>
>             >> http://cs.oswego.edu/pipermail/concurrency-interest/2016-
>         <http://cs.oswego.edu/pipermail/concurrency-interest/2016->
>             <http://cs.oswego.edu/pipermail/concurrency-interest/2016-
>         <http://cs.oswego.edu/pipermail/concurrency-interest/2016->>
>             >> September/015414.html
>             >>
>             >>
>             >> it seems compact strings introduced (at least
>         theoretical) non-benign
>             >> data race into String.hasCode() method.
>             >>
>             >> Here is a proposed patch:
>             >>
>             >>
>         http://cr.openjdk.java.net/~plevart/jdk9-dev/8166842_String
>         <http://cr.openjdk.java.net/~plevart/jdk9-dev/8166842_String>
>             <http://cr.openjdk.java.net/~plevart/jdk9-dev/8166842_String
>         <http://cr.openjdk.java.net/~plevart/jdk9-dev/8166842_String>>.
>
>             >> hashCode/webrev.01/
>             >>
>             >
>             > I'm far from convinced that the bug exists - theoretical or
>             otherwise -
>             > but the "fix" is harmless.
>             >
>             > When we were working on JSR-133 one of the conceptual
>         models is
>             that every
>             > write to a variable goes into the set of values that a
>         read may
>             potentially
>             > return (so no out-of-thin-air for example). happens-before
>         establishes
>             > constraints on which value can legally be returned - the most
>             recent. An
>             > additional property was that once a value was returned, a
>         later
>             read could
>             > not return an earlier value - in essence once a read returns a
>             given value,
>             > all earlier written values are removed from the set of
>         potential
>             values
>             > that can be read.
>             >
>             > Your bug requires that the code act as-if written:
>             >
>             > int temp = hash;
>             > if (temp == 0) {
>             >    hash = ...
>             > }
>             > return temp;
>
>             It's the other way I think:
>
>             int temp = hash; // read 0
>             if (hash == 0) // reread a non 0
>                  hash = temp = ...
>              return temp // return 0
>
>             It's unlikely but what prohibits that?
>
>             >
>             > and I do not believe that is allowed.
>             >
>             > David
>             >
>             >
>             >> For the bug:
>             >>
>             >>     https://bugs.openjdk.java.net/browse/JDK-8166842
>         <https://bugs.openjdk.java.net/browse/JDK-8166842>
>             <https://bugs.openjdk.java.net/browse/JDK-8166842
>         <https://bugs.openjdk.java.net/browse/JDK-8166842>>
>             >>
>             >>
>             >>
>             >> JDK 8 did not have this problem, so no back-porting
>         necessary.
>             >>
>             >> Regards, Peter
>             >>
>             >>
>
>             --
>             Sent from my phone
>
>
>