Java's TypeVariable.getBounds is not thread safe | Hacker News Reader

Java's TypeVariable.getBounds is not thread safe

vmlens.com

73 pointsbased223 comments

If this is a bug, it's a bug in the array cloning logic. It's easy to publish a reference to an incomplete data structure without a memory barrier before the assignment that publishes it - though x86/x64 platforms will save you by retiring writes in order [1]. You don't need to use the JDK to replicate this logic; you could write this code yourself. If it crashes the JVM, it's probably a violation of memory safety - it's not inconceivable that there's an exploit here, but at the very least it's a DoS vector for untrusted bytecode.

[1] See https://bartoszmilewski.com/2008/11/05/who-ordered-memory-fe... for a readable discussion about what x86/x64 does and doesn't do.

Why did you expect it was threadsafe? I can find nothing in its javadoc that would lead me to believe it was. Most of the Java standard library isn't threadsafe, that why there are concurrent objects in addition to their non-threadsafe cousins and language level synchronization statements. https://docs.oracle.com/javase/8/docs/api/java/lang/reflect/...

You can't avoid it. The java.lang.Class getTypeParameters() method will give you back the same instance.

    System.out.println(System.identityHashCode(java.util.List.class.getTypeParameters()[0]));
    System.out.println(System.identityHashCode(java.util.List.class.getTypeParameters()[0]));

    2018699554
    2018699554

It seems there's sort of an unwritten standard that static methods should be thread safe or avoid shared state :/

Another great failure is UUID.generateRandomUUID(); One would think that's thread safe... [maybe it's fixed now]

From oracle docs:

"A class that represents an immutable universally unique identifier (UUID). A UUID represents a 128-bit value."

Looks thread safe to me. No setters on the object.

Go look at the implementation...

http://grepcode.com/file/repository.grepcode.com/java/root/j...

I hope this has been fixed in subsequent releases!

SecureRandom is thread safe, and the global initialization is volatile. What am I missing?

The `volatile_var = local_var = X` seems safe - even if you break it into two assignments it only uses the local var afterward, so SecureRandom should be fully initialized in that thread. And though SecureRandom is non-final, it does look safe to use with double-initialization - it self-seeds if not explicitly seeded (it's not in this case), so even double-init shouldn't repeat a value (which is as strong of a statement as SecureRandom allows here - no idea if it makes that claim!).

So I'd think it only matters if `volatile` doesn't guarantee that non-final object fields are fully initialized... and I can't find anything that explicitly states one way or another, so I'm not sure. If it doesn't make that guarantee, then yeah - this could publish a partly-initialized SecureRandom without a generator, which would probably crash. But the contents of https://docs.oracle.com/javase/specs/jls/se8/html/jls-17.htm... imply to me that write-then-read is equivalent to a synchronized block or any other monitor sequence (and in the method they only use the local var, so it doesn't matter for the creator-thread), so I suspect it's fine. Just can't claim any further.

@exabrial: care to elaborate? What's unsafe / have you seen crashes due to uninitialized SecureRandom?

Keep in mind that data races in Java are not undefined behaviour. There are quite a few places where authors of standard library deliberately left out synchronization, for example in String.hashCode.

EDIT: In this case numberGenerator is volatile so it introduces happens-before relations between write and read.

Before you downvote me, make sure your reasoning is correct. I am taking about official jdk from Orale, not open jdk. See, this was fixed http://bugs.java.com/view_bug.do?bug_id=6611830

6b14 is actually damn old. The Java 6 VM is not supported anymore and in fact on the latest 7 or 8 it's definitv fixed for OpenJDK and Oracle JDK.

They basically have to be thread safe, because they're globally accessible. If you can't control access to a call that isn't thread safe, then you can't call it safely at all if there's any code in your process that you don't control (like, say, third-party libraries, or even first-party libraries).

I think it's reasonable to assume that any documented API must at least be callable.

Indeed it must be so -- at least it would be very strange to have a thread-unsafe static method where the documentation doesn't spell out how to use it safely[1]. Even in that case it would be a weird thing to even have a thread-unsafe static method (except accidentally, of course).

[1] The JDK documentation is pretty good on spelling out such caveats, IME.

Even code which isn't "threadsafe" shouldn't crash the VM.

Exactly, especially so because that would totally break sandboxing. Granted, applets are way beyond their best-before date, but there may be other reasons a SecurityManager sandbox is in place, perhaps for isolating multiple .wars from different sources, or maybe someone even wrote a sandbox for executing user-supplied code.

Java's raison d'être was arguably the byte code verifier. Being able to crash the JVM with validated byte code is always a bug.

Hm. Are other things in the reflect package threadsafe? By default I wouldn't expect it to be since it doesn't seem to claim it is, but it is a little surprising that a language-introspection feature could just crash because you're doing it at the same time as someone else. And it seems hard to guard against - you could carefully lock on the class for your own code, but you can't really guarantee libraries will work with that (e.g. they may get into deadlocks).

this crash is kind of interesting because i don't see any 'unsafe' code* in this getBounds() implementation so it shouldn't crash the JDK. like the code is broken in how it works but it shouldn't be possible to muck around with threading a race and then call clone and crash the JDK.

* the code is very abstract and touches a lot of classes so there could be some unsafe code hiding there i can't see.

Indeed, the memory model on ARM does not provide the same guarantees as x86. Given the behaviour described, it seems likely one could weaponise this kind of race condition, on ARM.

It seems to me that the main cause of the crash is the fact that the author used a special [classloader](https://github.com/vmlens/jcstress-examples/blob/master/src/...) which always reloads the class and is not thread-safe intentionally (classloaders should be Thread-safe). I did not investigate in depth but that seems to be the main problem to me.

My favorite non-thread-safe part of Java is this:

    SimpleDateFormat YMD = new SimpleDateFormat("yyyy-MM-dd");

If you call YMD.parse() from multiple threads, it will sometimes return the wrong date. Cleaning up the database after that bug was loads of fun.

This is documented properly though.

The assumption "threadsafe until proven otberwise" has bitten so many people, you'd think they would eventually learn.

Oh wait, they're Java programmers. ;p

Reply on news.ycombinator.com