Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Breaking java.lang.String

wouter.coekaerts.be

Post reply on HN
Print view View original

151–160 of 206 posts

Re: Breaking java.lang.String

#151

Post by ris58h »

layer8 wrote: post #128
ris58h wrote: post #87

Earlier quoted context omitted.

> A successfully constructed object should obey its documented interface contract, period. Could you provide the contract you are talking about?

Yes, the contract of String::equals: The result is true if and only if the argument is not null and is a String object that represents the same sequence of characters as this object. The article constructs two String objects representing the same character sequence, for which however equals() returns false, in violation of the above-quoted contract.

So they should add that mutating arguments during object construction may lead to not-so-successfully constructed (invalid) object. __Mission failed successfully__

Re: Breaking java.lang.String

#152

Post by dundarious »

layer8 wrote: post #128
ris58h wrote: post #87

Earlier quoted context omitted.

> A successfully constructed object should obey its documented interface contract, period. Could you provide the contract you are talking about?

Yes, the contract of String::equals: The result is true if and only if the argument is not null and is a String object that represents the same sequence of characters as this object. The article constructs two String objects representing the same character sequence, for which however equals() returns false, in violation of the above-quoted contract.

Functions have an assumed pre-requirement not explicitly spelled out in every single javadoc, that the program does not have a data race. May as well ask for every javadoc to explicitly include a pre-requirement that no cosmic rays flip bits of memory.

But to be serious again, I'm sure if you look at Array or whatever type is involved in char[], you'll find that it is explicitly marked as not thread-safe.

Re: Breaking java.lang.String

#153

Post by jjnoakes »

pjmlp wrote: post #51
jjnoakes wrote: post #6

Earlier quoted context omitted.

Right, but in rust, not using one is a compile time error. In Java (as you can see by the article), not using one is a silent bug at runtime.

Only for in-memory data structures under Rust's control, if it is related to OS IPC, Rust cannot do anything.

This isn't true.

Re: Breaking java.lang.String

#154

Post by brohee »

jillesvangurp wrote: post #40
gizmo686 wrote: post #14

Is this actually a bug? The default assumption in Java is that types are not thread-safe unless otherwise specified. Attempting to use types in a way that exceeds their documented thread safety has always been allowed to leave your program in an inconsistent state.

Not really. I'm sure there are some edge cases where the ability for someone to do silly things like this is harmful but otherwise this goes in category of things where the general advice is "just don't do that and you're fine". Basically, if you look at why the standard library would not go out of its way to prevent you from doing stuff like this it boils down to the same reason as why they put the string compressio…

How do you plan to have any assurance that none of your dependencies is doing it? You could well consume a string you didn't build and be affected.

Re: Breaking java.lang.String

#155

Post by tialaramex »

thfuran wrote: post #143
tialaramex wrote: post #72

Earlier quoted context omitted.

As you would expect in a vaguely modern language, Java's "List" interface is typically backed by a growable array as the type ArrayList. Only people who have no idea about caches would think List should necessarily be some sort of Linked List type. You would use Collections.unmodifiableList to make it unmodifiable.

Yes, ArrayLists are array-backed, but Arrays.asList instantiates a List and does not instantiate an array.

As I explained, List is an interface. You can't instantiate it, values are instances of types not interfaces.

All that return type is telling you is, surprise, asList promises what you're getting implements the List interface. See if you can guess how you can implement the List interface using an Array. Still struggling, here's the one line of a typical implementation:

return new Arrays.ArrayList(a);

Re: Breaking java.lang.String

#156

Post by dundarious »

hashmash wrote: post #32
gizmo686 wrote: post #29

Earlier quoted context omitted.

At best it is only a security vulnerability if you are running different trust domains within your process. That is something Java supports, but most Java code does not attempt to take advantage of it. Most Java programs have 100% of there code being allowed to use reflection to simply hack the program however they please, including corrupting the intern pool. But still, for the few people who actually trust Java's i…

If a security sensitive operation was (for whatever reason) calling startsWith (as shown in the blog), then it might return an incorrect result. I'm merely speculating that an exploit is possible, and I'm not aware of any code that would be affected. Regardless, the fix is quite simple. The public String.intern method just needs to validate the encoding first. This will have little impact on performance because the S…

Their point, which I agree with, is that shared memory concurrency requires correct code, and cooperative synchronization between threads, in order to avoid data races; and if your security model is to run untrusted or unverified code with shared memory concurrency, then you cannot ensure correct code or synchronization, and you cannot guarantee against data races.

It's perfectly fine to code String.intern() defensively against such dodgy String-s, but in any sizable program with shared memory concurrency and untrusted or unverified code, there will be millions of other potential data races. The only sensible choice would be to not use such a mix.

Re: Breaking java.lang.String

#157

Post by nayuki »

It is possible to fix this String constructor implementation without creating a defensive copy of the input array or having a TOCTOU vulnerability.

    // Change this implementation to a loop.
    public String(char[] value) {
        while (true) {
            byte[] temp = StringUTF16.compress(value);
            if (temp != null) {
                this.value = temp;
                this.coder = LATIN1;
                break;
            }
            
            temp = StringUTF16.toBytes(value);
            if (temp != null) {
                this.value = temp;
                this.coder = UTF16;
                break;
            }
        }
    }

    // This implementation stays the same.
    static byte[] StringUTF16.compress(char[] value) { ... }

    // Change this contract and implementation so that it returns null
    // if all characters are below 256, otherwise it returns byte[].
    // The difference is that previously, this function would never return null.
    // Now, we make sure that the function succeeds if and only if the
    // char array *requires* UTF-16 as opposed to Latin-1.
    static byte[] StringUTF16.compress(char[] value) { ... }

Re: Breaking java.lang.String

#158

Post by orra »

marginalia_nu wrote: post #145
dtech wrote: post #134

Earlier quoted context omitted.

afaik the person you replied to is wrong, using System.arrayCopy would definitely make a copy and not be optimized away. Maybe poster meant only copying the reference with `char[] copy = original` which indeed does nothing, but that's not a defensive copy.

This copy has no external observable effects (it's only temporarily used in the constructor and never stored anywhere), and since the JVM is (under the JMM) permitted work under the assumption the original array doesn't change unless the thread itself changes it or there is some happens-before relationship with something that does, and is permitted to re-order same-thread execution as-if-serial, it's fully permitted…

> In general arrays and concurrency doesn't mix very well in Java, and is something I'd avoid mixing on principle.

Sound sensible. I think what you've said is any semi-malicious code can give you a primitive array, then modify it from under you in another thread. That's not completely surprising but the clincher is, you can't even take a defensive copy.

Re: Breaking java.lang.String

#159

Post by dundarious »

nayuki wrote: post #157

It is possible to fix this String constructor implementation without creating a defensive copy of the input array or having a TOCTOU vulnerability. // Change this implementation to a loop. public String(char[] value) { while (true) { byte[] temp = StringUTF16.compress(value); if (temp != null) { this.value = temp; this.coder = LATIN1; break; } temp = StringUTF16.toBytes(value); if (temp != null) { this.value = temp;…

An unconditional loop with no guarantee of forward progress may loop indefinitely, and hence, is not a sensible solution to the problem.

Re: Breaking java.lang.String

#160

Post by marginalia_nu »

orra wrote: post #158
marginalia_nu wrote: post #145

Earlier quoted context omitted.

This copy has no external observable effects (it's only temporarily used in the constructor and never stored anywhere), and since the JVM is (under the JMM) permitted work under the assumption the original array doesn't change unless the thread itself changes it or there is some happens-before relationship with something that does, and is permitted to re-order same-thread execution as-if-serial, it's fully permitted…

> In general arrays and concurrency doesn't mix very well in Java, and is something I'd avoid mixing on principle. Sound sensible. I think what you've said is any semi-malicious code can give you a primitive array, then modify it from under you in another thread. That's not completely surprising but the clincher is, you can't even take a defensive copy.

Well I mean there are things you can do prevent the JVM from this particular optimization. But you need to use the right tools to tell Java to expect spooky action at a distance, and arraycopy just isn't sufficient.

This would probably do

  byte[] b;

  // A
  synchronized (a) { 
    b = System.arraycopy(a);
  }
  // B
Here A happens-before B, and you can assume a read fence at the start of synchronized block and a store fence at end of it, so consumers of b should get a consistent view of the array, and the synchronized block tells Java that you expect a to be modified by another thread so it can't assume it stays the same throughout the execution; and elision of the arraycopy isn't possible; so while the array may be in a weird state, it will at least stay consistent throughout the execution.
Post reply on HN