NIOSingleStepByteToMessageDecoder reentrancy safety

[rnro]

Motivation:

NIOSingleStepByteToMessageDecoder calls out part way through its processing step to a user-provided closure which can cause re-entrant behavior which violates the assumption made in the code that if the buffer is non-empty at the start that will be true later in the method.

Modifications:

NIOSingleStepByteToMessageDecoder no longer assumes a non-empty buffer in its final phase of buffer management.

Further changes to protect against re-entrancy shouldn't be necessary because the outside call to messageReceiver is the only one which is permitted to be re-entrant (decode and decodeLast are not).

Result:

NIOSingleStepByteToMessageDecoder can handle re-entrant processing calls.

[gjcairo]

Sneaky reentrancy :D

[gjcairo]

This test is testing your fix, but unless I misunderstood your comment before the test, I think it's not happening in the way you expected.

XCTAssertThrowsError isn't actually throwing, so the closure passed to process(buffer:messageReceiver:) won't actually throw when executed.

This isn't all bad though, because that means that the buffer will be nil after the given closure, which will test your fix as the method will continue without erroring when force-unwrapping the buffer. However, that's not what you were set to test here, since we're not exiting part way.

I think it would make sense to keep this test (if we don't already have one that tests the messageReceiver being throwing), but removing the XCTAssertThrowsError so that finishProcessing actually throws, and we can assert that process throws when the inner closure fails.

However, I would definitely have a separate test to test the actual fix here: to do that you can get rid of the error being thrown on decodeLast: we just need to call finishProcessing but make sure it doesn't throw to trigger the re-entrancy bug.

[rnro]

This was discussed out-of-band, but for future historians...

I did intend the behavior of XCTAssertThrowsError to check and then swallow the error thrown from the interior call to processing, but it should've been more obvious. Doing that allowed the interior call to leave self._buffer as nil.

Whilst investigating this in detail we discovered that self._buffer is nil because self._withNonCoWBuffer would set it to nil before calling the decode operation which then threw and then never replaced it. That seems dangerous and we should unconditionally replace the buffer after the decode operation whether or not it throws so I have made that change.

The test is left in place as a regression test with the catching of the error made explicit.

[gjcairo]

Thanks Rick!

[rnro]

Resolves #2639

[weissi]

what if body throws?

[gjcairo]

This is why we decided to move the self._buffer = buffer line into a defer block. This is actually what was triggering the original bug: body would throw but we'd never reinstate the original value of the buffer, so it would remain as nil, which caused issues when it happened reentrantly because the outer closure would come back to a context in which the buffer is now nil, and we were force-unwrapping it.
Rick mentioned this in #2881 (comment)

[weissi]

🙈 I read the diff in reverse somehow haha. nice catch!