4

Can I have a single instance of DefaultPasswordService and call its encryptPassword() method without worrying about thread safety issues?

The documentation doesn't make this clear.

pdeva
  • 43,605
  • 46
  • 133
  • 171
  • I ran a search on the API doc and it is interesting that they have explicitly mentioned thread safe 30 times - https://www.google.com/search?q=Thread+safe+site%3Ahttp%3A%2F%2Fshiro.apache.org%2Fstatic%2F1.2.1%2Fapidocs&oe=utf-8&oq=Thread+safe+site%3Ahttp%3A%2F%2Fshiro.apache.org%2Fstatic%2F1.2.1%2Fapidocs&gs_l=heirloom-serp.3...1898.3498.0.3809.12.12.0.0.0.0.62.543.12.12.0.msedr...0...1ac.1.34.heirloom-serp..12.0.0.BvaMfgrbmJQ – Y123 Apr 23 '15 at 22:55
  • I would assume it is thread safe (with caution) since the example in the documentation does not create a new object of the password service before use - http://shiro.apache.org/static/1.2.1/apidocs/org/apache/shiro/authc/credential/DefaultPasswordService.html. – Y123 Apr 23 '15 at 22:55

3 Answers3

2

Yes, you can. You can easily verify this by reading the source code:

public String encryptPassword(Object plaintext) {
  Hash hash = hashPassword(plaintext);
  checkHashFormatDurability();
  return this.hashFormat.format(hash);
}

The worst thing that can happen is that the checkHashFormatDurability method prints a warning multiple times, but the encryptPassword method will always work as expected.

Note however, that the DefaultPasswordService is itself not thread-safe, neither after construction or when calling a setter. Therefore, you need to worry about safe publication of the shared instance before using it in multiple threads.

Rafael Winterhalter
  • 42,759
  • 13
  • 108
  • 192
  • The key point is that the method does not access or update any mutable state apart from thread-confined objects that it creates for itself. – Stephen C Apr 30 '15 at 09:51
1

Its safe. I generated a trace logging all method calls and concurrently accessed fields from the following test class:

public class TestShiro  implements Runnable {

private static final DefaultPasswordService defaultPasswordService = new DefaultPasswordService();

protected void exec() {
/*  calling 
 * defaultPasswordService.setHashFormat(   new Shiro1CryptFormat() ); 
 * will lead to a data race
 */
    defaultPasswordService.encryptPassword( Thread.currentThread().getName());

}

private AtomicInteger threadCount = new AtomicInteger();

public void test() throws Exception
{
    for(int i = 0; i < 8 ;i++)
    {
        Thread thread = new Thread(this, "First Test Thread " + i);
        this.threadCount.incrementAndGet();
        thread.start();
    }


    while( this.threadCount.get() > 0 )
    {
        Thread.sleep(1000);
    }


    Thread.sleep(10 * 1000);
}


public void run()
{
    exec();
    threadCount.decrementAndGet();

}

public static void main(String[] args) throws Exception
{
     (new TestShiro()).test();
}
}

The Trace for one thread looks like this:

com.anarsoft.agent.regression.MultiThreadedOneInstanceTemplate.run
com.anarsoft.agent.regression.TestShiro.exec
org.apache.shiro.authc.credential.DefaultPasswordService.encryptPassword
org.apache.shiro.authc.credential.DefaultPasswordService.hashPassword
org.apache.shiro.authc.credential.DefaultPasswordService.createByteSource
org.apache.shiro.util.ByteSource$Util.bytes
org.apache.shiro.util.ByteSource$Util.isCompatible
org.apache.shiro.util.SimpleByteSource.<init>
org.apache.shiro.codec.CodecSupport.toBytes
org.apache.shiro.codec.CodecSupport.toBytes
org.apache.shiro.authc.credential.DefaultPasswordService.createHashRequest
org.apache.shiro.crypto.hash.SimpleHashRequest.<init>
org.apache.shiro.authc.credential.DefaultPasswordService.hashPassword
    read  from field  org.apache.shiro.authc.credential.DefaultPasswordService.hashService from object 861842890
org.apache.shiro.crypto.hash.SimpleHashRequest.<init>
org.apache.shiro.crypto.hash.DefaultHashService.computeHash
org.apache.shiro.crypto.hash.DefaultHashService.getAlgorithmName
org.apache.shiro.crypto.hash.DefaultHashService.getHashAlgorithmName
    read  from field  org.apache.shiro.crypto.hash.DefaultHashService.algorithmName from object 1033490990
org.apache.shiro.crypto.hash.DefaultHashService.getAlgorithmName
org.apache.shiro.crypto.hash.DefaultHashService.getIterations
org.apache.shiro.crypto.hash.DefaultHashService.getHashIterations
    read  from field  org.apache.shiro.crypto.hash.DefaultHashService.iterations from object 1033490990
org.apache.shiro.crypto.hash.DefaultHashService.getIterations
org.apache.shiro.crypto.hash.DefaultHashService.getPublicSalt
org.apache.shiro.crypto.hash.DefaultHashService.getPrivateSalt
    read  from field  org.apache.shiro.crypto.hash.DefaultHashService.privateSalt from object 1033490990
org.apache.shiro.crypto.hash.DefaultHashService.isGeneratePublicSalt
    read  from field  org.apache.shiro.crypto.hash.DefaultHashService.generatePublicSalt from object 1033490990
org.apache.shiro.crypto.hash.DefaultHashService.getRandomNumberGenerator
    read  from field  org.apache.shiro.crypto.hash.DefaultHashService.rng from object 1033490990
org.apache.shiro.crypto.hash.DefaultHashService.getPublicSalt
org.apache.shiro.crypto.SecureRandomNumberGenerator.nextBytes
org.apache.shiro.crypto.SecureRandomNumberGenerator.getDefaultNextBytesSize
    read  from field  org.apache.shiro.crypto.SecureRandomNumberGenerator.defaultNextBytesSize from object 520016214
org.apache.shiro.crypto.SecureRandomNumberGenerator.nextBytes
org.apache.shiro.crypto.SecureRandomNumberGenerator.nextBytes
org.apache.shiro.crypto.SecureRandomNumberGenerator.nextBytes
    read  from field  org.apache.shiro.crypto.SecureRandomNumberGenerator.secureRandom from object 520016214
org.apache.shiro.crypto.hash.DefaultHashService.getPrivateSalt
    read  from field  org.apache.shiro.crypto.hash.DefaultHashService.privateSalt from object 1033490990
org.apache.shiro.crypto.SecureRandomNumberGenerator.nextBytes
org.apache.shiro.crypto.hash.DefaultHashService.combine
org.apache.shiro.crypto.hash.SimpleHash.<init>
org.apache.shiro.util.StringUtils.hasText
org.apache.shiro.util.StringUtils.hasLength
org.apache.shiro.crypto.hash.SimpleHash.convertSaltToBytes
org.apache.shiro.crypto.hash.SimpleHash.toByteSource
org.apache.shiro.crypto.hash.SimpleHash.convertSourceToBytes
org.apache.shiro.crypto.hash.SimpleHash.toByteSource
org.apache.shiro.crypto.hash.SimpleHash.hash
org.apache.shiro.crypto.hash.SimpleHash.hash
org.apache.shiro.crypto.hash.SimpleHash.getDigest
java.util.HashMap.getNode
    read  from field  java.util.HashMap.table from object 832279283
java.util.HashMap.getNode
    read  from field  java.util.HashMap$Node.hash from object 22805895
java.util.HashMap.getNode
    read  from field  java.util.HashMap$Node.key from object 22805895
java.util.LinkedHashMap.get
    read  from field  java.util.LinkedHashMap.accessOrder from object 832279283
java.util.LinkedHashMap.get
    read  from field  java.util.HashMap$Node.value from object 22805895
java.util.HashMap.getNode
    read  from field  java.util.HashMap.table from object 1924582348
java.util.HashMap.getNode
    read  from field  java.util.HashMap$Node.hash from object 2097514481
java.util.HashMap.getNode
    read  from field  java.util.HashMap$Node.key from object 2097514481
java.util.HashMap.get
    read  from field  java.util.HashMap$Node.value from object 2097514481
org.apache.shiro.crypto.hash.SimpleHash.getDigest
org.apache.shiro.crypto.hash.SimpleHash.setIterations
org.apache.shiro.authc.credential.DefaultPasswordService.checkHashFormatDurability
org.apache.shiro.authc.credential.DefaultPasswordService.checkHashFormatDurability
    read  from field  org.apache.shiro.authc.credential.DefaultPasswordService.hashFormatWarned from object 861842890
org.apache.shiro.authc.credential.DefaultPasswordService.checkHashFormatDurability
    read  from field  org.apache.shiro.authc.credential.DefaultPasswordService.hashFormat from object 861842890
org.apache.shiro.authc.credential.DefaultPasswordService.encryptPassword
    read  from field  org.apache.shiro.authc.credential.DefaultPasswordService.hashFormat from object 861842890
org.apache.shiro.authc.credential.DefaultPasswordService.checkHashFormatDurability
org.apache.shiro.crypto.hash.format.Shiro1CryptFormat.format
org.apache.shiro.util.SimpleByteSource.toBase64
org.apache.shiro.codec.Base64.encodeToString
org.apache.shiro.codec.Base64.encode
org.apache.shiro.codec.Base64.encode
org.apache.shiro.codec.CodecSupport.toString
org.apache.shiro.codec.CodecSupport.toString
org.apache.shiro.crypto.hash.SimpleHash.toBase64
org.apache.shiro.codec.Base64.encodeToString
org.apache.shiro.codec.Base64.encode
org.apache.shiro.codec.Base64.encode
org.apache.shiro.codec.CodecSupport.toString
org.apache.shiro.codec.CodecSupport.toString
com.anarsoft.agent.regression.MultiThreadedOneInstanceTemplate.run
    read  from field  com.anarsoft.agent.regression.MultiThreadedOneInstanceTemplate.threadCount from object 1461149300

So there are only reads, from fields written by creation of DefaultPasswordService. In my test the creation happens in the main thread.

pdeva
  • 43,605
  • 46
  • 133
  • 171
Thomas Krieger
  • 1,607
  • 11
  • 12
  • how did u get the trace output like 'read from field' etc. is there a library that does that? – pdeva Apr 29 '15 at 04:44
  • I created it with the new currently unpublished version of http://vmlens.com. I will be finished with the final tests this week so you can try it on your own. – Thomas Krieger Apr 29 '15 at 11:40
  • I don't see how this proves that the method is thread-safe. You can only be sure of that if either the javadoc says it is thread-safe, or you can tell it is thread-safe from the source code. – Stephen C Apr 30 '15 at 09:50
  • If you generate traces of your program and check if all field accesses can be ordered according to the java memory model you schould be able to find benign race conditions. Of course you still have the issue that a race occures because fields were not accessed atomatic, which actually could happen in the method checkHashFormatDurability(). In my tests !(format instanceof ParsableHashFormat) && log.isWarnEnabled()) always returned false, so this is no issue in this case – Thomas Krieger Apr 30 '15 at 13:20
1

It is thread safe in that there are no writes to class members during the encryption process.

Technically, you do need to ensure that there are no calls to setHashService or setHashFormat during the execution of encryptPassword(...). For example, a call to setHashService(null) would cause encryptPassword(...) to raise a NPE.

In practice, that is probably not an issue, as it is unlikely that there would be a reason to change the HashService, or HashFormat, after the service is up and running.

Devon_C_Miller
  • 16,248
  • 3
  • 45
  • 71