Poorly-formed requests crash the NIO server

[timburks]

moving from #350 (comment).

New Issue Checklist

• Updated grpc-swift to the latest version
• I read the Contribution Guidelines
• I searched for existing GitHub issues(especially, there are many resolved issues related to SSL)

Issue Description

A SwiftNIO-based gRPC server can crash when sent an empty HTTP request, e.g. curl http://localhost:8080:

Complete output when running grpc-swift, including the stack trace and command used

$ swift run EchoNIO serve
starting insecure server
Precondition failed: : file /Users/timburks/Desktop/grpc-swift/.build/checkouts/swift-nio.git-3446246794268775558/Sources/NIOHTTP1/HTTPServerProtocolErrorHandler.swift, line 67
Illegal instruction: 4

As noted by @Lukasa:

That's a NIO precondition, and it gets hit if the HTTPServerRequestParts are being sent out-of-order. Specifically, in this case, we received a sequence of parts where we received a .end before a .head.

Environment

Key
Value




OS Version
Mac OS 10.14.3


Swift Version
4.2.1


gRPC-Swift Version
0.7.0 / master



  

[MrMage]

@Lukasa I think clients sending malformed (or simply unexpected) requests should not bring down the NIO server process. Do you have any guidance on how we could improve this behavior?

[Lukasa]

@MrMage As I said in #350, this does not happen when clients send malformed requests.

This occurs when the user (which is to say, grpc-swift, the server, code under the control of this project right here) attempts to send a malformed response. The precondition is in a write, so only occurs when the grpc-swift attempts to send data in an invalid order. In this case, a quick check of the crashing line reveals that grpc-swift is sending .end without preceding .head part. This is a particular surprise because it's a .end with trailers attached. This is not something SwiftNIO can render (where is the response header?) so we crash instead.

Checking the call stack shows that the following situation happens:

1. We receive a HTTP/1 request. HTTPProtocolSwitcher configures a http1 pipeline and adds a WebCORSHandler. It passes on the request data.
2. HTTPDecoder receives the request and decodes a .head. That is forwarded on.
3. The WebCORSHandler receives this request. It decodes the request head, but it's not OPTIONS so it just passes the data on.
4. The HTTP1ToRawGRPCServerCodec receives the request head. It also passes the data on.
5. GRPCChannelHandler receives the request head. It looks for a GRPC-formatted request URI and doesn't get one. It calls: ctx.writeAndFlush(self.wrapOutboundOut(.status(.unimplemented(method: requestHead.uri))), promise: nil)
6. This write is received by HTTP1ToRawGRPCServerCodec. It has a complex write method, but we're writing a .status. This block expects only to send trailers, so it translates this write into a single .end. This is invalid, and is the source of your error.
7. The write passes through the WebCORSHandler without change.
8. It gets to HTTPServerProtocolErrorHandler, which is keeping track of the state of the connection so it knows whether it can send 4XX responses on decode errors. This handler complains that the connection is transitioning to an unknown state due to an invalid send order, and hits the precondition.

The TL;DR is that GRPCChannelHandler violates an invariant that HTTP1ToRawGRPCServerCodec expects. HTTP1ToRawGRPCServerCodec (implicitly) expects that a .status message will always be preceded by a .headers message, but GRPCChannelHandler does not do that when it receives a GRPC request with a malformed request URI. That's not acceptable, and leads to our crash. It should be sufficient to send a .headers response back first.

Alternatively, if you want to support a "trailers-only" mode in this case, the GRPCChannelHandler can keep track of whether it is partway through a response when it receives .status. If it isn't, it can instead just write a .headers followed by .end. Note that this probably won't do what you want for the HTTP/2 case though, so you may find it better to simply send a do-nothing header block and then immediately send the trailers.

[MrMage]

@Lukasa sorry, my bad! I had misunderstood that the client caused the crash, not the library user. The crash was based on me not fully thinking through the response flow. Thank you for taking the time to outline this.

If it isn't, it can instead just write a .headers followed by .end. Note that this probably won't do what you want for the HTTP/2 case though, so you may find it better to simply send a do-nothing header block and then immediately send the trailers.

Your suggested solution sounds like the right fit; I will try that once George's error handling PR has been merged.

Just for clarification — the first option would be to send the status in a headers block, followed by an empty end, while the second option would be more like an empty headers block, with the status in the end block? In that case it indeed sounds like the latter option is what we want.

[Lukasa]

Yes, that's correct. The second option works better right now because the HTTP2ToHTTP1Codec does not currently know how to coalesce .headers followed by .end into a single HEADERS(END_STREAM) frame.

[MrMage]

Yes, that's correct. The second option works better right now because the HTTP2ToHTTP1Codec does not currently know how to coalesce .headers followed by .end into a single HEADERS(END_STREAM) frame.

Sounds reasonable; I wouldn't even be sure if that would be the codec's task. I will look at the latter option, then.

[glbrntt]

@MrMage #364 should fix this.

[MrMage]

@glbrntt even better, thank you! Sorry, lost track of all the things in there ;-)