Fixes #4217 - SslConnection.DecryptedEnpoint.flush eternal busy loop.

[sbordet]

#4217

Releasing the encrypted output buffer so that it can be re-acquired
with an expanded capacity.

Signed-off-by: Simone Bordet [email protected]

[gregw]

This fix looks OK to me, but do we have a possible similar problem in fill?

[sbordet]

@gregw in fill() we handle the BUFFER_UNDERFLOW correctly, but we don't handle the BUFFER_OVERFLOW that this issue would potentially generate. Instead we end up in the default case and throw IllegalStateException. Harsh, but we don't spin.

Handling BUFFER_OVERFLOW would be this:

case BUFFER_OVERFLOW:
    if (BufferUtil.isEmpty(_decryptedInput))
    {
        releaseDecryptedInputBuffer();
        continue;
    }
    throw new IllegalStateException();

The idea is that we release the too small application buffer we decrypt into, and loop around to get a new _decryptedInput with a larger capacity.
However, now we do spin around; if - for any reason - turns out that the capacity is the same as before and unwrap() still produces BUFFER_OVERFLOW then now we spin loop.

The same problem may happen in flush() with the fix in 2e633a4: wrap() returns BUFFER_UNDERFLOW, we release encrypted output, loop around, get a new buffer, but if the capacity is the same and wrap() returns BUFFER_UNDERFLOW again we spin loop.

The change to handle the fill() side is trivial. Do you think we should do it?

[gregw]

I now think that we should only do the reallocate loop if we know the buffer size has changed.

We should do for both fill and flush

[gregw]

We should check if the buffer capacity differs from the sllEngine size before reallocating and looping

[gregw]

LGTM other than a minor question

[gregw]

is it worthwhile doing

if (hsSession == null || hsSession == session)

or can they never be the same?

[sbordet]

Good point, will add that.

[gregw]

Also would be good to have a test, even one that just sent a hand crafted 5 byte frame.

[sbordet]

@gregw will try to craft a test case... the test that I removed was failing because it was not causing the buffer expansion and now that SslConnection has the additional check on the buffer size it was aborting and thus failing the test.

[sbordet]

@gregw updated and wrote a test case.

[gregw]

lgtm