You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by al...@apache.org on 2018/04/22 20:21:15 UTC
[1/3] mesos git commit: Libprocess: Fixed a crash in the presence of
request/response trailers.
Repository: mesos
Updated Branches:
refs/heads/master 654e5984f -> 2d7b7ef6d
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/d20c5530
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/d20c5530
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/d20c5530
Branch: refs/heads/master
Commit: d20c5530ccacbac991c5ab06f8fbce40d1817cb5
Parents: 654e598
Author: Alexander Rukletsov <al...@apache.org>
Authored: Thu Mar 15 15:33:19 2018 +0100
Committer: Alexander Rukletsov <al...@apache.org>
Committed: Sun Apr 22 22:12:04 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/d20c5530/3rdparty/libprocess/src/decoder.hpp
----------------------------------------------------------------------
diff --git a/3rdparty/libprocess/src/decoder.hpp b/3rdparty/libprocess/src/decoder.hpp
index dcb5df5..8ed8e92 100644
--- a/3rdparty/libprocess/src/decoder.hpp
+++ b/3rdparty/libprocess/src/decoder.hpp
@@ -629,7 +629,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;
@@ -647,7 +651,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;
@@ -658,6 +666,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.
@@ -692,6 +702,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;
@@ -865,7 +880,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
@@ -880,7 +898,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;
@@ -898,7 +920,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;
@@ -909,6 +935,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.
@@ -979,6 +1007,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;
[3/3] mesos git commit: Stout: Ensured exceptions are caught for
`picojson::parse()`.
Posted by al...@apache.org.
Stout: Ensured exceptions are caught for `picojson::parse()`.
`picojson::parse()` might throw in some cases, including when a parsed
number is out of bounds. This patch captures possible exceptions at
the call site and converts them to `Error`s.
Review: https://reviews.apache.org/r/66091
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/2d7b7ef6
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/2d7b7ef6
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/2d7b7ef6
Branch: refs/heads/master
Commit: 2d7b7ef6d17e81395214741a613a68a6fb86483f
Parents: d6552b0
Author: Alexander Rukletsov <al...@apache.org>
Authored: Tue Mar 13 13:37:49 2018 +0100
Committer: Alexander Rukletsov <al...@apache.org>
Committed: Sun Apr 22 22:20:23 2018 +0200
----------------------------------------------------------------------
3rdparty/stout/include/stout/json.hpp | 21 +++++++++++++++++----
3rdparty/stout/tests/json_tests.cpp | 14 ++++++++++++++
2 files changed, 31 insertions(+), 4 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/2d7b7ef6/3rdparty/stout/include/stout/json.hpp
----------------------------------------------------------------------
diff --git a/3rdparty/stout/include/stout/json.hpp b/3rdparty/stout/include/stout/json.hpp
index 102cd99..5e738cf 100644
--- a/3rdparty/stout/include/stout/json.hpp
+++ b/3rdparty/stout/include/stout/json.hpp
@@ -890,13 +890,26 @@ inline Try<Value> parse(const std::string& s)
// Because PicoJson supports repeated parsing of multiple objects/arrays in a
// stream, it will quietly ignore trailing non-whitespace characters. We would
// rather throw an error, however, so use `last_char` to check for this.
+ //
+ // TODO(alexr): Address cases when `s` is empty or consists only of whitespace
+ // characters.
const char* lastVisibleChar =
parseBegin + s.find_last_not_of(strings::WHITESPACE);
- // Parse the string, returning a pointer to the character
- // immediately following the last one parsed.
- const char* parseEnd =
- picojson::parse(value, parseBegin, parseBegin + s.size(), &error);
+ // Parse the string, returning a pointer to the character immediately
+ // following the last one parsed. Convert exceptions to `Error`s.
+ //
+ // TODO(alexr): Remove `try-catch` wrapper once picojson stops throwing
+ // on parsing, see https://github.com/kazuho/picojson/issues/94
+ const char* parseEnd;
+ try {
+ parseEnd =
+ picojson::parse(value, parseBegin, parseBegin + s.size(), &error);
+ } catch (const std::overflow_error&) {
+ return Error("Value out of range");
+ } catch (...) {
+ return Error("Unknown JSON parse error");
+ }
if (!error.empty()) {
return Error(error);
http://git-wip-us.apache.org/repos/asf/mesos/blob/2d7b7ef6/3rdparty/stout/tests/json_tests.cpp
----------------------------------------------------------------------
diff --git a/3rdparty/stout/tests/json_tests.cpp b/3rdparty/stout/tests/json_tests.cpp
index af00e42..508d843 100644
--- a/3rdparty/stout/tests/json_tests.cpp
+++ b/3rdparty/stout/tests/json_tests.cpp
@@ -270,6 +270,20 @@ TEST(JsonTest, ParseError)
" ";
EXPECT_ERROR(JSON::parse<JSON::Object>(jsonString));
+
+ jsonString = R"~(
+ {
+ "double1": 123123123121231231231231321312312312123123122E112312387129381723\x0d\x0a\x0d\x0a\x0d\x0a\x0d
+ })~";
+
+ EXPECT_ERROR(JSON::parse<JSON::Object>(jsonString));
+
+ jsonString = R"~(
+ {
+ "double2": -332861120361594135E400
+ })~";
+
+ EXPECT_ERROR(JSON::parse<JSON::Object>(jsonString));
}
[2/3] mesos git commit: Libprocess: Avoided magic values in
decoder.hpp.
Posted by al...@apache.org.
Libprocess: Avoided magic values in decoder.hpp.
Review: https://reviews.apache.org/r/66638
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/d6552b06
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/d6552b06
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/d6552b06
Branch: refs/heads/master
Commit: d6552b06cf2d1a345a8ac3ea61c0cb7eb5d293e3
Parents: d20c553
Author: Alexander Rukletsov <al...@apache.org>
Authored: Mon Apr 16 19:18:49 2018 +0200
Committer: Alexander Rukletsov <al...@apache.org>
Committed: Sun Apr 22 22:12:05 2018 +0200
----------------------------------------------------------------------
3rdparty/libprocess/src/decoder.hpp | 118 +++++++++++++++++--------------
1 file changed, 64 insertions(+), 54 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/d6552b06/3rdparty/libprocess/src/decoder.hpp
----------------------------------------------------------------------
diff --git a/3rdparty/libprocess/src/decoder.hpp b/3rdparty/libprocess/src/decoder.hpp
index 8ed8e92..a078435 100644
--- a/3rdparty/libprocess/src/decoder.hpp
+++ b/3rdparty/libprocess/src/decoder.hpp
@@ -41,6 +41,16 @@
namespace process {
+namespace http_parsing {
+
+// We expect callbacks to return 0 on success and 1 on failure. These constants
+// are introduced solely to make decoders' code easier to read and are not meant
+// to be used outside.
+constexpr int SUCCESS = 0;
+constexpr int FAILURE = 1;
+
+} // namespace http_parsing {
+
// TODO(benh): Make DataDecoder abstract and make RequestDecoder a
// concrete subclass.
class DataDecoder
@@ -115,17 +125,17 @@ private:
decoder->request = new http::Request();
- return 0;
+ return http_parsing::SUCCESS;
}
static int on_chunk_complete(http_parser* p)
{
- return 0;
+ return http_parsing::SUCCESS;
}
static int on_chunk_header(http_parser* p)
{
- return 0;
+ return http_parsing::SUCCESS;
}
static int on_url(http_parser* p, const char* data, size_t length)
@@ -139,7 +149,7 @@ private:
// `on_message_complete`.
decoder->url.append(data, length);
- return 0;
+ return http_parsing::SUCCESS;
}
static int on_header_field(http_parser* p, const char* data, size_t length)
@@ -156,7 +166,7 @@ private:
decoder->field.append(data, length);
decoder->header = HEADER_FIELD;
- return 0;
+ return http_parsing::SUCCESS;
}
static int on_header_value(http_parser* p, const char* data, size_t length)
@@ -165,7 +175,7 @@ private:
CHECK_NOTNULL(decoder->request);
decoder->value.append(data, length);
decoder->header = HEADER_VALUE;
- return 0;
+ return http_parsing::SUCCESS;
}
static int on_headers_complete(http_parser* p)
@@ -184,7 +194,7 @@ private:
decoder->request->keepAlive = http_should_keep_alive(&decoder->parser) != 0;
- return 0;
+ return http_parsing::SUCCESS;
}
static int on_body(http_parser* p, const char* data, size_t length)
@@ -192,7 +202,7 @@ private:
DataDecoder* decoder = (DataDecoder*) p->data;
CHECK_NOTNULL(decoder->request);
decoder->request->body.append(data, length);
- return 0;
+ return http_parsing::SUCCESS;
}
static int on_message_complete(http_parser* p)
@@ -237,7 +247,7 @@ private:
if (decoded.isError()) {
decoder->failure = true;
- return 1;
+ return http_parsing::FAILURE;
}
decoder->request->url.query = decoded.get();
@@ -249,7 +259,7 @@ private:
Try<std::string> decompressed = gzip::decompress(decoder->request->body);
if (decompressed.isError()) {
decoder->failure = true;
- return 1;
+ return http_parsing::FAILURE;
}
decoder->request->body = decompressed.get();
@@ -262,7 +272,7 @@ private:
decoder->requests.push_back(decoder->request);
decoder->request = nullptr;
- return 0;
+ return http_parsing::SUCCESS;
}
bool failure;
@@ -363,22 +373,22 @@ private:
decoder->response->body.clear();
decoder->response->path.clear();
- return 0;
+ return http_parsing::SUCCESS;
}
static int on_chunk_complete(http_parser* p)
{
- return 0;
+ return http_parsing::SUCCESS;
}
static int on_chunk_header(http_parser* p)
{
- return 0;
+ return http_parsing::SUCCESS;
}
static int on_url(http_parser* p, const char* data, size_t length)
{
- return 0;
+ return http_parsing::SUCCESS;
}
static int on_header_field(http_parser* p, const char* data, size_t length)
@@ -395,7 +405,7 @@ private:
decoder->field.append(data, length);
decoder->header = HEADER_FIELD;
- return 0;
+ return http_parsing::SUCCESS;
}
static int on_header_value(http_parser* p, const char* data, size_t length)
@@ -404,7 +414,7 @@ private:
CHECK_NOTNULL(decoder->response);
decoder->value.append(data, length);
decoder->header = HEADER_VALUE;
- return 0;
+ return http_parsing::SUCCESS;
}
static int on_headers_complete(http_parser* p)
@@ -418,7 +428,7 @@ private:
decoder->field.clear();
decoder->value.clear();
- return 0;
+ return http_parsing::SUCCESS;
}
static int on_body(http_parser* p, const char* data, size_t length)
@@ -426,7 +436,7 @@ private:
ResponseDecoder* decoder = (ResponseDecoder*) p->data;
CHECK_NOTNULL(decoder->response);
decoder->response->body.append(data, length);
- return 0;
+ return http_parsing::SUCCESS;
}
static int on_message_complete(http_parser* p)
@@ -442,7 +452,7 @@ private:
http::Status::string(decoder->parser.status_code);
} else {
decoder->failure = true;
- return 1;
+ return http_parsing::FAILURE;
}
// We can only provide the gzip encoding.
@@ -452,7 +462,7 @@ private:
Try<std::string> decompressed = gzip::decompress(decoder->response->body);
if (decompressed.isError()) {
decoder->failure = true;
- return 1;
+ return http_parsing::FAILURE;
}
decoder->response->body = decompressed.get();
@@ -465,12 +475,12 @@ private:
decoder->responses.push_back(decoder->response);
decoder->response = nullptr;
- return 0;
+ return http_parsing::SUCCESS;
}
static int on_status(http_parser* p, const char* data, size_t length)
{
- return 0;
+ return http_parsing::SUCCESS;
}
bool failure;
@@ -602,27 +612,27 @@ private:
decoder->response->type = http::Response::PIPE;
decoder->writer = None();
- return 0;
+ return http_parsing::SUCCESS;
}
static int on_chunk_complete(http_parser* p)
{
- return 0;
+ return http_parsing::SUCCESS;
}
static int on_chunk_header(http_parser* p)
{
- return 0;
+ return http_parsing::SUCCESS;
}
static int on_status(http_parser* p, const char* data, size_t length)
{
- return 0;
+ return http_parsing::SUCCESS;
}
static int on_url(http_parser* p, const char* data, size_t length)
{
- return 0;
+ return http_parsing::SUCCESS;
}
static int on_header_field(http_parser* p, const char* data, size_t length)
@@ -632,7 +642,7 @@ private:
// 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;
+ return http_parsing::FAILURE;
}
if (decoder->header != HEADER_FIELD) {
@@ -644,7 +654,7 @@ private:
decoder->field.append(data, length);
decoder->header = HEADER_FIELD;
- return 0;
+ return http_parsing::SUCCESS;
}
static int on_header_value(http_parser* p, const char* data, size_t length)
@@ -654,12 +664,12 @@ private:
// 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;
+ return http_parsing::FAILURE;
}
decoder->value.append(data, length);
decoder->header = HEADER_VALUE;
- return 0;
+ return http_parsing::SUCCESS;
}
static int on_headers_complete(http_parser* p)
@@ -682,7 +692,7 @@ private:
http::Status::string(decoder->parser.status_code);
} else {
decoder->failure = true;
- return 1;
+ return http_parsing::FAILURE;
}
// We cannot provide streaming gzip decompression!
@@ -690,7 +700,7 @@ private:
decoder->response->headers.get("Content-Encoding");
if (encoding.isSome() && encoding.get() == "gzip") {
decoder->failure = true;
- return 1;
+ return http_parsing::FAILURE;
}
CHECK_NONE(decoder->writer);
@@ -709,7 +719,7 @@ private:
// auxiliary data structure.
decoder->response = nullptr;
- return 0;
+ return http_parsing::SUCCESS;
}
static int on_body(http_parser* p, const char* data, size_t length)
@@ -721,7 +731,7 @@ private:
http::Pipe::Writer writer = decoder->writer.get(); // Remove const.
writer.write(std::string(data, length));
- return 0;
+ return http_parsing::SUCCESS;
}
static int on_message_complete(http_parser* p)
@@ -732,7 +742,7 @@ private:
// earlier (e.g., due to invalid status code).
if (decoder->writer.isNone()) {
CHECK(decoder->failure);
- return 1;
+ return http_parsing::FAILURE;
}
http::Pipe::Writer writer = decoder->writer.get(); // Remove const.
@@ -740,7 +750,7 @@ private:
decoder->writer = None();
- return 0;
+ return http_parsing::SUCCESS;
}
bool failure;
@@ -863,17 +873,17 @@ private:
decoder->writer = None();
decoder->decompressor.reset();
- return 0;
+ return http_parsing::SUCCESS;
}
static int on_chunk_complete(http_parser* p)
{
- return 0;
+ return http_parsing::SUCCESS;
}
static int on_chunk_header(http_parser* p)
{
- return 0;
+ return http_parsing::SUCCESS;
}
static int on_url(http_parser* p, const char* data, size_t length)
@@ -882,7 +892,7 @@ private:
// URL should not be parsed after `on_headers_complete` has been called.
if (decoder->request == nullptr) {
- return 1;
+ return http_parsing::FAILURE;
}
// The current http_parser library (version 2.6.2 and below)
@@ -891,7 +901,7 @@ private:
// `on_header_complete`.
decoder->url.append(data, length);
- return 0;
+ return http_parsing::SUCCESS;
}
static int on_header_field(http_parser* p, const char* data, size_t length)
@@ -901,7 +911,7 @@ private:
// 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;
+ return http_parsing::FAILURE;
}
if (decoder->header != HEADER_FIELD) {
@@ -913,7 +923,7 @@ private:
decoder->field.append(data, length);
decoder->header = HEADER_FIELD;
- return 0;
+ return http_parsing::SUCCESS;
}
static int on_header_value(http_parser* p, const char* data, size_t length)
@@ -923,12 +933,12 @@ private:
// 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;
+ return http_parsing::FAILURE;
}
decoder->value.append(data, length);
decoder->header = HEADER_VALUE;
- return 0;
+ return http_parsing::SUCCESS;
}
static int on_headers_complete(http_parser* p)
@@ -985,7 +995,7 @@ private:
if (decoded.isError()) {
decoder->failure = true;
- return 1;
+ return http_parsing::FAILURE;
}
decoder->request->url.query = std::move(decoded.get());
@@ -1014,7 +1024,7 @@ private:
// auxiliary data structure.
decoder->request = nullptr;
- return 0;
+ return http_parsing::SUCCESS;
}
static int on_body(http_parser* p, const char* data, size_t length)
@@ -1032,7 +1042,7 @@ private:
if (decompressed.isError()) {
decoder->failure = true;
- return 1;
+ return http_parsing::FAILURE;
}
body = std::move(decompressed.get());
@@ -1042,7 +1052,7 @@ private:
writer.write(std::move(body));
- return 0;
+ return http_parsing::SUCCESS;
}
static int on_message_complete(http_parser* p)
@@ -1053,7 +1063,7 @@ private:
// earlier (e.g., due to invalid query parameters).
if (decoder->writer.isNone()) {
CHECK(decoder->failure);
- return 1;
+ return http_parsing::FAILURE;
}
http::Pipe::Writer writer = decoder->writer.get(); // Remove const.
@@ -1062,14 +1072,14 @@ private:
!decoder->decompressor->finished()) {
writer.fail("Failed to decompress body");
decoder->failure = true;
- return 1;
+ return http_parsing::FAILURE;
}
writer.close();
decoder->writer = None();
- return 0;
+ return http_parsing::SUCCESS;
}
bool failure;