Vapor forgets about an active request if the user returns an error whilst streaming the body (leading to file descriptor leaks)

[weissi]

Description

With Vapor 4.76.0 (and presumably all(?) earlier versions) it's quite easy to leak a NIO Channel and a file descriptor because Vapor essentially forgets the active HTTP request. This might be used as a denial-of-service vector if a route that will consistently error asynchronously can be found easily.

Problem in more detail

The issue is that in this code in HTTPServerRequestDecoder.swift

    mutating func didError(_ error: Error) -> Result {
        switch self.state {
        case .idle:
            self.state = .error(error)
            return .init(action: .close(error), callRead: false)
        case .writing:
            self.state = .error(error)
            return .init(action: .nothing, callRead: false)
        case .error:
            return .init(action: .nothing, callRead: false)
        }
    }

decides that when didError happens we are not going to call read (callRead: false) and when in .writing (which means streaming data to request.body.drain) it also sets the action to .nothing.

The result is that because we in error, we won't run any more user code, we are also not reading or closing.

This however means that the only way for us to get rid of that Channel/socket is if NIO itself can detect that the connection went away. That's not reliably possible because the remote might have just abandoned us, so no more packets will ever arrive. On macOS / iOS even in the face of a FIN/RST NIO won't be able to learn about this because it can only do so reliably if it's registered for reads (we aren't, not reading) or writes (we aren't, not writing).

Long story short: Vapor forgets about this request and NIO can't help it in all situations. Or I guess one could say that Vapor wants a kick from NIO (some read/write error or connection closure) but NIO can't necessarily do that because 1) the remote peer might just abandon us (without RST/FIN) and on Darwin NIO can't reliably detect RST/FIN if nothing's reading or writing.

Possible fix

It depends on what semantics Vapor wants. Probably it might be as easy as doing action: .close when .error whilst .writing. I don't think these errors are recoverable so closing it probably the best idea.

Not sure what exact semantics Vapor wants. If Vapor wants this to be recoverable, then it would need to set callRead: true but immediately dump any further body data received after entering .error.

In general callRead: false should probably not be used in terminal states like .error or .end. Instead, if receiving data in .error we should dump the data or just close the connection/stream.

Reproduction:

For this route

app.on(.POST, "error-later", ":delay", body: .stream) { request in
    let delayMS = request.parameters.get("delay", as: Int64.self) ?? 2_000
    let delay = TimeAmount.milliseconds(delayMS)
    print("received", request)
    var haveWritten = false
    return Response(status: .ok, version: request.version, body: .init(stream: { bodyWriter in
        request.body.drain { chunk in
            switch chunk {
            case .buffer(let buffer):
                if !haveWritten {
                    haveWritten = true
                    print("received first \(buffer.readableBytes) bytes of the request. Scheduling write in \(delay).")
                    return request.eventLoop.flatScheduleTask(in: delay) {
                        struct E: Error {}
                        return request.eventLoop.makeFailedFuture(E())
                    }.futureResult
                } else {
                    print("subsequence \(buffer.readableBytes) bytes")
                    return request.eventLoop.makeSucceededFuture(())
                }
            case .end:
                print("received end", request)
                return bodyWriter.write(.end)
            case .error(let error):
                print("received error", error, request)
                return bodyWriter.write(.end)
            }
        }
    }))
}

run

curl -v -m 1 -d "$(cat /dev/zero | head -c 1000000 | tr '\0' Y)" http://localhost:8080/error-later/2000

maybe 10 times and after a minute or so run (replace VaporHello with your binary name)

lsof -Pn -p $(pgrep VaporHello) | grep TCP; lskq -p $(pgrep VaporHello) | grep SOCKET

You'll likely see something like

$ lsof -Pn -p $(pgrep VaporHello) | grep TCP; lskq -p $(pgrep VaporHello) | grep SOCKET
VaporHell 79873 johannes   14u    IPv4 0x3925c7c9375db4d1        0t0                 TCP *:8080 (LISTEN)
VaporHell 79873 johannes   15u    IPv4 0x3925c7c93a385eb1        0t0                 TCP 127.0.0.1:8080->127.0.0.1:54526 (CLOSED)
VaporHell 79873 johannes   16u    IPv4 0x3925c7c9375dbfe1        0t0                 TCP 127.0.0.1:8080->127.0.0.1:54528 (CLOSED)
VaporHell 79873 johannes   17u    IPv4 0x3925c7c93a1174d1        0t0                 TCP 127.0.0.1:8080->127.0.0.1:54529 (CLOSED)
VaporHell 79873 johannes   18u    IPv4 0x3925c7c93a12deb1        0t0                 TCP 127.0.0.1:8080->127.0.0.1:55109 (CLOSED)
VaporHell 79873 johannes   19u    IPv4 0x3925c7c9376d74d1        0t0                 TCP 127.0.0.1:8080->127.0.0.1:55110 (CLOSED)
VaporHell 79873 johannes   20u    IPv4 0x3925c7c939f869c1        0t0                 TCP 127.0.0.1:8080->127.0.0.1:55111 (CLOSED)
VaporHell 79873 johannes   21u    IPv4 0x3925c7c937789eb1        0t0                 TCP 127.0.0.1:8080->127.0.0.1:55112 (CLOSED)
VaporHell 79873 johannes   22u    IPv4 0x3925c7c93760d601        0t0                 TCP 127.0.0.1:8080->127.0.0.1:55114 (CLOSED)
VaporHell 79873 johannes   23u    IPv4 0x3925c7c9375da9c1        0t0                 TCP 127.0.0.1:8080->127.0.0.1:55117 (CLOSED)
VaporHell 79873 johannes   24u    IPv4 0x3925c7c939f87fe1        0t0                 TCP 127.0.0.1:8080->127.0.0.1:55119 (CLOSED)
VaporHello           79873 fd               6  k--                 14 READ      SOCKET   -       a--- ---- ----- ----- ---- ---  --                  0 

As we can see, we have 10 file descriptors to sockets that are CLOSED. In the real world they may also appear as ESTABLISHED because if the remote peer just abandoned it, the kernel also doesn't know it's gone...