loolwsd/IoUtil.cpp | 42 +++++++++++------------------------------- loolwsd/LOOLWebSocket.hpp | 46 +++++++++++++++++++++++++++++++++------------- loolwsd/test/helpers.hpp | 28 +++++----------------------- 3 files changed, 49 insertions(+), 67 deletions(-)
New commits: commit 84607b43a31574533471defcb4756ba855f835f1 Author: Ashod Nakashian <[email protected]> Date: Thu Nov 24 18:26:07 2016 -0500 loolwsd: support reading long messages directly Since Poco receiveFrame expected the buffer to be at least as large as the frame, otherwise the socket had to be closed, we sent a 'nextmessage' frame before sending large frames with the payload size. This caused many problems, not least related to threading and lack of atomicity when sending large frames. There is another API in Poco that doesn't have this strict requirement, one that expects Poco::Buffer and resizes it as necessary. One potential issue is the possibility that a malicious attacker might send very large frames to force the server into allocating large buffers to read them, thereby destibilizing the server, if not bringing it down altogether. Change-Id: I05014d54c3a1464f629ed82d761a7a65e4941985 Reviewed-on: https://gerrit.libreoffice.org/31184 Reviewed-by: Ashod Nakashian <[email protected]> Tested-by: Ashod Nakashian <[email protected]> diff --git a/loolwsd/IoUtil.cpp b/loolwsd/IoUtil.cpp index 035dcf8..adf5c55 100644 --- a/loolwsd/IoUtil.cpp +++ b/loolwsd/IoUtil.cpp @@ -50,17 +50,17 @@ void SocketProcessor(const std::shared_ptr<LOOLWebSocket>& ws, // Timeout given is in microseconds. static const Poco::Timespan waitTime(POLL_TIMEOUT_MS * 1000); - const auto bufferSize = READ_BUFFER_SIZE * 100; + constexpr auto bufferSize = READ_BUFFER_SIZE * 8; + int flags = 0; int n = -1; bool stop = false; std::vector<char> payload(bufferSize); + Poco::Buffer<char> buffer(bufferSize); try { ws->setReceiveTimeout(0); - payload.resize(0); - for (;;) { stop = stopPredicate(); @@ -79,10 +79,12 @@ void SocketProcessor(const std::shared_ptr<LOOLWebSocket>& ws, try { - payload.resize(payload.capacity()); + payload.resize(0); + buffer.resize(0); n = -1; - n = ws->receiveFrame(payload.data(), payload.capacity(), flags); - payload.resize(n > 0 ? n : 0); + n = ws->receiveFrame(buffer, flags); + LOG_WRN("GOT: [" << LOOLProtocol::getAbbreviatedMessage(buffer.begin(), buffer.size()) << "]"); + payload.insert(payload.end(), buffer.begin(), buffer.end()); } catch (const Poco::TimeoutException&) { @@ -99,7 +101,7 @@ void SocketProcessor(const std::shared_ptr<LOOLWebSocket>& ws, assert(n > 0); - const std::string firstLine = LOOLProtocol::getFirstLine(payload); + const std::string firstLine = LOOLProtocol::getFirstLine(buffer.begin(), buffer.size()); if ((flags & WebSocket::FrameFlags::FRAME_FLAG_FIN) != WebSocket::FrameFlags::FRAME_FLAG_FIN) { // One WS message split into multiple frames. @@ -107,8 +109,7 @@ void SocketProcessor(const std::shared_ptr<LOOLWebSocket>& ws, LOG_WRN("SocketProcessor [" << name << "]: Receiving multi-parm frame."); while (true) { - char buffer[READ_BUFFER_SIZE * 10]; - n = ws->receiveFrame(buffer, sizeof(buffer), flags); + n = ws->receiveFrame(buffer, flags); if (n <= 0 || (flags & WebSocket::FRAME_OP_BITMASK) == WebSocket::FRAME_OP_CLOSE) { LOG_WRN("SocketProcessor [" << name << "]: Connection closed while reading multiframe message."); @@ -116,7 +117,7 @@ void SocketProcessor(const std::shared_ptr<LOOLWebSocket>& ws, break; } - payload.insert(payload.end(), buffer, buffer + n); + payload.insert(payload.end(), buffer.begin(), buffer.end()); if ((flags & WebSocket::FrameFlags::FRAME_FLAG_FIN) == WebSocket::FrameFlags::FRAME_FLAG_FIN) { // No more frames. @@ -124,27 +125,6 @@ void SocketProcessor(const std::shared_ptr<LOOLWebSocket>& ws, } } } - else - { - int size = 0; - Poco::StringTokenizer tokens(firstLine, " ", Poco::StringTokenizer::TOK_IGNORE_EMPTY | Poco::StringTokenizer::TOK_TRIM); - // Check if it is a "nextmessage:" and in that case read the large - // follow-up message separately, and handle that only. - if (tokens.count() == 2 && tokens[0] == "nextmessage:" && - LOOLProtocol::getTokenInteger(tokens[1], "size", size) && size > 0) - { - LOG_TRC("SocketProcessor [" << name << "]: Getting large message of " << size << " bytes."); - if (size > MAX_MESSAGE_SIZE) - { - LOG_ERR("SocketProcessor [" << name << "]: Large-message size (" << size << ") over limit or invalid."); - } - else - { - payload.resize(size); - continue; - } - } - } if (n <= 0 || (flags & WebSocket::FRAME_OP_BITMASK) == WebSocket::FRAME_OP_CLOSE) { diff --git a/loolwsd/LOOLWebSocket.hpp b/loolwsd/LOOLWebSocket.hpp index e10af2a..fb1d88b 100644 --- a/loolwsd/LOOLWebSocket.hpp +++ b/loolwsd/LOOLWebSocket.hpp @@ -108,6 +108,39 @@ public: return -1; } + + /// Wrapper for Poco::Net::WebSocket::receiveFrame() that handles PING frames + /// (by replying with a PONG frame) and PONG frames. PONG frames are ignored. + /// Should we also factor out the handling of non-final and continuation frames into this? + int receiveFrame(Poco::Buffer<char>& buffer, int& flags) + { +#ifdef ENABLE_DEBUG + // Delay receiving the frame + std::this_thread::sleep_for(getWebSocketDelay()); +#endif + // Timeout given is in microseconds. + static const Poco::Timespan waitTime(POLL_TIMEOUT_MS * 1000); + + while (poll(waitTime, Poco::Net::Socket::SELECT_READ)) + { + const int n = Poco::Net::WebSocket::receiveFrame(buffer, flags); + if (n > 0 && (flags & WebSocket::FRAME_OP_BITMASK) == WebSocket::FRAME_OP_PING) + { + sendFrame(buffer.begin(), n, WebSocket::FRAME_FLAG_FIN | WebSocket::FRAME_OP_PONG); + } + else if ((flags & WebSocket::FRAME_OP_BITMASK) == WebSocket::FRAME_OP_PONG) + { + // In case we do send pongs in the future. + } + else + { + return n; + } + } + + return -1; + } + /// Wrapper for Poco::Net::WebSocket::sendFrame() that handles large frames. int sendFrame(const char* buffer, const int length, const int flags = FRAME_TEXT) { @@ -117,19 +150,6 @@ public: #endif std::unique_lock<std::mutex> lock(_mutex); - // Size after which messages will be sent preceded with - // 'nextmessage' frame to let the receiver know in advance - // the size of larger coming message. All messages up to this - // size are considered small messages. - constexpr int SMALL_MESSAGE_SIZE = READ_BUFFER_SIZE / 2; - - if (length > SMALL_MESSAGE_SIZE) - { - const std::string nextmessage = "nextmessage: size=" + std::to_string(length); - Poco::Net::WebSocket::sendFrame(nextmessage.data(), nextmessage.size()); - Log::debug("Message is long, sent " + nextmessage); - } - const int result = Poco::Net::WebSocket::sendFrame(buffer, length, flags); lock.unlock(); diff --git a/loolwsd/test/helpers.hpp b/loolwsd/test/helpers.hpp index 30aeac1..7e5f039 100644 --- a/loolwsd/test/helpers.hpp +++ b/loolwsd/test/helpers.hpp @@ -197,6 +197,7 @@ std::vector<char> getResponseMessage(LOOLWebSocket& ws, const std::string& prefi int retries = timeoutMs / 500; const Poco::Timespan waitTime(retries ? timeoutMs * 1000 / retries : timeoutMs * 1000); std::vector<char> response; + Poco::Buffer<char> buffer(READ_BUFFER_SIZE); bool timedout = false; ws.setReceiveTimeout(0); @@ -210,9 +211,10 @@ std::vector<char> getResponseMessage(LOOLWebSocket& ws, const std::string& prefi timedout = false; } - response.resize(READ_BUFFER_SIZE); - int bytes = ws.receiveFrame(response.data(), response.size(), flags); - response.resize(bytes >= 0 ? bytes : 0); + response.resize(0); + buffer.resize(0); + const int bytes = ws.receiveFrame(buffer, flags); + response.insert(response.end(), buffer.begin(), buffer.end()); std::cerr << name << "Got " << LOOLProtocol::getAbbreviatedFrameDump(response.data(), bytes, flags) << std::endl; const auto message = LOOLProtocol::getFirstLine(response); if (bytes > 0 && (flags & Poco::Net::WebSocket::FRAME_OP_BITMASK) != Poco::Net::WebSocket::FRAME_OP_CLOSE) @@ -221,26 +223,6 @@ std::vector<char> getResponseMessage(LOOLWebSocket& ws, const std::string& prefi { return response; } - else if (LOOLProtocol::matchPrefix("nextmessage", message)) - { - int size = 0; - if (LOOLProtocol::getTokenIntegerFromMessage(message, "size", size) && size > 0) - { - response.resize(size); - bytes = ws.receiveFrame(response.data(), response.size(), flags); - response.resize(bytes >= 0 ? bytes : 0); - std::cerr << name << "Got " << LOOLProtocol::getAbbreviatedFrameDump(response.data(), bytes, flags) << std::endl; - if (bytes > 0 && - LOOLProtocol::matchPrefix(prefix, LOOLProtocol::getFirstLine(response))) - { - return response; - } - } - } - } - else - { - response.resize(0); } if (bytes <= 0) _______________________________________________ Libreoffice-commits mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/libreoffice-commits
