Repository: netty/netty
Issue: http2: Off-by-one accounting with flow control and padding
Labels: defect

Per [the spec](http://httpwg.org/specs/rfc7540.html#DATA):

> The entire DATA frame payload is included in flow control, including the Pad Length and Padding fields if present.

However, it seems the pad length isn't being accounted for.
https://github.com/netty/netty/blob/netty-4.1.1.Final/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameReader.java#L397
https://github.com/netty/netty/blob/netty-4.1.1.Final/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoder.java#L203

I suggest we [add 1 to readPadding(), when padding is present](https://github.com/netty/netty/blob/netty-4.1.1.Final/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameReader.java#L598). This does mean that the `padding` argument could be 256 (instead of maxing out at 255).

@carl-mastrangelo, FYI


Issue comments:
We also need to audit outbound path.

cc: @Scottmitch, @normanmaurer, @nmittler 

@ejona86 should I take this?

@buchgr, please do.

This will also need to be updated:
https://github.com/netty/netty/blob/4.1/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2DataFrame.java#L72

If we add 1 to readPadding() we will have be sure to account that the byte is already read and the [int dataLength = payload.readableBytes() - padding;](https://github.com/netty/netty/blob/netty-4.1.1.Final/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameReader.java#L403) must account for this (and also for the other usages of `readPadding()`). An alternative would be to just adding `1` when we call the FrameListener [1] to ensure it is accounted for in flow control ... then we don't have to update usages of `readPadding()`. We should also update the comments on the `FrameListener` if we are going to include the "padding length byte" in the `padding` value.

[1] https://github.com/netty/netty/blob/netty-4.1.1.Final/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameReader.java#L410

ok i ll take this. will see what turns out best in code. can discuss on the pr.

thanks @buchgr !

A quick look at the outbound path seems to indicate, that the pad length field is also not counted towards flow control there.

Interestingly, the spec seems to allow zero length padding, that is the pad field is present with a value of zero.

> Note: A frame can be increased in size by one octet by including a Pad Length field with a value of zero.

I have to take a more detailed look tomorrow morning (11pm here), but this might indicate that we are doing padding wrong as when padding is 1, we really add two bytes (pad length and 1 byte padding). So probably we should just have padding to be a value in [0, 256], with a padding of 1 being special cased on the frame writer as pad length field present with value zero.


Pull request review comments:
Either keep the return value int or don't cast to short?

You might comment that the +1 is due to the pad length, which comes before most of the payload whereas the padding itself comes after.

Probably should add a bit of javadoc to make it clear how it is different from `padding`. The implementation is "obvious" in what it does, but not quite _why_ it does.

can we share this method? It seems to be used in a few spots. Maybe put it in `Http2CodecUtil`?

explain how this padding value will be translated to bytes on the wire. it may be unexpected that 256 is allowed when the frame definition only allows for 8 byte length.


Patch that resolved the issue:
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2DataFrame.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2DataFrame.java
index 84c99e86fd0..fe879c98e23 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2DataFrame.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2DataFrame.java
@@ -20,6 +20,7 @@
 import io.netty.util.IllegalReferenceCountException;
 import io.netty.util.internal.UnstableApi;
 
+import static io.netty.handler.codec.http2.Http2CodecUtil.verifyPadding;
 import static io.netty.util.internal.ObjectUtil.checkNotNull;
 
 /**
@@ -64,14 +65,13 @@ public DefaultHttp2DataFrame(ByteBuf content, boolean endStream) {
      *
      * @param content non-{@code null} payload
      * @param endStream whether this data should terminate the stream
-     * @param padding additional bytes that should be added to obscure the true content size
+     * @param padding additional bytes that should be added to obscure the true content size. Must be between 0 and
+     *                256 (inclusive).
      */
     public DefaultHttp2DataFrame(ByteBuf content, boolean endStream, int padding) {
         this.content = checkNotNull(content, "content");
         this.endStream = endStream;
-        if (padding < 0 || padding > Http2CodecUtil.MAX_UNSIGNED_BYTE) {
-            throw new IllegalArgumentException("padding must be non-negative and less than 256");
-        }
+        verifyPadding(padding);
         this.padding = padding;
     }
 
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameReader.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameReader.java
index 55e7f6d060f..312789ecb24 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameReader.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameReader.java
@@ -396,11 +396,11 @@ private void verifyContinuationFrame() throws Http2Exception {
 
     private void readDataFrame(ChannelHandlerContext ctx, ByteBuf payload,
             Http2FrameListener listener) throws Http2Exception {
-        short padding = readPadding(payload);
+        int padding = readPadding(payload);
 
         // Determine how much data there is to read by removing the trailing
         // padding.
-        int dataLength = payload.readableBytes() - padding;
+        int dataLength = lengthWithoutTrailingPadding(payload.readableBytes(), padding);
         if (dataLength < 0) {
             throw streamError(streamId, FRAME_SIZE_ERROR,
                     "Frame payload too small for padding.");
@@ -424,7 +424,7 @@ private void readHeadersFrame(final ChannelHandlerContext ctx, ByteBuf payload,
             final boolean exclusive = (word1 & 0x80000000L) != 0;
             final int streamDependency = (int) (word1 & 0x7FFFFFFFL);
             final short weight = (short) (payload.readUnsignedByte() + 1);
-            final ByteBuf fragment = payload.readSlice(payload.readableBytes() - padding);
+            final ByteBuf fragment = payload.readSlice(lengthWithoutTrailingPadding(payload.readableBytes(), padding));
 
             // Create a handler that invokes the listener when the header block is complete.
             headersContinuation = new HeadersContinuation() {
@@ -471,7 +471,7 @@ public void processFragment(boolean endOfHeaders, ByteBuf fragment,
         };
 
         // Process the initial fragment, invoking the listener's callback if end of headers.
-        final ByteBuf fragment = payload.readSlice(payload.readableBytes() - padding);
+        final ByteBuf fragment = payload.readSlice(lengthWithoutTrailingPadding(payload.readableBytes(), padding));
         headersContinuation.processFragment(flags.endOfHeaders(), fragment, listener);
     }
 
@@ -542,7 +542,7 @@ public void processFragment(boolean endOfHeaders, ByteBuf fragment,
         };
 
         // Process the initial fragment, invoking the listener's callback if end of headers.
-        final ByteBuf fragment = payload.readSlice(payload.readableBytes() - padding);
+        final ByteBuf fragment = payload.readSlice(lengthWithoutTrailingPadding(payload.readableBytes(), padding));
         headersContinuation.processFragment(flags.endOfHeaders(), fragment, listener);
     }
 
@@ -589,13 +589,24 @@ private void readUnknownFrame(ChannelHandlerContext ctx, ByteBuf payload, Http2F
     }
 
     /**
-     * If padding is present in the payload, reads the next byte as padding. Otherwise, returns zero.
+     * If padding is present in the payload, reads the next byte as padding. The padding also includes the one byte
+     * width of the pad length field. Otherwise, returns zero.
      */
-    private short readPadding(ByteBuf payload) {
+    private int readPadding(ByteBuf payload) {
         if (!flags.paddingPresent()) {
             return 0;
         }
-        return payload.readUnsignedByte();
+        return payload.readUnsignedByte() + 1;
+    }
+
+    /**
+     * The padding parameter consists of the 1 byte pad length field and the trailing padding bytes. This method
+     * returns the number of readable bytes without the trailing padding.
+     */
+    private static int lengthWithoutTrailingPadding(int readableBytes, int padding) {
+        return padding == 0
+                ? readableBytes
+                : readableBytes - (padding - 1);
     }
 
     /**
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameWriter.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameWriter.java
index 67c90c7f587..ecf623f6197 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameWriter.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameWriter.java
@@ -46,6 +46,7 @@
 import static io.netty.handler.codec.http2.Http2CodecUtil.isMaxFrameSizeValid;
 import static io.netty.handler.codec.http2.Http2CodecUtil.writeFrameHeaderInternal;
 import static io.netty.handler.codec.http2.Http2CodecUtil.writeUnsignedInt;
+import static io.netty.handler.codec.http2.Http2CodecUtil.verifyPadding;
 import static io.netty.handler.codec.http2.Http2CodecUtil.writeUnsignedShort;
 import static io.netty.handler.codec.http2.Http2Error.FRAME_SIZE_ERROR;
 import static io.netty.handler.codec.http2.Http2Exception.connectionError;
@@ -71,7 +72,7 @@ public class DefaultHttp2FrameWriter implements Http2FrameWriter, Http2FrameSize
     private static final String STREAM_ID = "Stream ID";
     private static final String STREAM_DEPENDENCY = "Stream Dependency";
     /**
-     * This buffer is allocated to the maximum padding size needed, and filled with padding.
+     * This buffer is allocated to the maximum size of the padding field, and filled with zeros.
      * When padding is needed it can be taken as a slice of this buffer. Users should call {@link ByteBuf#retain()}
      * before using their slice.
      */
@@ -163,8 +164,8 @@ public ChannelFuture writeData(ChannelHandlerContext ctx, int streamId, ByteBuf
                 ctx.write(lastFrame ? frameData : frameData.retain(), promiseAggregator.newPromise());
 
                 // Write the frame padding.
-                if (framePaddingBytes > 0) {
-                    ctx.write(ZERO_BUFFER.slice(0, framePaddingBytes), promiseAggregator.newPromise());
+                if (paddingBytes(framePaddingBytes) > 0) {
+                    ctx.write(ZERO_BUFFER.slice(0, paddingBytes(framePaddingBytes)), promiseAggregator.newPromise());
                 }
             } while (!lastFrame);
         } catch (Throwable t) {
@@ -302,7 +303,7 @@ public ChannelFuture writePushPromise(ChannelHandlerContext ctx, int streamId,
             // Read the first fragment (possibly everything).
             Http2Flags flags = new Http2Flags().paddingPresent(padding > 0);
             // INT_FIELD_LENGTH is for the length of the promisedStreamId
-            int nonFragmentLength = INT_FIELD_LENGTH + padding + flags.getPaddingPresenceFieldLength();
+            int nonFragmentLength = INT_FIELD_LENGTH + padding;
             int maxFragmentLength = maxFrameSize - nonFragmentLength;
             ByteBuf fragment = headerBlock.readRetainedSlice(min(headerBlock.readableBytes(), maxFragmentLength));
 
@@ -320,8 +321,9 @@ public ChannelFuture writePushPromise(ChannelHandlerContext ctx, int streamId,
             // Write the first fragment.
             ctx.write(fragment, promiseAggregator.newPromise());
 
-            if (padding > 0) { // Write out the padding, if any.
-                ctx.write(ZERO_BUFFER.slice(0, padding), promiseAggregator.newPromise());
+            // Write out the padding, if any.
+            if (paddingBytes(padding) > 0) {
+                ctx.write(ZERO_BUFFER.slice(0, paddingBytes(padding)), promiseAggregator.newPromise());
             }
 
             if (!flags.endOfHeaders()) {
@@ -426,7 +428,7 @@ private ChannelFuture writeHeadersInternal(ChannelHandlerContext ctx,
                     new Http2Flags().endOfStream(endStream).priorityPresent(hasPriority).paddingPresent(padding > 0);
 
             // Read the first fragment (possibly everything).
-            int nonFragmentBytes = padding + flags.getNumPriorityBytes() + flags.getPaddingPresenceFieldLength();
+            int nonFragmentBytes = padding + flags.getNumPriorityBytes();
             int maxFragmentLength = maxFrameSize - nonFragmentBytes;
             ByteBuf fragment = headerBlock.readRetainedSlice(min(headerBlock.readableBytes(), maxFragmentLength));
 
@@ -450,8 +452,9 @@ private ChannelFuture writeHeadersInternal(ChannelHandlerContext ctx,
             // Write the first fragment.
             ctx.write(fragment, promiseAggregator.newPromise());
 
-            if (padding > 0) { // Write out the padding, if any.
-                ctx.write(ZERO_BUFFER.slice(0, padding), promiseAggregator.newPromise());
+            // Write out the padding, if any.
+            if (paddingBytes(padding) > 0) {
+                ctx.write(ZERO_BUFFER.slice(0, paddingBytes(padding)), promiseAggregator.newPromise());
             }
 
             if (!flags.endOfHeaders()) {
@@ -473,8 +476,7 @@ private ChannelFuture writeHeadersInternal(ChannelHandlerContext ctx,
     private ChannelFuture writeContinuationFrames(ChannelHandlerContext ctx, int streamId,
             ByteBuf headerBlock, int padding, SimpleChannelPromiseAggregator promiseAggregator) {
         Http2Flags flags = new Http2Flags().paddingPresent(padding > 0);
-        int nonFragmentLength = padding + flags.getPaddingPresenceFieldLength();
-        int maxFragmentLength = maxFrameSize - nonFragmentLength;
+        int maxFragmentLength = maxFrameSize - padding;
         // TODO: same padding is applied to all frames, is this desired?
         if (maxFragmentLength <= 0) {
             return promiseAggregator.setFailure(new IllegalArgumentException(
@@ -484,7 +486,7 @@ private ChannelFuture writeContinuationFrames(ChannelHandlerContext ctx, int str
         if (headerBlock.isReadable()) {
             // The frame header (and padding) only changes on the last frame, so allocate it once and re-use
             int fragmentReadableBytes = min(headerBlock.readableBytes(), maxFragmentLength);
-            int payloadLength = fragmentReadableBytes + nonFragmentLength;
+            int payloadLength = fragmentReadableBytes + padding;
             ByteBuf buf = ctx.alloc().buffer(CONTINUATION_FRAME_HEADER_LENGTH);
             writeFrameHeaderInternal(buf, payloadLength, CONTINUATION, flags, streamId);
             writePaddingLength(buf, padding);
@@ -493,7 +495,7 @@ private ChannelFuture writeContinuationFrames(ChannelHandlerContext ctx, int str
                 fragmentReadableBytes = min(headerBlock.readableBytes(), maxFragmentLength);
                 ByteBuf fragment = headerBlock.readRetainedSlice(fragmentReadableBytes);
 
-                payloadLength = fragmentReadableBytes + nonFragmentLength;
+                payloadLength = fragmentReadableBytes + padding;
                 if (headerBlock.isReadable()) {
                     ctx.write(buf.retain(), promiseAggregator.newPromise());
                 } else {
@@ -509,18 +511,28 @@ private ChannelFuture writeContinuationFrames(ChannelHandlerContext ctx, int str
                 ctx.write(fragment, promiseAggregator.newPromise());
 
                 // Write out the padding, if any.
-                if (padding > 0) {
-                    ctx.write(ZERO_BUFFER.slice(0, padding), promiseAggregator.newPromise());
+                if (paddingBytes(padding) > 0) {
+                    ctx.write(ZERO_BUFFER.slice(0, paddingBytes(padding)), promiseAggregator.newPromise());
                 }
             } while(headerBlock.isReadable());
         }
         return promiseAggregator;
     }
 
-    private static void writePaddingLength(ByteBuf buf, int paddingLength) {
-        if (paddingLength > 0) {
+    /**
+     * Returns the number of padding bytes that should be appended to the end of a frame.
+     */
+    private static int paddingBytes(int padding) {
+        // The padding parameter contains the 1 byte pad length field as well as the trailing padding bytes.
+        // Subtract 1, so to only get the number of padding bytes that need to be appended to the end of a frame.
+        return padding - 1;
+    }
+
+    private static void writePaddingLength(ByteBuf buf, int padding) {
+        if (padding > 0) {
             // It is assumed that the padding length has been bounds checked before this
-            buf.writeByte(paddingLength);
+            // Minus 1, as the pad length field is included in the padding parameter and is 1 byte wide.
+            buf.writeByte(padding - 1);
         }
     }
 
@@ -536,12 +548,6 @@ private static void verifyStreamOrConnectionId(int streamId, String argumentName
         }
     }
 
-    private static void verifyPadding(int padding) {
-        if (padding < 0 || padding > MAX_UNSIGNED_BYTE) {
-            throw new IllegalArgumentException("Invalid padding value: " + padding);
-        }
-    }
-
     private static void verifyWeight(short weight) {
         if (weight < MIN_WEIGHT || weight > MAX_WEIGHT) {
             throw new IllegalArgumentException("Invalid weight: " + weight);
@@ -601,7 +607,7 @@ ByteBuf slice(int data, int padding, boolean endOfStream) {
                 flags.endOfStream(endOfStream);
                 frameHeader = buffer.readSlice(DATA_FRAME_HEADER_LENGTH).writerIndex(0);
 
-                int payloadLength = data + padding + flags.getPaddingPresenceFieldLength();
+                int payloadLength = data + padding;
                 writeFrameHeaderInternal(frameHeader, payloadLength, DATA, flags, streamId);
                 writePaddingLength(frameHeader, padding);
             }
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2HeadersFrame.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2HeadersFrame.java
index d12b4365e34..6ad54e097f8 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2HeadersFrame.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2HeadersFrame.java
@@ -17,6 +17,7 @@
 
 import io.netty.util.internal.UnstableApi;
 
+import static io.netty.handler.codec.http2.Http2CodecUtil.verifyPadding;
 import static io.netty.util.internal.ObjectUtil.checkNotNull;
 
 /**
@@ -51,14 +52,13 @@ public DefaultHttp2HeadersFrame(Http2Headers headers, boolean endStream) {
      *
      * @param headers the non-{@code null} headers to send
      * @param endStream whether these headers should terminate the stream
-     * @param padding additional bytes that should be added to obscure the true content size
+     * @param padding additional bytes that should be added to obscure the true content size. Must be between 0 and
+     *                256 (inclusive).
      */
     public DefaultHttp2HeadersFrame(Http2Headers headers, boolean endStream, int padding) {
         this.headers = checkNotNull(headers, "headers");
         this.endStream = endStream;
-        if (padding < 0 || padding > Http2CodecUtil.MAX_UNSIGNED_BYTE) {
-            throw new IllegalArgumentException("padding must be non-negative and less than 256");
-        }
+        verifyPadding(padding);
         this.padding = padding;
     }
 
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2CodecUtil.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2CodecUtil.java
index b1f608482c0..8079bd00f7a 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2CodecUtil.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2CodecUtil.java
@@ -46,6 +46,11 @@ public final class Http2CodecUtil {
 
     public static final int PING_FRAME_PAYLOAD_LENGTH = 8;
     public static final short MAX_UNSIGNED_BYTE = 0xFF;
+    /**
+     * The maximum number of padding bytes. That is the 255 padding bytes appended to the end of a frame and the 1 byte
+     * pad length field.
+     */
+    public static final int MAX_PADDING = 256;
     public static final int MAX_UNSIGNED_SHORT = 0xFFFF;
     public static final long MAX_UNSIGNED_INT = 0xFFFFFFFFL;
     public static final int FRAME_HEADER_LENGTH = 9;
@@ -340,5 +345,11 @@ private boolean tryPromise() {
         }
     }
 
+    public static void verifyPadding(int padding) {
+        if (padding < 0 || padding > MAX_PADDING) {
+            throw new IllegalArgumentException(String.format("Invalid padding '%d'. Padding must be between 0 and " +
+                                                             "%d (inclusive).", padding, MAX_PADDING));
+        }
+    }
     private Http2CodecUtil() { }
 }
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2DataWriter.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2DataWriter.java
index 911e336220d..26bfab7bd49 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2DataWriter.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2DataWriter.java
@@ -32,7 +32,10 @@ public interface Http2DataWriter {
      * @param ctx the context to use for writing.
      * @param streamId the stream for which to send the frame.
      * @param data the payload of the frame. This will be released by this method.
-     * @param padding the amount of padding to be added to the end of the frame
+     * @param padding additional bytes that should be added to obscure the true content size. Must be between 0 and
+     *                256 (inclusive). A 1 byte padding is encoded as just the pad length field with value 0.
+     *                A 256 byte padding is encoded as the pad length field with value 255 and 255 padding bytes
+     *                appended to the end of the frame.
      * @param endStream indicates if this is the last frame to be sent for the stream.
      * @param promise the promise for the write.
      * @return the future for the write.
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2FrameListener.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2FrameListener.java
index 39d51f38776..f4de4f51406 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2FrameListener.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2FrameListener.java
@@ -30,7 +30,8 @@ public interface Http2FrameListener {
      * @param ctx the context from the handler where the frame was read.
      * @param streamId the subject stream for the frame.
      * @param data payload buffer for the frame. This buffer will be released by the codec.
-     * @param padding the number of padding bytes found at the end of the frame.
+     * @param padding additional bytes that should be added to obscure the true content size. Must be between 0 and
+     *                256 (inclusive).
      * @param endOfStream Indicates whether this is the last frame to be sent from the remote endpoint for this stream.
      * @return the number of bytes that have been processed by the application. The returned bytes are used by the
      * inbound flow controller to determine the appropriate time to expand the inbound flow control window (i.e. send
@@ -60,7 +61,8 @@ int onDataRead(ChannelHandlerContext ctx, int streamId, ByteBuf data, int paddin
      * @param ctx the context from the handler where the frame was read.
      * @param streamId the subject stream for the frame.
      * @param headers the received headers.
-     * @param padding the number of padding bytes found at the end of the frame.
+     * @param padding additional bytes that should be added to obscure the true content size. Must be between 0 and
+     *                256 (inclusive).
      * @param endOfStream Indicates whether this is the last frame to be sent from the remote endpoint
      *            for this stream.
      */
@@ -89,7 +91,8 @@ void onHeadersRead(ChannelHandlerContext ctx, int streamId, Http2Headers headers
      *            connection.
      * @param weight the new weight for the stream.
      * @param exclusive whether or not the stream should be the exclusive dependent of its parent.
-     * @param padding the number of padding bytes found at the end of the frame.
+     * @param padding additional bytes that should be added to obscure the true content size. Must be between 0 and
+     *                256 (inclusive).
      * @param endOfStream Indicates whether this is the last frame to be sent from the remote endpoint
      *            for this stream.
      */
@@ -176,7 +179,8 @@ void onPriorityRead(ChannelHandlerContext ctx, int streamId, int streamDependenc
      * @param streamId the stream the frame was sent on.
      * @param promisedStreamId the ID of the promised stream.
      * @param headers the received headers.
-     * @param padding the number of padding bytes found at the end of the frame.
+     * @param padding additional bytes that should be added to obscure the true content size. Must be between 0 and
+     *                256 (inclusive).
      */
     void onPushPromiseRead(ChannelHandlerContext ctx, int streamId, int promisedStreamId,
             Http2Headers headers, int padding) throws Http2Exception;
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2FrameWriter.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2FrameWriter.java
index 48fd7d58df3..111e6ba2c52 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2FrameWriter.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2FrameWriter.java
@@ -51,7 +51,8 @@ interface Configuration {
      * @param ctx the context to use for writing.
      * @param streamId the stream for which to send the frame.
      * @param headers the headers to be sent.
-     * @param padding the amount of padding to be added to the end of the frame
+     * @param padding additional bytes that should be added to obscure the true content size. Must be between 0 and
+     *                256 (inclusive).
      * @param endStream indicates if this is the last frame to be sent for the stream.
      * @param promise the promise for the write.
      * @return the future for the write.
@@ -69,7 +70,8 @@ ChannelFuture writeHeaders(ChannelHandlerContext ctx, int streamId, Http2Headers
      *            depend on the connection.
      * @param weight the weight for this stream.
      * @param exclusive whether this stream should be the exclusive dependant of its parent.
-     * @param padding the amount of padding to be added to the end of the frame
+     * @param padding additional bytes that should be added to obscure the true content size. Must be between 0 and
+     *                256 (inclusive).
      * @param endStream indicates if this is the last frame to be sent for the stream.
      * @param promise the promise for the write.
      * @return the future for the write.
@@ -145,7 +147,8 @@ ChannelFuture writePing(ChannelHandlerContext ctx, boolean ack, ByteBuf data,
      * @param streamId the stream for which to send the frame.
      * @param promisedStreamId the ID of the promised stream.
      * @param headers the headers to be sent.
-     * @param padding the amount of padding to be added to the end of the frame
+     * @param padding additional bytes that should be added to obscure the true content size. Must be between 0 and
+     *                256 (inclusive).
      * @param promise the promise for the write.
      * @return the future for the write.
      */
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2LocalFlowController.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2LocalFlowController.java
index a3b2bc6ceef..69e11f845c7 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2LocalFlowController.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2LocalFlowController.java
@@ -42,7 +42,8 @@ public interface Http2LocalFlowController extends Http2FlowController {
      * stream} is {@code null} or closed, flow control should only be applied to the connection window and the bytes are
      * immediately consumed.
      * @param data payload buffer for the frame.
-     * @param padding the number of padding bytes found at the end of the frame.
+     * @param padding additional bytes that should be added to obscure the true content size. Must be between 0 and
+     *                256 (inclusive).
      * @param endOfStream Indicates whether this is the last frame to be sent from the remote endpoint for this stream.
      * @throws Http2Exception if any flow control errors are encountered.
      */


Test patch:
diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2FrameRoundtripTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2FrameRoundtripTest.java
index 99f285d9194..3e771b459d4 100644
--- a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2FrameRoundtripTest.java
+++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2FrameRoundtripTest.java
@@ -39,7 +39,7 @@
 import java.util.List;
 
 import static io.netty.buffer.Unpooled.EMPTY_BUFFER;
-import static io.netty.handler.codec.http2.Http2CodecUtil.DEFAULT_MAX_HEADER_SIZE;
+import static io.netty.handler.codec.http2.Http2CodecUtil.*;
 import static io.netty.handler.codec.http2.Http2TestUtil.randomString;
 import static io.netty.util.CharsetUtil.UTF_8;
 import static java.lang.Math.min;
@@ -148,17 +148,17 @@ public void emptyDataShouldMatch() throws Exception {
     @Test
     public void dataShouldMatch() throws Exception {
         final ByteBuf data = data(10);
-        writer.writeData(ctx, STREAM_ID, data.slice(), 0, false, ctx.newPromise());
+        writer.writeData(ctx, STREAM_ID, data.slice(), 1, false, ctx.newPromise());
         readFrames();
-        verify(listener).onDataRead(eq(ctx), eq(STREAM_ID), eq(data), eq(0), eq(false));
+        verify(listener).onDataRead(eq(ctx), eq(STREAM_ID), eq(data), eq(1), eq(false));
     }
 
     @Test
     public void dataWithPaddingShouldMatch() throws Exception {
         final ByteBuf data = data(10);
-        writer.writeData(ctx, STREAM_ID, data.slice(), 0xFF, true, ctx.newPromise());
+        writer.writeData(ctx, STREAM_ID, data.slice(), MAX_PADDING, true, ctx.newPromise());
         readFrames();
-        verify(listener).onDataRead(eq(ctx), eq(STREAM_ID), eq(data), eq(0xFF), eq(true));
+        verify(listener).onDataRead(eq(ctx), eq(STREAM_ID), eq(data), eq(MAX_PADDING), eq(true));
     }
 
     @Test
@@ -210,9 +210,9 @@ public void emptyHeadersShouldMatch() throws Exception {
     @Test
     public void emptyHeadersWithPaddingShouldMatch() throws Exception {
         final Http2Headers headers = EmptyHttp2Headers.INSTANCE;
-        writer.writeHeaders(ctx, STREAM_ID, headers, 0xFF, true, ctx.newPromise());
+        writer.writeHeaders(ctx, STREAM_ID, headers, MAX_PADDING, true, ctx.newPromise());
         readFrames();
-        verify(listener).onHeadersRead(eq(ctx), eq(STREAM_ID), eq(headers), eq(0xFF), eq(true));
+        verify(listener).onHeadersRead(eq(ctx), eq(STREAM_ID), eq(headers), eq(MAX_PADDING), eq(true));
     }
 
     @Test
@@ -243,18 +243,18 @@ public void headersFrameWithPriorityShouldMatch() throws Exception {
     @Test
     public void headersWithPaddingWithoutPriorityShouldMatch() throws Exception {
         final Http2Headers headers = headers();
-        writer.writeHeaders(ctx, STREAM_ID, headers, 0xFF, true, ctx.newPromise());
+        writer.writeHeaders(ctx, STREAM_ID, headers, MAX_PADDING, true, ctx.newPromise());
         readFrames();
-        verify(listener).onHeadersRead(eq(ctx), eq(STREAM_ID), eq(headers), eq(0xFF), eq(true));
+        verify(listener).onHeadersRead(eq(ctx), eq(STREAM_ID), eq(headers), eq(MAX_PADDING), eq(true));
     }
 
     @Test
     public void headersWithPaddingWithPriorityShouldMatch() throws Exception {
         final Http2Headers headers = headers();
-        writer.writeHeaders(ctx, STREAM_ID, headers, 2, (short) 3, true, 0xFF, true, ctx.newPromise());
+        writer.writeHeaders(ctx, STREAM_ID, headers, 2, (short) 3, true, 1, true, ctx.newPromise());
         readFrames();
         verify(listener).onHeadersRead(eq(ctx), eq(STREAM_ID), eq(headers), eq(2), eq((short) 3), eq(true),
-                eq(0xFF), eq(true));
+                eq(1), eq(true));
     }
 
     @Test
@@ -269,16 +269,16 @@ public void continuedHeadersShouldMatch() throws Exception {
     @Test
     public void continuedHeadersWithPaddingShouldMatch() throws Exception {
         final Http2Headers headers = largeHeaders();
-        writer.writeHeaders(ctx, STREAM_ID, headers, 2, (short) 3, true, 0xFF, true, ctx.newPromise());
+        writer.writeHeaders(ctx, STREAM_ID, headers, 2, (short) 3, true, MAX_PADDING, true, ctx.newPromise());
         readFrames();
         verify(listener).onHeadersRead(eq(ctx), eq(STREAM_ID), eq(headers), eq(2), eq((short) 3), eq(true),
-                eq(0xFF), eq(true));
+                eq(MAX_PADDING), eq(true));
     }
 
     @Test
     public void headersThatAreTooBigShouldFail() throws Exception {
         final Http2Headers headers = headersOfSize(DEFAULT_MAX_HEADER_SIZE + 1);
-        writer.writeHeaders(ctx, STREAM_ID, headers, 2, (short) 3, true, 0xFF, true, ctx.newPromise());
+        writer.writeHeaders(ctx, STREAM_ID, headers, 2, (short) 3, true, MAX_PADDING, true, ctx.newPromise());
         try {
             readFrames();
             fail();
@@ -308,9 +308,9 @@ public void pushPromiseFrameShouldMatch() throws Exception {
     @Test
     public void pushPromiseWithPaddingShouldMatch() throws Exception {
         final Http2Headers headers = headers();
-        writer.writePushPromise(ctx, STREAM_ID, 2, headers, 0xFF, ctx.newPromise());
+        writer.writePushPromise(ctx, STREAM_ID, 2, headers, MAX_PADDING, ctx.newPromise());
         readFrames();
-        verify(listener).onPushPromiseRead(eq(ctx), eq(STREAM_ID), eq(2), eq(headers), eq(0xFF));
+        verify(listener).onPushPromiseRead(eq(ctx), eq(STREAM_ID), eq(2), eq(headers), eq(MAX_PADDING));
     }
 
     @Test