Repository: mesos Updated Branches: refs/heads/master 41e94c5af -> fc17c574a
Request /files/read.json with a negative length value causes error. [MESOS-5060] The patch did the following changes: 1. Fix the length logic in files.cpp. 2. Add some tests to test the /files/read.json endponit with negative length. Review: https://reviews.apache.org/r/46373/ Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/fc17c574 Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/fc17c574 Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/fc17c574 Branch: refs/heads/master Commit: fc17c574aa9634777d1e0fa6feda1e3c7127d7f8 Parents: 41e94c5 Author: zhou xing <[email protected]> Authored: Mon May 30 12:26:45 2016 -0700 Committer: Vinod Kone <[email protected]> Committed: Mon May 30 12:26:45 2016 -0700 ---------------------------------------------------------------------- src/files/files.cpp | 28 ++++++++++++++++++++-------- src/tests/files_tests.cpp | 31 +++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 8 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/fc17c574/src/files/files.cpp ---------------------------------------------------------------------- diff --git a/src/files/files.cpp b/src/files/files.cpp index 074d31c..873664d 100644 --- a/src/files/files.cpp +++ b/src/files/files.cpp @@ -128,7 +128,7 @@ private: Future<Response> _read( off_t offset, - ssize_t length, + Option<size_t> length, const string& path, const Option<string>& jsonp); @@ -434,7 +434,7 @@ Future<Response> FilesProcess::read( offset = result.get(); } - ssize_t length = -1; + Option<size_t> length; if (request.url.query.get("length").isSome()) { Try<ssize_t> result = numify<ssize_t>( @@ -444,7 +444,19 @@ Future<Response> FilesProcess::read( return BadRequest("Failed to parse length: " + result.error() + ".\n"); } - length = result.get(); + // TODO(tomxing): The pailer in the webui sends `length=-1` at first to + // determine the length of the file, so we allow a length of -1. + // Setting `length=-1` has the same effect as not providing a length: we + // read to the end of the file, up to the maximum read length. + // Will change this logic in MESOS-5334. + if (result.get() < -1) { + return BadRequest( + strings::format("Negative length provided: %d.\n", result.get()).get()); + } + + if (result.get() > -1){ + length = result.get(); + } } string requestedPath = path.get(); @@ -464,7 +476,7 @@ Future<Response> FilesProcess::read( Future<Response> FilesProcess::_read( off_t offset, - ssize_t length, + Option<size_t> length, const string& path, const Option<string>& jsonp) { @@ -511,12 +523,12 @@ Future<Response> FilesProcess::_read( offset = size; } - if (length == -1) { + if (length.isNone()) { length = size - offset; } // Cap the read length at 16 pages. - length = std::min<ssize_t>(length, os::pagesize() * 16); + length = std::min<size_t>(length.get(), os::pagesize() * 16); if (offset >= size) { os::close(fd.get()); @@ -549,9 +561,9 @@ Future<Response> FilesProcess::_read( } // Read 'length' bytes (or to EOF). - boost::shared_array<char> data(new char[length]); + boost::shared_array<char> data(new char[length.get()]); - return io::read(fd.get(), data.get(), static_cast<size_t>(length)) + return io::read(fd.get(), data.get(), length.get()) .then(lambda::bind( __read, fd.get(), http://git-wip-us.apache.org/repos/asf/mesos/blob/fc17c574/src/tests/files_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/files_tests.cpp b/src/tests/files_tests.cpp index 08f357b..b8f4f3c 100644 --- a/src/tests/files_tests.cpp +++ b/src/tests/files_tests.cpp @@ -197,6 +197,37 @@ TEST_F(FilesTest, ReadTest) AWAIT_EXPECT_RESPONSE_STATUS_EQ(Forbidden().status, response); + // TODO(tomxing): The pailer in the webui will send length=-1 at first to + // determine the length of the file, so we need to accept a length of -1. + // Setting `length=-1` has the same effect as not providing a length: we + // read to the end of the file, up to the maximum read length. + // Will change or remove this test case in MESOS-5334. + // Read a valid file with length set as -1. + response = process::http::get( + upid, + "read", + "path=/myname&length=-1&offset=0"); + + AWAIT_EXPECT_RESPONSE_STATUS_EQ(OK().status, response); + AWAIT_EXPECT_RESPONSE_BODY_EQ(stringify(expected), response); + + // Read a valid file with negative length(not -1). + response = process::http::get(upid, "read", "path=/myname&length=-2"); + + AWAIT_EXPECT_RESPONSE_STATUS_EQ(BadRequest().status, response); + AWAIT_EXPECT_RESPONSE_BODY_EQ( + "Negative length provided: -2.\n", + response); + + // Read a valid file with positive length. + expected.values["offset"] = 0; + expected.values["data"] = "bo"; + + response = process::http::get(upid, "read", "path=/myname&offset=0&length=2"); + + AWAIT_EXPECT_RESPONSE_STATUS_EQ(OK().status, response); + AWAIT_EXPECT_RESPONSE_BODY_EQ(stringify(expected), response); + // Missing file. AWAIT_EXPECT_RESPONSE_STATUS_EQ( NotFound().status,
