From d42f31ca788484a9e9d73db657ebd026a49348de Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Mon, 6 Apr 2015 22:31:36 +0900 Subject: [PATCH] nghttpx: Fix bug that data buffered in SSL object are not read This is same issue described in https://github.com/h2o/h2o/issues/268. That is if SSL object has decrypted data buffered inside it, and application does not read it for some reason (e.g., rate limit), we have to check the existence of data using SSL_pending. This is because buffered data inside SSL is not notified by io watcher. It is obvious, but we totally missed it. nghttpx code normally reads everything until SSL_read returns error (want-read). But if rate limit is involved, we stop reading early. Also in HTTP/1 code, while processing one request, we just read until buffer is filled up. In these cases, we may suffer from this problem. This commit fixes this problem, by performing SSL_pending() and if it has buffered data and read io watcher is enabled, we feed event using ev_feed_event(). --- src/shrpx_client_handler.cc | 9 ++++++++- src/shrpx_connection.cc | 9 ++++++++- src/shrpx_connection.h | 2 ++ src/shrpx_rate_limit.cc | 21 ++++++++++++++++----- src/shrpx_rate_limit.h | 14 ++++++++++++-- 5 files changed, 46 insertions(+), 9 deletions(-) diff --git a/src/shrpx_client_handler.cc b/src/shrpx_client_handler.cc index c8065bb4..b397adf3 100644 --- a/src/shrpx_client_handler.cc +++ b/src/shrpx_client_handler.cc @@ -560,7 +560,14 @@ int ClientHandler::validate_next_proto() { int ClientHandler::do_read() { return read_(*this); } int ClientHandler::do_write() { return write_(*this); } -int ClientHandler::on_read() { return on_read_(*this); } +int ClientHandler::on_read() { + auto rv = on_read_(*this); + if (rv != 0) { + return rv; + } + conn_.handle_tls_pending_read(); + return 0; +} int ClientHandler::on_write() { return on_write_(*this); } const std::string &ClientHandler::get_ipaddr() const { return ipaddr_; } diff --git a/src/shrpx_connection.cc b/src/shrpx_connection.cc index 5edc480b..51a5d39c 100644 --- a/src/shrpx_connection.cc +++ b/src/shrpx_connection.cc @@ -41,7 +41,7 @@ Connection::Connection(struct ev_loop *loop, int fd, SSL *ssl, size_t read_burst, IOCb writecb, IOCb readcb, TimerCb timeoutcb, void *data) : tls{ssl}, wlimit(loop, &wev, write_rate, write_burst), - rlimit(loop, &rev, read_rate, read_burst), writecb(writecb), + rlimit(loop, &rev, read_rate, read_burst, ssl), writecb(writecb), readcb(readcb), timeoutcb(timeoutcb), loop(loop), data(data), fd(fd) { ev_io_init(&wev, writecb, fd, EV_WRITE); @@ -303,4 +303,11 @@ ssize_t Connection::read_clear(void *data, size_t len) { return nread; } +void Connection::handle_tls_pending_read() { + if (!ev_is_active(&rev)) { + return; + } + rlimit.handle_tls_pending_read(); +} + } // namespace shrpx diff --git a/src/shrpx_connection.h b/src/shrpx_connection.h index fc6a14a3..4e348412 100644 --- a/src/shrpx_connection.h +++ b/src/shrpx_connection.h @@ -83,6 +83,8 @@ struct Connection { ssize_t writev_clear(struct iovec *iov, int iovcnt); ssize_t read_clear(void *data, size_t len); + void handle_tls_pending_read(); + TLSConnection tls; ev_io wev; ev_io rev; diff --git a/src/shrpx_rate_limit.cc b/src/shrpx_rate_limit.cc index 3f2ec95b..85836c14 100644 --- a/src/shrpx_rate_limit.cc +++ b/src/shrpx_rate_limit.cc @@ -35,8 +35,9 @@ void regencb(struct ev_loop *loop, ev_timer *w, int revents) { } } // namespace -RateLimit::RateLimit(struct ev_loop *loop, ev_io *w, size_t rate, size_t burst) - : w_(w), loop_(loop), rate_(rate), burst_(burst), avail_(burst), +RateLimit::RateLimit(struct ev_loop *loop, ev_io *w, size_t rate, size_t burst, + SSL *ssl) + : w_(w), loop_(loop), ssl_(ssl), rate_(rate), burst_(burst), avail_(burst), startw_req_(false) { ev_timer_init(&t_, regencb, 0., 1.); t_.data = this; @@ -45,9 +46,7 @@ RateLimit::RateLimit(struct ev_loop *loop, ev_io *w, size_t rate, size_t burst) } } -RateLimit::~RateLimit() { - ev_timer_stop(loop_, &t_); -} +RateLimit::~RateLimit() { ev_timer_stop(loop_, &t_); } size_t RateLimit::avail() const { if (rate_ == 0) { @@ -79,6 +78,7 @@ void RateLimit::regen() { if (avail_ > 0 && startw_req_) { ev_io_start(loop_, w_); + handle_tls_pending_read(); } } @@ -86,6 +86,7 @@ void RateLimit::startw() { startw_req_ = true; if (rate_ == 0 || avail_ > 0) { ev_io_start(loop_, w_); + handle_tls_pending_read(); return; } } @@ -95,4 +96,14 @@ void RateLimit::stopw() { ev_io_stop(loop_, w_); } +void RateLimit::handle_tls_pending_read() { + if (!ssl_ || SSL_pending(ssl_) == 0) { + return; + } + + // Note that ev_feed_event works without starting watcher, but we + // only call this function if watcher is active. + ev_feed_event(loop_, w_, EV_READ); +} + } // namespace shrpx diff --git a/src/shrpx_rate_limit.h b/src/shrpx_rate_limit.h index db51df40..4550d012 100644 --- a/src/shrpx_rate_limit.h +++ b/src/shrpx_rate_limit.h @@ -29,21 +29,31 @@ #include +#include + namespace shrpx { class RateLimit { public: - RateLimit(struct ev_loop *loop, ev_io *w, size_t rate, size_t burst); + // We need |ssl| object to check that it has unread decrypted bytes. + RateLimit(struct ev_loop *loop, ev_io *w, size_t rate, size_t burst, + SSL *ssl = nullptr); ~RateLimit(); size_t avail() const; void drain(size_t n); void regen(); void startw(); void stopw(); + // Feeds event if ssl_ object has unread decrypted bytes. This is + // required since it is buffered in ssl_ object, io event is not + // generated unless new incoming data is received. + void handle_tls_pending_read(); + private: - ev_io *w_; ev_timer t_; + ev_io *w_; struct ev_loop *loop_; + SSL *ssl_; size_t rate_; size_t burst_; size_t avail_;