From 961a9379d5dae931aa1ce21ff3e3f53889874c83 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ak=C4=B1n=20ELDEN?= <43653687+akinelden@users.noreply.github.com> Date: Sat, 29 Jul 2023 19:09:25 +0300 Subject: [PATCH] Fix #1590 (#1630) * ClientImpl: Connection=close header control moved from process_request to send_ * Connection=close header control moved from send_ to handle_request * SSLClient::connect_with_proxy error handling improved * to_string definition added for Error::ProxyConnection * Comment improvement --------- Co-authored-by: akinelden --- httplib.h | 65 ++++++++++++++++++++++++++++++++----------------------- 1 file changed, 38 insertions(+), 27 deletions(-) diff --git a/httplib.h b/httplib.h index 9a26484..1a6c212 100644 --- a/httplib.h +++ b/httplib.h @@ -950,6 +950,7 @@ enum class Error { UnsupportedMultipartBoundaryChars, Compression, ConnectionTimeout, + ProxyConnection, // For internal use only SSLPeerCouldBeClosed_, @@ -1888,6 +1889,7 @@ inline std::string to_string(const Error error) { return "Unsupported HTTP multipart boundary characters"; case Error::Compression: return "Compression failed"; case Error::ConnectionTimeout: return "Connection timed out"; + case Error::ProxyConnection: return "Proxy connection failed"; case Error::Unknown: return "Unknown"; default: break; } @@ -6748,6 +6750,21 @@ inline bool ClientImpl::handle_request(Stream &strm, Request &req, if (!ret) { return false; } + if (res.get_header_value("Connection") == "close" || + (res.version == "HTTP/1.0" && res.reason != "Connection established")) { + // TODO this requires a not-entirely-obvious chain of calls to be correct + // for this to be safe. + + // This is safe to call because handle_request is only called by send_ + // which locks the request 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. + std::lock_guard guard(socket_mutex_); + shutdown_ssl(socket_, true); + shutdown_socket(socket_); + close_socket(socket_); + } + if (300 < res.status && res.status < 400 && follow_location_) { req = req_save; ret = redirect(req, res, error); @@ -7162,24 +7179,6 @@ inline bool ClientImpl::process_request(Stream &strm, Request &req, } } - if (res.get_header_value("Connection") == "close" || - (res.version == "HTTP/1.0" && res.reason != "Connection established")) { - // TODO this requires a not-entirely-obvious chain of calls to be correct - // for this to be safe. Maybe a code refactor (such as moving this out to - // the send function and getting rid of the recursiveness of the mutex) - // could make this more obvious. - - // This is safe to call because process_request is only called by - // handle_request which is only called by send, which locks the request - // 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. - std::lock_guard guard(socket_mutex_); - shutdown_ssl(socket_, true); - shutdown_socket(socket_); - close_socket(socket_); - } - // Log if (logger_) { logger_(req, res); } @@ -8287,14 +8286,14 @@ inline bool SSLClient::create_and_connect_socket(Socket &socket, Error &error) { inline bool SSLClient::connect_with_proxy(Socket &socket, Response &res, bool &success, Error &error) { success = true; - Response res2; + Response proxy_res; if (!detail::process_client_socket( socket.sock, read_timeout_sec_, read_timeout_usec_, write_timeout_sec_, write_timeout_usec_, [&](Stream &strm) { Request req2; req2.method = "CONNECT"; req2.path = host_and_port_; - return process_request(strm, req2, res2, false, error); + return process_request(strm, req2, proxy_res, false, error); })) { // Thread-safe to close everything because we are assuming there are no // requests in flight @@ -8305,12 +8304,12 @@ inline bool SSLClient::connect_with_proxy(Socket &socket, Response &res, return false; } - if (res2.status == 407) { + if (proxy_res.status == 407) { if (!proxy_digest_auth_username_.empty() && !proxy_digest_auth_password_.empty()) { std::map auth; - if (detail::parse_www_authenticate(res2, auth, true)) { - Response res3; + if (detail::parse_www_authenticate(proxy_res, auth, true)) { + proxy_res = Response(); if (!detail::process_client_socket( socket.sock, read_timeout_sec_, read_timeout_usec_, write_timeout_sec_, write_timeout_usec_, [&](Stream &strm) { @@ -8321,7 +8320,7 @@ inline bool SSLClient::connect_with_proxy(Socket &socket, Response &res, req3, auth, 1, detail::random_string(10), proxy_digest_auth_username_, proxy_digest_auth_password_, true)); - return process_request(strm, req3, res3, false, error); + return process_request(strm, req3, proxy_res, false, error); })) { // Thread-safe to close everything because we are assuming there are // no requests in flight @@ -8332,12 +8331,24 @@ inline bool SSLClient::connect_with_proxy(Socket &socket, Response &res, return false; } } - } else { - res = res2; - return false; } } + // If status code is not 200, proxy request is failed. + // Set error to ProxyConnection and return proxy response + // as the response of the request + if (proxy_res.status != 200) + { + error = Error::ProxyConnection; + res = std::move(proxy_res); + // Thread-safe to close everything because we are assuming there are + // no requests in flight + shutdown_ssl(socket, true); + shutdown_socket(socket); + close_socket(socket); + return false; + } + return true; }