Swiftdiff options
| author | Nick Hudson <nick.hudson@isode.com> | 2013-10-08 09:57:42 (GMT) |
|---|---|---|
| committer | Nick Hudson <nick.hudson@isode.com> | 2013-10-08 11:00:06 (GMT) |
| commit | 5cef6746d1997e4ecc9ad2c69901b69128a4d958 (patch) | |
| tree | 96f42c00a7790743cb9a120b80af2c7372026172 | |
| parent | 716cf1b389b4f88bad61e56587575b6f97ee57ca (diff) | |
| download | stroke-5cef6746d1997e4ecc9ad2c69901b69128a4d958.zip stroke-5cef6746d1997e4ecc9ad2c69901b69128a4d958.tar.bz2 | |
Fix synchronization problem in ByteArray
We noticed that in certain circumstances a stream of data being sent
to a server was being corrupted.
According to the "onDataWritten" signal, we could see that the data
which Stroke thought it was writing was valid, but by adding debug
code to the JavaConnection class, we could see that what was actually
being sent over the socket was wrong. For example, where
"onDataWritten" would report something like
some text for the server
the actual data being written to the socket (as shown by
toString() of the bytestream) would be something like:
some text fo\200\300\200\300\200\300\200\300\200\300\200\300\200\300\200\300\200\300\200\300\200\300\200\300
i.e. the length of data is correct, but the last part of the buffer is broken.
We saw this on non-TLS connections, but never on TLS connections.
The reason for this (verified after some debugging) is that the
"ByteData.getData()" method was unsynchronized. In the failing cases,
two threads are calling this method at once. The first one finds that
"dataCopy_" is null, and so new's it and starts filling it with data.
The second thread calls "getData()" before this completes, which means
it sees "dataCopy_" as non-null, and uses that value (even though the
first thread hasn't finished populating it yet).
In the failing scenario, the two threads involved were (1) thread that
was handling the "onDataWritten()" callback (which called "getData()"
to get a String that it sent to a debug stream) and (2) the
JavaConnection code (which wants to write the data to the socket).
It seems likely that the reason this doesn't happen for TLS
connections is that in that case, the JavaConnection object will be
processing a ByteArray object that has been generated via the
SSLEngine (rather than the one which "onDataWritten()" sees, and so
the chance of two threads both calling "getData()" is reduced.
(I have not followed the TLS code path thoroughly to verify this).
So this change makes any method in ByteArray that touches "dataCopy_"
be synchronized (as well as hashCode() as suggested by findbugs)
Test-information:
Having inserted some debug code, I could reproduce the "data
corruption" problem reliably.
After adding the "synchronized" directive to "getData()", I could no
longer reproduce the corruption.
Ran MLC with this patch and works with no problems
Change-Id: I02008736a2a8bd44f3702c4526fd67369a3c136a
Signed-off-by: Nick Hudson <nick.hudson@isode.com>
| -rw-r--r-- | src/com/isode/stroke/base/ByteArray.java | 8 |
1 files changed, 4 insertions, 4 deletions
diff --git a/src/com/isode/stroke/base/ByteArray.java b/src/com/isode/stroke/base/ByteArray.java index 077c58c..22d81b7 100644 --- a/src/com/isode/stroke/base/ByteArray.java +++ b/src/com/isode/stroke/base/ByteArray.java @@ -77,7 +77,7 @@ public class ByteArray { * @return array copy of internal data, will never be null, but may * contain zero elements. */ - public byte[] getData() { + public synchronized byte[] getData() { if (dataCopy_ == null) { dataCopy_ = new byte[getSize()]; for (int i = 0; i < data_.size(); i++) { @@ -167,7 +167,7 @@ public class ByteArray { * @param b a single byte * @return a reference to the updated object */ - public ByteArray append(byte b) { + public synchronized ByteArray append(byte b) { dataCopy_ = null; /* Invalidate cache */ data_.add(Byte.valueOf(b)); return this; @@ -191,7 +191,7 @@ public class ByteArray { } @Override - public int hashCode() { + public synchronized int hashCode() { int hash = 3; hash = 97 * hash + (this.data_ != null ? this.data_.hashCode() : 0); return hash; @@ -278,7 +278,7 @@ public class ByteArray { /** * Clears the contents of this ByteArray, leaving it with zero elements. */ - public void clear() { + public synchronized void clear() { data_ = new Vector<Byte>(); dataCopy_ = null; } |