8 ms·

Author of the blog here. Yes, this is a bug. While the javadoc doesn't state it explicitly, immutable classes in the core library are expected to be thread safe

by coekie 3y ago

Author of the blog here. Yes, this is a bug. While the javadoc doesn't state it explicitly, immutable classes in the core library are expected to be thread safe. Java tries to be and mostly succeeds at being a safe language, where (by default) the guarantees of its internals cannot be broken no matter what user code does. The JVM preserves its own integrity. There are some other deliberate holes in that safety, such as using reflection to access private members, and instrumentation, where it is clear you are stepping outside the safety zone, but even that is getting locked down now with the "Integrity and Strong Encapsulation" effort https://openjdk.org/jeps/8305968 https://openjdk.org/jeps/8305968 . In general, most code we write would not (and should not) try to protect against such abuse, but classes in the core platform play by different rules. Btw, this has been accepted into the bug database now: https://bugs.java.com/bugdatabase/view_bug?bug_id=JDK-8311906 https://bugs.java.com/bugdatabase/view_bug?bug_id=JDK-831190... . I expect this to be fixed in a future release.

haspok 3y ago
How would you fix this? Possibly by copying the input array to a local array as the first step? But then you might copy a lot of data, which is in 99% of the cases totally unnecessary. Have a separate threadsafe and a non-threadsafe constructor for char[]? EDIT: another idea is to do a hash of the input array, and compare between start and end. But that also requires an O(n) walk through the input...
- silon42 3y ago
  /s redesign a language to prevent parallel mutable use of the array
  - pshirshov 3y ago
    This won't happen (unfortunately) for obvious reasons.
- tpxl 3y ago
  > But that also requires an O(n) walk through the input... Compressing requires an O(n) walk through the input either way.
  - orra 3y ago
    Yes. In fact, both String.valueOf(char[]) and constructor String(char[]) are already documented as copying the array contents.
- the_mitsuhiko 3y ago
  I'm not sure how much of the underlying implementation details of arrays are leaking into JNI, but one way to approach it would be with first class copy-on-write for arrays or opt-in immutability (freezing).
- barrkel 3y ago
  Copy the array, then use alias analysis to remove the copy most of the time, by having two versions of the String constructor, with a version chosen based on aliasing.
- masklinn 3y ago
  > How would you fix this? Possibly by copying the input array to a local array as the first step? Upon encountering a non-Latin1 char, convert the existing latin1 array to an utf16 one, append the char (converted), then resume iteration in utf16 copy mode. This way you know the char which made you bail out on Latin1 is part of the output.
ris58h 3y ago
Accepting an issue doesn't mean that there is a bug. > While the javadoc doesn't state it explicitly, immutable classes in the core library are expected to be thread safe. If it's not documented it's just an assumption.
- PedroBatista 3y ago
  Yes, but at some point one has come out of the theory domain and face the real world. So in practical terms it’s closer to a bug and even the “official” guys from Java accepted it as a bug.
  - ris58h 3y ago
    Could you provide a link to the documentation to proof it's a bug? The author could call it 'probably bug' or 'misleading String constructor documentation' but instead they state it's a bug without any proofs.
    - deleted 3y ago
      [deleted]
    - PedroBatista 3y ago
      Here: https://bugs.java.com/bugdatabase/view_bug?bug_id=JDK-8311906 https://bugs.java.com/bugdatabase/view_bug?bug_id=JDK-831190... It was accepted as a bug. I'm not saying I would condemn anyone to the death penalty based on that but being accepted as a bug there has to have a significant weight attached to it.
      - ris58h 3y ago
        > It was accepted as a bug. I know. It was in this thread above. So there is no link to the specification to proof that it's a bug.
marginalia_nu 3y ago
Dunno, this seems like you're violating the java memory model and then then fully expected weird shit happens. If you're mutating a shared state between threads without proper synchronization, and there is no way for String to do this on its own, the CPU is permitted to do stuff like out-of-order execution as there is no happens-before relationship between the write and the read. The JVM is further permitted to optimize the code with the assumption that the data stays the same in the absence of volatile/synchronized/other synchronziation primitives, as long as the observable outcome is the same "as if serial" execution. The understanding 'It first tries to encode the characters as latin-1 using StringUTF16.compress. If that fails, it returns null and the constructor falls back to using UTF-16. ' is incorrect in unsynchronized code. The reality is that it's permitted to do all these things at the same time in any order (or speculatively do both at the same time) as long as the observable consequences are the same. This relies on the assumption that the data stays the same. If you violate that assumption, you get bizarre and often unpredictable behavior. I don't think this is a bug in String. It may be a security vulnerability in the JVM, in a log4j-esque "it's working as we intended but holy crap did we ever not intend on this interaction" manner.
- haspok 3y ago
  Something like a ConcurrentModificationException would be nice to have in the "weird shit happens" case, because you may not even be aware of it, and then it might cause headache much further down the line. If you could point upstream that would save a lot of headache and grey hair...
  - marginalia_nu 3y ago
    There's no way to reliably detect this type of concurrent modification that doesn't involve introducing expensive barrier operations that evict the CPU cache on multiple points along the code flow and choke out-of-order execution and limit what the JIT can do. If you do not have such a barrier, the CPU is permitted to assume the data hasn't been changed, and may cache it, making detection of concurrent modification impossible. The JIT will also optimize your code based on this assumption. Even if you go out of your way to check that the data is the same you may see stale data when you check! It may run your code out of order, and even JIT-recompile it out of order, and your code that's like int oldValue = val; doSomething(val); if (val != oldValue) throw new ConcurrentModificationException(); may be re-recompiled like this, since it looks like doSomething can't modify val or oldValue int oldValue = val; if (val != oldValue) throw new ConcurrentModificationException(); doSomething(val); and then since this if check is trivially always unnecessary, and nothing of consequence ever reads oldValue it may delete them entirely doSomething(val); Introducing a barrier would likely make most Java programs 10x slower 'cause Java does a lot of throwaway string allocation on the assumption it's cheap and short-lived GC is virtually free. The Java Memory Model is a great accomplishment and if you heed it you will write performant multi-threaded applications with an ease that is rare in other languages. If on the other hand you do not heed the JMM, you will at best get a CME alerting you of this case, but in most cases just get holes in your feet.
shultays 3y ago
char[] is not an immutable class. Thread safety issue happens on that type, not the fault of String type. It is weird Java considered it a bug, I expect it to be resolved as "wont do"
- moring 3y ago
  Thread safety of char[] is the cause of the bug, but the effect is that String instances can violate their invariants, which they shouldn't even in case of thread-unsafe code.
  - pshirshov 3y ago
    In my opinion the abstraction is leaky but everything works according to the spec. I might tell you about another "bug" and "invariant violation" which is possible but is not a bug. Try to use Sets or Maps with keys having broken hashcode.
    - orra 3y ago
      I can't agree with this analogy. This is not a defective user implementation of hashcode; it is a defective platform implementation of equals (and starts with, etc.).
      - pshirshov 3y ago
        This is a broken user code written without any JMM understanding.
      - thfuran 3y ago
        A proper implementation of hashcode can return nonsense if you mutate the object during hashing.
        orra 3y ago
        Sure, but here we're not mutating the object during the String.startsWith() call. Hence I'd say it's a TOCTOU bug in String.valueOf().
pshirshov 3y ago
In my opinion this is not a bug and there is no general fix for that without significant overhaul of the whole runtime and standard library. In my opinion the author might need to read the vm and memory model spec and understand that such things are expected in Java. There are real bugs in standard library singletons (e.g. concurrent Filesystem calls might fail during singleton initialization, while plugins are loaded). These bugs are being ignored for years. The author must not expect any "fix" for this in foreseeable future, but they might expect being laughed at.
- marginalia_nu 3y ago
  Eh, I don't think the author is to be ridiculed. We're all wrong from time to time, I'm wrong in most of what I say, I'm sure you've been wrong on occasions, this is fine. The author's views on how concurrency works is very common and taught in schools and textbooks, but also quite inadequate. Instead let's take this opportunity to talk and teach about the JMM and deepen the collective understanding of the many unintuitive behaviors of multi-threaded applications.
  - haspok 3y ago
    Maybe there could be a JMM 2.0 where at least some of the unexpected behaviour is removed. Maybe even at the cost of performance. If something is misunderstood by >99% of the average programmer population then clearly it is not the best solution to the problem.
    - marginalia_nu 3y ago
      If that many programmers don't understand the JMM, it's because that many programmers haven't looked at it. It is the key to reasoning about Java concurrency. Any other model that is not the JMM is flat out wrong. It is virtually impossible to write correct concurrent Java without understanding the memory model.
- barrkel 3y ago
  A defensive copy is the cheap way to fix this. The bug is a potential security vulnerability since it can affect the value of interned strings, which can break other libraries.
Genbox 3y ago
Immutability and thread-safety are two different things. There are no thread-safety guarantees made by the JVM or the runtime for the String type, and I suspect your bug report will get the same answer. You are right that there is an expecation or assumption of such (from the users), which makes it even more interesting when it breaks. Locking a thread for every allocation of a string is very likely cost-prohibitive and not a great trade-off for the common case. It would make more sense to add something like StringFactory.CreateString(char[]) and instruct developers in using it if thread safety is nessecary.

by coekie 3y ago

haspok 3y ago

How would you fix this? Possibly by copying the input array to a local array as the first step? But then you might copy a lot of data, which is in 99% of the cases totally unnecessary. Have a separate threadsafe and a non-threadsafe constructor for char[]? EDIT: another idea is to do a hash of the input array, and compare between start and end. But that also requires an O(n) walk through the input...

silon42 3y ago
/s redesign a language to prevent parallel mutable use of the array
- pshirshov 3y ago
  This won't happen (unfortunately) for obvious reasons.
tpxl 3y ago
> But that also requires an O(n) walk through the input... Compressing requires an O(n) walk through the input either way.
- orra 3y ago
  Yes. In fact, both String.valueOf(char[]) and constructor String(char[]) are already documented as copying the array contents.
the_mitsuhiko 3y ago
I'm not sure how much of the underlying implementation details of arrays are leaking into JNI, but one way to approach it would be with first class copy-on-write for arrays or opt-in immutability (freezing).
barrkel 3y ago
Copy the array, then use alias analysis to remove the copy most of the time, by having two versions of the String constructor, with a version chosen based on aliasing.
masklinn 3y ago
> How would you fix this? Possibly by copying the input array to a local array as the first step? Upon encountering a non-Latin1 char, convert the existing latin1 array to an utf16 one, append the char (converted), then resume iteration in utf16 copy mode. This way you know the char which made you bail out on Latin1 is part of the output.

ris58h 3y ago

Accepting an issue doesn't mean that there is a bug. > While the javadoc doesn't state it explicitly, immutable classes in the core library are expected to be thread safe. If it's not documented it's just an assumption.

PedroBatista 3y ago
Yes, but at some point one has come out of the theory domain and face the real world. So in practical terms it’s closer to a bug and even the “official” guys from Java accepted it as a bug.
- ris58h 3y ago
  Could you provide a link to the documentation to proof it's a bug? The author could call it 'probably bug' or 'misleading String constructor documentation' but instead they state it's a bug without any proofs.
  - deleted 3y ago
    [deleted]
  - PedroBatista 3y ago
    Here: https://bugs.java.com/bugdatabase/view_bug?bug_id=JDK-8311906 https://bugs.java.com/bugdatabase/view_bug?bug_id=JDK-831190... It was accepted as a bug. I'm not saying I would condemn anyone to the death penalty based on that but being accepted as a bug there has to have a significant weight attached to it.
    - ris58h 3y ago
      > It was accepted as a bug. I know. It was in this thread above. So there is no link to the specification to proof that it's a bug.

marginalia_nu 3y ago

Dunno, this seems like you're violating the java memory model and then then fully expected weird shit happens. If you're mutating a shared state between threads without proper synchronization, and there is no way for String to do this on its own, the CPU is permitted to do stuff like out-of-order execution as there is no happens-before relationship between the write and the read. The JVM is further permitted to optimize the code with the assumption that the data stays the same in the absence of volatile/synchronized/other synchronziation primitives, as long as the observable outcome is the same "as if serial" execution. The understanding 'It first tries to encode the characters as latin-1 using StringUTF16.compress. If that fails, it returns null and the constructor falls back to using UTF-16. ' is incorrect in unsynchronized code. The reality is that it's permitted to do all these things at the same time in any order (or speculatively do both at the same time) as long as the observable consequences are the same. This relies on the assumption that the data stays the same. If you violate that assumption, you get bizarre and often unpredictable behavior. I don't think this is a bug in String. It may be a security vulnerability in the JVM, in a log4j-esque "it's working as we intended but holy crap did we ever not intend on this interaction" manner.

haspok 3y ago
Something like a ConcurrentModificationException would be nice to have in the "weird shit happens" case, because you may not even be aware of it, and then it might cause headache much further down the line. If you could point upstream that would save a lot of headache and grey hair...
- marginalia_nu 3y ago
  There's no way to reliably detect this type of concurrent modification that doesn't involve introducing expensive barrier operations that evict the CPU cache on multiple points along the code flow and choke out-of-order execution and limit what the JIT can do. If you do not have such a barrier, the CPU is permitted to assume the data hasn't been changed, and may cache it, making detection of concurrent modification impossible. The JIT will also optimize your code based on this assumption. Even if you go out of your way to check that the data is the same you may see stale data when you check! It may run your code out of order, and even JIT-recompile it out of order, and your code that's like int oldValue = val; doSomething(val); if (val != oldValue) throw new ConcurrentModificationException(); may be re-recompiled like this, since it looks like doSomething can't modify val or oldValue int oldValue = val; if (val != oldValue) throw new ConcurrentModificationException(); doSomething(val); and then since this if check is trivially always unnecessary, and nothing of consequence ever reads oldValue it may delete them entirely doSomething(val); Introducing a barrier would likely make most Java programs 10x slower 'cause Java does a lot of throwaway string allocation on the assumption it's cheap and short-lived GC is virtually free. The Java Memory Model is a great accomplishment and if you heed it you will write performant multi-threaded applications with an ease that is rare in other languages. If on the other hand you do not heed the JMM, you will at best get a CME alerting you of this case, but in most cases just get holes in your feet.

shultays 3y ago

char[] is not an immutable class. Thread safety issue happens on that type, not the fault of String type. It is weird Java considered it a bug, I expect it to be resolved as "wont do"

moring 3y ago
Thread safety of char[] is the cause of the bug, but the effect is that String instances can violate their invariants, which they shouldn't even in case of thread-unsafe code.
- pshirshov 3y ago
  In my opinion the abstraction is leaky but everything works according to the spec. I might tell you about another "bug" and "invariant violation" which is possible but is not a bug. Try to use Sets or Maps with keys having broken hashcode.
  - orra 3y ago
    I can't agree with this analogy. This is not a defective user implementation of hashcode; it is a defective platform implementation of equals (and starts with, etc.).
    - pshirshov 3y ago
      This is a broken user code written without any JMM understanding.
    - thfuran 3y ago
      A proper implementation of hashcode can return nonsense if you mutate the object during hashing.
      - orra 3y ago
        Sure, but here we're not mutating the object during the String.startsWith() call. Hence I'd say it's a TOCTOU bug in String.valueOf().

pshirshov 3y ago

In my opinion this is not a bug and there is no general fix for that without significant overhaul of the whole runtime and standard library. In my opinion the author might need to read the vm and memory model spec and understand that such things are expected in Java. There are real bugs in standard library singletons (e.g. concurrent Filesystem calls might fail during singleton initialization, while plugins are loaded). These bugs are being ignored for years. The author must not expect any "fix" for this in foreseeable future, but they might expect being laughed at.

marginalia_nu 3y ago
Eh, I don't think the author is to be ridiculed. We're all wrong from time to time, I'm wrong in most of what I say, I'm sure you've been wrong on occasions, this is fine. The author's views on how concurrency works is very common and taught in schools and textbooks, but also quite inadequate. Instead let's take this opportunity to talk and teach about the JMM and deepen the collective understanding of the many unintuitive behaviors of multi-threaded applications.
- haspok 3y ago
  Maybe there could be a JMM 2.0 where at least some of the unexpected behaviour is removed. Maybe even at the cost of performance. If something is misunderstood by >99% of the average programmer population then clearly it is not the best solution to the problem.
  - marginalia_nu 3y ago
    If that many programmers don't understand the JMM, it's because that many programmers haven't looked at it. It is the key to reasoning about Java concurrency. Any other model that is not the JMM is flat out wrong. It is virtually impossible to write correct concurrent Java without understanding the memory model.
barrkel 3y ago
A defensive copy is the cheap way to fix this. The bug is a potential security vulnerability since it can affect the value of interned strings, which can break other libraries.

Genbox 3y ago

Immutability and thread-safety are two different things. There are no thread-safety guarantees made by the JVM or the runtime for the String type, and I suspect your bug report will get the same answer. You are right that there is an expecation or assumption of such (from the users), which makes it even more interesting when it breaks. Locking a thread for every allocation of a string is very likely cost-prohibitive and not a great trade-off for the common case. It would make more sense to add something like StringFactory.CreateString(char[]) and instruct developers in using it if thread safety is nessecary.