Repository: netty/netty
Issue: HTTP2 server incorrectly sends preface string

We discovered this problem when we found the OkHttp client couldn't talk to the netty server.

https://tools.ietf.org/html/draft-ietf-httpbis-http2-10#section-3.5

The wording of the spec is slightly confusing, but only the client sends the magic preface string that begins with "PRI *".

The server connection header consists of just a SETTINGS frame
(Section 6.5) that MUST be the first frame the server sends in the
 HTTP/2 connection.

I'm happy to tackle this but would like to discuss a couple of alternatives.

1) The simple solution is to wire a "boolean server"  down through to Http2FrameEncoder and Http2FrameDecoder so they know whether they are client or server.  I hacked this together and it's straightforward, but kind of ugly.

2) The "nicer"(?) solution might be to split out the preface sending / receiving feature from Http2FrameEncoder and Http2FrameDecoder entirely, in favor of an explicit Http2PrefaceWriter and Http2PrefaceReader you add to the pipeline, which do their thing and then remove themselves.

Thoughts?

@normanmaurer @nmittler @adriancole @jh


Issue comments:
There are a couple places where client/server context is important, so it might end up propagating itself anyway.

For example, push_promise setting should only be sent by the client, push promises should only be sent by the server. client vs server stream ids (already handled I think).

That said, I don't mind however this is implemented, as long as it becomes implemented!

I went with option 2 to see what that would look like, and I kind of like the result.

Fixed by #2403


Pull request review comments:
This is to prevent control from passing on if we're still reading the preface.  Could probably change this to checking if any bytes are left in the incoming ByteBuf that haven't been consumed yet.

Note!  Advice wanted.

nit accidental wildcard

IntelliJ keeps doing this automatically no matter what I do, but only for the Netty project.  I assume this is actually what the Netty style settings want?

NM: not accidental!!  lots of this in the netty codebase... ignore me.

Maybe both ?

You could just use `preface.isReadable()` :)

Both of which of the 3?
- Close channel
- Throw exception synchronously up the I/O stack
- ctx.fire?

Nice, will fix.

So I debugged through and it turns out simply throwing a synchronous exception does exactly what you'd want.

1) It's caught by the catch in ChannelHandlerInvokerUtil.invokeChannelReadNow which fires the exception down the handler chain.

2) Http2ConnectionHandler.exceptionCaught sees the connection error, sends a GoAway frame, and closes the channel.

So I think anything other than synchronous throw is redundant here, assuming the handler chain is already setup.

You need to call ((ByteBuf) msg).release() before you break to not introduce a buffer leak here.

Oh, I see.  Because I'm not sending it on down the pipeline, right?

Exactly :)

-- 
Norman Maurer

Am 17. April 2014 bei 21:28:19, Scott Blum (notifications@github.com) schrieb:

In codec-http2/src/main/java/io/netty/handler/codec/http2/draft10/frame/decoder/Http2ServerPrefaceReader.java:

> +
> +/**
> - \* Reads the initial client preface, then removes itself from the pipeline.
> - \* Only the server pipeline should do this.
> - *
> - \* https://tools.ietf.org/html/draft-ietf-httpbis-http2-10#section-3.5
> - */
>   +public class Http2ServerPrefaceReader extends ChannelHandlerAdapter {
>   +
> -    private final ByteBuf preface = connectionPrefaceBuf();
>   +
> -    @Override
> -    public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception {
> -        if (preface.isReadable() && msg instanceof ByteBuf) {
> -            processHttp2Preface(ctx, (ByteBuf) msg);
> -            if (preface.isReadable()) {
>   Oh, I see. Because I'm not sending it on down the pipeline, right?

—
Reply to this email directly or view it on GitHub.


Patch that resolved the issue:
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/draft10/Http2OrHttpChooser.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/draft10/Http2OrHttpChooser.java
index db51b7c8572..765a09c026e 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/draft10/Http2OrHttpChooser.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/draft10/Http2OrHttpChooser.java
@@ -25,6 +25,7 @@
 import io.netty.handler.codec.http.HttpResponseEncoder;
 import io.netty.handler.codec.http2.draft10.connection.Http2ConnectionHandler;
 import io.netty.handler.codec.http2.draft10.frame.Http2FrameCodec;
+import io.netty.handler.codec.http2.draft10.frame.decoder.Http2ServerPrefaceReader;
 import io.netty.handler.ssl.SslHandler;
 
 import javax.net.ssl.SSLEngine;
@@ -129,6 +130,7 @@ private boolean initPipeline(ChannelHandlerContext ctx) {
      */
     protected void addHttp2Handlers(ChannelHandlerContext ctx) {
         ChannelPipeline pipeline = ctx.pipeline();
+        pipeline.addLast("http2ServerPrefaceReader", new Http2ServerPrefaceReader());
         pipeline.addLast("http2FrameCodec", new Http2FrameCodec());
         pipeline.addLast("http2ConnectionHandler", new Http2ConnectionHandler(true));
         pipeline.addLast("http2RequestHandler", createHttp2RequestHandler());
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/draft10/connection/Http2ConnectionHandler.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/draft10/connection/Http2ConnectionHandler.java
index 43edec66490..ad01a45eb55 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/draft10/connection/Http2ConnectionHandler.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/draft10/connection/Http2ConnectionHandler.java
@@ -15,16 +15,6 @@
 
 package io.netty.handler.codec.http2.draft10.connection;
 
-import static io.netty.handler.codec.http2.draft10.Http2Error.PROTOCOL_ERROR;
-import static io.netty.handler.codec.http2.draft10.Http2Error.STREAM_CLOSED;
-import static io.netty.handler.codec.http2.draft10.Http2Exception.format;
-import static io.netty.handler.codec.http2.draft10.Http2Exception.protocolError;
-import static io.netty.handler.codec.http2.draft10.connection.Http2ConnectionUtil.toHttp2Exception;
-import static io.netty.handler.codec.http2.draft10.connection.Http2Stream.State.HALF_CLOSED_LOCAL;
-import static io.netty.handler.codec.http2.draft10.connection.Http2Stream.State.HALF_CLOSED_REMOTE;
-import static io.netty.handler.codec.http2.draft10.connection.Http2Stream.State.OPEN;
-import static io.netty.handler.codec.http2.draft10.connection.Http2Stream.State.RESERVED_LOCAL;
-import static io.netty.handler.codec.http2.draft10.connection.Http2Stream.State.RESERVED_REMOTE;
 import io.netty.channel.ChannelFuture;
 import io.netty.channel.ChannelFutureListener;
 import io.netty.channel.ChannelHandlerAdapter;
@@ -48,6 +38,11 @@
 import io.netty.handler.codec.http2.draft10.frame.Http2WindowUpdateFrame;
 import io.netty.util.ReferenceCountUtil;
 
+import static io.netty.handler.codec.http2.draft10.Http2Error.*;
+import static io.netty.handler.codec.http2.draft10.Http2Exception.*;
+import static io.netty.handler.codec.http2.draft10.connection.Http2ConnectionUtil.*;
+import static io.netty.handler.codec.http2.draft10.connection.Http2Stream.State.*;
+
 /**
  * Handler for HTTP/2 connection state. Manages inbound and outbound flow control for data frames.
  * Handles error conditions as defined by the HTTP/2 spec and controls appropriate shutdown of the
@@ -153,7 +148,7 @@ public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) throws E
             processHttp2Exception(ctx, (Http2Exception) cause);
         }
 
-        ctx.fireExceptionCaught(cause);
+        super.exceptionCaught(ctx, cause);
     }
 
     @Override
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/draft10/frame/decoder/Http2FrameDecoder.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/draft10/frame/decoder/Http2FrameDecoder.java
index 73694c8e461..cd81870d77d 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/draft10/frame/decoder/Http2FrameDecoder.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/draft10/frame/decoder/Http2FrameDecoder.java
@@ -15,12 +15,6 @@
 
 package io.netty.handler.codec.http2.draft10.frame.decoder;
 
-import static io.netty.handler.codec.http2.draft10.Http2Error.PROTOCOL_ERROR;
-import static io.netty.handler.codec.http2.draft10.Http2Exception.format;
-import static io.netty.handler.codec.http2.draft10.frame.Http2FrameCodecUtil.FRAME_HEADER_LENGTH;
-import static io.netty.handler.codec.http2.draft10.frame.Http2FrameCodecUtil.FRAME_LENGTH_MASK;
-import static io.netty.handler.codec.http2.draft10.frame.Http2FrameCodecUtil.connectionPrefaceBuf;
-import static io.netty.handler.codec.http2.draft10.frame.Http2FrameCodecUtil.readUnsignedInt;
 import io.netty.buffer.ByteBuf;
 import io.netty.channel.ChannelHandlerContext;
 import io.netty.handler.codec.ByteToMessageDecoder;
@@ -31,6 +25,8 @@
 
 import java.util.List;
 
+import static io.netty.handler.codec.http2.draft10.frame.Http2FrameCodecUtil.*;
+
 /**
  * Decodes {@link Http2Frame} objects from an input {@link ByteBuf}. The frames that this handler
  * emits can be configured by providing a {@link Http2FrameUnmarshaller}. By default, the
@@ -42,14 +38,12 @@
 public class Http2FrameDecoder extends ByteToMessageDecoder {
 
     private enum State {
-        PREFACE,
         FRAME_HEADER,
         FRAME_PAYLOAD,
         ERROR
     }
 
     private final Http2FrameUnmarshaller frameUnmarshaller;
-    private final ByteBuf preface;
     private State state;
     private int payloadLength;
 
@@ -62,23 +56,13 @@ public Http2FrameDecoder(Http2FrameUnmarshaller frameUnmarshaller) {
             throw new NullPointerException("frameUnmarshaller");
         }
         this.frameUnmarshaller = frameUnmarshaller;
-        preface = connectionPrefaceBuf();
-        state = State.PREFACE;
+        state = State.FRAME_HEADER;
     }
 
     @Override
     protected void decode(ChannelHandlerContext ctx, ByteBuf in, List<Object> out) throws Exception {
         try {
             switch (state) {
-            case PREFACE:
-                processHttp2Preface(ctx, in);
-                if (state == State.PREFACE) {
-                    // Still processing the preface.
-                    break;
-                }
-
-                // Successfully processed the HTTP2 preface.
-
             case FRAME_HEADER:
                 processFrameHeader(in);
                 if (state == State.FRAME_HEADER) {
@@ -104,30 +88,6 @@ protected void decode(ChannelHandlerContext ctx, ByteBuf in, List<Object> out) t
         }
     }
 
-    private void processHttp2Preface(ChannelHandlerContext ctx, ByteBuf in) throws Http2Exception {
-        int prefaceRemaining = preface.readableBytes();
-        int bytesRead = Math.min(in.readableBytes(), prefaceRemaining);
-
-        // Read the portion of the input up to the length of the preface, if reached.
-        ByteBuf sourceSlice = in.readSlice(bytesRead);
-
-        // Read the same number of bytes from the preface buffer.
-        ByteBuf prefaceSlice = preface.readSlice(bytesRead);
-
-        // If the input so far doesn't match the preface, break the connection.
-        if (bytesRead == 0 || !prefaceSlice.equals(sourceSlice)) {
-            throw format(PROTOCOL_ERROR, "Invalid HTTP2 preface");
-        }
-
-        if ((prefaceRemaining - bytesRead) > 0) {
-            // Wait until the entire preface has arrived.
-            return;
-        }
-
-        // Start processing the first header.
-        state = State.FRAME_HEADER;
-    }
-
     private void processFrameHeader(ByteBuf in) throws Http2Exception {
         if (in.readableBytes() < FRAME_HEADER_LENGTH) {
             // Wait until the entire frame header has been read.
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/draft10/frame/decoder/Http2ServerPrefaceReader.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/draft10/frame/decoder/Http2ServerPrefaceReader.java
new file mode 100644
index 00000000000..582eda39805
--- /dev/null
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/draft10/frame/decoder/Http2ServerPrefaceReader.java
@@ -0,0 +1,72 @@
+/*
+ * Copyright 2014 The Netty Project
+ *
+ * The Netty Project licenses this file to you under the Apache License, version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at:
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package io.netty.handler.codec.http2.draft10.frame.decoder;
+
+import io.netty.buffer.ByteBuf;
+import io.netty.channel.ChannelHandlerAdapter;
+import io.netty.channel.ChannelHandlerContext;
+import io.netty.handler.codec.http2.draft10.Http2Exception;
+
+import static io.netty.handler.codec.http2.draft10.Http2Error.*;
+import static io.netty.handler.codec.http2.draft10.Http2Exception.*;
+import static io.netty.handler.codec.http2.draft10.frame.Http2FrameCodecUtil.*;
+
+/**
+ * Reads the initial client preface, then removes itself from the pipeline.
+ * Only the server pipeline should do this.
+ *
+ * https://tools.ietf.org/html/draft-ietf-httpbis-http2-10#section-3.5
+ */
+public class Http2ServerPrefaceReader extends ChannelHandlerAdapter {
+
+    private final ByteBuf preface = connectionPrefaceBuf();
+
+    @Override
+    public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception {
+        if (preface.isReadable() && msg instanceof ByteBuf) {
+            ByteBuf buf = (ByteBuf) msg;
+            processHttp2Preface(ctx, buf);
+            if (preface.isReadable()) {
+                // More preface left to process.
+                buf.release();
+                return;
+            }
+        }
+        super.channelRead(ctx, msg);
+    }
+
+    private void processHttp2Preface(ChannelHandlerContext ctx, ByteBuf in) throws Http2Exception {
+        int prefaceRemaining = preface.readableBytes();
+        int bytesRead = Math.min(in.readableBytes(), prefaceRemaining);
+
+        // Read the portion of the input up to the length of the preface, if reached.
+        ByteBuf sourceSlice = in.readSlice(bytesRead);
+
+        // Read the same number of bytes from the preface buffer.
+        ByteBuf prefaceSlice = preface.readSlice(bytesRead);
+
+        // If the input so far doesn't match the preface, break the connection.
+        if (bytesRead == 0 || !prefaceSlice.equals(sourceSlice)) {
+            throw format(PROTOCOL_ERROR, "Invalid HTTP2 preface");
+        }
+
+        if (!preface.isReadable()) {
+            // Entire preface has been read, remove ourselves from the pipeline.
+            ctx.pipeline().remove(this);
+        }
+    }
+
+}
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/draft10/frame/encoder/Http2ClientPrefaceWriter.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/draft10/frame/encoder/Http2ClientPrefaceWriter.java
new file mode 100644
index 00000000000..fcb7745a91d
--- /dev/null
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/draft10/frame/encoder/Http2ClientPrefaceWriter.java
@@ -0,0 +1,73 @@
+/*
+ * Copyright 2014 The Netty Project
+ *
+ * The Netty Project licenses this file to you under the Apache License, version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at:
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package io.netty.handler.codec.http2.draft10.frame.encoder;
+
+import io.netty.channel.ChannelFuture;
+import io.netty.channel.ChannelFutureListener;
+import io.netty.channel.ChannelHandlerAdapter;
+import io.netty.channel.ChannelHandlerContext;
+
+import static io.netty.handler.codec.http2.draft10.frame.Http2FrameCodecUtil.*;
+
+/**
+ * Sends the initial client preface, then removes itself from the pipeline.
+ * Only the client pipeline should do this.
+ *
+ * https://tools.ietf.org/html/draft-ietf-httpbis-http2-10#section-3.5
+ */
+public class Http2ClientPrefaceWriter extends ChannelHandlerAdapter {
+
+    private boolean prefaceWritten;
+
+    public Http2ClientPrefaceWriter() {
+    }
+
+    @Override
+    public void channelActive(ChannelHandlerContext ctx) throws Exception {
+        // The channel just became active - send the HTTP2 connection preface to the remote
+        // endpoint.
+        sendPreface(ctx);
+
+        super.channelActive(ctx);
+    }
+
+    @Override
+    public void handlerAdded(ChannelHandlerContext ctx) throws Exception {
+        // This handler was just added to the context. In case it was handled after
+        // the connection became active, send the HTTP2 connection preface now.
+        sendPreface(ctx);
+    }
+
+    /**
+     * Sends the HTTP2 connection preface to the remote endpoint, if not already sent.
+     */
+    private void sendPreface(final ChannelHandlerContext ctx) {
+        if (!prefaceWritten && ctx.channel().isActive()) {
+            prefaceWritten = true;
+            ctx.writeAndFlush(connectionPrefaceBuf()).addListener(new ChannelFutureListener() {
+                @Override
+                public void operationComplete(ChannelFuture future) throws Exception {
+                    if (!future.isSuccess() && ctx.channel().isOpen()) {
+                        // The write failed, close the connection.
+                        ctx.close();
+                    } else {
+                        ctx.pipeline().remove(Http2ClientPrefaceWriter.this);
+                    }
+                }
+            });
+        }
+    }
+}
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/draft10/frame/encoder/Http2FrameEncoder.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/draft10/frame/encoder/Http2FrameEncoder.java
index 24e2015a910..38ab272076d 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/draft10/frame/encoder/Http2FrameEncoder.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/draft10/frame/encoder/Http2FrameEncoder.java
@@ -15,10 +15,7 @@
 
 package io.netty.handler.codec.http2.draft10.frame.encoder;
 
-import static io.netty.handler.codec.http2.draft10.frame.Http2FrameCodecUtil.connectionPrefaceBuf;
 import io.netty.buffer.ByteBuf;
-import io.netty.channel.ChannelFuture;
-import io.netty.channel.ChannelFutureListener;
 import io.netty.channel.ChannelHandlerContext;
 import io.netty.handler.codec.MessageToByteEncoder;
 import io.netty.handler.codec.http2.draft10.frame.Http2Frame;
@@ -33,7 +30,6 @@
 public class Http2FrameEncoder extends MessageToByteEncoder<Http2Frame> {
 
     private final Http2FrameMarshaller frameMarshaller;
-    private boolean prefaceWritten;
 
     public Http2FrameEncoder() {
         this(new Http2StandardFrameMarshaller());
@@ -46,22 +42,6 @@ public Http2FrameEncoder(Http2FrameMarshaller frameMarshaller) {
         this.frameMarshaller = frameMarshaller;
     }
 
-    @Override
-    public void channelActive(ChannelHandlerContext ctx) throws Exception {
-        // The channel just became active - send the HTTP2 connection preface to the remote
-        // endpoint.
-        sendPreface(ctx);
-
-        super.channelActive(ctx);
-    }
-
-    @Override
-    public void handlerAdded(ChannelHandlerContext ctx) throws Exception {
-        // This handler was just added to the context. In case it was handled after
-        // the connection became active, send the HTTP2 connection preface now.
-        sendPreface(ctx);
-    }
-
     @Override
     protected void encode(ChannelHandlerContext ctx, Http2Frame frame, ByteBuf out)
             throws Exception {
@@ -71,22 +51,4 @@ protected void encode(ChannelHandlerContext ctx, Http2Frame frame, ByteBuf out)
             ctx.fireExceptionCaught(t);
         }
     }
-
-    /**
-     * Sends the HTTP2 connection preface to the remote endpoint, if not already sent.
-     */
-    private void sendPreface(final ChannelHandlerContext ctx) {
-        if (!prefaceWritten && ctx.channel().isActive()) {
-            prefaceWritten = true;
-            ctx.writeAndFlush(connectionPrefaceBuf()).addListener(new ChannelFutureListener() {
-                @Override
-                public void operationComplete(ChannelFuture future) throws Exception {
-                    if (!future.isSuccess() && ctx.channel().isOpen()) {
-                        // The write failed, close the connection.
-                        ctx.close();
-                    }
-                }
-            });
-        }
-    }
 }
diff --git a/example/src/main/java/io/netty/example/http2/client/Http2ClientInitializer.java b/example/src/main/java/io/netty/example/http2/client/Http2ClientInitializer.java
index 5efee94a995..9b515a593e1 100644
--- a/example/src/main/java/io/netty/example/http2/client/Http2ClientInitializer.java
+++ b/example/src/main/java/io/netty/example/http2/client/Http2ClientInitializer.java
@@ -15,7 +15,6 @@
  */
 package io.netty.example.http2.client;
 
-import static io.netty.util.internal.logging.InternalLogLevel.INFO;
 import io.netty.channel.ChannelInitializer;
 import io.netty.channel.ChannelPipeline;
 import io.netty.channel.SimpleChannelInboundHandler;
@@ -24,11 +23,14 @@
 import io.netty.handler.codec.http2.draft10.connection.Http2ConnectionHandler;
 import io.netty.handler.codec.http2.draft10.frame.Http2DataFrame;
 import io.netty.handler.codec.http2.draft10.frame.Http2FrameCodec;
+import io.netty.handler.codec.http2.draft10.frame.encoder.Http2ClientPrefaceWriter;
 import io.netty.handler.ssl.SslHandler;
 import org.eclipse.jetty.npn.NextProtoNego;
 
 import javax.net.ssl.SSLEngine;
 
+import static io.netty.util.internal.logging.InternalLogLevel.*;
+
 /**
  * Configures the client pipeline to support HTTP/2 frames.
  */
@@ -50,6 +52,7 @@ public void initChannel(SocketChannel ch) throws Exception {
         ChannelPipeline pipeline = ch.pipeline();
 
         pipeline.addLast("ssl", new SslHandler(engine));
+        pipeline.addLast("http2ClientPrefaceWriter", new Http2ClientPrefaceWriter());
         pipeline.addLast("http2FrameCodec", new Http2FrameCodec());
         pipeline.addLast("http2FrameLogger", new Http2FrameLogger(INFO));
         pipeline.addLast("http2ConnectionHandler", new Http2ConnectionHandler(false));


Test patch: