Author: rhuijben Date: Mon Nov 2 14:31:58 2015 New Revision: 1712013 URL: http://svn.apache.org/viewvc?rev=1712013&view=rev Log: In the http2 protocol handler: properly handle pushed requests, by setting up all the stream state and then cleanly denying them.
* protocols/http2_protocol.c (http2_handle_promise): Add implementation. Setup future stream and delegate remaining handling to parent stream. * protocols/http2_protocol.h (serf_http2_stream_t): Add new variable. * protocols/http2_stream.c (serf_http2__stream_create): Tweak init code. (stream_promise_item, stream_promise_done): New functions. (serf_http2__stream_handle_hpack): Add specific implementation for promised streams. Modified: serf/trunk/protocols/http2_protocol.c serf/trunk/protocols/http2_protocol.h serf/trunk/protocols/http2_stream.c Modified: serf/trunk/protocols/http2_protocol.c URL: http://svn.apache.org/viewvc/serf/trunk/protocols/http2_protocol.c?rev=1712013&r1=1712012&r2=1712013&view=diff ============================================================================== --- serf/trunk/protocols/http2_protocol.c (original) +++ serf/trunk/protocols/http2_protocol.c Mon Nov 2 14:31:58 2015 @@ -392,13 +392,62 @@ http2_handle_promise(void *baton, const char *data, apr_size_t len) { - serf_http2_stream_t *stream = baton; + serf_http2_stream_t *parent_stream = baton; + serf_http2_protocol_t *h2= parent_stream->h2; + serf_http2_stream_t *promised_stream; + apr_int32_t streamid; + const struct promise_t + { + unsigned char s3, s2, s1, s0; + } *promise; if (len != HTTP2_PROMISE_DATA_SIZE) return SERF_ERROR_HTTP2_FRAME_SIZE_ERROR; - /* ### TODO: Prepare reading promise on stream */ - SERF_H2_assert(stream->h2 != NULL); + SERF_H2_assert(h2 != NULL); + + promise = (const void *)data; + + /* Highest bit is reserved */ + streamid = ((promise->s3 & 0x7F) << 24) | (promise->s2 << 16) + |(promise->s1 << 8) | promise->s0; + + if (streamid == 0 + || (streamid < h2->rl_next_streamid) + || (streamid & 0x01) != (h2->rl_next_streamid & 0x01)) + { + /* The promised stream identifier MUST bet a valid choice for the + next stream sent by the sender */ + + /* A receiver MUST treat the receipt of a PUSH_PROMISE that promises an + illegal stream identifier (Section 5.1.1) as a connection error + (Section 5.4.1) of type PROTOCOL_ERROR. Note that an illegal stream + identifier is an identifier for a stream that is not currently in the + "idle" state.*/ + + return SERF_ERROR_HTTP2_PROTOCOL_ERROR; + } + else if (parent_stream->status != H2S_OPEN + && parent_stream->status != H2S_HALFCLOSED_LOCAL) + { + /* PUSH_PROMISE frames MUST only be sent on a peer-initiated stream that + is in either the "open" or "half-closed (remote)" state. The stream + identifier of a PUSH_PROMISE frame indicates the stream it is + associated with. If the stream identifier field specifies the value + 0x0, a recipient MUST respond with a connection error (Section 5.4.1) + of type PROTOCOL_ERROR.*/ + + return SERF_ERROR_HTTP2_PROTOCOL_ERROR; + } + + promised_stream = serf_http2__stream_get(h2, streamid, TRUE, FALSE); + if (!promised_stream || promised_stream->status != H2S_IDLE) + return SERF_ERROR_HTTP2_PROTOCOL_ERROR; + + promised_stream->status = H2S_RESERVED_REMOTE; + + /* Store data to allow stream to handle the promise */ + parent_stream->new_reserved_stream = promised_stream; return APR_SUCCESS; } @@ -933,8 +982,7 @@ http2_process(serf_http2_protocol_t *h2) http2_handle_priority, stream, h2->allocator); } - - if (frametype == HTTP2_FRAME_TYPE_PUSH_PROMISE) + else if (frametype == HTTP2_FRAME_TYPE_PUSH_PROMISE) { body = serf_bucket_prefix_create(body, HTTP2_PROMISE_DATA_SIZE, @@ -1026,8 +1074,9 @@ http2_process(serf_http2_protocol_t *h2) h2->hpack_tbl, h2->allocator); } } - else /* We have a data bucket */ + else if (! reset_reason) { + /* We have a data bucket */ body = serf_http2__stream_handle_data( stream, body, frametype, (frameflags & HTTP2_FLAG_END_STREAM), @@ -1442,7 +1491,6 @@ serf_http2__stream_get(serf_http2_protoc if (create_for_remote && (streamid & 0x01) == (h2->rl_next_streamid & 0x01)) { - serf_http2_stream_t *rs; stream = serf_http2__stream_create(h2, streamid, h2->lr_default_window, h2->rl_default_window, Modified: serf/trunk/protocols/http2_protocol.h URL: http://svn.apache.org/viewvc/serf/trunk/protocols/http2_protocol.h?rev=1712013&r1=1712012&r2=1712013&view=diff ============================================================================== --- serf/trunk/protocols/http2_protocol.h (original) +++ serf/trunk/protocols/http2_protocol.h Mon Nov 2 14:31:58 2015 @@ -142,6 +142,9 @@ typedef struct serf_http2_stream_t H2S_CLOSED } status; + /* Used while receiving a promise stream */ + struct serf_http2_stream_t *new_reserved_stream; + /* TODO: Priority, etc. */ } serf_http2_stream_t; Modified: serf/trunk/protocols/http2_stream.c URL: http://svn.apache.org/viewvc/serf/trunk/protocols/http2_stream.c?rev=1712013&r1=1712012&r2=1712013&view=diff ============================================================================== --- serf/trunk/protocols/http2_stream.c (original) +++ serf/trunk/protocols/http2_stream.c Mon Nov 2 14:31:58 2015 @@ -47,10 +47,10 @@ serf_http2__stream_create(serf_http2_pro stream->h2 = h2; stream->alloc = alloc; - stream->data = serf_bucket_mem_alloc(alloc, sizeof(*stream->data)); - stream->next = stream->prev = NULL; + /* Delay creating this? */ + stream->data = serf_bucket_mem_alloc(alloc, sizeof(*stream->data)); stream->data->request = NULL; stream->data->response_agg = NULL; @@ -63,6 +63,7 @@ serf_http2__stream_create(serf_http2_pro stream->streamid = -1; /* Undetermined yet */ stream->status = (streamid >= 0) ? H2S_IDLE : H2S_INIT; + stream->new_reserved_stream = NULL; return stream; } @@ -209,6 +210,65 @@ stream_setup_response(serf_http2_stream_ stream->data->response_agg = agg; } +static apr_status_t +stream_promise_item(void *baton, + const char *key, + apr_size_t key_sz, + const char *value, + apr_size_t value_sz) +{ + serf_http2_stream_t *parent_stream = baton; + serf_http2_stream_t *stream = parent_stream->new_reserved_stream; + + SERF_H2_assert(stream != NULL); + + /* TODO: Store key+value somewhere to allow asking the application + if it is interested in the promised stream. + + Most likely it is not interested *yet* as the HTTP/2 spec + recommends pushing promised items *before* the stream that + references them. + + So we probably want to store the request anyway, to allow + matching this against a later added outgoing request. + */ + return APR_SUCCESS; +} + +static apr_status_t +stream_promise_done(void *baton, + serf_bucket_t *done_agg) +{ + serf_http2_stream_t *parent_stream = baton; + serf_http2_stream_t *stream = parent_stream->new_reserved_stream; + + SERF_H2_assert(stream != NULL); + SERF_H2_assert(stream->status == H2S_RESERVED_REMOTE); + parent_stream->new_reserved_stream = NULL; /* End of PUSH_PROMISE */ + + /* Anything else? */ + + + /* ### Absolute minimal implementation. + Just sending that we are not interested in the initial SETTINGS + would be the easier approach. */ + serf_http2__stream_reset(stream, SERF_ERROR_HTTP2_REFUSED_STREAM, TRUE); + + + + + /* Exit condition: + * Either we should accept the stream and are ready to receive + HEADERS and DATA on it. + * Or we aren't and reject the stream + */ + SERF_H2_assert(stream->status == H2S_CLOSED + || stream->data->request != NULL); + + /* We must return a proper error or EOF here! */ + return APR_EOF; +} + serf_bucket_t * serf_http2__stream_handle_hpack(serf_http2_stream_t *stream, serf_bucket_t *bucket, @@ -219,23 +279,45 @@ serf_http2__stream_handle_hpack(serf_htt serf_config_t *config, serf_bucket_alloc_t *allocator) { - if (!stream->data->response_agg) - stream_setup_response(stream, config); + if (frametype == HTTP2_FRAME_TYPE_HEADERS) + { + if (!stream->data->response_agg) + stream_setup_response(stream, config); - bucket = serf__bucket_hpack_decode_create(bucket, NULL, NULL, max_entry_size, - hpack_tbl, allocator); + bucket = serf__bucket_hpack_decode_create(bucket, NULL, NULL, max_entry_size, + hpack_tbl, allocator); - serf_bucket_aggregate_append(stream->data->response_agg, bucket); + serf_bucket_aggregate_append(stream->data->response_agg, bucket); - if (end_stream) - { - if (stream->status == H2S_HALFCLOSED_LOCAL) - stream->status = H2S_CLOSED; - else - stream->status = H2S_HALFCLOSED_REMOTE; + if (end_stream) + { + if (stream->status == H2S_HALFCLOSED_LOCAL) + stream->status = H2S_CLOSED; + else + stream->status = H2S_HALFCLOSED_REMOTE; + } + return NULL; /* We want to drain the bucket ourselves */ } + else + { + serf_bucket_t *agg; + SERF_H2_assert(frametype == HTTP2_FRAME_TYPE_PUSH_PROMISE); + + /* First create the HPACK decoder as requested */ + bucket = serf__bucket_hpack_decode_create(bucket, + stream_promise_item, stream, + max_entry_size, + hpack_tbl, allocator); + + /* And now wrap around it the easiest way to get an EOF callback */ + agg = serf_bucket_aggregate_create(allocator); + serf_bucket_aggregate_append(agg, bucket); - return NULL; + serf_bucket_aggregate_hold_open(agg, stream_promise_done, stream); + + /* And return the aggregate, so the bucket will be drained for us */ + return agg; + } } serf_bucket_t *