From 1cbbdffd710c46f9a3b0a80b68eeadc1178ceb89 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Sat, 29 Aug 2015 16:37:12 +0200 Subject: [PATCH 01/13] buffer: Use evbuffer --- include/measurement_kit/net/buffer.hpp | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/include/measurement_kit/net/buffer.hpp b/include/measurement_kit/net/buffer.hpp index 75e5de68a..c6b27fb69 100644 --- a/include/measurement_kit/net/buffer.hpp +++ b/include/measurement_kit/net/buffer.hpp @@ -6,6 +6,7 @@ #define MEASUREMENT_KIT_NET_BUFFER_HPP #include +#include #include #include @@ -72,19 +73,15 @@ class Iovec : public NonCopyable, public NonMovable { class Buffer : public NonCopyable, public NonMovable { - evbuffer *evbuf; + Evbuffer evbuf; public: Buffer(evbuffer *b = NULL) { - if ((evbuf = evbuffer_new()) == NULL) - throw std::bad_alloc(); - if (b != NULL && evbuffer_add_buffer(evbuf, b) != 0) { - evbuffer_free(evbuf); + if (b != NULL && evbuffer_add_buffer(evbuf, b) != 0) throw std::runtime_error("evbuffer_add_buffer failed"); - } } - ~Buffer(void) { evbuffer_free(evbuf); /* Cannot be NULL */ } + ~Buffer(void) {} /* * I expect to read (write) from (into) the input (output) From 364b077680d677271725ff325db5300a5a5a0c9c Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Sun, 30 Aug 2015 20:32:02 +0200 Subject: [PATCH 02/13] Cosmetic changes --- include/measurement_kit/net/buffer.hpp | 91 +++++++++++++------------- src/net/buffer.cpp | 7 +- 2 files changed, 50 insertions(+), 48 deletions(-) diff --git a/include/measurement_kit/net/buffer.hpp b/include/measurement_kit/net/buffer.hpp index c6b27fb69..87c4f8108 100644 --- a/include/measurement_kit/net/buffer.hpp +++ b/include/measurement_kit/net/buffer.hpp @@ -32,22 +32,22 @@ using namespace measurement_kit::common; // Helper class for Buffer (see below) class Iovec : public NonCopyable, public NonMovable { - evbuffer_iovec *iov = NULL; + evbuffer_iovec *iov = nullptr; size_t count = 0; public: Iovec(size_t n = 0) { if (n < 1) return; - if ((iov = (evbuffer_iovec *)calloc(n, sizeof(*iov))) == NULL) + if ((iov = (evbuffer_iovec *)calloc(n, sizeof(*iov))) == nullptr) throw std::bad_alloc(); count = n; } - ~Iovec(void) { measurement_kit::xfree(iov); } + ~Iovec() { measurement_kit::xfree(iov); } evbuffer_iovec *operator[](int i) { - if (iov == NULL) + if (iov == nullptr) throw std::runtime_error("Iovec is null"); /* * It is safe to cast i to size_t, because we exclude @@ -55,33 +55,33 @@ class Iovec : public NonCopyable, public NonMovable { */ if (i < 0 || (size_t)i >= count) throw std::runtime_error("Invalid index"); - return (&iov[i]); + return &iov[i]; } - evbuffer_iovec *get_base(void) { - if (iov == NULL) + evbuffer_iovec *get_base() { + if (iov == nullptr) throw std::runtime_error("Iovec is null"); - return (iov); + return iov; } - size_t get_length(void) { - if (iov == NULL) + size_t get_length() { + if (iov == nullptr) throw std::runtime_error("Iovec is null"); - return (count); + return count; } }; class Buffer : public NonCopyable, public NonMovable { - + private: Evbuffer evbuf; public: - Buffer(evbuffer *b = NULL) { - if (b != NULL && evbuffer_add_buffer(evbuf, b) != 0) + Buffer(evbuffer *b = nullptr) { + if (b != nullptr && evbuffer_add_buffer(evbuf, b) != 0) throw std::runtime_error("evbuffer_add_buffer failed"); } - ~Buffer(void) {} + ~Buffer() {} /* * I expect to read (write) from (into) the input (output) @@ -90,39 +90,39 @@ class Buffer : public NonCopyable, public NonMovable { */ Buffer &operator<<(evbuffer *source) { - if (source == NULL) - throw std::runtime_error("source is NULL"); + if (source == nullptr) + throw std::runtime_error("source is nullptr"); if (evbuffer_add_buffer(evbuf, source) != 0) throw std::runtime_error("evbuffer_add_buffer failed"); - return (*this); + return *this; } Buffer &operator>>(evbuffer *dest) { - if (dest == NULL) - throw std::runtime_error("dest is NULL"); + if (dest == nullptr) + throw std::runtime_error("dest is nullptr"); if (evbuffer_add_buffer(dest, evbuf) != 0) throw std::runtime_error("evbuffer_add_buffer failed"); - return (*this); + return *this; } Buffer &operator<<(Buffer &source) { *this << source.evbuf; - return (*this); + return *this; } Buffer &operator>>(Buffer &source) { *this >> source.evbuf; - return (*this); + return *this; } - size_t length(void) { return (evbuffer_get_length(evbuf)); } + size_t length() { return evbuffer_get_length(evbuf); } /* * The following is useful to feed a parser (e.g., the http-parser) * with all (or part of) the content of `this`. */ void foreach (std::function fn) { - auto required_size = evbuffer_peek(evbuf, -1, NULL, NULL, 0); + auto required_size = evbuffer_peek(evbuf, -1, nullptr, nullptr, 0); if (required_size < 0) throw std::runtime_error("unexpected error"); if (required_size == 0) @@ -130,7 +130,7 @@ class Buffer : public NonCopyable, public NonMovable { Iovec iov(required_size); auto used = - evbuffer_peek(evbuf, -1, NULL, iov.get_base(), iov.get_length()); + evbuffer_peek(evbuf, -1, nullptr, iov.get_base(), iov.get_length()); if (used != required_size) throw std::runtime_error("unexpected error"); @@ -147,7 +147,7 @@ class Buffer : public NonCopyable, public NonMovable { if (evbuffer_drain(evbuf, count) != 0) throw std::runtime_error("evbuffer_drain failed"); } - void discard(void) { discard(length()); } + void discard() { discard(length()); } /* * This function is a template because sometimes we want to read @@ -183,15 +183,15 @@ class Buffer : public NonCopyable, public NonMovable { if (!ispeek) discard(nbytes); - return (out); + return out; } template std::basic_string read(size_t upto) { return readpeek(false, upto); } - template std::basic_string read(void) { - return (read(length())); + template std::basic_string read() { + return read(length()); } std::string read(size_t upto) { return read(upto); } @@ -208,8 +208,8 @@ class Buffer : public NonCopyable, public NonMovable { */ template std::basic_string readn(size_t n) { if (n > length()) - return (std::basic_string()); /* Empty str */ - return (read(n)); + return std::basic_string(); /* Empty str */ + return read(n); } std::string readn(size_t n) { return readn(n); } @@ -218,11 +218,11 @@ class Buffer : public NonCopyable, public NonMovable { size_t eol_length = 0; auto search_result = - evbuffer_search_eol(evbuf, NULL, &eol_length, EVBUFFER_EOL_CRLF); + evbuffer_search_eol(evbuf, nullptr, &eol_length, EVBUFFER_EOL_CRLF); if (search_result.pos < 0) { if (length() > maxline) - return (std::make_tuple(-1, "")); - return (std::make_tuple(0, "")); + return std::make_tuple(-1, ""); + return std::make_tuple(0, ""); } /* @@ -233,9 +233,9 @@ class Buffer : public NonCopyable, public NonMovable { throw std::runtime_error("unexpected error"); auto len = (size_t)search_result.pos + eol_length; if (len > maxline) - return (std::make_tuple(-2, "")); + return std::make_tuple(-2, ""); - return (std::make_tuple(0, read(len))); + return std::make_tuple(0, read(len)); } /* @@ -247,30 +247,30 @@ class Buffer : public NonCopyable, public NonMovable { Buffer &operator<<(std::string in) { write(in); - return (*this); + return *this; } void write(std::vector in) { write(in.data(), in.size()); } Buffer &operator<<(std::vector in) { write(in); - return (*this); + return *this; } void write(const char *in) { - if (in == NULL) - throw std::runtime_error("in is NULL"); + if (in == nullptr) + throw std::runtime_error("in is nullptr"); write(in, strlen(in)); } Buffer &operator<<(const char *in) { write(in); - return (*this); + return *this; } void write(const void *buf, size_t count) { - if (buf == NULL) - throw std::runtime_error("buf is NULL"); + if (buf == nullptr) + throw std::runtime_error("buf is nullptr"); if (evbuffer_add(evbuf, buf, count) != 0) throw std::runtime_error("evbuffer_add failed"); } @@ -319,5 +319,6 @@ class Buffer : public NonCopyable, public NonMovable { } }; -}} +} // namespace net +} // namespace measurement_kit #endif diff --git a/src/net/buffer.cpp b/src/net/buffer.cpp index 21c93f461..74a5b8ec1 100644 --- a/src/net/buffer.cpp +++ b/src/net/buffer.cpp @@ -2,10 +2,10 @@ // Measurement-kit is free software. See AUTHORS and LICENSE for more // information on the copying conditions. -#include - #include +#include + namespace measurement_kit { namespace net { @@ -23,4 +23,5 @@ void Buffer::write_uint32(uint32_t num) { write(&num, sizeof (num)); } -}} +} // namespace net +} // namespace measurement_kit From b163300a68c4ed8682ffd1aa17e9a7a9a104a1a5 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Sun, 30 Aug 2015 20:37:46 +0200 Subject: [PATCH 03/13] Remove over engineering 1) no need to implement readpeek() as a template 2) no need to write std::vector --- include/measurement_kit/http/http.hpp | 2 +- include/measurement_kit/net/buffer.hpp | 50 ++++---------------- include/measurement_kit/ooni/http_test.hpp | 2 +- test/http/http.cpp | 24 +++++----- test/net/buffer.cpp | 53 ++++++---------------- 5 files changed, 39 insertions(+), 92 deletions(-) diff --git a/include/measurement_kit/http/http.hpp b/include/measurement_kit/http/http.hpp index 61a708106..dde2a9b22 100644 --- a/include/measurement_kit/http/http.hpp +++ b/include/measurement_kit/http/http.hpp @@ -603,7 +603,7 @@ class Request : public NonCopyable, public NonMovable { // to reduce unnecessary copies Buffer buf; serializer.serialize(buf); - *stream << buf.read(); + *stream << buf.read(); stream->on_flush([this]() { logger->debug("http: request sent... waiting for response"); diff --git a/include/measurement_kit/net/buffer.hpp b/include/measurement_kit/net/buffer.hpp index 87c4f8108..27baee063 100644 --- a/include/measurement_kit/net/buffer.hpp +++ b/include/measurement_kit/net/buffer.hpp @@ -149,27 +149,15 @@ class Buffer : public NonCopyable, public NonMovable { } void discard() { discard(length()); } - /* - * This function is a template because sometimes we want to read - * text (in which case, std::basic_string aka std::string is - * the appropriate type) and sometimes we want instead to read - * binary data (in which case std::basic_string is more - * appropriate, because it is surprising to store binary data into - * an std::string, which is designed to hold text). - */ - template - std::basic_string readpeek(bool ispeek, size_t upto) { + std::string readpeek(bool ispeek, size_t upto) { size_t nbytes = 0; - std::basic_string out; - - if (sizeof(T) != 1) - throw std::runtime_error("Wide chars not supported"); + std::string out; foreach([&](evbuffer_iovec *iov) { if (upto < iov->iov_len) iov->iov_len = upto; - out.append((const T *)iov->iov_base, iov->iov_len); + out.append((const char *)iov->iov_base, iov->iov_len); upto -= iov->iov_len; nbytes += iov->iov_len; @@ -186,19 +174,11 @@ class Buffer : public NonCopyable, public NonMovable { return out; } - template std::basic_string read(size_t upto) { - return readpeek(false, upto); - } - - template std::basic_string read() { - return read(length()); - } - - std::string read(size_t upto) { return read(upto); } + std::string read(size_t upto) { return readpeek(false, upto); } - std::string read() { return read(); } + std::string read() { return read(length()); } - std::string peek(size_t upto) { return readpeek(true, upto); } + std::string peek(size_t upto) { return readpeek(true, upto); } std::string peek() { return peek(length()); } @@ -206,14 +186,11 @@ class Buffer : public NonCopyable, public NonMovable { * The semantic of readn() is that we return a string only * when we have exactly N bytes available. */ - template std::basic_string readn(size_t n) { - if (n > length()) - return std::basic_string(); /* Empty str */ - return read(n); + std::string readn(size_t n) { + if (n > length()) return ""; + return read(n); } - std::string readn(size_t n) { return readn(n); } - std::tuple readline(size_t maxline) { size_t eol_length = 0; @@ -235,7 +212,7 @@ class Buffer : public NonCopyable, public NonMovable { if (len > maxline) return std::make_tuple(-2, ""); - return std::make_tuple(0, read(len)); + return std::make_tuple(0, read(len)); } /* @@ -250,13 +227,6 @@ class Buffer : public NonCopyable, public NonMovable { return *this; } - void write(std::vector in) { write(in.data(), in.size()); } - - Buffer &operator<<(std::vector in) { - write(in); - return *this; - } - void write(const char *in) { if (in == nullptr) throw std::runtime_error("in is nullptr"); diff --git a/include/measurement_kit/ooni/http_test.hpp b/include/measurement_kit/ooni/http_test.hpp index 3eaf5d9d7..4fb123410 100644 --- a/include/measurement_kit/ooni/http_test.hpp +++ b/include/measurement_kit/ooni/http_test.hpp @@ -51,7 +51,7 @@ class HTTPTest : public ooni::NetTest { if (error == 0) { rr["response"]["headers"] = std::map(response.headers); - rr["response"]["body"] = response.body.read(); + rr["response"]["body"] = response.body.read(); rr["response"]["response_line"] = response.response_line; rr["response"]["code"] = response.status_code; } else { diff --git a/test/http/http.cpp b/test/http/http.cpp index b07b60099..36709fc9a 100644 --- a/test/http/http.cpp +++ b/test/http/http.cpp @@ -347,7 +347,7 @@ TEST_CASE("HTTP Request serializer works as expected") { }, "0123456789"); Buffer buffer; serializer.serialize(buffer); - auto serialized = buffer.read(); + auto serialized = buffer.read(); std::string expect = "GET /antani?clacsonato=yes HTTP/1.0\r\n"; expect += "User-Agent: Antani/1.0.0.0\r\n"; expect += "Host: www.example.com\r\n"; @@ -378,7 +378,7 @@ TEST_CASE("HTTP Request works as expected") { std::cout << kv.first << ": " << kv.second << "\r\n"; } std::cout << "\r\n"; - std::cout << response.body.read(128) << "\r\n"; + std::cout << response.body.read(128) << "\r\n"; std::cout << "[snip]\r\n"; measurement_kit::break_loop(); }); @@ -446,7 +446,7 @@ TEST_CASE("HTTP Request correctly receives errors") { std::cout << kv.first << ": " << kv.second << "\r\n"; } std::cout << "\r\n"; - std::cout << response.body.read(128) << "\r\n"; + std::cout << response.body.read(128) << "\r\n"; std::cout << "[snip]\r\n"; measurement_kit::break_loop(); }); @@ -475,7 +475,7 @@ TEST_CASE("HTTP Request works as expected over Tor") { std::cout << kv.first << ": " << kv.second << "\r\n"; } std::cout << "\r\n"; - std::cout << response.body.read(128) << "\r\n"; + std::cout << response.body.read(128) << "\r\n"; std::cout << "[snip]\r\n"; measurement_kit::break_loop(); }); @@ -496,7 +496,7 @@ TEST_CASE("HTTP Client works as expected") { {"Accept", "*/*"}, }, "", [&](Error, Response&& response) { std::cout << "Google:\r\n"; - std::cout << response.body.read(128) << "\r\n"; + std::cout << response.body.read(128) << "\r\n"; std::cout << "[snip]\r\n"; if (++count >= 3) { measurement_kit::break_loop(); @@ -510,7 +510,7 @@ TEST_CASE("HTTP Client works as expected") { }, { {"Accept", "*/*"}, }, "", [&](Error, Response&& response) { - std::cout << response.body.read(128) << "\r\n"; + std::cout << response.body.read(128) << "\r\n"; std::cout << "[snip]\r\n"; if (++count >= 3) { measurement_kit::break_loop(); @@ -524,7 +524,7 @@ TEST_CASE("HTTP Client works as expected") { }, { {"Accept", "*/*"}, }, "", [&](Error, Response&& response) { - std::cout << response.body.read(128) << "\r\n"; + std::cout << response.body.read(128) << "\r\n"; std::cout << "[snip]\r\n"; if (++count >= 3) { measurement_kit::break_loop(); @@ -550,7 +550,7 @@ TEST_CASE("HTTP Client works as expected over Tor") { }, "", [&](Error error, Response&& response) { std::cout << "Error: " << (int) error << std::endl; std::cout << "Google:\r\n"; - std::cout << response.body.read(128) << "\r\n"; + std::cout << response.body.read(128) << "\r\n"; std::cout << "[snip]\r\n"; if (++count >= 3) { measurement_kit::break_loop(); @@ -566,7 +566,7 @@ TEST_CASE("HTTP Client works as expected over Tor") { {"Accept", "*/*"}, }, "", [&](Error error, Response&& response) { std::cout << "Error: " << (int) error << std::endl; - std::cout << response.body.read(128) << "\r\n"; + std::cout << response.body.read(128) << "\r\n"; std::cout << "[snip]\r\n"; if (++count >= 3) { measurement_kit::break_loop(); @@ -582,7 +582,7 @@ TEST_CASE("HTTP Client works as expected over Tor") { {"Accept", "*/*"}, }, "", [&](Error error, Response&& response) { std::cout << "Error: " << (int) error << std::endl; - std::cout << response.body.read(128) << "\r\n"; + std::cout << response.body.read(128) << "\r\n"; std::cout << "[snip]\r\n"; if (++count >= 3) { measurement_kit::break_loop(); @@ -605,7 +605,7 @@ TEST_CASE("Make sure that we can access OONI's bouncer using httpo://...") { }, "{\"test-helpers\": [\"dns\"]}", [](Error error, Response&& response) { std::cout << "Error: " << (int) error << std::endl; - std::cout << response.body.read() << "\r\n"; + std::cout << response.body.read() << "\r\n"; std::cout << "[snip]\r\n"; measurement_kit::break_loop(); }); @@ -747,7 +747,7 @@ TEST_CASE("Make sure that settings are not modified") { // XXX: assumes that Tor is not running on port 9999 REQUIRE(error != 0); std::cout << "Error: " << (int) error << std::endl; - std::cout << response.body.read() << "\r\n"; + std::cout << response.body.read() << "\r\n"; std::cout << "[snip]\r\n"; measurement_kit::break_loop(); }); diff --git a/test/net/buffer.cpp b/test/net/buffer.cpp index 20d66dc13..7120d99a0 100644 --- a/test/net/buffer.cpp +++ b/test/net/buffer.cpp @@ -35,7 +35,7 @@ TEST_CASE("Insertion/extraction work correctly for evbuffer") { SECTION("Insertion works correctly") { buff << source; REQUIRE(buff.length() == 65536); - r = buff.read(); + r = buff.read(); REQUIRE(r == sa); } @@ -191,77 +191,67 @@ TEST_CASE("Read works correctly") { Buffer buff; SECTION("Read does not misbehave when the buffer is empty") { - auto s = buff.read(65535); + auto s = buff.read(65535); REQUIRE(s.length() == 0); } SECTION("We can read less bytes than the total size") { buff.write_rand(32768); - auto s = buff.read(1024); + auto s = buff.read(1024); REQUIRE(buff.length() == 32768 - 1024); REQUIRE(s.length() == 1024); } SECTION("We can read exactly the total size") { buff.write_rand(32768); - auto s = buff.read(32768); + auto s = buff.read(32768); REQUIRE(buff.length() == 0); REQUIRE(s.length() == 32768); } SECTION("Read with no args reads all") { buff.write_rand(32768); - auto s = buff.read(); + auto s = buff.read(); REQUIRE(buff.length() == 0); REQUIRE(s.length() == 32768); } SECTION("No harm if we ask more than the total size") { buff.write_rand(32768); - auto s = buff.read(65535); + auto s = buff.read(65535); REQUIRE(buff.length() == 0); REQUIRE(s.length() == 32768); } - - SECTION("Reading wide chars is not supported") { - buff.write_rand(32768); - REQUIRE_THROWS(buff.read(1024)); - } } TEST_CASE("Readn works correctly") { Buffer buff; SECTION("Readn does not misbehave when the buffer is empty") { - auto s = buff.readn(65535); + auto s = buff.readn(65535); REQUIRE(s.length() == 0); } SECTION("We can readn less bytes than the total size") { buff.write_rand(32768); - auto s = buff.readn(1024); + auto s = buff.readn(1024); REQUIRE(buff.length() == 32768 - 1024); REQUIRE(s.length() == 1024); } SECTION("We can readn exactly the total size") { buff.write_rand(32768); - auto s = buff.readn(32768); + auto s = buff.readn(32768); REQUIRE(buff.length() == 0); REQUIRE(s.length() == 32768); } SECTION("Empty string returned if we ask more than the total size") { buff.write_rand(32768); - auto s = buff.readn(65535); + auto s = buff.readn(65535); REQUIRE(buff.length() == 32768); REQUIRE(s.length() == 0); } - - SECTION("Readn-ing wide chars is not supported") { - buff.write_rand(32768); - REQUIRE_THROWS(buff.readn(1024)); - } } TEST_CASE("Readline works correctly", "[Buffer]") { @@ -337,36 +327,23 @@ TEST_CASE("Write works correctly", "[Buffer]") { Buffer buff; auto pc = "0123456789"; auto str = std::string(pc); - auto vect = std::vector(str.begin(), str.end()); SECTION("Writing a C++ string works") { buff.write(str); REQUIRE(buff.length() == 10); - REQUIRE(buff.read() == str); + REQUIRE(buff.read() == str); } SECTION("Inserting a C++ string works") { buff << str; REQUIRE(buff.length() == 10); - REQUIRE(buff.read() == str); - } - - SECTION("Writing a C++ vector works") { - buff.write(vect); - REQUIRE(buff.length() == 10); - REQUIRE(buff.read() == str); - } - - SECTION("Inserting a C++ vector works") { - buff << vect; - REQUIRE(buff.length() == 10); - REQUIRE(buff.read() == str); + REQUIRE(buff.read() == str); } SECTION("Writing a C string works") { buff.write(pc); REQUIRE(buff.length() == 10); - REQUIRE(buff.read() == str); + REQUIRE(buff.read() == str); } SECTION("Writing a NULL C string throws") { @@ -376,7 +353,7 @@ TEST_CASE("Write works correctly", "[Buffer]") { SECTION("Inserting a C string works") { buff << pc; REQUIRE(buff.length() == 10); - REQUIRE(buff.read() == str); + REQUIRE(buff.read() == str); } SECTION("Inserting a NULL C string throws") { @@ -386,7 +363,7 @@ TEST_CASE("Write works correctly", "[Buffer]") { SECTION("Writing pointer-and-size works") { buff.write((void *) pc, 10); REQUIRE(buff.length() == 10); - REQUIRE(buff.read() == str); + REQUIRE(buff.read() == str); } SECTION("Writing NULL pointer and size throws") { From 463a13404db787ba992a1b038506aa8bdf69589a Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Sun, 30 Aug 2015 21:08:10 +0200 Subject: [PATCH 04/13] Avoid Iovec --- include/measurement_kit/net/buffer.hpp | 99 ++++---------------------- 1 file changed, 14 insertions(+), 85 deletions(-) diff --git a/include/measurement_kit/net/buffer.hpp b/include/measurement_kit/net/buffer.hpp index 27baee063..fa48d24d0 100644 --- a/include/measurement_kit/net/buffer.hpp +++ b/include/measurement_kit/net/buffer.hpp @@ -18,59 +18,14 @@ #include #include #include +#include #include -/* Apparently not defined on MacOSX */ -#ifndef IOV_MAX -#define IOV_MAX 32 -#endif - namespace measurement_kit { namespace net { using namespace measurement_kit::common; -// Helper class for Buffer (see below) -class Iovec : public NonCopyable, public NonMovable { - evbuffer_iovec *iov = nullptr; - size_t count = 0; - - public: - Iovec(size_t n = 0) { - if (n < 1) - return; - if ((iov = (evbuffer_iovec *)calloc(n, sizeof(*iov))) == nullptr) - throw std::bad_alloc(); - count = n; - } - - ~Iovec() { measurement_kit::xfree(iov); } - - evbuffer_iovec *operator[](int i) { - if (iov == nullptr) - throw std::runtime_error("Iovec is null"); - /* - * It is safe to cast i to size_t, because we exclude - * the case in which i is negative first. - */ - if (i < 0 || (size_t)i >= count) - throw std::runtime_error("Invalid index"); - return &iov[i]; - } - - evbuffer_iovec *get_base() { - if (iov == nullptr) - throw std::runtime_error("Iovec is null"); - return iov; - } - - size_t get_length() { - if (iov == nullptr) - throw std::runtime_error("Iovec is null"); - return count; - } -}; - class Buffer : public NonCopyable, public NonMovable { private: Evbuffer evbuf; @@ -127,14 +82,14 @@ class Buffer : public NonCopyable, public NonMovable { throw std::runtime_error("unexpected error"); if (required_size == 0) return; - - Iovec iov(required_size); - auto used = - evbuffer_peek(evbuf, -1, nullptr, iov.get_base(), iov.get_length()); + std::unique_ptr raii; + raii.reset(new evbuffer_iovec[required_size]); // Guarantee cleanup + auto iov = raii.get(); + auto used = evbuffer_peek(evbuf, -1, nullptr, iov, required_size); if (used != required_size) throw std::runtime_error("unexpected error"); - for (auto i = 0; i < required_size && fn(iov[i]); ++i) + for (auto i = 0; i < required_size && fn(&iov[i]); ++i) /* nothing */; } @@ -252,40 +207,14 @@ class Buffer : public NonCopyable, public NonMovable { void write_uint32(uint32_t); void write_rand(size_t count) { - if (count == 0) - return; - - Iovec iov(IOV_MAX); - int i, n_extents; - - n_extents = evbuffer_reserve_space(evbuf, count, iov.get_base(), - iov.get_length()); - if (n_extents < 0) - throw std::runtime_error("evbuffer_reserve_space failed"); - - for (i = 0; i < n_extents && count > 0; i++) { - /* - * Note: `iov[i]` throws if `i` is out of bounds. - */ - if (count < iov[i]->iov_len) - iov[i]->iov_len = count; - /* - * Implementation note: I'm not sure this is fast - * enough for Neubot needs; I'll check... - */ - evutil_secure_rng_get_bytes(iov[i]->iov_base, iov[i]->iov_len); - count -= iov[i]->iov_len; - } - - /* - * Used to be an assert. I'd rather have a statement that - * throws on failure, so that it can be tested. - */ - if (!(count == 0 && i >= 0 && (size_t)i <= iov.get_length())) - throw std::runtime_error("unexpected error"); - - if (evbuffer_commit_space(evbuf, iov.get_base(), i) != 0) - throw std::runtime_error("evbuffer_commit_space failed"); + if (count == 0) return; + char *p = new char[count]; + evutil_secure_rng_get_bytes(p, count); + auto ctrl = evbuffer_add_reference(evbuf, p, count, + [](const void *, size_t, void *p) { + delete[] static_cast(p); + }, p); + if (ctrl != 0) throw std::runtime_error("evbuffer_add_reference"); } }; From 873a60fcab5fdd12a07898be08a066a7a6a8f9e2 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Sun, 30 Aug 2015 21:14:49 +0200 Subject: [PATCH 05/13] iwyu and clang-format --- include/measurement_kit/net/buffer.hpp | 63 +++++++++++--------------- 1 file changed, 27 insertions(+), 36 deletions(-) diff --git a/include/measurement_kit/net/buffer.hpp b/include/measurement_kit/net/buffer.hpp index fa48d24d0..cf32dc7c2 100644 --- a/include/measurement_kit/net/buffer.hpp +++ b/include/measurement_kit/net/buffer.hpp @@ -7,19 +7,23 @@ #include #include -#include -#include #include - -#include +#include #include -#include -#include -#include +#include #include -#include +#include +#include +#include + +#include +#include +#include +#include + +struct evbuffer; namespace measurement_kit { namespace net { @@ -45,16 +49,14 @@ class Buffer : public NonCopyable, public NonMovable { */ Buffer &operator<<(evbuffer *source) { - if (source == nullptr) - throw std::runtime_error("source is nullptr"); + if (source == nullptr) throw std::runtime_error("source is nullptr"); if (evbuffer_add_buffer(evbuf, source) != 0) throw std::runtime_error("evbuffer_add_buffer failed"); return *this; } Buffer &operator>>(evbuffer *dest) { - if (dest == nullptr) - throw std::runtime_error("dest is nullptr"); + if (dest == nullptr) throw std::runtime_error("dest is nullptr"); if (evbuffer_add_buffer(dest, evbuf) != 0) throw std::runtime_error("evbuffer_add_buffer failed"); return *this; @@ -78,19 +80,14 @@ class Buffer : public NonCopyable, public NonMovable { */ void foreach (std::function fn) { auto required_size = evbuffer_peek(evbuf, -1, nullptr, nullptr, 0); - if (required_size < 0) - throw std::runtime_error("unexpected error"); - if (required_size == 0) - return; + if (required_size < 0) throw std::runtime_error("unexpected error"); + if (required_size == 0) return; std::unique_ptr raii; raii.reset(new evbuffer_iovec[required_size]); // Guarantee cleanup auto iov = raii.get(); auto used = evbuffer_peek(evbuf, -1, nullptr, iov, required_size); - if (used != required_size) - throw std::runtime_error("unexpected error"); - - for (auto i = 0; i < required_size && fn(&iov[i]); ++i) - /* nothing */; + if (used != required_size) throw std::runtime_error("unexpected error"); + for (auto i = 0; i < required_size && fn(&iov[i]); ++i) /* nothing */; } /* @@ -108,9 +105,8 @@ class Buffer : public NonCopyable, public NonMovable { size_t nbytes = 0; std::string out; - foreach([&](evbuffer_iovec *iov) { - if (upto < iov->iov_len) - iov->iov_len = upto; + foreach ([&](evbuffer_iovec *iov) { + if (upto < iov->iov_len) iov->iov_len = upto; out.append((const char *)iov->iov_base, iov->iov_len); @@ -123,8 +119,7 @@ class Buffer : public NonCopyable, public NonMovable { * We do this after foreach() because we are not supposed * to modify the underlying `evbuf` during foreach(). */ - if (!ispeek) - discard(nbytes); + if (!ispeek) discard(nbytes); return out; } @@ -152,8 +147,7 @@ class Buffer : public NonCopyable, public NonMovable { auto search_result = evbuffer_search_eol(evbuf, nullptr, &eol_length, EVBUFFER_EOL_CRLF); if (search_result.pos < 0) { - if (length() > maxline) - return std::make_tuple(-1, ""); + if (length() > maxline) return std::make_tuple(-1, ""); return std::make_tuple(0, ""); } @@ -164,8 +158,7 @@ class Buffer : public NonCopyable, public NonMovable { if (eol_length != 1 && eol_length != 2) throw std::runtime_error("unexpected error"); auto len = (size_t)search_result.pos + eol_length; - if (len > maxline) - return std::make_tuple(-2, ""); + if (len > maxline) return std::make_tuple(-2, ""); return std::make_tuple(0, read(len)); } @@ -183,8 +176,7 @@ class Buffer : public NonCopyable, public NonMovable { } void write(const char *in) { - if (in == nullptr) - throw std::runtime_error("in is nullptr"); + if (in == nullptr) throw std::runtime_error("in is nullptr"); write(in, strlen(in)); } @@ -194,8 +186,7 @@ class Buffer : public NonCopyable, public NonMovable { } void write(const void *buf, size_t count) { - if (buf == nullptr) - throw std::runtime_error("buf is nullptr"); + if (buf == nullptr) throw std::runtime_error("buf is nullptr"); if (evbuffer_add(evbuf, buf, count) != 0) throw std::runtime_error("evbuffer_add failed"); } @@ -210,8 +201,8 @@ class Buffer : public NonCopyable, public NonMovable { if (count == 0) return; char *p = new char[count]; evutil_secure_rng_get_bytes(p, count); - auto ctrl = evbuffer_add_reference(evbuf, p, count, - [](const void *, size_t, void *p) { + auto ctrl = evbuffer_add_reference( + evbuf, p, count, [](const void *, size_t, void *p) { delete[] static_cast(p); }, p); if (ctrl != 0) throw std::runtime_error("evbuffer_add_reference"); From 1fab917eacb714d0fe50545a8e7378c91ea09159 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Sun, 30 Aug 2015 23:31:59 +0200 Subject: [PATCH 06/13] Evbuffer is already non-copyable non-movable --- include/measurement_kit/net/buffer.hpp | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/include/measurement_kit/net/buffer.hpp b/include/measurement_kit/net/buffer.hpp index cf32dc7c2..7e506e397 100644 --- a/include/measurement_kit/net/buffer.hpp +++ b/include/measurement_kit/net/buffer.hpp @@ -5,7 +5,6 @@ #ifndef MEASUREMENT_KIT_NET_BUFFER_HPP #define MEASUREMENT_KIT_NET_BUFFER_HPP -#include #include #include @@ -28,11 +27,9 @@ struct evbuffer; namespace measurement_kit { namespace net { -using namespace measurement_kit::common; - -class Buffer : public NonCopyable, public NonMovable { +class Buffer { private: - Evbuffer evbuf; + common::Evbuffer evbuf; public: Buffer(evbuffer *b = nullptr) { From 0eda3b991a682609b46f431c319ee548beb4b187 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Mon, 31 Aug 2015 11:10:23 +0200 Subject: [PATCH 07/13] Teach readline to return well defined errors --- include/measurement_kit/net/buffer.hpp | 9 ++++++--- test/net/buffer.cpp | 2 +- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/include/measurement_kit/net/buffer.hpp b/include/measurement_kit/net/buffer.hpp index 7e506e397..59547b3d5 100644 --- a/include/measurement_kit/net/buffer.hpp +++ b/include/measurement_kit/net/buffer.hpp @@ -5,6 +5,7 @@ #ifndef MEASUREMENT_KIT_NET_BUFFER_HPP #define MEASUREMENT_KIT_NET_BUFFER_HPP +#include #include #include @@ -138,13 +139,14 @@ class Buffer { return read(n); } - std::tuple readline(size_t maxline) { + std::tuple readline(size_t maxline) { size_t eol_length = 0; auto search_result = evbuffer_search_eol(evbuf, nullptr, &eol_length, EVBUFFER_EOL_CRLF); if (search_result.pos < 0) { - if (length() > maxline) return std::make_tuple(-1, ""); + if (length() > maxline) + return std::make_tuple(common::EOLNotFoundError(), ""); return std::make_tuple(0, ""); } @@ -155,7 +157,8 @@ class Buffer { if (eol_length != 1 && eol_length != 2) throw std::runtime_error("unexpected error"); auto len = (size_t)search_result.pos + eol_length; - if (len > maxline) return std::make_tuple(-2, ""); + if (len > maxline) + return std::make_tuple(common::LineTooLongError(), ""); return std::make_tuple(0, read(len)); } diff --git a/test/net/buffer.cpp b/test/net/buffer.cpp index 7120d99a0..d598fa6c1 100644 --- a/test/net/buffer.cpp +++ b/test/net/buffer.cpp @@ -257,7 +257,7 @@ TEST_CASE("Readn works correctly") { TEST_CASE("Readline works correctly", "[Buffer]") { Buffer buff; auto s = std::string(); - int error = 0; + Error error; SECTION("We can read LF terminated lines") { buff << "HTTP/1.1 200 Ok\n" From 681dac6e61ef21e69f4a0b7626cea53504afa9bc Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Mon, 31 Aug 2015 11:14:04 +0200 Subject: [PATCH 08/13] Be explicit in lambda capture list --- include/measurement_kit/net/buffer.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/measurement_kit/net/buffer.hpp b/include/measurement_kit/net/buffer.hpp index 59547b3d5..904d96efe 100644 --- a/include/measurement_kit/net/buffer.hpp +++ b/include/measurement_kit/net/buffer.hpp @@ -103,7 +103,7 @@ class Buffer { size_t nbytes = 0; std::string out; - foreach ([&](evbuffer_iovec *iov) { + foreach ([&nbytes, &out, &upto](evbuffer_iovec *iov) { if (upto < iov->iov_len) iov->iov_len = upto; out.append((const char *)iov->iov_base, iov->iov_len); From f3be0835948ced346fda302efb9906fd5397f78d Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Mon, 31 Aug 2015 11:28:12 +0200 Subject: [PATCH 09/13] Add documentation --- doc/net/buffer.md | 99 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 99 insertions(+) create mode 100644 doc/net/buffer.md diff --git a/doc/net/buffer.md b/doc/net/buffer.md new file mode 100644 index 000000000..029778488 --- /dev/null +++ b/doc/net/buffer.md @@ -0,0 +1,99 @@ +# NAME +Buffer - Buffer containing data. + +# LIBRARY +MeasurementKit (libmeasurement-kit, -lmeasurement-kit). + +# SYNOPSIS + +```C++ +#include + +using namespace measurement_kit::common; +using namespace measurement_kit::net; + +// Constructor + +Buffer buf; + +evbuffer *evbuf = /* something */ ; +Buffer buf(evbuf); // Move content from (evbuffer *) evbuf + +// Move data from/to other buffers (this does not entail copies) + +buf << bufferevent_get_input(bev); // From bev to buf +buf >> bufferevent_get_output(bev); // The other way round + +Buffer another; +buf << another; // Move data from another to buf +buf >> another; // The other way round + +// Basic building blocks for high level functions + +size_t n = buf.length(); + +// Walk over data and decide when to stop by returning false +buf.foreach([](const char *p, size_t n) -> bool { + /* Do something with `p` and `n` */ + return true; +}); + +// Discard, read and peek the buffer + +buf.discard(128); +buf.discard(); + +std::string s = buf.read(128); +std::string s = buf.read(); +std::string s = buf.peek(128); +std::string s = buf.peek(); + +std::string s = buf.readn(1024); +if (s == "") { + /* less than 1024 bytes buffered */ + return; +} + +Error error; +std::string s; +std::tie res = buf.readline(1024); +if (error != 0) { + /* readline failed, check error */ + return; +} + +// Write stuff + +std::string s; +buf.write(s); +buff << s; + +const char *s = "HTTP/1.1 200 Ok\r\n"; +buf.write(s); +buf << s << "\r\n"; + +buf.write("ciao", 4); + +buf.write_uint8('c'); +buf.write_uint16(1284); +buf.write_uint32(7); + +buf.write_rand(2048); + +// Write directly into a 1024 chunk buffer of the output buffer chain +buf.write(1024, [](char *buf, size_t cnt) -> size_t { + /* Write into `buf` up to `cnt` chars */ + return cnt; +}); +``` + +# DESCRIPTION + +The `Buffer` object encapsulates an `evbuffer`. It is used to efficiently +store data. You can move data from a `Buffer` to another `Buffer` at virtually +not cost (the same is true when moving data from/to an `evbuffer` since +that is internally used to implement a `Buffer`). + +# HISTORY + +The `Buffer` class appeared in MeasurementKit 0.1. From 654e89d7bb3fd67e9721ded323b226eecbec07cf Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Mon, 31 Aug 2015 11:40:25 +0200 Subject: [PATCH 10/13] Make foreach() easier to use --- include/measurement_kit/net/buffer.hpp | 22 ++++++++++------------ src/http/http.cpp | 9 ++++----- test/net/buffer.cpp | 15 +++++++-------- 3 files changed, 21 insertions(+), 25 deletions(-) diff --git a/include/measurement_kit/net/buffer.hpp b/include/measurement_kit/net/buffer.hpp index 904d96efe..70d10178b 100644 --- a/include/measurement_kit/net/buffer.hpp +++ b/include/measurement_kit/net/buffer.hpp @@ -76,7 +76,7 @@ class Buffer { * The following is useful to feed a parser (e.g., the http-parser) * with all (or part of) the content of `this`. */ - void foreach (std::function fn) { + void foreach(std::function fn) { auto required_size = evbuffer_peek(evbuf, -1, nullptr, nullptr, 0); if (required_size < 0) throw std::runtime_error("unexpected error"); if (required_size == 0) return; @@ -85,7 +85,10 @@ class Buffer { auto iov = raii.get(); auto used = evbuffer_peek(evbuf, -1, nullptr, iov, required_size); if (used != required_size) throw std::runtime_error("unexpected error"); - for (auto i = 0; i < required_size && fn(&iov[i]); ++i) /* nothing */; + for (auto i = 0; i < required_size && + fn((const char *) iov[i].iov_base, iov[i].iov_len); ++i) { + /* nothing */ ; + } } /* @@ -102,23 +105,18 @@ class Buffer { std::string readpeek(bool ispeek, size_t upto) { size_t nbytes = 0; std::string out; - - foreach ([&nbytes, &out, &upto](evbuffer_iovec *iov) { - if (upto < iov->iov_len) iov->iov_len = upto; - - out.append((const char *)iov->iov_base, iov->iov_len); - - upto -= iov->iov_len; - nbytes += iov->iov_len; + foreach([&nbytes, &out, &upto](const char *p, size_t n) { + if (upto < n) n = upto; + out.append(p, n); + upto -= n; + nbytes += n; return (upto > 0); }); - /* * We do this after foreach() because we are not supposed * to modify the underlying `evbuf` during foreach(). */ if (!ispeek) discard(nbytes); - return out; } diff --git a/src/http/http.cpp b/src/http/http.cpp index f56d5d546..78c8a0cf0 100644 --- a/src/http/http.cpp +++ b/src/http/http.cpp @@ -138,18 +138,17 @@ class ResponseParserImpl { void parse(void) { auto total = (size_t) 0; - buffer.foreach([&](evbuffer_iovec *iov) { + buffer.foreach([&](const char *base, size_t count) { parsing = true; - size_t n = http_parser_execute(&parser, &settings, - (const char *) iov->iov_base, iov->iov_len); + size_t n = http_parser_execute(&parser, &settings, base, count); parsing = false; if (parser.upgrade) { throw UpgradeError("Unexpected UPGRADE"); } - if (n != iov->iov_len) { + if (n != count) { throw ParserError("Parser error"); } - total += iov->iov_len; + total += count; return true; }); if (closing) { diff --git a/test/net/buffer.cpp b/test/net/buffer.cpp index d598fa6c1..f0e04f4aa 100644 --- a/test/net/buffer.cpp +++ b/test/net/buffer.cpp @@ -82,7 +82,7 @@ TEST_CASE("Foreach is robust to corner cases and errors", "[Buffer]") { Buffer buff; SECTION("No function is invoked when the buffer is empty") { - buff.foreach([](evbuffer_iovec *) { + buff.foreach([](const char *, size_t) { throw std::runtime_error("should not happen"); return (false); }); @@ -126,8 +126,8 @@ TEST_CASE("Foreach works correctly", "[Buffer]") { SECTION("Make sure that we walk through all the extents") { buff << evbuf; - buff.foreach([&](evbuffer_iovec *iov) { - r.append((char *) iov->iov_base, iov->iov_len); + buff.foreach([&](const char *p, size_t n) { + r.append(p, n); ++counter; return (true); }); @@ -141,8 +141,8 @@ TEST_CASE("Foreach works correctly", "[Buffer]") { SECTION("Make sure that stopping early works as expected") { buff << evbuf; - buff.foreach([&](evbuffer_iovec *iov) { - r.append((char *) iov->iov_base, iov->iov_len); + buff.foreach([&](const char *p, size_t n) { + r.append(p, n); ++counter; return (false); }); @@ -375,9 +375,8 @@ TEST_CASE("Write works correctly", "[Buffer]") { REQUIRE(buff.length() == 1048576); auto zeroes = 0, total = 0; - buff.foreach([&](evbuffer_iovec *iov) { - auto p = (char *) iov->iov_base; - for (size_t i = 0; i < iov->iov_len; ++i) { + buff.foreach([&](const char *p, size_t n) { + for (size_t i = 0; i < n; ++i) { for (auto j = 0; j < 8; ++j) { if ((p[i] & (1 << j)) == 0) ++zeroes; From 675d1cb2684f9cde22f0697ed58b442fc9f3f5d1 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Mon, 31 Aug 2015 11:48:21 +0200 Subject: [PATCH 11/13] Implement write into --- include/measurement_kit/net/buffer.hpp | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/include/measurement_kit/net/buffer.hpp b/include/measurement_kit/net/buffer.hpp index 70d10178b..ac5779ff1 100644 --- a/include/measurement_kit/net/buffer.hpp +++ b/include/measurement_kit/net/buffer.hpp @@ -205,6 +205,23 @@ class Buffer { }, p); if (ctrl != 0) throw std::runtime_error("evbuffer_add_reference"); } + + void write(size_t count, std::function func) { + if (count == 0) return; + char *p = new char[count]; + size_t used = func(p, count); + if (used > count) throw std::runtime_error("internal error"); + if (used == 0) { + delete[] p; + return; + } + auto ctrl = evbuffer_add_reference( + evbuf, p, used, [](const void *, size_t, void *p) { + delete[] static_cast(p); + }, p); + if (ctrl != 0) throw std::runtime_error("evbuffer_add_reference"); + } + }; } // namespace net From 45650cff5e6bc143ceefa518553859cd1c0c901d Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Mon, 31 Aug 2015 11:58:50 +0200 Subject: [PATCH 12/13] More regress tests --- test/net/buffer.cpp | 44 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/test/net/buffer.cpp b/test/net/buffer.cpp index f0e04f4aa..0e73c9ae9 100644 --- a/test/net/buffer.cpp +++ b/test/net/buffer.cpp @@ -321,6 +321,19 @@ TEST_CASE("Readline works correctly", "[Buffer]") { REQUIRE(s == ""); } + SECTION("EOL-not-found error is correctly reported") { + buff << "HTTP/1.1 200 Ok"; + std::tie(error, s) = buff.readline(3); + REQUIRE(error == EOLNotFoundError()); + REQUIRE(s == ""); + } + + SECTION("Line-too-long error is correctly reported") { + buff << "HTTP/1.1 200 Ok\n"; + std::tie(error, s) = buff.readline(3); + REQUIRE(error == LineTooLongError()); + REQUIRE(s == ""); + } } TEST_CASE("Write works correctly", "[Buffer]") { @@ -395,3 +408,34 @@ TEST_CASE("Write works correctly", "[Buffer]") { REQUIRE(freq < 0.51); } } + +TEST_CASE("Write into works correctly", "[Buffer]") { + Buffer buff; + + SECTION("Typical usage") { + buff.write(1024, [](char *buf, size_t cnt) { + REQUIRE(buf != nullptr); + REQUIRE(cnt == 1024); + memset(buf, 0, cnt); + return cnt; + }); + } + + SECTION("Should throw if we use more than needed") { + REQUIRE_THROWS(buff.write(1024, [](char *buf, size_t cnt) { + REQUIRE(buf != nullptr); + REQUIRE(cnt == 1024); + memset(buf, 0, cnt); + return cnt + 1; + })); + } + + SECTION("Should work OK if we use zero bytes") { + buff.write(1024, [](char *buf, size_t cnt) { + REQUIRE(buf != nullptr); + REQUIRE(cnt == 1024); + return 0; + }); + } + +} From 0abfaede13282f161dea280e91ab4826524b59a4 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Mon, 31 Aug 2015 12:43:41 +0200 Subject: [PATCH 13/13] Fix three memory errors 1) Return to an API using (const void *, size_t) rather than using (const char *, size_t) for a rather stupid reason: it seems that Catch is looking inside C strings assuming they are zero terminated, which is not the case for buffer.write(count, [](const char *, size_t) {}) because of course in that case we pass around just allocated data but not initialized. Hence the memory errors. Passing `void *` fixes the issue because Catch does not of course look inside a `void *`. 2) Make sure we create `std::unique_ptr` otherwise `delete` (rather than `delete[]`) is called 3) Make sure we delete what was allocated before raising an exception in `buffer.write(n, [](const char *, size_t) {})` --- include/measurement_kit/net/buffer.hpp | 17 ++++++++++------- src/http/http.cpp | 5 +++-- test/net/buffer.cpp | 19 ++++++++++--------- 3 files changed, 23 insertions(+), 18 deletions(-) diff --git a/include/measurement_kit/net/buffer.hpp b/include/measurement_kit/net/buffer.hpp index ac5779ff1..42db853a3 100644 --- a/include/measurement_kit/net/buffer.hpp +++ b/include/measurement_kit/net/buffer.hpp @@ -76,17 +76,17 @@ class Buffer { * The following is useful to feed a parser (e.g., the http-parser) * with all (or part of) the content of `this`. */ - void foreach(std::function fn) { + void foreach(std::function fn) { auto required_size = evbuffer_peek(evbuf, -1, nullptr, nullptr, 0); if (required_size < 0) throw std::runtime_error("unexpected error"); if (required_size == 0) return; - std::unique_ptr raii; + std::unique_ptr raii; raii.reset(new evbuffer_iovec[required_size]); // Guarantee cleanup auto iov = raii.get(); auto used = evbuffer_peek(evbuf, -1, nullptr, iov, required_size); if (used != required_size) throw std::runtime_error("unexpected error"); for (auto i = 0; i < required_size && - fn((const char *) iov[i].iov_base, iov[i].iov_len); ++i) { + fn(iov[i].iov_base, iov[i].iov_len); ++i) { /* nothing */ ; } } @@ -105,9 +105,9 @@ class Buffer { std::string readpeek(bool ispeek, size_t upto) { size_t nbytes = 0; std::string out; - foreach([&nbytes, &out, &upto](const char *p, size_t n) { + foreach([&nbytes, &out, &upto](const void *p, size_t n) { if (upto < n) n = upto; - out.append(p, n); + out.append((const char *) p, n); upto -= n; nbytes += n; return (upto > 0); @@ -206,11 +206,14 @@ class Buffer { if (ctrl != 0) throw std::runtime_error("evbuffer_add_reference"); } - void write(size_t count, std::function func) { + void write(size_t count, std::function func) { if (count == 0) return; char *p = new char[count]; size_t used = func(p, count); - if (used > count) throw std::runtime_error("internal error"); + if (used > count) { + delete[] p; + throw std::runtime_error("internal error"); + } if (used == 0) { delete[] p; return; diff --git a/src/http/http.cpp b/src/http/http.cpp index 78c8a0cf0..1d6d1bb55 100644 --- a/src/http/http.cpp +++ b/src/http/http.cpp @@ -138,9 +138,10 @@ class ResponseParserImpl { void parse(void) { auto total = (size_t) 0; - buffer.foreach([&](const char *base, size_t count) { + buffer.foreach([&](const void *base, size_t count) { parsing = true; - size_t n = http_parser_execute(&parser, &settings, base, count); + size_t n = http_parser_execute(&parser, &settings, + (const char *) base, count); parsing = false; if (parser.upgrade) { throw UpgradeError("Unexpected UPGRADE"); diff --git a/test/net/buffer.cpp b/test/net/buffer.cpp index 0e73c9ae9..d5b6af367 100644 --- a/test/net/buffer.cpp +++ b/test/net/buffer.cpp @@ -82,7 +82,7 @@ TEST_CASE("Foreach is robust to corner cases and errors", "[Buffer]") { Buffer buff; SECTION("No function is invoked when the buffer is empty") { - buff.foreach([](const char *, size_t) { + buff.foreach([](const void *, size_t) { throw std::runtime_error("should not happen"); return (false); }); @@ -126,8 +126,8 @@ TEST_CASE("Foreach works correctly", "[Buffer]") { SECTION("Make sure that we walk through all the extents") { buff << evbuf; - buff.foreach([&](const char *p, size_t n) { - r.append(p, n); + buff.foreach([&](const void *p, size_t n) { + r.append((const char *) p, n); ++counter; return (true); }); @@ -141,8 +141,8 @@ TEST_CASE("Foreach works correctly", "[Buffer]") { SECTION("Make sure that stopping early works as expected") { buff << evbuf; - buff.foreach([&](const char *p, size_t n) { - r.append(p, n); + buff.foreach([&](const void *p, size_t n) { + r.append((const char *) p, n); ++counter; return (false); }); @@ -388,7 +388,8 @@ TEST_CASE("Write works correctly", "[Buffer]") { REQUIRE(buff.length() == 1048576); auto zeroes = 0, total = 0; - buff.foreach([&](const char *p, size_t n) { + buff.foreach([&](const void *pp, size_t n) { + const char *p = (const char *) pp; for (size_t i = 0; i < n; ++i) { for (auto j = 0; j < 8; ++j) { if ((p[i] & (1 << j)) == 0) @@ -413,7 +414,7 @@ TEST_CASE("Write into works correctly", "[Buffer]") { Buffer buff; SECTION("Typical usage") { - buff.write(1024, [](char *buf, size_t cnt) { + buff.write(1024, [](void *buf, size_t cnt) { REQUIRE(buf != nullptr); REQUIRE(cnt == 1024); memset(buf, 0, cnt); @@ -422,7 +423,7 @@ TEST_CASE("Write into works correctly", "[Buffer]") { } SECTION("Should throw if we use more than needed") { - REQUIRE_THROWS(buff.write(1024, [](char *buf, size_t cnt) { + REQUIRE_THROWS(buff.write(1024, [](void *buf, size_t cnt) { REQUIRE(buf != nullptr); REQUIRE(cnt == 1024); memset(buf, 0, cnt); @@ -431,7 +432,7 @@ TEST_CASE("Write into works correctly", "[Buffer]") { } SECTION("Should work OK if we use zero bytes") { - buff.write(1024, [](char *buf, size_t cnt) { + buff.write(1024, [](void *buf, size_t cnt) { REQUIRE(buf != nullptr); REQUIRE(cnt == 1024); return 0;