Skip to content

Commit 85390d5

Browse files
DavidiusdadiDavidiusdadi
committed
Merge pull request TooTallNate#193 from Davidiusdadi/master
fixed critical bugs causing random protocol errors and bug that caused server to hang on stop()
2 parents 53fced8 + 158b9bc commit 85390d5

File tree

3 files changed

+63
-36
lines changed

3 files changed

+63
-36
lines changed

‎src/main/java/org/java_websocket/SSLSocketChannel2.java‎

Lines changed: 39 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,9 @@
3131
* Implements the relevant portions of the SocketChannel interface with the SSLEngine wrapper.
3232
*/
3333
public class SSLSocketChannel2 implements ByteChannel, WrappedByteChannel {
34+
/**
35+
* This object is used to feed the {@link SSLEngine}'s wrap and unwrap methods during the handshake phase.
36+
**/
3437
protected static ByteBuffer emptybuffer = ByteBuffer.allocate( 0 );
3538

3639
protected ExecutorService exec;
@@ -49,12 +52,9 @@ public class SSLSocketChannel2 implements ByteChannel, WrappedByteChannel {
4952
/** used to set interestOP SelectionKey.OP_WRITE for the underlying channel */
5053
protected SelectionKey selectionKey;
5154

52-
53-
protected SSLEngineResult engineResult;
5455
protected SSLEngine sslEngine;
55-
56-
57-
private Status engineStatus = Status.BUFFER_UNDERFLOW;
56+
protected SSLEngineResult readEngineResult;
57+
protected SSLEngineResult writeEngineResult;
5858

5959
public SSLSocketChannel2( SocketChannel channel , SSLEngine sslEngine , ExecutorService exec , SelectionKey key ) throws IOException {
6060
if( channel == null || sslEngine == null || exec == null )
@@ -64,6 +64,8 @@ public SSLSocketChannel2( SocketChannel channel , SSLEngine sslEngine , Executor
6464
this.sslEngine = sslEngine;
6565
this.exec = exec;
6666

67+
readEngineResult = writeEngineResult = new SSLEngineResult( Status.BUFFER_UNDERFLOW, sslEngine.getHandshakeStatus(), 0, 0 ); // init to prevent NPEs
68+
6769
tasks = new ArrayList<Future<?>>( 3 );
6870
if( key != null ) {
6971
key.interestOps( key.interestOps() | SelectionKey.OP_WRITE );
@@ -93,8 +95,12 @@ private void consumeFutureUninterruptible( Future<?> f ) {
9395
}
9496
}
9597

98+
/**
99+
* This method will do whatever necessary to process the sslengine handshake.
100+
* Thats why it's called both from the {@link #read(ByteBuffer)} and {@link #write(ByteBuffer)}
101+
**/
96102
private synchronized void processHandshake() throws IOException {
97-
if( engineResult.getHandshakeStatus() == HandshakeStatus.NOT_HANDSHAKING )
103+
if( sslEngine.getHandshakeStatus() == HandshakeStatus.NOT_HANDSHAKING )
98104
return; // since this may be called either from a reading or a writing thread and because this method is synchronized it is necessary to double check if we are still handshaking.
99105
if( !tasks.isEmpty() ) {
100106
Iterator<Future<?>> it = tasks.iterator();
@@ -110,8 +116,8 @@ private synchronized void processHandshake() throws IOException {
110116
}
111117
}
112118

113-
if( engineResult.getHandshakeStatus() == SSLEngineResult.HandshakeStatus.NEED_UNWRAP ) {
114-
if( !isBlocking() || engineStatus == Status.BUFFER_UNDERFLOW ) {
119+
if( sslEngine.getHandshakeStatus() == SSLEngineResult.HandshakeStatus.NEED_UNWRAP ) {
120+
if( !isBlocking() || readEngineResult.getStatus() == Status.BUFFER_UNDERFLOW ) {
115121
inCrypt.compact();
116122
int read = socketChannel.read( inCrypt );
117123
if( read == -1 ) {
@@ -121,36 +127,37 @@ private synchronized void processHandshake() throws IOException {
121127
}
122128
inData.compact();
123129
unwrap();
124-
if( engineResult.getHandshakeStatus() == HandshakeStatus.FINISHED ) {
130+
if( sslEngine.getHandshakeStatus() == HandshakeStatus.FINISHED ) {
125131
createBuffers( sslEngine.getSession() );
126132
return;
127133
}
128134
}
129135
consumeDelegatedTasks();
130-
assert ( engineResult.getHandshakeStatus() != HandshakeStatus.NOT_HANDSHAKING );
131-
if( tasks.isEmpty() || engineResult.getHandshakeStatus() == SSLEngineResult.HandshakeStatus.NEED_WRAP ) {
136+
assert ( sslEngine.getHandshakeStatus() != HandshakeStatus.NOT_HANDSHAKING );
137+
if( tasks.isEmpty() || sslEngine.getHandshakeStatus() == SSLEngineResult.HandshakeStatus.NEED_WRAP ) {
132138
socketChannel.write( wrap( emptybuffer ) );
133-
if( engineResult.getHandshakeStatus() == HandshakeStatus.FINISHED ) {
139+
if( sslEngine.getHandshakeStatus() == HandshakeStatus.FINISHED ) {
134140
createBuffers( sslEngine.getSession() );
135141
}
136142
}
137143
}
138144

139145
private synchronized ByteBuffer wrap( ByteBuffer b ) throws SSLException {
140146
outCrypt.compact();
141-
engineResult = sslEngine.wrap( b, outCrypt );
147+
writeEngineResult = sslEngine.wrap( b, outCrypt );
142148
outCrypt.flip();
143149
return outCrypt;
144150
}
145151

152+
/**
153+
* performs the unwrap operation by unwrapping from {@link #inCrypt} to {@link #inData}
154+
**/
146155
private synchronized ByteBuffer unwrap() throws SSLException {
147156
int rem;
148157
do {
149158
rem = inData.remaining();
150-
engineResult = sslEngine.unwrap( inCrypt, inData );
151-
engineStatus = engineResult.getStatus();
152-
} while ( engineStatus == SSLEngineResult.Status.OK && ( rem != inData.remaining() || engineResult.getHandshakeStatus() == HandshakeStatus.NEED_UNWRAP ) );
153-
159+
readEngineResult = sslEngine.unwrap( inCrypt, inData );
160+
} while ( readEngineResult.getStatus() == SSLEngineResult.Status.OK && ( rem != inData.remaining() || sslEngine.getHandshakeStatus() == HandshakeStatus.NEED_UNWRAP ) );
154161
inData.flip();
155162
return inData;
156163
}
@@ -200,6 +207,8 @@ public int write( ByteBuffer src ) throws IOException {
200207
/**
201208
* Blocks when in blocking mode until at least one byte has been decoded.<br>
202209
* When not in blocking mode 0 may be returned.
210+
*
211+
* @return the number of bytes read.
203212
**/
204213
public int read( ByteBuffer dst ) throws IOException {
205214
if( !dst.hasRemaining() )
@@ -216,11 +225,16 @@ public int read( ByteBuffer dst ) throws IOException {
216225
}
217226
}
218227
}
219-
228+
/* 1. When "dst" is smaller than "inData" readRemaining will fill "dst" with data decoded in a previous read call.
229+
* 2. When "inCrypt" contains more data than "inData" has remaining space, unwrap has to be called on more time(readRemaining)
230+
*/
220231
int purged = readRemaining( dst );
221232
if( purged != 0 )
222233
return purged;
223234

235+
/* We only continue when we really need more data from the network.
236+
* Thats the case if inData is empty or inCrypt holds to less data than necessary for decryption
237+
*/
224238
assert ( inData.position() == 0 );
225239
inData.clear();
226240

@@ -229,7 +243,7 @@ public int read( ByteBuffer dst ) throws IOException {
229243
else
230244
inCrypt.compact();
231245

232-
if( ( isBlocking() && inCrypt.position() == 0 ) || engineStatus == Status.BUFFER_UNDERFLOW )
246+
if( isBlocking() || readEngineResult.getStatus() == Status.BUFFER_UNDERFLOW )
233247
if( socketChannel.read( inCrypt ) == -1 ) {
234248
return -1;
235249
}
@@ -243,6 +257,9 @@ public int read( ByteBuffer dst ) throws IOException {
243257
return transfered;
244258
}
245259

260+
/**
261+
* {@link #read(ByteBuffer)} may not be to leave all buffers(inData, inCrypt)
262+
**/
246263
private int readRemaining( ByteBuffer dst ) throws SSLException {
247264
if( inData.hasRemaining() ) {
248265
return transfereTo( inData, dst );
@@ -273,7 +290,7 @@ public void close() throws IOException {
273290
}
274291

275292
private boolean isHandShakeComplete() {
276-
HandshakeStatus status = engineResult.getHandshakeStatus();
293+
HandshakeStatus status = sslEngine.getHandshakeStatus();
277294
return status == SSLEngineResult.HandshakeStatus.FINISHED || status == SSLEngineResult.HandshakeStatus.NOT_HANDSHAKING;
278295
}
279296

@@ -304,7 +321,7 @@ public boolean isOpen() {
304321

305322
@Override
306323
public boolean isNeedWrite() {
307-
return outCrypt.hasRemaining() || !isHandShakeComplete();
324+
return outCrypt.hasRemaining() || !isHandShakeComplete(); // FIXME this condition can cause high cpu load during handshaking when network is slow
308325
}
309326

310327
@Override
@@ -314,8 +331,7 @@ public void writeMore() throws IOException {
314331

315332
@Override
316333
public boolean isNeedRead() {
317-
return inData.hasRemaining() || ( inCrypt.hasRemaining() && engineResult.getStatus() != Status.BUFFER_UNDERFLOW &&
318-
engineResult.getStatus() != Status.CLOSED );
334+
return inData.hasRemaining() || ( inCrypt.hasRemaining() && readEngineResult.getStatus() != Status.BUFFER_UNDERFLOW && readEngineResult.getStatus() != Status.CLOSED );
319335
}
320336

321337
@Override

‎src/main/java/org/java_websocket/WebSocketImpl.java‎

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,7 @@ public class WebSocketImpl implements WebSocket {
9393
private Opcode current_continuous_frame_opcode = null;
9494

9595
/** the bytes of an incomplete received handshake */
96-
private ByteBuffer tmpHandshakeBytes;
96+
private ByteBuffer tmpHandshakeBytes = ByteBuffer.allocate( 0 );
9797

9898
/** stores the handshake sent by this websocket ( Role.CLIENT only ) */
9999
private ClientHandshake handshakerequest = null;
@@ -149,8 +149,11 @@ public WebSocketImpl( WebSocketListener listener , List<Draft> drafts , Socket s
149149
*
150150
*/
151151
public void decode( ByteBuffer socketBuffer ) {
152-
if( !socketBuffer.hasRemaining() || flushandclosestate )
152+
assert ( socketBuffer.hasRemaining() );
153+
154+
if( flushandclosestate ) {
153155
return;
156+
}
154157

155158
if( DEBUG )
156159
System.out.println( "process(" + socketBuffer.remaining() + "): {" + ( socketBuffer.remaining() > 1000 ? "too big to display" : new String( socketBuffer.array(), socketBuffer.position(), socketBuffer.remaining() ) ) + "}" );
@@ -159,19 +162,24 @@ public void decode( ByteBuffer socketBuffer ) {
159162
decodeFrames( socketBuffer );
160163
} else {
161164
if( decodeHandshake( socketBuffer ) ) {
162-
decodeFrames( socketBuffer );
165+
assert ( tmpHandshakeBytes.hasRemaining() != socketBuffer.hasRemaining() || !socketBuffer.hasRemaining() ); // the buffers will never have remaining bytes at the same time
166+
167+
if( socketBuffer.hasRemaining() ) {
168+
decodeFrames( socketBuffer );
169+
} else if( tmpHandshakeBytes.hasRemaining() ) {
170+
decodeFrames( tmpHandshakeBytes );
171+
}
163172
}
164173
}
165174
assert ( isClosing() || isFlushAndClose() || !socketBuffer.hasRemaining() );
166175
}
167-
168176
/**
169177
* Returns whether the handshake phase has is completed.
170178
* In case of a broken handshake this will be never the case.
171179
**/
172180
private boolean decodeHandshake( ByteBuffer socketBufferNew ) {
173181
ByteBuffer socketBuffer;
174-
if( tmpHandshakeBytes == null ) {
182+
if( tmpHandshakeBytes.capacity() == 0 ) {
175183
socketBuffer = socketBufferNew;
176184
} else {
177185
if( tmpHandshakeBytes.remaining() < socketBufferNew.remaining() ) {
@@ -260,7 +268,7 @@ private boolean decodeHandshake( ByteBuffer socketBufferNew ) {
260268
draft.setParseMode( role );
261269
Handshakedata tmphandshake = draft.translateHandshake( socketBuffer );
262270
if( tmphandshake instanceof ServerHandshake == false ) {
263-
flushAndClose( CloseFrame.PROTOCOL_ERROR, "Wwrong http function", false );
271+
flushAndClose( CloseFrame.PROTOCOL_ERROR, "wrong http function", false );
264272
return false;
265273
}
266274
ServerHandshake handshake = (ServerHandshake) tmphandshake;
@@ -286,7 +294,7 @@ private boolean decodeHandshake( ByteBuffer socketBufferNew ) {
286294
close( e );
287295
}
288296
} catch ( IncompleteHandshakeException e ) {
289-
if( tmpHandshakeBytes == null ) {
297+
if( tmpHandshakeBytes.capacity() == 0 ) {
290298
socketBuffer.reset();
291299
int newsize = e.getPreferedSize();
292300
if( newsize == 0 ) {

‎src/main/java/org/java_websocket/server/WebSocketServer.java‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
import java.nio.ByteBuffer;
99
import java.nio.channels.ByteChannel;
1010
import java.nio.channels.CancelledKeyException;
11+
import java.nio.channels.ClosedByInterruptException;
1112
import java.nio.channels.SelectableChannel;
1213
import java.nio.channels.SelectionKey;
1314
import java.nio.channels.Selector;
@@ -325,6 +326,7 @@ public void run() {
325326
ByteBuffer buf = takeBuffer();
326327
try {
327328
if( SocketChannelIOHelper.read( buf, conn, (ByteChannel) conn.channel ) ) {
329+
assert ( buf.hasRemaining() );
328330
conn.inQueue.put( buf );
329331
queue( conn );
330332
i.remove();
@@ -339,9 +341,6 @@ public void run() {
339341
} catch ( IOException e ) {
340342
pushBuffer( buf );
341343
throw e;
342-
} catch ( RuntimeException e ) {
343-
pushBuffer( buf );
344-
throw e;
345344
}
346345
}
347346
if( key.isWritable() ) {
@@ -359,21 +358,25 @@ public void run() {
359358
try {
360359
if( SocketChannelIOHelper.readMore( buf, conn, c ) )
361360
iqueue.add( conn );
361+
assert ( buf.hasRemaining() );
362362
conn.inQueue.put( buf );
363363
queue( conn );
364-
} finally {
364+
} catch ( IOException e ) {
365365
pushBuffer( buf );
366+
throw e;
366367
}
367368

368369
}
369370
} catch ( CancelledKeyException e ) {
370371
// an other thread may cancel the key
372+
} catch ( ClosedByInterruptException e ) {
373+
return; // do the same stuff as when InterruptedException is thrown
371374
} catch ( IOException ex ) {
372375
if( key != null )
373376
key.cancel();
374377
handleIOException( key, conn, ex );
375378
} catch ( InterruptedException e ) {
376-
return;// FIXME controlled shutdown
379+
return;// FIXME controlled shutdown (e.g. take care of buffermanagement)
377380
}
378381
}
379382

@@ -418,7 +421,7 @@ private void pushBuffer( ByteBuffer buf ) throws InterruptedException {
418421
}
419422

420423
private void handleIOException( SelectionKey key, WebSocket conn, IOException ex ) {
421-
//onWebsocketError( conn, ex );// conn may be null here
424+
// onWebsocketError( conn, ex );// conn may be null here
422425
if( conn != null ) {
423426
conn.closeConnection( CloseFrame.ABNORMAL_CLOSE, ex.getMessage() );
424427
} else if( key != null ) {

0 commit comments

Comments
 (0)