Skip to content

Use ThreadLocalRandom when available for StringHelpers #1332

Merged
merged 1 commit into from Oct 11, 2012

3 participants

@Shadowfiend
Lift Web Framework member

StringHelpers.randomString uses a SecureRandom instance. Because of SecureRandom's thread safety assurances, this results in locking every time a random number is computed. Java 7 has a version of SecureRandom that automatically provides a thread-local instance, called ThreadLocalRandom. We should use it when it's available (i.e., on Java 7) and fall back on SecureRandom otherwise.

Ideally, we would also make the randomness swappable here so that the user can provide their own implementation if needed (e.g., if they want to stay on Java 6 and still use something thread-local).

Original discussion at https://groups.google.com/forum/?fromgroups=#!topic/liftweb/Bbhk-L5wXIc .

@Shadowfiend Shadowfiend Use ThreadLocalRandom in StringHelpers.randomString on Java 7.
On Java <7, we use the default SecureRandom, which locks on access.
ThreadLocalRandom will not lock as it is thread-local (!).
f2de993
@dpp dpp merged commit 823c96b into master Oct 11, 2012
@sebols
sebols commented Feb 21, 2013

I happened to see this pull request when googling on some Java concurrency classes.

I don't know much about Lift, so sorry if I am out of line here, but I hope you made sure that ThreadLocalRandom
provides sufficiently random numbers for whatever StringHelpers.randomString() is used for.

First of all I actually doubt that jvm locking was what slowed down the method. Depending on the secure random provider in use, nextbytes() might lock for other reasons. It can lock while waiting for entropy from /dev/random on Linux. This can cause significant delays when your system does not have sufficient entropy. Entropy for /dev/random will be collected from things such as network traffic, mouse movement, disk access etc. If securerandom can block or not depends on the underlying provider, which depends on the VM and OS.

Secondly, threadLocalRandom has the same horrible random attributes as Random. This means that you can calculate the seed and all subsequent "random" numbers from two subsequent outputs of the method. This means that this method can be used where you want random data that should look random (such as game event), but never ever when it needs to be cryptographically secure such as for XSS protection. Not that I think that this is how it is used, but some production code might assume that the method is secure. It used to be secure, but any application using StringHelpers.randomString() after this change is compromisable.

I just hope that people don't use nextFuncname or StringHelpers.randomString() for anything of security importance after this change. I guess there is a risk that webapps use these for random tokens. Even if the old implementation did not guarantee unpredictability it did provide it at least.

@dpp Sorry if fhis is the wrong forum. I just wanted to make sure that you guys are aware of this.

@Shadowfiend
Lift Web Framework member

Huh. It was my distinct impression when I first investigated this issue that ThreadLocalRandom used SecureRandom, but looking around again makes it clear that is not the case at all. Given that, I suggest we back this change out; if we choose to do that, I'll make a post on the original list thread indicating the fact that we've done it and why.

@sebols
sebols commented Feb 21, 2013

Ok. Scala and Lift are not my specialities, but I guess the SecureRandom implementation could be reverted to and a separate method such as StringHelpers.randomUnsecureString() could be implemented and used whenever something random looking, but not necessarily cryptographically secure is needed.
But it makes more sense to just use Random in the unsecure choice or at use ThreadLocalRandom on Java 7+ and Random on the older ones as an optimization.
I doubt that the lock contention was the slow part of the securerandom use. The slow part is generating cryptographically secure random numbers.

@Shadowfiend
Lift Web Framework member

As far as I know, randomString's most important usage is in nextFuncName, where it does need to be cryptographically secure. I don't think there's a pressing need for a separate unsecure version. It was the nextFuncName invocations that were blocking multiple threads in our app.

@dpp
Lift Web Framework member
dpp commented Feb 21, 2013

In general, this kind of discussion should be had on the Lift mailing list, not on the ticket.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.