Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Use ThreadLocalRandom when available for StringHelpers #1332

Merged
merged 1 commit into from

3 participants

@Shadowfiend
Owner

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 from
@sebols

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
Owner

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

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
Owner

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
Owner

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
Commits on Oct 11, 2012
  1. @Shadowfiend

    Use ThreadLocalRandom in StringHelpers.randomString on Java 7.

    Shadowfiend authored
    On Java <7, we use the default SecureRandom, which locks on access.
    ThreadLocalRandom will not lock as it is thread-local (!).
This page is out of date. Refresh to see the latest.
Showing with 19 additions and 2 deletions.
  1. +19 −2 core/util/src/main/scala/net/liftweb/util/StringHelpers.scala
View
21 core/util/src/main/scala/net/liftweb/util/StringHelpers.scala
@@ -19,6 +19,7 @@ package util
import java.security.SecureRandom
import java.util.regex._
+import java.util.Random
import java.lang.Character._
import java.lang.{StringBuilder => GoodSB}
import scala.xml.NodeSeq
@@ -32,7 +33,23 @@ object StringHelpers extends StringHelpers
trait StringHelpers {
/** random numbers generator */
- private val _random = new SecureRandom
+ private lazy val _slowRandom = new SecureRandom
+ private lazy val _currentTlrMethod = {
+ try {
+ val tlr = Class.forName("java.util.concurrent.ThreadLocalRandom")
+ Full(tlr.getMethod("current"))
+ } catch {
+ case e =>
+ Failure("ThreadLocalRandom is not available.", Full(e), Empty)
+ }
+ }
+ private def withRng[T](block: (Random)=>T) = {
+ _currentTlrMethod.map { meth =>
+ block(meth.invoke(null).asInstanceOf[Random])
+ } openOr {
+ _slowRandom.synchronized(block(_slowRandom))
+ }
+ }
/**
* If str is surrounded by quotes it return the content between the quotes
@@ -176,7 +193,7 @@ trait StringHelpers {
if (pos >= size) sb
else {
val randNum = if ((pos % 6) == 0) {
- _random.synchronized(_random.nextInt)
+ withRng(_.nextInt)
} else {
lastRand
}
Something went wrong with that request. Please try again.