Repository: mesos Updated Branches: refs/heads/1.3.x 308f6221c -> 770ec58bb
Libprocess: Fixed a crash in the presence of request/response trailers. Prior to this patch, both `StreamingRequest` and `StreamingResponse` decoders made an assumption that the underlying HTTP parser never yields a header after having reported that all headers have been processed, i.e., `on_header_field()` and `on_header_value()` callbacks are never called after `on_headers_complete` has been called. This assumption is incorrect: HTTP parser can yield a header after body for trailers in chunked requests and responses or for malformed messages. This patch relaxes the assumption. Instead of crashing, HTTP message parsing is gracefully aborted and a decoding error is returned. Review: https://reviews.apache.org/r/66090 Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/b887783c Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/b887783c Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/b887783c Branch: refs/heads/1.3.x Commit: b887783c3701c76ea2c1f9fbf5c31860ee0cc3b1 Parents: 308f622 Author: Alexander Rukletsov <[email protected]> Authored: Thu Mar 15 15:33:19 2018 +0100 Committer: Alexander Rukletsov <[email protected]> Committed: Sun Apr 22 22:31:49 2018 +0200 ---------------------------------------------------------------------- 3rdparty/libprocess/src/decoder.hpp | 43 ++++++++++++++++++++++++++++---- 1 file changed, 38 insertions(+), 5 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/b887783c/3rdparty/libprocess/src/decoder.hpp ---------------------------------------------------------------------- diff --git a/3rdparty/libprocess/src/decoder.hpp b/3rdparty/libprocess/src/decoder.hpp index 31e8851..2d54511 100644 --- a/3rdparty/libprocess/src/decoder.hpp +++ b/3rdparty/libprocess/src/decoder.hpp @@ -625,7 +625,11 @@ private: { StreamingResponseDecoder* decoder = (StreamingResponseDecoder*) p->data; - CHECK_NOTNULL(decoder->response); + // TODO(alexr): We currently do not support trailers, i.e., headers after + // `on_headers_complete` has been called, and instead treat them as errors. + if (decoder->response == nullptr) { + return 1; + } if (decoder->header != HEADER_FIELD) { decoder->response->headers[decoder->field] = decoder->value; @@ -643,7 +647,11 @@ private: { StreamingResponseDecoder* decoder = (StreamingResponseDecoder*) p->data; - CHECK_NOTNULL(decoder->response); + // TODO(alexr): We currently do not support trailers, i.e., headers after + // `on_headers_complete` has been called, and instead treat them as errors. + if (decoder->response == nullptr) { + return 1; + } decoder->value.append(data, length); decoder->header = HEADER_VALUE; @@ -654,6 +662,8 @@ private: { StreamingResponseDecoder* decoder = (StreamingResponseDecoder*) p->data; + // This asserts not only that `on_message_begin` has been previously called, + // but also that `on_headers_complete` is not called more than once. CHECK_NOTNULL(decoder->response); // Add final header. @@ -688,6 +698,11 @@ private: // Send the response to the caller, but keep a Pipe::Writer for // streaming the body content into the response. decoder->responses.push_back(decoder->response); + + // TODO(alexr): We currently do not support trailers, i.e., extra headers + // after `on_headers_complete` has been called. When we add trailer support, + // we need a thread-safe way to surface them up in the response or some + // auxiliary data structure. decoder->response = nullptr; return 0; @@ -861,7 +876,10 @@ private: { StreamingRequestDecoder* decoder = (StreamingRequestDecoder*) p->data; - CHECK_NOTNULL(decoder->request); + // URL should not be parsed after `on_headers_complete` has been called. + if (decoder->request == nullptr) { + return 1; + } // The current http_parser library (version 2.6.2 and below) // does not support incremental parsing of URLs. To compensate @@ -876,7 +894,11 @@ private: { StreamingRequestDecoder* decoder = (StreamingRequestDecoder*) p->data; - CHECK_NOTNULL(decoder->request); + // TODO(alexr): We currently do not support trailers, i.e., headers after + // `on_headers_complete` has been called, and instead treat them as errors. + if (decoder->request == nullptr) { + return 1; + } if (decoder->header != HEADER_FIELD) { decoder->request->headers[decoder->field] = decoder->value; @@ -894,7 +916,11 @@ private: { StreamingRequestDecoder* decoder = (StreamingRequestDecoder*) p->data; - CHECK_NOTNULL(decoder->request); + // TODO(alexr): We currently do not support trailers, i.e., headers after + // `on_headers_complete` has been called, and instead treat them as errors. + if (decoder->request == nullptr) { + return 1; + } decoder->value.append(data, length); decoder->header = HEADER_VALUE; @@ -905,6 +931,8 @@ private: { StreamingRequestDecoder* decoder = (StreamingRequestDecoder*) p->data; + // This asserts not only that `on_message_begin` has been previously called, + // but also that `on_headers_complete` is not called more than once. CHECK_NOTNULL(decoder->request); // Add final header. @@ -975,6 +1003,11 @@ private: // Send the request to the caller, but keep a Pipe::Writer for // streaming the body content into the request. decoder->requests.push_back(decoder->request); + + // TODO(alexr): We currently do not support trailers, i.e., extra headers + // after `on_headers_complete` has been called. When we add trailer support, + // we need a thread-safe way to surface them up in the request or some + // auxiliary data structure. decoder->request = nullptr; return 0;
