Fix crash sending window increment after close (#95)

Motivation:

When reads have yet to be delivered to a channel and we receive a
CHANNEL_CLOSE message, we forcibly deliver all of the reads. As part of
doing this, we process the window size changes and potentially send
window increment messages. These messages are invalid to send at this
time, and the state machine rightly forbids us from sending them, which
causes a crash.

Modifications:

- Change the code to only futz with the channel window if the channel is
  still open.

Result:

No crashing after channel close.

Author: Lukasa

Sources/NIOSSH/Child Channels/SSHChildChannel.swift

@@ -640,7 +640,8 @@ private extension SSHChildChannel {
     private func deliverSingleRead(_ data: PendingContent) {
         switch data {
         case .data(let data):
-            if let increment = self.windowManager.unbufferBytes(data.data.readableBytes) {
+            // We only futz with the window manager if the channel is not already closed.
+            if !self.didClose, let increment = self.windowManager.unbufferBytes(data.data.readableBytes) {
                 let update = SSHMessage.ChannelWindowAdjustMessage(recipientChannel: self.state.remoteChannelIdentifier!, bytesToAdd: UInt32(increment))
                 self.processOutboundMessage(.channelWindowAdjust(update), promise: nil)
             }

Tests/NIOSSHTests/ChildChannelMultiplexerTests.swift

@@ -2,7 +2,7 @@
 //
 // This source file is part of the SwiftNIO open source project
 //
-// Copyright (c) 2019 Apple Inc. and the SwiftNIO project authors
+// Copyright (c) 2019-2021 Apple Inc. and the SwiftNIO project authors
 // Licensed under Apache License v2.0
 //
 // See LICENSE.txt for license information
@@ -1192,12 +1192,7 @@ final class ChildChannelMultiplexerTests: XCTestCase {
         XCTAssertNoThrow(try harness.multiplexer.receiveMessage(self.openConfirmation(originalChannelID: channelID!, peerChannelID: 1)))
 
         // The default window size is 1<<24 bytes. Sadly, we need a buffer that size.
-        // Sorry for the unsafe code, but otherwise this test takes _ages_.
-        var buffer = channel.allocator.buffer(capacity: (1 << 24) + 1)
-        buffer.writeWithUnsafeMutableBytes(minimumWritableBytes: (1 << 24) + 1) { ptr in
-            memset(ptr.baseAddress!, 0, (1 << 24) + 1)
-            return (1 << 24) + 1
-        }
+        let buffer = ByteBuffer.bigBuffer
 
         // We're going to write one byte short.
         XCTAssertEqual(harness.flushedMessages.count, 1)
@@ -1237,6 +1232,46 @@ final class ChildChannelMultiplexerTests: XCTestCase {
         self.assertChannelClose(harness.flushedMessages.last, recipientChannel: 1)
     }
 
+    func testWeDontResizeTheWindowOnClose() throws {
+        let harness = self.harnessForbiddingInboundChannels()
+        defer {
+            harness.finish()
+        }
+
+        var childChannel: Channel?
+        harness.multiplexer.createChildChannel(channelType: .session) { channel, _ in
+            childChannel = channel
+            return channel.setOption(ChannelOptions.autoRead, value: false)
+        }
+
+        guard let channel = childChannel else {
+            XCTFail("Did not create child channel")
+            return
+        }
+
+        // Activate channel.
+        let channelID = self.assertChannelOpen(harness.flushedMessages.first)
+        XCTAssertNoThrow(try harness.multiplexer.receiveMessage(self.openConfirmation(originalChannelID: channelID!, peerChannelID: 1)))
+
+        // The default window size is 1<<24 bytes. Sadly, we need a buffer that size.
+        let buffer = ByteBuffer.bigBuffer
+
+        // We're going to write the whole window.
+        XCTAssertEqual(harness.flushedMessages.count, 1)
+        XCTAssertNoThrow(try harness.multiplexer.receiveMessage(self.data(peerChannelID: channelID!,
+                                                                          data: buffer.getSlice(at: buffer.readerIndex, length: 1 << 24)!)))
+
+        // Auto read is off, so nothing happens.
+        XCTAssertEqual(harness.flushedMessages.count, 1)
+
+        // Now we send a close message. This is going to forcibly close the channel immediately.
+        XCTAssertNoThrow(try harness.multiplexer.receiveMessage(self.close(peerChannelID: channelID!)))
+
+        // This should trigger no message.
+        // (Actually, it should trigger a close, but that's a different bug and to be fixed in a different patch.)
+        XCTAssertEqual(harness.flushedMessages.count, 1)
+    }
+
     func testRespectingMaxMessageSize() throws {
         let harness = self.harnessForbiddingInboundChannels()
         defer {
@@ -1694,3 +1729,12 @@ final class ChildChannelMultiplexerTests: XCTestCase {
         self.assertChannelOpenConfirmation(harness.flushedMessages.first, recipientChannel: 1)
     }
 }
+
+extension ByteBuffer {
+    /// A buffer `(1 << 24) + 1` bytes large.
+    fileprivate static let bigBuffer: ByteBuffer = {
+        // The default window size is 1<<24 bytes. Sadly, we need a buffer that size.
+        // We store it in a static so that we don't have to re-create it for every test.
+        ByteBuffer(repeating: 0, count: (1 << 24) + 1)
+    }()
+}

scripts/soundness.sh

@@ -18,7 +18,7 @@ here="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
 
 function replace_acceptable_years() {
     # this needs to replace all acceptable forms with 'YEARS'
-    sed -e 's/2017-20[12][890]/YEARS/' -e 's/2019-2020/YEARS/' -e 's/2019/YEARS/' -e 's/2020/YEARS/' -e 's/2021/YEARS/'
+    sed -e 's/20[12][78901]-20[12][8901]/YEARS/' -e 's/2019-2020/YEARS/' -e 's/2019/YEARS/' -e 's/2020/YEARS/' -e 's/2021/YEARS/'
 }
 
 command -v swiftformat >/dev/null 2>&1 || { echo >&2 "swiftformat needs to be installed but is not available in the path."; exit 1; }