From c2d497abc582ad406a5ca563ee502acdae5b4a7e Mon Sep 17 00:00:00 2001 From: Nikos Athanasiou Date: Sat, 5 Jun 2021 21:23:18 +0300 Subject: [PATCH] Avoid returning references that are mutex protected (#297) * Fix unsafe calls and safeguard WebSocketMessage from being called w/ temporaries * Use unnamed namespace to express internal linkage * Avoid returning references that are mutex protected Motivation for this MR The antipattern of returning references to mutex protected members was removed. Since a caller can hold the reference it would make all class level locking meaningless. Instead values are returned. The IXWebSocketPerMessageDeflateOptions class was shrunk by 7 bytes (1 padding + 2*3) after changing the int members to the used uint8_t; side effects of that were handled. An inefficient "string -> int" was replaced by standard library. As seen here http://coliru.stacked-crooked.com/a/46b5990bafb9c626 this gives an order of magnitude better performance. --- ixwebsocket/IXWebSocket.cpp | 10 ++++++---- ixwebsocket/IXWebSocket.h | 4 ++-- .../IXWebSocketPerMessageDeflateOptions.cpp | 20 ++++++------------- .../IXWebSocketPerMessageDeflateOptions.h | 4 ++-- 4 files changed, 16 insertions(+), 22 deletions(-) diff --git a/ixwebsocket/IXWebSocket.cpp b/ixwebsocket/IXWebSocket.cpp index 1675533e..cf23b8c3 100644 --- a/ixwebsocket/IXWebSocket.cpp +++ b/ixwebsocket/IXWebSocket.cpp @@ -41,7 +41,8 @@ namespace ix , _pingIntervalSecs(kDefaultPingIntervalSecs) { _ws.setOnCloseCallback( - [this](uint16_t code, const std::string& reason, size_t wireSize, bool remote) { + [this](uint16_t code, const std::string& reason, size_t wireSize, bool remote) + { _onMessageCallback( ix::make_unique(WebSocketMessageType::Close, emptyMsg, @@ -75,7 +76,7 @@ namespace ix _extraHeaders = headers; } - const std::string& WebSocket::getUrl() const + const std::string WebSocket::getUrl() const { std::lock_guard lock(_configMutex); return _url; @@ -94,7 +95,7 @@ namespace ix _socketTLSOptions = socketTLSOptions; } - const WebSocketPerMessageDeflateOptions& WebSocket::getPerMessageDeflateOptions() const + const WebSocketPerMessageDeflateOptions WebSocket::getPerMessageDeflateOptions() const { std::lock_guard lock(_configMutex); return _perMessageDeflateOptions; @@ -384,7 +385,8 @@ namespace ix [this](const std::string& msg, size_t wireSize, bool decompressionError, - WebSocketTransport::MessageKind messageKind) { + WebSocketTransport::MessageKind messageKind) + { WebSocketMessageType webSocketMessageType; switch (messageKind) { diff --git a/ixwebsocket/IXWebSocket.h b/ixwebsocket/IXWebSocket.h index 3d5c8c52..7cfe0088 100644 --- a/ixwebsocket/IXWebSocket.h +++ b/ixwebsocket/IXWebSocket.h @@ -92,8 +92,8 @@ namespace ix ReadyState getReadyState() const; static std::string readyStateToString(ReadyState readyState); - const std::string& getUrl() const; - const WebSocketPerMessageDeflateOptions& getPerMessageDeflateOptions() const; + const std::string getUrl() const; + const WebSocketPerMessageDeflateOptions getPerMessageDeflateOptions() const; int getPingInterval() const; size_t bufferedAmount() const; diff --git a/ixwebsocket/IXWebSocketPerMessageDeflateOptions.cpp b/ixwebsocket/IXWebSocketPerMessageDeflateOptions.cpp index 259ddcba..63d72173 100644 --- a/ixwebsocket/IXWebSocketPerMessageDeflateOptions.cpp +++ b/ixwebsocket/IXWebSocketPerMessageDeflateOptions.cpp @@ -14,12 +14,12 @@ namespace ix { /// Default values as defined in the RFC const uint8_t WebSocketPerMessageDeflateOptions::kDefaultServerMaxWindowBits = 15; - static const int minServerMaxWindowBits = 8; - static const int maxServerMaxWindowBits = 15; + static const uint8_t minServerMaxWindowBits = 8; + static const uint8_t maxServerMaxWindowBits = 15; const uint8_t WebSocketPerMessageDeflateOptions::kDefaultClientMaxWindowBits = 15; - static const int minClientMaxWindowBits = 8; - static const int maxClientMaxWindowBits = 15; + static const uint8_t minClientMaxWindowBits = 8; + static const uint8_t maxClientMaxWindowBits = 15; WebSocketPerMessageDeflateOptions::WebSocketPerMessageDeflateOptions( bool enabled, @@ -85,11 +85,7 @@ namespace ix if (startsWith(token, "server_max_window_bits=")) { - std::string val = token.substr(token.find_last_of("=") + 1); - std::stringstream ss; - ss << val; - int x; - ss >> x; + uint8_t x = std::stoi(token.substr(token.find_last_of("=") + 1)); // Sanitize values to be in the proper range [8, 15] in // case a server would give us bogus values @@ -99,11 +95,7 @@ namespace ix if (startsWith(token, "client_max_window_bits=")) { - std::string val = token.substr(token.find_last_of("=") + 1); - std::stringstream ss; - ss << val; - int x; - ss >> x; + uint8_t x = std::stoi(token.substr(token.find_last_of("=") + 1)); // Sanitize values to be in the proper range [8, 15] in // case a server would give us bogus values diff --git a/ixwebsocket/IXWebSocketPerMessageDeflateOptions.h b/ixwebsocket/IXWebSocketPerMessageDeflateOptions.h index 3e960f11..7cd33c0c 100644 --- a/ixwebsocket/IXWebSocketPerMessageDeflateOptions.h +++ b/ixwebsocket/IXWebSocketPerMessageDeflateOptions.h @@ -39,8 +39,8 @@ namespace ix bool _enabled; bool _clientNoContextTakeover; bool _serverNoContextTakeover; - int _clientMaxWindowBits; - int _serverMaxWindowBits; + uint8_t _clientMaxWindowBits; + uint8_t _serverMaxWindowBits; void sanitizeClientMaxWindowBits(); };