Is this MessageDigest wrapper thread-safe?

Question

I wrote my own simplified MessageDigest wrapper and now I'm wondering if it's thread safe.

public final class SimpleIMD implements ImmutableMessageDigest {
    private final MessageDigest md;

    public SimpleIMD(MessageDigest md) {
        this.md = this.cloneMessageDigest(md);
    }

    private MessageDigest cloneMessageDigest(MessageDigest original) {
        MessageDigest clone = null;
        try {
            clone = (MessageDigest) original.clone();
        } catch (CloneNotSupportedException cnse) {
            throw new RuntimeException(
                "Failed to instantiate a new SimpleImd instance.",
                cnse
            );
        } finally {
            return clone;
        }
    }

    @Override
    public byte[] digest() {
        return this.md.digest();
    }

    @Override
    public ImmutableMessageDigest update(byte[] arr, int offset, int len) {
        MessageDigest newMsgDigest = this.cloneMessageDigest(this.md);
        newMsgDigest.update(arr, offset, len);
        return new SimpleIMD(newMsgDigest);
    }
}

The class seems to be _immutable_ and hence thread-safe. However, it doesn't automatically mean that the code _using_ the class is thread-safe. — Mick Mnemonic, Aug 20 '16 at 00:17
Well, if `MessageDigest.digest` is not thread-safe, then the code isn't either. No mention of thread-safety in the Javadoc, so the safest assumption is that your code is broken. It's just an assumption however, and has to be checked — Dici, Aug 20 '16 at 00:19
@MickMnemonic I think I need to perform digest on a fresh copy to be on the safe side. But still... what if something strange happens while cloning? It's not really atomic and I fear I need a lock somewhere. — Janez Kuhar, Aug 20 '16 at 00:20
@Dici No, [MessageDigest](https://docs.oracle.com/javase/8/docs/api/java/security/MessageDigest.html) isn't thread-safe. — Janez Kuhar, Aug 20 '16 at 00:22
@JanezKuhar, how is the `MessageDigest` you pass to the constructor used _elsewhere_ in the code? I think your thread-safety issue depends solely on that question. — Mick Mnemonic, Aug 20 '16 at 00:32
@MickMnemonic That fortunately doesn't matter since it's deep-copied in. And I am only interested in thread-safety regarding `SimpleIMD` objects alone, e.g. about concurrent calls to `digest()` and `update()`. — Janez Kuhar, Aug 20 '16 at 00:36
Yes, I think it doesn't matter if this class is simply a wrapper around the passed in `MessageDigest`. If that `MessageDigest` object is not used anywhere else, I don't see what could go wrong with cloning (or why you'd need to clone in the first place). Alternatively, you could [just create a new instance each time you need one](http://stackoverflow.com/questions/17554998/need-thread-safe-messagedigest-in-java). — Mick Mnemonic, Aug 20 '16 at 00:49
In my opinion as long as the underlying object is not thread-safe and that we expose a `digest` method which directly delegates the call to the underlying object, then the code is not thread-safe unless `digest` has no side-effect — Dici, Aug 20 '16 at 01:47

Janez Kuhar · Accepted Answer · 2020-08-19T14:02:37.660

As I suspected, my code wasn't thread safe.

For example, if 2 threads shared the same SimpleIMD object and the order of method invocations in time would go something like this:

  T1                 T2
  |                  |
update()             |
  |                  |
  |                digest()
  |                  |
  |                  |
  |                  |

then there is no guarantee that update() will finish before digest().

In fact, digest() may even reset the underlying MessageDigest instance before update() and T1 will have the wrong hash. This problem can be solved by performing digest() on a fresh copy.

So the method:

@Override
public byte[] digest() {
    return this.md.digest();
}

has to be changed to:

@Override
public byte[] digest() {
    return this.cloneMessageDigest(this.md).digest();
}

UPDATE

The solution presented here is incorporated into ImmutableMessageDigest that's part of Caesar, an open source library I wrote.

Is this MessageDigest wrapper thread-safe?

1 Answers1