From 80ce6327bb1e16472bf461f1561342f5cb454b2a Mon Sep 17 00:00:00 2001 From: Emiel Bruijntjes Date: Fri, 15 Jun 2018 09:19:27 +0200 Subject: [PATCH] fixed ssl handling: the sslconnected class incorrectly cached the readability/writability state of a socket, even after it already did a read- or write-operation, which could lead to the system getting to read mode when no data was expected --- src/linux_tcp/poll.h | 127 +++++++++++++++++++++++++++++++++++ src/linux_tcp/sslconnected.h | 54 ++++++++++----- src/linux_tcp/sslhandshake.h | 8 +-- src/linux_tcp/sslshutdown.h | 6 +- src/linux_tcp/tcpconnected.h | 6 +- src/linux_tcp/wait.h | 97 -------------------------- 6 files changed, 176 insertions(+), 122 deletions(-) create mode 100644 src/linux_tcp/poll.h delete mode 100644 src/linux_tcp/wait.h diff --git a/src/linux_tcp/poll.h b/src/linux_tcp/poll.h new file mode 100644 index 0000000..72f04b2 --- /dev/null +++ b/src/linux_tcp/poll.h @@ -0,0 +1,127 @@ +/** + * Poll.h + * + * Class to check or wait for a socket to become readable and/or writable + * + * @copyright 2018 Copernica BV + */ + + /** + * Include guard + */ +#pragma once + +/** + * Begin of namespace + */ +namespace AMQP { + +/** + * Class definition + */ +class Poll +{ +private: + /** + * Set with just one filedescriptor + * @var fd_set + */ + fd_set _set; + + /** + * The current socket // @todo what is it exactly? + * @var int + */ + int _socket; + +public: + /** + * Constructor + * @param fd the filedescriptor that we're waiting on + */ + Poll(int fd) : _socket(fd) + { + // initialize the set + FD_ZERO(&_set); + + // add the one socket + FD_SET(_socket, &_set); + } + + /** + * No copying + * @param that + */ + Poll(const Poll &that) = delete; + + /** + * Destructor + */ + virtual ~Poll() = default; + + /** + * Wait until the filedescriptor becomes readable + * @param block block until readable + * @return bool + */ + bool readable(bool block) + { + // wait for the socket + if (block) return select(_socket + 1, &_set, nullptr, nullptr, nullptr) > 0; + + // we do not want to block, so we use a small timeout + struct timeval timeout; + + // no timeout at all + timeout.tv_sec = timeout.tv_usec = 0; + + // no timeout at all + return select(_socket + 1, &_set, nullptr, nullptr, &timeout) > 0; + } + + /** + * Wait until the filedescriptor becomes writable + * @param block block until readable + * @return bool + */ + bool writable(bool block) + { + // wait for the socket + if (block) return select(_socket + 1, nullptr, &_set, nullptr, nullptr) > 0; + + // we do not want to block, so we use a small timeout + struct timeval timeout; + + // no timeout at all + timeout.tv_sec = timeout.tv_usec = 0; + + // no timeout at all + return select(_socket + 1, nullptr, &_set, nullptr, &timeout) > 0; + } + + /** + * Wait until a filedescriptor becomes active (readable or writable) + * @param block block until readable + * @return bool + */ + bool active(bool block) + { + // wait for the socket + if (block) return select(_socket + 1, &_set, &_set, nullptr, nullptr) > 0; + + // we do not want to block, so we use a small timeout + struct timeval timeout; + + // no timeout at all + timeout.tv_sec = timeout.tv_usec = 0; + + // no timeout at all + return select(_socket + 1, &_set, &_set, nullptr, &timeout) > 0; + } +}; + +/** + * End of namespace + */ +} + diff --git a/src/linux_tcp/sslconnected.h b/src/linux_tcp/sslconnected.h index 07d575f..d8a4c5f 100644 --- a/src/linux_tcp/sslconnected.h +++ b/src/linux_tcp/sslconnected.h @@ -16,7 +16,7 @@ */ #include "tcpoutbuffer.h" #include "tcpinbuffer.h" -#include "wait.h" +#include "poll.h" #include "sslwrapper.h" #include "sslshutdown.h" @@ -248,13 +248,38 @@ private: return this; } + /** + * Check if the socket is readable + * @return bool + */ + bool isReadable() const + { + // object to poll a socket + Poll poll(_socket); + + // wait until socket is readable, but do not block + return poll.readable(false); + } + + /** + * Check if the socket is writable + * @return bool + */ + bool isWritable() const + { + // object to poll a socket + Poll poll(_socket); + + // wait until socket is writable, but do not block + return poll.writable(false); + } + /** * Perform a write operation * @param monitor object to check the existance of the connection object - * @param readable is the connection also readable and should we call a read operation afterwards? * @return TcpState* */ - TcpState *write(const Monitor &monitor, bool readable) + TcpState *write(const Monitor &monitor) { // assume default state _state = state_idle; @@ -280,19 +305,18 @@ private: // the operation failed, we may have to repeat our call return repeat(monitor, state_sending, error); } - while (_out && !readable); + while (_out && !isReadable()); // proceed with the read operation or the event loop - return readable ? receive(monitor, false) : proceed(); + return isReadable() ? receive(monitor) : proceed(); } /** * Perform a receive operation * @param monitor object to check the existance of the connection object - * @param writable is the socket writable, and should we start a write operation after this operation? * @return TcpState */ - TcpState *receive(const Monitor &monitor, bool writable) + TcpState *receive(const Monitor &monitor) { // we are going to check for errors after the openssl operations, so we make // sure that the error queue is currently completely empty @@ -319,7 +343,7 @@ private: while (OpenSSL::SSL_pending(_ssl) > 0); // proceed with the write operation or the event loop - return writable && _out ? write(monitor, false) : proceed(); + return _out && isWritable() ? write(monitor) : proceed(); } @@ -378,16 +402,16 @@ public: if (fd != _socket) return this; // if we were busy with a write operation, we have to repeat that - if (_state == state_sending) return write(monitor, flags & readable); + if (_state == state_sending) return write(monitor); // same is true for read operations, they should also be repeated - if (_state == state_receiving) return receive(monitor, flags & writable); + if (_state == state_receiving) return receive(monitor); // if the socket is readable, we are going to receive data - if (flags & readable) return receive(monitor, flags & writable); + if (flags & readable) return receive(monitor); // socket is not readable (so it must be writable), do we have data to write? - if (_out) return write(monitor, false); + if (_out) return write(monitor); // the only scenario in which we can end up here is the socket should be // closed, but instead of moving to the shutdown-state right, we call proceed() @@ -406,7 +430,7 @@ public: if (_state == state_receiving) return this; // create an object to wait for the filedescriptor to becomes active - Wait wait(_socket); + Poll poll(_socket); // we are going to check for errors after the openssl operations, so we make // sure that the error queue is currently completely empty @@ -443,8 +467,8 @@ public: // check the type of error, and wait now switch (error) { - case SSL_ERROR_WANT_READ: wait.readable(); break; - case SSL_ERROR_WANT_WRITE: wait.active(); break; + case SSL_ERROR_WANT_READ: poll.readable(true); break; + case SSL_ERROR_WANT_WRITE: poll.active(true); break; } } } diff --git a/src/linux_tcp/sslhandshake.h b/src/linux_tcp/sslhandshake.h index bb5cf30..06178c6 100644 --- a/src/linux_tcp/sslhandshake.h +++ b/src/linux_tcp/sslhandshake.h @@ -17,7 +17,7 @@ */ #include "tcpoutbuffer.h" #include "sslconnected.h" -#include "wait.h" +#include "poll.h" #include "sslwrapper.h" #include "sslcontext.h" @@ -230,7 +230,7 @@ public: virtual TcpState *flush(const Monitor &monitor) override { // create an object to wait for the filedescriptor to becomes active - Wait wait(_socket); + Poll poll(_socket); // keep looping while (true) @@ -249,8 +249,8 @@ public: // if openssl reports that socket readability or writability is needed, // we wait for that until this situation is reached - case SSL_ERROR_WANT_READ: wait.readable(); break; - case SSL_ERROR_WANT_WRITE: wait.active(); break; + case SSL_ERROR_WANT_READ: poll.readable(true); break; + case SSL_ERROR_WANT_WRITE: poll.active(true); break; // something is wrong, we proceed to the next state default: return reportError(monitor); diff --git a/src/linux_tcp/sslshutdown.h b/src/linux_tcp/sslshutdown.h index bda7b70..f3a57c9 100644 --- a/src/linux_tcp/sslshutdown.h +++ b/src/linux_tcp/sslshutdown.h @@ -212,7 +212,7 @@ public: virtual TcpState *flush(const Monitor &monitor) override { // create an object to wait for the filedescriptor to becomes active - Wait wait(_socket); + Poll poll(_socket); // keep looping while (true) @@ -234,8 +234,8 @@ public: // if openssl reports that socket readability or writability is needed, // we wait for that until this situation is reached - case SSL_ERROR_WANT_READ: wait.readable(); break; - case SSL_ERROR_WANT_WRITE: wait.active(); break; + case SSL_ERROR_WANT_READ: poll.readable(true); break; + case SSL_ERROR_WANT_WRITE: poll.active(true); break; // something is wrong, we proceed to the next state default: return reporterror(monitor); diff --git a/src/linux_tcp/tcpconnected.h b/src/linux_tcp/tcpconnected.h index 34b6c47..8273a85 100644 --- a/src/linux_tcp/tcpconnected.h +++ b/src/linux_tcp/tcpconnected.h @@ -18,7 +18,7 @@ */ #include "tcpoutbuffer.h" #include "tcpinbuffer.h" -#include "wait.h" +#include "poll.h" /** * Set up namespace @@ -260,13 +260,13 @@ public: virtual TcpState *flush(const Monitor &monitor) override { // create an object to wait for the filedescriptor to becomes active - Wait wait(_socket); + Poll poll(_socket); // keep running until the out buffer is not empty while (_out) { // poll the socket, is it already writable? - if (!wait.writable()) return this; + if (!poll.writable(true)) return this; // socket is writable, send as much data as possible auto *newstate = process(monitor, _socket, writable); diff --git a/src/linux_tcp/wait.h b/src/linux_tcp/wait.h deleted file mode 100644 index 5db5242..0000000 --- a/src/linux_tcp/wait.h +++ /dev/null @@ -1,97 +0,0 @@ -/** - * Wait.h - * - * Class to wait for a socket to become readable and/or writable - * - * @copyright 2018 Copernica BV - */ - - /** - * Include guard - */ -#pragma once - -/** - * Begin of namespace - */ -namespace AMQP { - -/** - * Class definition - */ -class Wait -{ -private: - /** - * Set with just one filedescriptor - * @var fd_set - */ - fd_set _set; - - /** - * The current socket // @todo what is it exactly? - * @var int - */ - int _socket; - -public: - /** - * Constructor - * @param fd the filedescriptor that we're waiting on - */ - Wait(int fd) : _socket(fd) - { - // initialize the set - FD_ZERO(&_set); - - // add the one socket - FD_SET(_socket, &_set); - } - - /** - * No copying - * @param that - */ - Wait(const Wait &that) = delete; - - /** - * Destructor - */ - virtual ~Wait() = default; - - /** - * Wait until the filedescriptor becomes readable - * @return bool - */ - bool readable() - { - // wait for the socket - return select(_socket + 1, &_set, nullptr, nullptr, nullptr) > 0; - } - - /** - * Wait until the filedescriptor becomes writable - * @return bool - */ - bool writable() - { - // wait for the socket - return select(_socket + 1, nullptr, &_set, nullptr, nullptr) > 0; - } - - /** - * Wait until a filedescriptor becomes active (readable or writable) - * @return bool - */ - bool active() - { - // wait for the socket - return select(_socket + 1, &_set, &_set, nullptr, nullptr) > 0; - } -}; - -/** - * End of namespace - */ -} -