Fixes memory leak when decrypting scuttlebutt incoming messages #209
Conversation
| @@ -271,6 +271,7 @@ static Pointer dup(byte[] bytes) { | |||
|
|
|||
| static <T> T dup(byte[] bytes, BiFunction<Pointer, Integer, T> ctr) { | |||
| Pointer ptr = Sodium.dup(bytes); | |||
|
|
|||
|
|
||
| } | ||
| } catch (Exception e) { | ||
| throw new StreamException(e.getMessage()); |
There was a problem hiding this comment.
Can we be more specific - Exception is very broad.
Also can we throw StreamException(e) so we get the whole stacktrace?
… sodium Pointers were only being freed when Destroyed by the garbage collector.
|
thanks @atoulme - done :) |
| @@ -193,7 +193,7 @@ public Bytes bytes() { | |||
| /** | |||
| * A SecretBox nonce. | |||
| */ | |||
| public static final class Nonce { | |||
| public static final class Nonce implements AutoCloseable { | |||
There was a problem hiding this comment.
I'm not sure I agree with making a Nonce "auto closeable". It's not a resource, so it feels odd to treat it like one. We could certainly make it implement Destroyable, just like the secret keys.
However, whatever we do, it must be applied consistently across all cava-sodium types.
There was a problem hiding this comment.
@cleishm - I see where you're coming from. I've pushed a new commit to this PR to implement Destroyable instead.
There's currently a couple of other classes (below) that implement AutoCloseable that aren't 'resources' (like sockets) but on searching, it looks like the majority implement Destroyable like you suggested so I'll follow that convention.
public final class AES256GCM implements AutoCloseable {
public final class Box implements AutoCloseable {
Perhaps these can be made to implement 'destroyable' also in a separate PR instead.
There was a problem hiding this comment.
It's a different flow. You open a secret box and close it - you don't destroy it. It's more the pointers of the keys and nonces generated along the way we need to clean up.
….Nonce class to be more consistent with the rest of cava.
I believe this changes fixes #205 .
I think this was happening because the underlying JNI
Pointerfor theheaderandbodynonces weren't being freed until the garbage collector destroyed the objects which no longer had any references. Until the garbage collector destroyed them, the lib sodiummallocwould periodically fail because there wasn't enough memory available to malloc - but once the garbage collector calleddestroyon the objects memory became free to allocate again, and the decryption would start succeeding again.Note: this is just a theory from reading the code (i'm not sure how I'd test it empirically) but the changes appear to stop the error happening.
This change also means that if the decryption fails due to a lib sodium error, a
StreamExceptionwill be thrown and the underlying socket (scuttlebutt connection) will be closed.