search

saltyrtc / saltyrtc-client-java

SaltyRTC Java implementation.
Apache License 2.0
3 stars 4 forks source link

Synchronize read methods in `CombinedSequence` #123

Closed ovalseven8 closed 5 years ago

ovalseven8 commented 5 years ago

Synchronization is not guaranteed to work unless both read and write operations are synchronized.

lgrahl commented 5 years ago

We already declared the library as not thread safe. Any further synchronisation can easily lead to deadlocks, so I want to avoid diving deeper into synchronisation.

ovalseven8 commented 5 years ago

@dbrgn @lgrahl I do not really understand why https://github.com/saltyrtc/saltyrtc-client-java/commit/b095a1ce7a74a53a68194fdddf93f485cc3ba783 is even necessary? In case it is necessary, then it's definitely also necessary to synchronize the read methods.

ovalseven8 commented 5 years ago

After some further skimming in the current implementation the read methods do indeed not need synchronization. So, sorry for the pull request.

The question is if the synchronization for the next() method is needed or not.

dbrgn commented 5 years ago

@ovalseven8 why did you close the PR? You are right, it is certainly a bug, if the class documentation mentions that the class is thread safe when it isn't.

The original intent was to make the entire library thread safe, but we have since decided that it's better if we don't give that guarantee and leave it to the user how to synchronize access to the library methods. However, until we start cleaning up and can ensure that there is definitely no danger in making classes like this one not synchronized, we should fix bugs.

ovalseven8 commented 5 years ago

To summarize:

dbrgn commented 5 years ago

The class documentation of CombinedSequence says it is "thread safe". So far, this was not true. That's what this PR fixes.

Yep

The current implementation, however, does not rely on the thread-safety of the entire class. So the change will not fix actual bugs. On the other side, the change also shouldn't introduce any other issues like deadlocks.

Exactly. Even though the PR doesn't fix any real-world bugs, it fixes a mismatch between documentation and code.

Must the next() method be synchronized in the current implementation?

I'm pretty sure that I stumbled over an issue when doing unit tests (which may run in parallel), not when integrating the code into another project. I'm not sure if it's really necessary in the current codebase, but since it's a security-relevant class (nonces in NaCl may never be re-used, and the CSN is part of the nonce) I personally think that it's worth synchronizing even if the class is not currently accessed concurrently, just to avoid potential bugs, at a very small cost (1 lock). (If this were Rust code, we'd already be covered by disallowing concurrent access to this class, but unfortunately that's not possible in Java.)