2012/2/22 Mark Thomas <ma...@apache.org>:
> On 21/02/2012 16:31, Konstantin Kolinko wrote:
>> Regarding StreamInbound#doClose(), doPing(), doPong()
>
> Thanks for the review.
>
>>
>> 1. in doClose():
>> status = status + is.read();
>> There is no check whether is.read() is not -1 above.
>
> Added.
>
>> 2.All 3 methods have such loops:
>> while (read > -1) {
>> data.position(data.position() + read);
>> read = is.read(data.array(), data.position(),
>> data.remaining());
>> }
>>
>> I am not sure what happens if data.remaining() becomes 0 but there are
>> still data in the stream, e.g. from ill-intended client.
>
> This is handled by the underlying WsInputStream. It ensures that only
> the number of bytes declared in the packet header may be read before EOF.
>
>> I think that if there is more data than buffer can handle it will hang
>> looping
>> indefinitely with read=0.
>
> Since these are all control frames the size of the payload is limited to
> a maximum of 125 bytes therefore I believe we are safe here.
>
>> 3. doPong() method has unbounded while() loop. It wouldn't hang, but I
>> think it is bad that there is no control on how many data are
>> consumed.
>
> Again, the limit is 125 bytes. This is enforced by the payload length
> checks in onData() above combined with the behaviour of WsInputStream.
>
Oh, I see now. That check in onData().
1. I think a comment wouldn't hurt saying that content length and
packet fragmentation is already checked by the caller.
2. Maybe change the method signature to explicitly accept
WsInputStream. With that argument it is possible to use the value of
(wsIs.getPayloadLength() + 1) to allocate the buffer instead of those
explicit values of 126+/-2.
>
> Of course, I could have made a mistake here so if you see a hole in my
> logic please shout. I'll add some comments to the code to clarify what
> is going on.
>
Reviewing WsInputStream I see the following potential issues:
1. processFrameHeader() does not check whether any of read() calls in
it return -1.
If the stream prematurely ended this method will complete happily,
reading a lot of FF and payloadLength of 0 (thanks that "new byte[8]"
creates a new array of all zeros).
It is good that OPCODE_CONTINUATION is zero and that detects this
situation for the second and later frames, but the whole situation is
odd.
2. There is no error flag. If a read() or processFrameHeader()
operation fails with IOException because of some check and caller
ignores the exception, the next read call will try to read something.
Other minor issues
1. Maybe lower visibility of MessageInbound#bb, #cb and of
TestWebSocket#isContinuation. They are package-visible at the moment.
Best regards,
Konstantin Kolinko
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
