fix: segfault when calling read on an invalid fd

include/dpp/cluster.h

@@ -166,14 +166,6 @@ class DPP_EXPORT cluster {
 	 */
 	std::map<std::string,slashcommand_handler_t> named_commands;
 #endif
-
-	/**
-	 * @brief Reschedule a timer for its next tick
-	 * 
-	 * @param t Timer to reschedule
-	 */
-	void timer_reschedule(timer_t& t);
-
 	/**
 	 * @brief Thread pool
 	 */

include/dpp/discordclient.h

@@ -37,12 +37,19 @@
 #include <mutex>
 #include <shared_mutex>
 
+/**
+ * @brief Discord API version for shard websockets and HTTPS API requests
+ */
 #define DISCORD_API_VERSION	"10"
+
+/**
+ * @brief HTTPS Request base path for API calls
+ */
 #define API_PATH	        "/api/v" DISCORD_API_VERSION
 
 namespace dpp {
 
-// Forward declarations
+/* Forward declarations */
 class cluster;
 
 /**
@@ -52,8 +59,15 @@ class cluster;
  */
 class zlibcontext;
 
+/**
+ * @brief Size of decompression buffer for zlib compressed traffic
+ */
+constexpr size_t DECOMP_BUFFER_SIZE = 512 * 1024;
+
 /**
  * @brief Represents different event opcodes sent and received on a shard websocket
+ *
+ * These are used internally to route frames.
  */
 enum shard_frame_type : int {
 
@@ -261,10 +275,19 @@ class DPP_EXPORT discord_client : public websocket_client
 	void start_connecting();
 
 	/**
-	 * @brief Timer for reconnecting
+	 * @brief Timer for use when reconnecting.
+	 *
+	 * The client will wait 5 seconds before retrying a connection, to comply
+	 * with Discord rate limiting for websocket connections.
 	 */
 	timer reconnect_timer{0};
 
+	/**
+	 * @brief Stores the most recent ping message on this shard, which we check
+	 * for to monitor latency
+	 */
+	std::string last_ping_message;
+
 private:
 
 	/**
@@ -289,6 +312,10 @@ class DPP_EXPORT discord_client : public websocket_client
 
 	/**
 	 * @brief ZLib decompression buffer
+	 *
+	 * If compression is not in use, this remains set to
+	 * a vector of length zero, but when compression is
+	 * enabled it will be resized to a DECOMP_BUFFER_SIZE buffer.
 	 */
 	std::vector<unsigned char*> decomp_buffer;
 

src/dpp/cluster/timer.cpp

@@ -21,29 +21,30 @@
 #include <dpp/timer.h>
 #include <dpp/cluster.h>
 #include <dpp/json.h>
+#include <atomic>
 
 namespace dpp {
 
-timer lasthandle = 1;
+std::atomic<timer> next_handle = 1;
 
 timer cluster::start_timer(timer_callback_t on_tick, uint64_t frequency, timer_callback_t on_stop) {
-	std::lock_guard<std::mutex> l(timer_guard);
-	timer_t newtimer;
+	timer_t new_timer;
 
-	newtimer.handle = lasthandle++;
-	newtimer.next_tick = time(nullptr) + frequency;
-	newtimer.on_tick = on_tick;
-	newtimer.on_stop = on_stop;
-	newtimer.frequency = frequency;
+	new_timer.handle = next_handle++;
+	new_timer.next_tick = time(nullptr) + frequency;
+	new_timer.on_tick = on_tick;
+	new_timer.on_stop = on_stop;
+	new_timer.frequency = frequency;
 
-	next_timer.emplace(newtimer);
+	std::lock_guard<std::mutex> l(timer_guard);
+	next_timer.emplace(new_timer);
 
-	return newtimer.handle;
+	return new_timer.handle;
 }
 
 bool cluster::stop_timer(timer t) {
 	/*
-	 * Because iterating a priority queue is O(log n) we don't actually walk the queue
+	 * Because iterating a priority queue is at best O(log n) we don't actually walk the queue
 	 * looking for the timer to remove. Instead, we just insert the timer handle into a std::set
 	 * to inform the tick_timers() function later if it sees a handle in this set, it is to
 	 * have its on_stop() called and it is not to be rescheduled.
@@ -53,13 +54,9 @@ bool cluster::stop_timer(timer t) {
 	return true;
 }
 
-void cluster::timer_reschedule(timer_t& t) {
-
-}
-
 void cluster::tick_timers() {
 	time_t now = time(nullptr);
-	time_t time_frame{};
+
 	if (next_timer.empty()) {
 		return;
 	}

src/dpp/discordclient.cpp

@@ -43,8 +43,6 @@
 
 namespace dpp {
 
-constexpr size_t DECOMP_BUFFER_SIZE = 512 * 1024;
-
 /**
  * @brief This is an opaque class containing zlib library specific structures.
  * We define it this way so that the public facing D++ library doesn't require
@@ -58,11 +56,6 @@ class zlibcontext {
 	z_stream d_stream;
 };
 
-/**
- * @brief Stores the most recent ping message on this shard, which we check for to monitor latency
- */
-thread_local static std::string last_ping_message;
-
 discord_client::discord_client(dpp::cluster* _cluster, uint32_t _shard_id, uint32_t _max_shards, const std::string &_token, uint32_t _intents, bool comp, websocket_protocol_t ws_proto)
        : websocket_client(_cluster, _cluster->default_gateway, "443", comp ? (ws_proto == ws_json ? PATH_COMPRESSED_JSON : PATH_COMPRESSED_ETF) : (ws_proto == ws_json ? PATH_UNCOMPRESSED_JSON : PATH_UNCOMPRESSED_ETF)),
 	compressed(comp),
@@ -189,7 +182,7 @@ void discord_client::run()
 
 bool discord_client::handle_frame(const std::string &buffer, ws_opcode opcode)
 {
-	std::string& data = (std::string&)buffer;
+	auto& data = (std::string&)buffer;
 
 	/* gzip compression is a special case */
 	if (compressed) {
@@ -198,13 +191,13 @@ bool discord_client::handle_frame(const std::string &buffer, ws_opcode opcode)
 		&& (uint8_t)buffer[buffer.size() - 1] == 0xFF) {
 			/* Decompress buffer */
 			decompressed.clear();
-			zlib->d_stream.next_in = (Bytef *)buffer.c_str();
-			zlib->d_stream.avail_in = (uInt)buffer.size();
+			zlib->d_stream.next_in = (Bytef*)buffer.data();
+			zlib->d_stream.avail_in = static_cast<uInt>(buffer.size());
 			do {
-				zlib->d_stream.next_out = (Bytef*)decomp_buffer.data();
+				zlib->d_stream.next_out = reinterpret_cast<Bytef*>(decomp_buffer.data());
 				zlib->d_stream.avail_out = DECOMP_BUFFER_SIZE;
 				int ret = inflate(&(zlib->d_stream), Z_NO_FLUSH);
-				int have = DECOMP_BUFFER_SIZE - zlib->d_stream.avail_out;
+				size_t have = DECOMP_BUFFER_SIZE - zlib->d_stream.avail_out;
 				switch (ret)
 				{
 					case Z_NEED_DICT:
@@ -486,9 +479,8 @@ void discord_client::one_second_timer()
 			if (message_queue.size()) {
 				std::string message = message_queue.front();
 				message_queue.pop_front();
-				/* Checking here with .find() saves us having to deserialise the json
-				 * to find pings in our queue. The assumption is that the format of the
-				 * ping isn't going to change.
+				/* Checking here by string comparison saves us having to deserialise the json
+				 * to find pings in our queue.
 				 */
 				if (!last_ping_message.empty() && message == last_ping_message) {
 					ping_start = utility::time_f();

src/dpp/sslclient.cpp

@@ -296,6 +296,10 @@ void ssl_client::complete_handshake(const socket_events* ev)
 
 void ssl_client::on_read(socket fd, const struct socket_events& ev) {
 
+	if (sfd == INVALID_SOCKET) {
+		return;
+	}
+
 	if (plaintext && connected) {
 		int r = (int) ::recv(sfd, server_to_client_buffer, DPP_BUFSIZE, 0);
 		if (r <= 0) {
@@ -373,6 +377,11 @@ void ssl_client::on_read(socket fd, const struct socket_events& ev) {
 }
 
 void ssl_client::on_write(socket fd, const struct socket_events& e) {
+
+	if (sfd == INVALID_SOCKET) {
+		return;
+	}
+
 	if (!tcp_connect_done) {
 		tcp_connect_done = true;
 	}
@@ -400,7 +409,7 @@ void ssl_client::on_write(socket fd, const struct socket_events& e) {
 				throw dpp::connection_exception(err_ssl_version, "Failed to set minimum SSL version!");
 			}
 		}
-		if (!ssl->ssl) {
+		if (ssl != nullptr && ssl->ssl == nullptr) {
 			/* Create SSL session */
 			std::lock_guard<std::mutex> lock(ssl_mutex);
 			ssl->ssl = SSL_new(openssl_context.get());