Safety net for infinite unproductive loop

[farahaniali]

As a safety net for handling an infinite loop happening inside decrypt method (that has been reported here https://issues.apache.org/jira/browse/HTTPCORE-782), we propose a unproductive loop detection mechanism being added to it.
The mechanism counts the number of times the loop circles without consuming any data from the encrypted buffer while the unwrapping method reports a successful operation by returning OK status.
If we reach to 1000 unproductive loop, we throw an exception to escape the unproductive loop.

We are trying to have this feature inside production system, but have not seen the issue in weeks.

[ok2c]

@farahaniali This work-around is hideous. I would prefer a proper fix to the problem instead.

[garydgregory]

I agree with @ok2c , this type of change may be acceptable for local debugging but it doesn't belong in production. You may want to gather use case specific metrics and report them here. A reproducer would be best as usual.

[farahaniali]

@ok2c and @garydgregory do not disagree with the fact that this is the simplest and arbitrary looking solution. Open to suggestion so I can incorporate any available mechanism to have a safety net in place.
One suggestion would be to have cleaner and more aggressive safety net at the end of the try block:

private void decryptData(final IOSession protocolSession) throws IOException {
        final HandshakeStatus handshakeStatus = sslEngine.getHandshakeStatus();
        if ((handshakeStatus == HandshakeStatus.NOT_HANDSHAKING || handshakeStatus == HandshakeStatus.FINISHED)
                && inEncrypted.hasData()) {
            final ByteBuffer inEncryptedBuf = inEncrypted.acquire();
            inEncryptedBuf.flip();
            try {
                while (inEncryptedBuf.hasRemaining()) {
                    final ByteBuffer inPlainBuf = inPlain.acquire();
                    try {
                        final SSLEngineResult result = doUnwrap(inEncryptedBuf, inPlainBuf);
                        if (!inEncryptedBuf.hasRemaining() && result.getHandshakeStatus() == HandshakeStatus.NEED_UNWRAP) {
                            throw new SSLException("Unable to complete SSL handshake");
                        }
                        if (sslEngine.isInboundDone()) {
                            endOfStream = true;
                        }
                        if (inPlainBuf.position() > 0) {
                            inPlainBuf.flip();
                            try {
                                ensureHandler().inputReady(protocolSession, inPlainBuf.hasRemaining() ? inPlainBuf : null);
                            } finally {
                                inPlainBuf.clear();
                            }
                        }
                        if (result.getStatus() != SSLEngineResult.Status.OK) {
                            if (result.getStatus() == SSLEngineResult.Status.BUFFER_UNDERFLOW && endOfStream) {
                                throw new SSLException("Unable to decrypt incoming data due to unexpected end of stream");
                            }
                            break;
                        }
                        if (result.bytesConsumed() == 0) {
                            throw new SSLException(String.format("Unable to decrypt incoming data due to unproductive cycle. Position on the buffer %s and the limit is %s with handshake status of %s and EndOfStream flag as %s", inEncryptedBuf.position(), inEncryptedBuf.limit(), result.getHandshakeStatus(), endOfStream));
                        }
                    } finally {
                        inPlain.release();
                    }
                }
            } finally {
                inEncryptedBuf.compact();
                // Release inEncrypted if empty
                if (inEncryptedBuf.position() == 0) {
                    inEncrypted.release();
                }
            }
        }
        if (endOfStream && !inEncrypted.hasData()) {
            ensureHandler().inputReady(protocolSession, null);
        }
    }

which has the following piece of code in place that would throw an SSLException right away if it finds out we got OK status but not progressed on the input encrypted buffer.

if (result.bytesConsumed() == 0) {
                            throw new SSLException(.... 

[arturobernalg]

@farahaniali IMO Hard-coding a 1 000-cycle guard is just another band-aid.

[ok2c]

@farahaniali If can propose a safe-guard that does not involve a magic number like 1000 I will happily review.

[farahaniali]

@farahaniali If can propose a safe-guard that does not involve a magic number like 1000 I will happily review.

@ok2c Just update the PR.
Reaching this if block means no buffer_underflow, buffer_overflow and finished status on the result. Therefore not consuming from inEncryptedBuf is problematic.

[ok2c]

@farahaniali All right. That makes more sense. I will merge the change-set to master but not to the stable 5.3.x branch for now. We will see how the TLS code behaves in the coming months.

I will also tweak the exception message a bit.