4 ms·

Out of interest, how should this be handled? Is this a bug in Java which should be fixed (looks like that to me)? My understanding was Java generally doesn't d

Out of interest, how should this be handled? Is this a bug in Java which should be fixed (looks like that to me)? My understanding was Java generally doesn't do "you did an undefined behaviour, so it's your fault", except for specifically marked very low-level interfaces.

thfuran 3y ago
Java definitely does "you wrote thread dangerous code, so it's your fault" for APIs not marked as being thread safe.
- ccooffee 3y ago
  This is yet another way that running untrusted code inside the same JVM is a terrible mess. There's a lot of JVM state that gets "locked in" on first use (e.g. <clinit>) and a malicious bit of code could corrupt a LOT of shared data (like the post's mentioned string internment zone) even if you sanitize all of your inputs and outputs. I wonder if you could do something nasty with this bug from inside an IntelliJ plugin...
- kaba0 3y ago
  For what it's worth, Java does at least give some guarantees in case of data races -- the observed value will always be one that was explicitly set by one thread. This is different from most other languages, e.g. in C,C++, unsafe Rust it is UB. Of course it can and still will result in invalid states.
  - tialaramex 3y ago
    > Of course it can and still will result in invalid states. While of course they can't stop you from creating "invalid" states of your own types, whether through a data race or just bad coding - Java's own types should not have invalid states which can come into existence this way. For example, suppose we've got a Goose type, and it can be Happy or Sad, and when it's Sad it has a Reason, when it is Happy there's no Reason. We, as Java, should not design this type so that it's possible for it to get flipped from Happy to Sad without choosing a Reason. As a result, after a race the Goose might be Happy when you expected Sad, or vice versa, but it can't enter the invalid state where it's Sad but for no Reason.
  - thfuran 3y ago
    If you read a double or long while writing to it from another thread, you are not guaranteed to read a value that was ever explicitly set because they update 32 bits at a time rather than atomically.
    - kaba0 3y ago
      As per the specification, you are right. As per the actual implementation, no :D it is as cheap, if not cheaper to set it atomically on 64-bit.
      - deleted 3y ago
        [deleted]
cempaka 3y ago
I can only think of a couple of ways to fix this, none of them ideal from a performance perspective: - Make a defensive copy of the passed-in character array, which would be immediately discarded when it is encoded to bytes. This basically sucks given how often String creation happens in a typical Java program. - Dispense with the whole use of the coder to check for non-equality of Strings, and insist on a character-by-character comparison (possibly from different encodings) for every call to String.equals() where the lengths are the same. Again, this would be a lot of wasted cycles in an extremely common JVM op. I think the right answer here is to add something to the Javadoc about "modifying the input character array concurrently with String construction invites dragons." :-)
- CJefferson 3y ago
  Another way of fixing it would be, whichever character caused 'StringUTF16.compress' to fail (we'd need to return it), make sure that character is kept in the final string (could just assign it at the end, remember it's value and location earlier). By merging in the code of StringUTF16.compress and StringUTF16.toBytes into one function (and they are both very small), this wouldn't even have to slow things down, once you noticed location 'i' has char 'c' which isn't LATIN1, you make the new buffer, copy 0..i-1 over, then your known non-LATIN1 'i', then i+1..len. This might be very slightly slower, but would fix (I think) the problem of breaking interning, which feels quite painful.
  - marvy 3y ago
    Clever! I like this! I feel a bit jealous that I didn't think of it :) It's not quite as easy as it seems, because these methods are "intrinsics". That is, the pure Java code you see is not always the one that gets used; instead, the JIT compiler can use a faster implementation that uses vectorized assembly or whatever. (That's why you see "@IntrinsicCandidate" on compress() and toBytes() in StringUTF16.java) But I think your idea would also be possible to implement in vectorized assembly, so it still works! The rule for most things (such as ArrayList) in the JDK is: "if you use race conditions to break this thing, that's your problem, not ours". But String is different: it's one of the few things meant to be "rock-solid, can't break it even if you try", so I think this bug in String would qualify as a potential security issue in their mind: there are many places in Java that trust Strings not to act weird, and some of them are even in native code deep in the guts of the VM. On the other hand, String is used all over the place so having to introduce a performance regression to fix this bug would be quite painful. All of the other proposed solutions in this thread introduce an extra copy, and an extra pass over the string. Your fix is basically no extra cost, and as a bonus, can be tweaked so each char in the array is read only once. Which means that the bug can now be fixed "guilt-free", if anyone from the JDK team is reading this thread. Though they have some pretty clever people there too, so they might have thought of it eventually for all I know.
vbezhenar 3y ago
If I would design JVM from the scratch, I'd introduce frozen arrays. There are so many array copies inside JDK code, it hurts. Whether it's possible to retrofit JVM today, I don't know.
- kaba0 3y ago
  Not sure if you are referencing this, but there were a proposal: https://openjdk.org/jeps/8261007 https://openjdk.org/jeps/8261007

thfuran 3y ago

Java definitely does "you wrote thread dangerous code, so it's your fault" for APIs not marked as being thread safe.

ccooffee 3y ago
This is yet another way that running untrusted code inside the same JVM is a terrible mess. There's a lot of JVM state that gets "locked in" on first use (e.g. <clinit>) and a malicious bit of code could corrupt a LOT of shared data (like the post's mentioned string internment zone) even if you sanitize all of your inputs and outputs. I wonder if you could do something nasty with this bug from inside an IntelliJ plugin...
kaba0 3y ago
For what it's worth, Java does at least give some guarantees in case of data races -- the observed value will always be one that was explicitly set by one thread. This is different from most other languages, e.g. in C,C++, unsafe Rust it is UB. Of course it can and still will result in invalid states.
- tialaramex 3y ago
  > Of course it can and still will result in invalid states. While of course they can't stop you from creating "invalid" states of your own types, whether through a data race or just bad coding - Java's own types should not have invalid states which can come into existence this way. For example, suppose we've got a Goose type, and it can be Happy or Sad, and when it's Sad it has a Reason, when it is Happy there's no Reason. We, as Java, should not design this type so that it's possible for it to get flipped from Happy to Sad without choosing a Reason. As a result, after a race the Goose might be Happy when you expected Sad, or vice versa, but it can't enter the invalid state where it's Sad but for no Reason.
- thfuran 3y ago
  If you read a double or long while writing to it from another thread, you are not guaranteed to read a value that was ever explicitly set because they update 32 bits at a time rather than atomically.
  - kaba0 3y ago
    As per the specification, you are right. As per the actual implementation, no :D it is as cheap, if not cheaper to set it atomically on 64-bit.
    - deleted 3y ago
      [deleted]

cempaka 3y ago

I can only think of a couple of ways to fix this, none of them ideal from a performance perspective: - Make a defensive copy of the passed-in character array, which would be immediately discarded when it is encoded to bytes. This basically sucks given how often String creation happens in a typical Java program. - Dispense with the whole use of the coder to check for non-equality of Strings, and insist on a character-by-character comparison (possibly from different encodings) for every call to String.equals() where the lengths are the same. Again, this would be a lot of wasted cycles in an extremely common JVM op. I think the right answer here is to add something to the Javadoc about "modifying the input character array concurrently with String construction invites dragons." :-)

CJefferson 3y ago
Another way of fixing it would be, whichever character caused 'StringUTF16.compress' to fail (we'd need to return it), make sure that character is kept in the final string (could just assign it at the end, remember it's value and location earlier). By merging in the code of StringUTF16.compress and StringUTF16.toBytes into one function (and they are both very small), this wouldn't even have to slow things down, once you noticed location 'i' has char 'c' which isn't LATIN1, you make the new buffer, copy 0..i-1 over, then your known non-LATIN1 'i', then i+1..len. This might be very slightly slower, but would fix (I think) the problem of breaking interning, which feels quite painful.
- marvy 3y ago
  Clever! I like this! I feel a bit jealous that I didn't think of it :) It's not quite as easy as it seems, because these methods are "intrinsics". That is, the pure Java code you see is not always the one that gets used; instead, the JIT compiler can use a faster implementation that uses vectorized assembly or whatever. (That's why you see "@IntrinsicCandidate" on compress() and toBytes() in StringUTF16.java) But I think your idea would also be possible to implement in vectorized assembly, so it still works! The rule for most things (such as ArrayList) in the JDK is: "if you use race conditions to break this thing, that's your problem, not ours". But String is different: it's one of the few things meant to be "rock-solid, can't break it even if you try", so I think this bug in String would qualify as a potential security issue in their mind: there are many places in Java that trust Strings not to act weird, and some of them are even in native code deep in the guts of the VM. On the other hand, String is used all over the place so having to introduce a performance regression to fix this bug would be quite painful. All of the other proposed solutions in this thread introduce an extra copy, and an extra pass over the string. Your fix is basically no extra cost, and as a bonus, can be tweaked so each char in the array is read only once. Which means that the bug can now be fixed "guilt-free", if anyone from the JDK team is reading this thread. Though they have some pretty clever people there too, so they might have thought of it eventually for all I know.

vbezhenar 3y ago

If I would design JVM from the scratch, I'd introduce frozen arrays. There are so many array copies inside JDK code, it hurts. Whether it's possible to retrofit JVM today, I don't know.

kaba0 3y ago
Not sure if you are referencing this, but there were a proposal: https://openjdk.org/jeps/8261007 https://openjdk.org/jeps/8261007