Various fixes for compliance with HTTP/2 spec

Motivation:

A few items were identified where the http2 codec is out of compliance
with the spec.

Modifications:

- Fixed handling of priority weight on the wire. Now adding 1 after
reading from the wire and subtracing 1 before writing.

- Fixed handling of next stream ID. Client streamIds were starting at 3,
but they need to start at 1 to allow the upgrade from HTTP/1.1. Also
making next stream ID logic more flexible. Allowing the next created
stream to be any number in the sequence following the previously created
stream.

- Disallowing SETTINGS frames with ENABLE_PUSH specified for server
endpoints. This means that attempts to write this frame from a server,
or read it from a client will fail.

Result:

The http2 implementation will be more inline with the spec.
This commit is contained in:
nmittler 2014-05-09 06:03:52 -07:00 committed by Norman Maurer
parent c62edc87a5
commit e22aed284b
12 changed files with 164 additions and 71 deletions

View File

@ -75,8 +75,7 @@ public abstract class AbstractHttp2ConnectionHandler extends ByteToMessageDecode
}
protected AbstractHttp2ConnectionHandler(Http2Connection connection) {
this(connection, new DefaultHttp2FrameReader(connection.isServer()),
new DefaultHttp2FrameWriter(connection.isServer()),
this(connection, new DefaultHttp2FrameReader(), new DefaultHttp2FrameWriter(),
new DefaultHttp2InboundFlowController(), new DefaultHttp2OutboundFlowController());
}
@ -165,9 +164,12 @@ public abstract class AbstractHttp2ConnectionHandler extends ByteToMessageDecode
Http2Settings settings = new Http2Settings();
settings.allowCompressedData(connection.local().allowCompressedData());
settings.initialWindowSize(inboundFlow.initialInboundWindowSize());
settings.pushEnabled(connection.local().allowPushTo());
settings.maxConcurrentStreams(connection.remote().maxStreams());
settings.maxHeaderTableSize(frameReader.maxHeaderTableSize());
if (!connection.isServer()) {
// Only set the pushEnabled flag if this is a client endpoint.
settings.pushEnabled(connection.local().allowPushTo());
}
return settings;
}
@ -295,12 +297,15 @@ public abstract class AbstractHttp2ConnectionHandler extends ByteToMessageDecode
throw protocolError("Sending settings after connection going away.");
}
if (settings.hasAllowCompressedData()) {
connection.local().allowCompressedData(settings.allowCompressedData());
if (settings.hasPushEnabled()) {
if (connection.isServer()) {
throw protocolError("Server sending SETTINGS frame with ENABLE_PUSH specified");
}
connection.local().allowPushTo(settings.pushEnabled());
}
if (settings.hasPushEnabled()) {
connection.local().allowPushTo(settings.pushEnabled());
if (settings.hasAllowCompressedData()) {
connection.local().allowCompressedData(settings.allowCompressedData());
}
if (settings.hasMaxConcurrentStreams()) {
@ -358,6 +363,9 @@ public abstract class AbstractHttp2ConnectionHandler extends ByteToMessageDecode
protected ChannelFuture writeAltSvc(ChannelHandlerContext ctx, ChannelPromise promise,
int streamId, long maxAge, int port, ByteBuf protocolId, String host, String origin)
throws Http2Exception {
if (!connection.isServer()) {
throw protocolError("Client sending ALT_SVC frame");
}
return frameWriter.writeAltSvc(ctx, promise, streamId, maxAge, port, protocolId, host,
origin);
}
@ -658,6 +666,13 @@ public abstract class AbstractHttp2ConnectionHandler extends ByteToMessageDecode
@Override
public void onSettingsRead(ChannelHandlerContext ctx, Http2Settings settings)
throws Http2Exception {
if (settings.hasPushEnabled()) {
if (!connection.isServer()) {
throw protocolError("Client received SETTINGS frame with ENABLE_PUSH specified");
}
connection.remote().allowPushTo(settings.pushEnabled());
}
if (settings.hasAllowCompressedData()) {
connection.remote().allowCompressedData(settings.allowCompressedData());
}
@ -666,10 +681,6 @@ public abstract class AbstractHttp2ConnectionHandler extends ByteToMessageDecode
connection.local().maxStreams(settings.maxConcurrentStreams());
}
if (settings.hasPushEnabled()) {
connection.remote().allowPushTo(settings.pushEnabled());
}
if (settings.hasMaxHeaderTableSize()) {
frameWriter.maxHeaderTableSize(settings.maxHeaderTableSize());
}
@ -759,6 +770,9 @@ public abstract class AbstractHttp2ConnectionHandler extends ByteToMessageDecode
@Override
public void onAltSvcRead(ChannelHandlerContext ctx, int streamId, long maxAge, int port,
ByteBuf protocolId, String host, String origin) throws Http2Exception {
if (connection.isServer()) {
throw protocolError("Server received ALT_SVC frame");
}
AbstractHttp2ConnectionHandler.this.onAltSvcRead(ctx, streamId, maxAge, port,
protocolId, host, origin);
}

View File

@ -41,17 +41,15 @@ public class DefaultHttp2Connection implements Http2Connection {
private final DefaultEndpoint remoteEndpoint;
private boolean goAwaySent;
private boolean goAwayReceived;
private boolean server;
public DefaultHttp2Connection(boolean server, boolean allowCompressedData) {
this.server = server;
localEndpoint = new DefaultEndpoint(server, allowCompressedData);
remoteEndpoint = new DefaultEndpoint(!server, false);
}
@Override
public boolean isServer() {
return server;
return localEndpoint.isServer();
}
@Override
@ -216,19 +214,26 @@ public class DefaultHttp2Connection implements Http2Connection {
* Simple endpoint implementation.
*/
private final class DefaultEndpoint implements Endpoint {
private final boolean server;
private int nextStreamId;
private int lastStreamCreated;
private int maxStreams = Integer.MAX_VALUE;
private boolean pushToAllowed = true;
private int maxStreams;
private boolean pushToAllowed;
private boolean allowCompressedData;
DefaultEndpoint(boolean serverEndpoint, boolean allowCompressedData) {
// Determine the starting stream ID for this endpoint. Zero is reserved for the
// connection and 1 is reserved for responding to an upgrade from HTTP 1.1.
// Client-initiated streams use odd identifiers and server-initiated streams use
// even.
nextStreamId = serverEndpoint ? 2 : 3;
DefaultEndpoint(boolean server, boolean allowCompressedData) {
this.allowCompressedData = allowCompressedData;
this.server = server;
// Determine the starting stream ID for this endpoint. Client-initiated streams
// are odd and server-initiated streams are even. Zero is reserved for the
// connection. Stream 1 is reserved client-initiated stream for responding to an
// upgrade from HTTP 1.1.
nextStreamId = server ? 2 : 1;
// Push is disallowed by default for servers and allowed for clients.
pushToAllowed = !server;
maxStreams = Integer.MAX_VALUE;
}
@Override
@ -244,7 +249,7 @@ public class DefaultHttp2Connection implements Http2Connection {
}
// Update the next and last stream IDs.
nextStreamId += 2;
nextStreamId = streamId + 2;
lastStreamCreated = streamId;
// Register the stream and mark it as active.
@ -253,6 +258,11 @@ public class DefaultHttp2Connection implements Http2Connection {
return stream;
}
@Override
public boolean isServer() {
return server;
}
@Override
public DefaultStream reservePushStream(int streamId, Http2Stream parent)
throws Http2Exception {
@ -271,7 +281,7 @@ public class DefaultHttp2Connection implements Http2Connection {
stream.state = isLocal() ? State.RESERVED_LOCAL : State.RESERVED_REMOTE;
// Update the next and last stream IDs.
nextStreamId += 2;
nextStreamId = streamId + 2;
lastStreamCreated = streamId;
// Register the stream.
@ -281,6 +291,9 @@ public class DefaultHttp2Connection implements Http2Connection {
@Override
public void allowPushTo(boolean allow) {
if (allow && server) {
throw new IllegalArgumentException("Servers do not allow push");
}
pushToAllowed = allow;
}
@ -323,14 +336,24 @@ public class DefaultHttp2Connection implements Http2Connection {
if (isGoAway()) {
throw protocolError("Cannot create a stream since the connection is going away");
}
verifyStreamId(streamId);
if (streamMap.size() + 1 > maxStreams) {
throw protocolError("Maximum streams exceeded for this endpoint.");
}
}
private void verifyStreamId(int streamId) throws Http2Exception {
if (nextStreamId < 0) {
throw protocolError("No more streams can be created on this connection");
}
if (streamId != nextStreamId) {
throw protocolError("Incorrect next stream ID requested: %d", streamId);
if (streamId < nextStreamId) {
throw protocolError("Request stream %d is behind the next expected stream %d",
streamId, nextStreamId);
}
if (streamMap.size() + 1 > maxStreams) {
throw protocolError("Maximum streams exceeded for this endpoint.");
boolean even = (streamId & 1) == 0;
if (server != even) {
throw protocolError("Request stream %d is not correct for %s connection", streamId,
server ? "server" : "client");
}
}

View File

@ -44,7 +44,6 @@ public class DefaultHttp2FrameReader implements Http2FrameReader {
ERROR
}
private final boolean server;
private final Http2HeadersDecoder headersDecoder;
private State state = State.FRAME_HEADER;
@ -54,12 +53,11 @@ public class DefaultHttp2FrameReader implements Http2FrameReader {
private int payloadLength;
private HeadersContinuation headersContinuation;
public DefaultHttp2FrameReader(boolean server) {
this(server, new DefaultHttp2HeadersDecoder());
public DefaultHttp2FrameReader() {
this(new DefaultHttp2HeadersDecoder());
}
public DefaultHttp2FrameReader(boolean server, Http2HeadersDecoder headersDecoder) {
this.server = server;
public DefaultHttp2FrameReader(Http2HeadersDecoder headersDecoder) {
this.headersDecoder = headersDecoder;
}
@ -373,10 +371,6 @@ public class DefaultHttp2FrameReader implements Http2FrameReader {
verifyStreamOrConnectionId(streamId, "Stream ID");
verifyPayloadLength(payloadLength);
if (server) {
throw protocolError("ALT_SVC frames must not be received by servers");
}
if (payloadLength < 8) {
throw protocolError("Frame length too small." + payloadLength);
}
@ -420,7 +414,7 @@ public class DefaultHttp2FrameReader implements Http2FrameReader {
long word1 = payload.readUnsignedInt();
final boolean exclusive = (word1 & 0x80000000L) > 0;
final int streamDependency = (int) (word1 & 0x7FFFFFFFL);
final short headersWeight = payload.readUnsignedByte();
final short weight = (short) (payload.readUnsignedByte() + 1);
final ByteBuf fragment = payload.readSlice(payload.readableBytes() - padding);
// Create a handler that invokes the observer when the header block is complete.
@ -437,7 +431,7 @@ public class DefaultHttp2FrameReader implements Http2FrameReader {
if (endOfHeaders) {
Http2Headers headers = builder().buildHeaders();
observer.onHeadersRead(ctx, headersStreamId, headers, streamDependency,
headersWeight, exclusive, padding, headersFlags.endOfStream(),
weight, exclusive, padding, headersFlags.endOfStream(),
headersFlags.endOfSegment());
close();
}
@ -480,7 +474,7 @@ public class DefaultHttp2FrameReader implements Http2FrameReader {
long word1 = payload.readUnsignedInt();
boolean exclusive = (word1 & 0x80000000L) > 0;
int streamDependency = (int) (word1 & 0x7FFFFFFFL);
short weight = payload.readUnsignedByte();
short weight = (short) (payload.readUnsignedByte() + 1);
observer.onPriorityRead(ctx, streamId, streamDependency, weight, exclusive);
}

View File

@ -21,6 +21,8 @@ import static io.netty.handler.codec.http2.Http2CodecUtil.MAX_FRAME_PAYLOAD_LENG
import static io.netty.handler.codec.http2.Http2CodecUtil.MAX_UNSIGNED_BYTE;
import static io.netty.handler.codec.http2.Http2CodecUtil.MAX_UNSIGNED_INT;
import static io.netty.handler.codec.http2.Http2CodecUtil.MAX_UNSIGNED_SHORT;
import static io.netty.handler.codec.http2.Http2CodecUtil.MAX_WEIGHT;
import static io.netty.handler.codec.http2.Http2CodecUtil.MIN_WEIGHT;
import static io.netty.handler.codec.http2.Http2CodecUtil.PRIORITY_ENTRY_LENGTH;
import static io.netty.handler.codec.http2.Http2CodecUtil.SETTINGS_COMPRESS_DATA;
import static io.netty.handler.codec.http2.Http2CodecUtil.SETTINGS_ENABLE_PUSH;
@ -43,15 +45,13 @@ import io.netty.channel.ChannelPromise;
*/
public class DefaultHttp2FrameWriter implements Http2FrameWriter {
private final boolean server;
private final Http2HeadersEncoder headersEncoder;
public DefaultHttp2FrameWriter(boolean server) {
this(server, new DefaultHttp2HeadersEncoder());
public DefaultHttp2FrameWriter() {
this(new DefaultHttp2HeadersEncoder());
}
public DefaultHttp2FrameWriter(boolean server, Http2HeadersEncoder headersEncoder) {
this.server = server;
public DefaultHttp2FrameWriter(Http2HeadersEncoder headersEncoder) {
this.headersEncoder = headersEncoder;
}
@ -133,7 +133,9 @@ public class DefaultHttp2FrameWriter implements Http2FrameWriter {
Http2Flags.EMPTY, streamId);
long word1 = exclusive ? (0x80000000L | streamDependency) : streamDependency;
writeUnsignedInt(word1, frame);
frame.writeByte(weight);
// Adjust the weight so that it fits into a single byte on the wire.
frame.writeByte(weight - 1);
return ctx.writeAndFlush(frame, promise);
} catch (RuntimeException e) {
throw failAndThrow(promise, e);
@ -189,6 +191,7 @@ public class DefaultHttp2FrameWriter implements Http2FrameWriter {
writeUnsignedInt(settings.maxConcurrentStreams(), frame);
}
if (settings.hasPushEnabled()) {
// Only write the enable push flag from client endpoints.
frame.writeByte(SETTINGS_ENABLE_PUSH);
writeUnsignedInt(settings.pushEnabled() ? 1L : 0L, frame);
}
@ -327,9 +330,6 @@ public class DefaultHttp2FrameWriter implements Http2FrameWriter {
public ChannelFuture writeAltSvc(ChannelHandlerContext ctx, ChannelPromise promise,
int streamId, long maxAge, int port, ByteBuf protocolId, String host, String origin) {
try {
if (!server) {
throw new IllegalArgumentException("ALT_SVC frames must not be sent by clients");
}
verifyStreamOrConnectionId(streamId, "Stream ID");
verifyMaxAge(maxAge);
verifyPort(port);
@ -428,7 +428,9 @@ public class DefaultHttp2FrameWriter implements Http2FrameWriter {
if (hasPriority) {
long word1 = exclusive ? (0x80000000L | streamDependency) : streamDependency;
writeUnsignedInt(word1, firstFrame);
firstFrame.writeByte(weight);
// Adjust the weight so that it fits into a single byte on the wire.
firstFrame.writeByte(weight - 1);
}
// Write the first fragment.
@ -542,7 +544,7 @@ public class DefaultHttp2FrameWriter implements Http2FrameWriter {
}
private static void verifyWeight(short weight) {
if (weight < 1 || weight > MAX_UNSIGNED_BYTE) {
if (weight < MIN_WEIGHT || weight > MAX_WEIGHT) {
throw new IllegalArgumentException("Invalid weight: " + weight);
}
}

View File

@ -16,8 +16,8 @@
package io.netty.handler.codec.http2;
import static io.netty.handler.codec.http2.Http2CodecUtil.DEFAULT_PRIORITY_WEIGHT;
import static io.netty.handler.codec.http2.Http2CodecUtil.MAX_UNSIGNED_BYTE;
import io.netty.handler.codec.http2.Http2PriorityTree.Priority;
import static io.netty.handler.codec.http2.Http2CodecUtil.MAX_WEIGHT;
import static io.netty.handler.codec.http2.Http2CodecUtil.MIN_WEIGHT;
import java.util.ArrayDeque;
import java.util.Collections;
@ -62,7 +62,7 @@ public class DefaultHttp2PriorityTree<T> implements Http2PriorityTree<T> {
if (parent < 0) {
throw new IllegalArgumentException("Parent stream ID must be >= 0");
}
if (weight < 1 || weight > MAX_UNSIGNED_BYTE) {
if (weight < MIN_WEIGHT || weight > MAX_WEIGHT) {
throw new IllegalArgumentException("Invalid weight: " + weight);
}

View File

@ -43,6 +43,8 @@ public final class Http2CodecUtil {
public static final int SETTING_ENTRY_LENGTH = 5;
public static final int PRIORITY_ENTRY_LENGTH = 5;
public static final int INT_FIELD_LENGTH = 4;
public static final short MAX_WEIGHT = (short) 256;
public static final short MIN_WEIGHT = (short) 1;
public static final short SETTINGS_HEADER_TABLE_SIZE = 1;
public static final short SETTINGS_ENABLE_PUSH = 2;

View File

@ -55,13 +55,19 @@ public interface Http2Connection {
*/
Http2Stream reservePushStream(int streamId, Http2Stream parent) throws Http2Exception;
/**
* Indicates whether or not this endpoint is the server-side of the connection.
*/
boolean isServer();
/**
* Sets whether server push is allowed to this endpoint.
*/
void allowPushTo(boolean allow);
/**
* Gets whether or not server push is allowed to this endpoint.
* Gets whether or not server push is allowed to this endpoint. This is always false
* for a server endpoint.
*/
boolean allowPushTo();
@ -97,7 +103,7 @@ public interface Http2Connection {
}
/**
* Indicates whether or not this endpoint is the server-side of the connection.
* Indicates whether or not the local endpoint for this connection is the server.
*/
boolean isServer();

View File

@ -116,17 +116,27 @@ public class DefaultHttp2ConnectionTest {
@Test
public void clientReservePushStreamShouldSucceed() throws Http2Exception {
Http2Stream stream = client.remote().createStream(2, true);
Http2Stream pushStream = client.local().reservePushStream(3, stream);
assertEquals(3, pushStream.id());
Http2Stream stream = server.remote().createStream(3, true);
Http2Stream pushStream = server.local().reservePushStream(4, stream);
assertEquals(4, pushStream.id());
assertEquals(State.RESERVED_LOCAL, pushStream.state());
assertEquals(1, client.activeStreams().size());
assertEquals(3, client.local().lastStreamCreated());
assertEquals(1, server.activeStreams().size());
assertEquals(4, server.local().lastStreamCreated());
}
@Test(expected = Http2Exception.class)
public void createStreamWithInvalidIdShouldThrow() throws Http2Exception {
server.remote().createStream(1, true);
public void newStreamBehindExpectedShouldThrow() throws Http2Exception {
server.local().createStream(0, true);
}
@Test(expected = Http2Exception.class)
public void newStreamNotForServerShouldThrow() throws Http2Exception {
server.local().createStream(11, true);
}
@Test(expected = Http2Exception.class)
public void newStreamNotForClientShouldThrow() throws Http2Exception {
client.local().createStream(10, true);
}
@Test(expected = Http2Exception.class)

View File

@ -62,8 +62,8 @@ public class DefaultHttp2FrameIOTest {
when(ctx.alloc()).thenReturn(alloc);
reader = new DefaultHttp2FrameReader(false);
writer = new DefaultHttp2FrameWriter(true);
reader = new DefaultHttp2FrameReader();
writer = new DefaultHttp2FrameWriter();
}
@Test
@ -118,7 +118,7 @@ public class DefaultHttp2FrameIOTest {
}
@Test
public void settingsShouldRoundtrip() throws Exception {
public void settingsShouldStripShouldRoundtrip() throws Exception {
Http2Settings settings = new Http2Settings();
settings.pushEnabled(true);
settings.maxHeaderTableSize(4096);

View File

@ -396,8 +396,17 @@ public class DelegatingHttp2ConnectionHandlerTest {
verify(observer).onSettingsAckRead(eq(ctx));
}
@Test(expected = Http2Exception.class)
public void clientSettingsReadWithPushShouldThrow() throws Exception {
when(connection.isServer()).thenReturn(false);
Http2Settings settings = new Http2Settings();
settings.pushEnabled(true);
decode().onSettingsRead(ctx, settings);
}
@Test
public void settingsReadShouldSetValues() throws Exception {
when(connection.isServer()).thenReturn(true);
Http2Settings settings = new Http2Settings();
settings.pushEnabled(true);
settings.initialWindowSize(123);
@ -416,12 +425,28 @@ public class DelegatingHttp2ConnectionHandlerTest {
}
@Test
public void goAwayShoultShouldUpdateConnectionState() throws Exception {
public void goAwayShouldReadShouldUpdateConnectionState() throws Exception {
decode().onGoAwayRead(ctx, 1, 2, EMPTY_BUFFER);
verify(connection).goAwayReceived();
verify(observer).onGoAwayRead(eq(ctx), eq(1), eq(2L), eq(EMPTY_BUFFER));
}
@Test(expected = Http2Exception.class)
public void serverAltSvcReadShouldThrow() throws Exception {
when(connection.isServer()).thenReturn(true);
decode().onAltSvcRead(ctx, STREAM_ID, 1, 2, Unpooled.EMPTY_BUFFER, "www.example.com", null);
}
@Test
public void clientAltSvcReadShouldNotifyObserver() throws Exception {
String host = "www.host.com";
String origin = "www.origin.com";
when(connection.isServer()).thenReturn(false);
decode().onAltSvcRead(ctx, STREAM_ID, 1, 2, EMPTY_BUFFER, host, origin);
verify(observer).onAltSvcRead(eq(ctx), eq(STREAM_ID), eq(1L), eq(2), eq(EMPTY_BUFFER),
eq(host), eq(origin));
}
@Test(expected = Http2Exception.class)
public void dataWriteAfterGoAwayShouldFail() throws Exception {
when(connection.isGoAway()).thenReturn(true);
@ -576,6 +601,23 @@ public class DelegatingHttp2ConnectionHandlerTest {
verify(reader).maxHeaderTableSize(2000);
}
@Test(expected = Http2Exception.class)
public void clientWriteAltSvcShouldThrow() throws Exception {
when(connection.isServer()).thenReturn(false);
handler.writeAltSvc(ctx, promise, STREAM_ID, 1, 2, Unpooled.EMPTY_BUFFER,
"www.example.com", null);
}
@Test
public void serverWriteAltSvcShouldSucceed() throws Exception {
String host = "www.host.com";
String origin = "www.origin.com";
when(connection.isServer()).thenReturn(true);
handler.writeAltSvc(ctx, promise, STREAM_ID, 1, 2, EMPTY_BUFFER, host, origin);
verify(writer).writeAltSvc(eq(ctx), eq(promise), eq(STREAM_ID), eq(1L), eq(2),
eq(EMPTY_BUFFER), eq(host), eq(origin));
}
private static ByteBuf dummyData() {
// The buffer is purposely 8 bytes so it will even work for a ping frame.
return Unpooled.wrappedBuffer("abcdefgh".getBytes(UTF_8));

View File

@ -69,7 +69,7 @@ public class Http2FrameRoundtripTest {
MockitoAnnotations.initMocks(this);
requestLatch = new CountDownLatch(1);
frameWriter = new DefaultHttp2FrameWriter(false);
frameWriter = new DefaultHttp2FrameWriter();
dataCaptor = ArgumentCaptor.forClass(ByteBuf.class);
sb = new ServerBootstrap();
@ -259,7 +259,7 @@ public class Http2FrameRoundtripTest {
FrameAdapter(Http2FrameObserver observer) {
this.observer = observer;
reader = new DefaultHttp2FrameReader(observer != null);
reader = new DefaultHttp2FrameReader();
}
@Override

View File

@ -192,10 +192,10 @@ public class Http2ClientConnectionHandler extends AbstractHttp2ConnectionHandler
}
private static Http2FrameReader frameReader() {
return new Http2InboundFrameLogger(new DefaultHttp2FrameReader(false), logger);
return new Http2InboundFrameLogger(new DefaultHttp2FrameReader(), logger);
}
private static Http2FrameWriter frameWriter() {
return new Http2OutboundFrameLogger(new DefaultHttp2FrameWriter(false), logger);
return new Http2OutboundFrameLogger(new DefaultHttp2FrameWriter(), logger);
}
}