terl / lazysodium-java

A Java implementation of the Libsodium crypto library. For the lazy dev.
https://github.com/terl/lazysodium-java/wiki
Mozilla Public License 2.0
135 stars 48 forks source link

Library loading concurrency issues #49

Closed dmitry-timofeev closed 5 years ago

dmitry-timofeev commented 5 years ago

Library loading, which happens on lazysodium instantiation, has a couple of issues:

  1. It always uses the same temporary directory, and clears it on instantiation and application exit. That leads to various concurrency issues when multiple processes use LazySodium. I bumped into LinkageError when ran several processes simultaneously, but I suspect a process crash is also possible with unlucky timing.
  2. It is not thread safe. If multiple threads instantiate SodiumJava about the same time, the process will crash with segfault. Here is a test demonstrating that:

    
    @Test
    public void testConcurrentLoading() throws Exception {
    int numThreads = 32;
    List<Thread> threads = new ArrayList<>(numThreads);
    CyclicBarrier startBarrier = new CyclicBarrier(numThreads);
    for (int i = 0; i < numThreads; i++) {
      threads.add(new Thread(
          () -> {
            try {
              startBarrier.await();
              LazySodiumJava lazySodium = new LazySodiumJava(new SodiumJava());
              assertNotEquals(-1, lazySodium.sodiumInit());
            } catch (InterruptedException e) {
              // Just return
            } catch (BrokenBarrierException e) {
              e.printStackTrace();
            }
          }
      ));
    }
    
    threads.forEach(Thread::start);
    
    for (Thread t : threads) {
      try {
        t.join();
      } catch (InterruptedException e) {
        threads.forEach(Thread::interrupt);
        throw e;
      }
    }
    }
  3. If app instantiates SodiumJava several times with the default constructor, it will copy libsodium.so as many times to the temp directory as invoked.

I've locally resolved the first issue and will submit the PR soon. I suggest to resolve the other too in the same PR, as they are closely related, WDYT?

gurpreet- commented 5 years ago

@dmitry-timofeev,

Amazing work! Thank you so much for this 😄 The power of open-source wins again.

I did have some reservations about using temporary directories. I tried many things but in the end, using temporary directories turned out to be the only reliable way I could extract the shared libraries from the JAR. I had not thought of synchronisation issues, but I am glad your PR solves that problem.

Let me review and I'll create a release as soon as possible.

dmitry-timofeev commented 5 years ago

Wonderful, thank you very much! If anything is needed regarding this modification — let me know.

gurpreet- commented 5 years ago

Just to let you know your changes have been incorporated and released in 3.7.1.

Thank you for your contribution 😄

dmitry-timofeev commented 5 years ago

Great, thank you!