Fix virtual call in ClientImpl::~ClientImpl() (#942)

* Fix virtual call in ClientImpl::~ClientImpl()

This fixes a warning in clang tidy:

> Call to virtual method 'ClientImpl::shutdown_ssl' during
> destruction bypasses virtual dispatch

ClientImpl::~ClientImpl() calls lock_socket_and_shutdown_and_close()
that itself calls shutdown_ssl().  However, shutdown_ssl() is virtual
and C++ does not perform virtual dispatch in destructors, which results
in the wrong overload being called.

This change adds a non-virtual shutdown_ssl_impl() function that is
called from ~SSLClient().  We also inline sock_socket_and_shutdown_and_close()
and removes the virtual call in ~ClientImpl().

* Inline and remove lock_socket_and_shutdown_and_close()

The function only has one caller.
This commit is contained in:
Mathias Laurin 2021-05-23 02:15:20 +02:00 committed by GitHub
parent ba34ea4ee8
commit 089b9daa1c
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23

View file

@ -1050,10 +1050,6 @@ protected:
void shutdown_socket(Socket &socket);
void close_socket(Socket &socket);
// Similar to shutdown_ssl and close_socket, this should NOT be called
// concurrently with a DIFFERENT thread sending requests from the socket
void lock_socket_and_shutdown_and_close();
bool process_request(Stream &strm, Request &req, Response &res,
bool close_connection, Error &error);
@ -1412,6 +1408,7 @@ public:
private:
bool create_and_connect_socket(Socket &socket, Error &error) override;
void shutdown_ssl(Socket &socket, bool shutdown_gracefully) override;
void shutdown_ssl_impl(Socket &socket, bool shutdown_socket);
bool process_socket(const Socket &socket,
std::function<bool(Stream &strm)> callback) override;
@ -5258,7 +5255,11 @@ inline ClientImpl::ClientImpl(const std::string &host, int port,
host_and_port_(host_ + ":" + std::to_string(port_)),
client_cert_path_(client_cert_path), client_key_path_(client_key_path) {}
inline ClientImpl::~ClientImpl() { lock_socket_and_shutdown_and_close(); }
inline ClientImpl::~ClientImpl() {
std::lock_guard<std::mutex> guard(socket_mutex_);
shutdown_socket(socket_);
close_socket(socket_);
}
inline bool ClientImpl::is_valid() const { return true; }
@ -5356,13 +5357,6 @@ inline void ClientImpl::close_socket(Socket &socket) {
socket.sock = INVALID_SOCKET;
}
inline void ClientImpl::lock_socket_and_shutdown_and_close() {
std::lock_guard<std::mutex> guard(socket_mutex_);
shutdown_ssl(socket_, true);
shutdown_socket(socket_);
close_socket(socket_);
}
inline bool ClientImpl::read_response_line(Stream &strm, const Request &req,
Response &res) {
std::array<char, 2048> buf;
@ -5911,7 +5905,10 @@ inline bool ClientImpl::process_request(Stream &strm, Request &req,
// mutex during the process. It would be a bug to call it from a different
// thread since it's a thread-safety issue to do these things to the socket
// if another thread is using the socket.
lock_socket_and_shutdown_and_close();
std::lock_guard<std::mutex> guard(socket_mutex_);
shutdown_ssl(socket_, true);
shutdown_socket(socket_);
close_socket(socket_);
}
// Log
@ -6864,7 +6861,7 @@ inline SSLClient::~SSLClient() {
// Make sure to shut down SSL since shutdown_ssl will resolve to the
// base function rather than the derived function once we get to the
// base class destructor, and won't free the SSL (causing a leak).
SSLClient::shutdown_ssl(socket_, true);
shutdown_ssl_impl(socket_, true);
}
inline bool SSLClient::is_valid() const { return ctx_; }
@ -7043,6 +7040,10 @@ inline bool SSLClient::initialize_ssl(Socket &socket, Error &error) {
}
inline void SSLClient::shutdown_ssl(Socket &socket, bool shutdown_gracefully) {
shutdown_ssl_impl(socket, shutdown_gracefully);
}
inline void SSLClient::shutdown_ssl_impl(Socket &socket, bool shutdown_gracefully) {
if (socket.sock == INVALID_SOCKET) {
assert(socket.ssl == nullptr);
return;