Author: rhuijben
Date: Sun Oct 18 10:45:31 2015
New Revision: 1709256
URL: http://svn.apache.org/viewvc?rev=1709256&view=rev
Log:
Tweak http2 buckets a bit. Remove an unneeded argument and the necessary
state for providing that value.
* serf-dev/dev/buckets/http2_frame_buckets.c
(http2_unframe_context_t): Remove unused variable. Reorder variables
based on their size to not unneededly keep a huge state.
(serf_bucket_http2_unframe_create): Store boolean as char.
(serf_http2_unframe_bucket_read_info): Rename to...
(serf_bucket_http2_unframe_read_info): ... this and remove payload length
argument. Update caller.
(serf_http2_unframe_read,
serf_http2_unframe_read_iovec,
serf_http2_unframe_peek,
serf_http2_unframe_get_remaining): Update caller.
(serf_http2_unpad_read_padsize): Implement http2 MUST.
* serf-dev/dev/serf_bucket_types.h
(serf_http2_unframe_bucket_read_info): Rename to...
(serf_bucket_http2_unframe_read_info): ... this and remove argument.
* serf-dev/dev/test/test_buckets.c
(test_http2_unframe_buckets): Update caller.
(test_http2_unpad_buckets): Update caller. Extend test to test a few
edge cases.
Modified:
serf/trunk/buckets/http2_frame_buckets.c
serf/trunk/serf_bucket_types.h
serf/trunk/test/test_buckets.c
Modified: serf/trunk/buckets/http2_frame_buckets.c
URL:
http://svn.apache.org/viewvc/serf/trunk/buckets/http2_frame_buckets.c?rev=1709256&r1=1709255&r2=1709256&view=diff
==============================================================================
--- serf/trunk/buckets/http2_frame_buckets.c (original)
+++ serf/trunk/buckets/http2_frame_buckets.c Sun Oct 18 10:45:31 2015
@@ -33,16 +33,15 @@ typedef struct http2_unframe_context_t
apr_size_t max_payload_size;
apr_size_t prefix_remaining;
- unsigned char prefix_buffer[FRAME_PREFIX_SIZE];
/* These fields are only set after prefix_remaining is 0 */
- apr_size_t payload_length; /* 0 <= payload_length < 2^24 */
- apr_int32_t stream_id; /* 0 <= stream_id < 2^31 */
+ apr_size_t payload_remaining; /* 0 <= payload_length < 2^24 */
+ apr_int32_t stream_id; /* 0 <= stream_id < 2^31 */
unsigned char frame_type;
unsigned char flags;
- apr_size_t payload_remaining;
- int destroy_stream;
+ unsigned char prefix_buffer[FRAME_PREFIX_SIZE];
+ char destroy_stream;
} http2_unframe_context_t;
serf_bucket_t *
@@ -57,14 +56,13 @@ serf_bucket_http2_unframe_create(serf_bu
ctx->stream = stream;
ctx->max_payload_size = max_payload_size;
ctx->prefix_remaining = sizeof(ctx->prefix_buffer);
- ctx->destroy_stream = destroy_stream;
+ ctx->destroy_stream = (destroy_stream != 0);
return serf_bucket_create(&serf_bucket_type_http2_unframe, allocator, ctx);
}
apr_status_t
-serf_http2_unframe_bucket_read_info(serf_bucket_t *bucket,
- apr_size_t *payload_length,
+serf_bucket_http2_unframe_read_info(serf_bucket_t *bucket,
apr_int32_t *stream_id,
unsigned char *frame_type,
unsigned char *flags)
@@ -76,8 +74,6 @@ serf_http2_unframe_bucket_read_info(serf
if (ctx->prefix_remaining == 0)
{
- if (payload_length)
- *payload_length = ctx->payload_length;
if (stream_id)
*stream_id = ctx->stream_id;
if (frame_type)
@@ -98,9 +94,9 @@ serf_http2_unframe_bucket_read_info(serf
if (ctx->prefix_remaining == 0)
{
- ctx->payload_length = (ctx->prefix_buffer[0] << 16)
- | (ctx->prefix_buffer[1] << 8)
- | (ctx->prefix_buffer[2]);
+ apr_size_t payload_length = (ctx->prefix_buffer[0] << 16)
+ | (ctx->prefix_buffer[1] << 8)
+ | (ctx->prefix_buffer[2]);
ctx->frame_type = ctx->prefix_buffer[3];
ctx->flags = ctx->prefix_buffer[4];
/* Highest bit of stream_id MUST be ignored */
@@ -109,11 +105,11 @@ serf_http2_unframe_bucket_read_info(serf
| (ctx->prefix_buffer[7] << 8)
| (ctx->prefix_buffer[8]);
- ctx->payload_remaining = ctx->payload_length;
+ ctx->payload_remaining = payload_length;
/* Use recursion to fill output arguments if necessary */
- serf_http2_unframe_bucket_read_info(bucket, payload_length,
- stream_id, frame_type, flags);
+ serf_bucket_http2_unframe_read_info(bucket, stream_id, frame_type,
+ flags);
/* https://tools.ietf.org/html/rfc7540#section-4.2
An endpoint MUST send an error code of FRAME_SIZE_ERROR if a frame
@@ -121,7 +117,7 @@ serf_http2_unframe_bucket_read_info(serf
limit defined for the frame type, or is too small to contain
mandatory frame data.
*/
- if (ctx->max_payload_size < ctx->payload_remaining)
+ if (ctx->max_payload_size < payload_length)
return SERF_ERROR_HTTP2_FRAME_SIZE_ERROR;
}
}
@@ -137,8 +133,7 @@ serf_http2_unframe_read(serf_bucket_t *b
http2_unframe_context_t *ctx = bucket->data;
apr_status_t status;
- status = serf_http2_unframe_bucket_read_info(bucket, NULL, NULL,
- NULL, NULL);
+ status = serf_bucket_http2_unframe_read_info(bucket, NULL, NULL, NULL);
if (status)
{
@@ -177,8 +172,7 @@ serf_http2_unframe_read_iovec(serf_bucke
http2_unframe_context_t *ctx = bucket->data;
apr_status_t status;
- status = serf_http2_unframe_bucket_read_info(bucket, NULL, NULL,
- NULL, NULL);
+ status = serf_bucket_http2_unframe_read_info(bucket, NULL, NULL, NULL);
if (status)
{
@@ -222,8 +216,7 @@ serf_http2_unframe_peek(serf_bucket_t *b
http2_unframe_context_t *ctx = bucket->data;
apr_status_t status;
- status = serf_http2_unframe_bucket_read_info(bucket, NULL, NULL,
- NULL, NULL);
+ status = serf_bucket_http2_unframe_read_info(bucket, NULL, NULL, NULL);
if (status)
{
@@ -258,8 +251,7 @@ serf_http2_unframe_get_remaining(serf_bu
http2_unframe_context_t *ctx = bucket->data;
apr_status_t status;
- status = serf_http2_unframe_bucket_read_info(bucket, NULL, NULL,
- NULL, NULL);
+ status = serf_bucket_http2_unframe_read_info(bucket, NULL, NULL, NULL);
if (status)
return SERF_LENGTH_UNKNOWN;
@@ -336,15 +328,17 @@ serf_http2_unpad_read_padsize(serf_bucke
|| remaining > APR_SIZE_MAX)
return APR_EGENERAL; /* Can't calculate padding size */
+ /* http://tools.ietf.org/html/rfc7540#section-6.1
+ If the length of the padding is the length of the
+ frame payload or greater, the recipient MUST treat this as a
+ connection error (Section 5.4.1) of type PROTOCOL_ERROR.
+
+ The frame payload includes the length byte, so when remaining
+ is 0, that isn't a protocol error */
if (remaining < ctx->pad_length)
- {
- ctx->payload_remaining = 0;
- ctx->pad_remaining = (apr_size_t)remaining;
- }
- else
- {
- ctx->payload_remaining = (apr_size_t)remaining - ctx->pad_length;
- }
+ return SERF_ERROR_HTTP2_PROTOCOL_ERROR;
+
+ ctx->payload_remaining = (apr_size_t)remaining - ctx->pad_length;
}
return status;
}
Modified: serf/trunk/serf_bucket_types.h
URL:
http://svn.apache.org/viewvc/serf/trunk/serf_bucket_types.h?rev=1709256&r1=1709255&r2=1709256&view=diff
==============================================================================
--- serf/trunk/serf_bucket_types.h (original)
+++ serf/trunk/serf_bucket_types.h Sun Oct 18 10:45:31 2015
@@ -767,14 +767,13 @@ serf_bucket_http2_unframe_create(serf_bu
/* Obtains the frame header state, reading from the bucket if necessary.
If the header was read successfully (or was already read before calling)
- the *payload_length, *stream_id, * frame_type and *flags values
- (when not pointing to NULL) will be set to the requested values.
+ the *stream_id, * frame_type and *flags values (when not pointing to NULL)
+ will be set to the requested values.
returns APR_SUCCESS when the header was already read before calling this,
function. Otherwise it will return the result of reading. */
apr_status_t
-serf_http2_unframe_bucket_read_info(serf_bucket_t *bucket,
- apr_size_t *payload_length,
+serf_bucket_http2_unframe_read_info(serf_bucket_t *bucket,
apr_int32_t *stream_id,
unsigned char *frame_type,
unsigned char *flags);
Modified: serf/trunk/test/test_buckets.c
URL:
http://svn.apache.org/viewvc/serf/trunk/test/test_buckets.c?rev=1709256&r1=1709255&r2=1709256&view=diff
==============================================================================
--- serf/trunk/test/test_buckets.c (original)
+++ serf/trunk/test/test_buckets.c Sun Oct 18 10:45:31 2015
@@ -1792,17 +1792,14 @@ static void test_http2_unframe_buckets(C
"\x00\x02\x00\x00\x00\x00", read_len));
{
- apr_size_t payload_len;
apr_int32_t stream_id;
unsigned char frame_type, flags;
CuAssertIntEquals(tc, 0,
- serf_http2_unframe_bucket_read_info(unframe,
- &payload_len,
+ serf_bucket_http2_unframe_read_info(unframe,
&stream_id,
&frame_type,
&flags));
- CuAssertIntEquals(tc, 12, payload_len);
CuAssertIntEquals(tc, 0, stream_id);
CuAssertIntEquals(tc, 4, frame_type);
CuAssertIntEquals(tc, 0, flags);
@@ -1822,17 +1819,14 @@ static void test_http2_unframe_buckets(C
read_len));
{
- apr_size_t payload_len;
apr_int32_t stream_id;
unsigned char frame_type, flags;
CuAssertIntEquals(tc, 0,
- serf_http2_unframe_bucket_read_info(unframe,
- &payload_len,
+ serf_bucket_http2_unframe_read_info(unframe,
&stream_id,
&frame_type,
&flags));
- CuAssertIntEquals(tc, 6, payload_len);
CuAssertIntEquals(tc, 0x03040506, stream_id);
CuAssertIntEquals(tc, 0x01, frame_type);
CuAssertIntEquals(tc, 0x02, flags);
@@ -1885,7 +1879,7 @@ static void test_http2_unpad_buckets(CuT
apr_int32_t streamid;
unsigned char frame_type, flags;
- status = serf_http2_unframe_bucket_read_info(unframe, NULL, &streamid,
+ status = serf_bucket_http2_unframe_read_info(unframe, &streamid,
&frame_type, &flags);
CuAssertIntEquals(tc, APR_SUCCESS, status);
@@ -1903,6 +1897,22 @@ static void test_http2_unpad_buckets(CuT
CuAssertIntEquals(tc, sizeof(result1), read_len);
read_and_check_bucket(tc, raw, "Extra");
+
+ raw = serf_bucket_simple_create("\0a", 2, NULL, NULL, alloc);
+ unpad = serf_bucket_http2_unpad_create(raw, TRUE, alloc);
+ read_and_check_bucket(tc, unpad, "a");
+
+ raw = serf_bucket_simple_create("\5a", 2, NULL, NULL, alloc);
+ unpad = serf_bucket_http2_unpad_create(raw, TRUE, alloc);
+
+ {
+ const char *data;
+ apr_size_t sz;
+
+ CuAssertIntEquals(tc, SERF_ERROR_HTTP2_PROTOCOL_ERROR,
+ serf_bucket_read(unpad, SERF_READ_ALL_AVAIL,
+ &data, &sz));
+ }
}
CuSuite *test_buckets(void)
