From de9dba2d8f8464a7d35fd34a699e7dfcead63dfd Mon Sep 17 00:00:00 2001 From: Shaunak Kishore Date: Tue, 17 Aug 2021 07:37:19 -0700 Subject: [PATCH] Remove easy handles before curl_multi_cleanup Summary: Before destroying a curl multi handle, we must detach all easy handles attached to it: https://curl.se/libcurl/c/curl_multi_cleanup.html Today, we don't respect this constraint at all. Let's fix that. We destroy curl multi handles in two cases: during the request, when its refcount goes to 0, and at the end of the request, on a call to sweep. In both of these cases, we don't place any ordering invariant between destroying multi handles and their attached easy handles today - we can destroy the multi handles with easy handles attached, or worse, destroy the easy handle, then destroy the multi handle with a dangling reference! We fix these cases by tracking a bidirectional mapping between multi handles and their attached easy handle. Whenever we destroy either handle, we first detach all edges incident to it. We need to be a little bit careful on sweep, because it happens after we start to tear down request memory. To handle that case, I added a "leak" param to CurlMultiResource::remove that lets us detach the edge without allocating memory or calling destructors. I also tied the add/remove metadata operations to the underlying curl operations, so that we don't add/remove an edge when the curl operation fails. Reviewed By: ricklavoie Differential Revision: D30344219 fbshipit-source-id: 553a7dae54a96737f82180d3b9abec317e81e8a4 --- hphp/runtime/ext/curl/curl-multi-await.cpp | 11 +-- hphp/runtime/ext/curl/curl-multi-resource.cpp | 100 ++++++++++++++++---------- hphp/runtime/ext/curl/curl-multi-resource.h | 14 ++-- hphp/runtime/ext/curl/curl-resource.cpp | 13 ++-- hphp/runtime/ext/curl/curl-resource.h | 8 ++- hphp/runtime/ext/curl/ext_curl.cpp | 8 +-- 6 files changed, 88 insertions(+), 66 deletions(-) diff --git a/hphp/runtime/ext/curl/curl-multi-await.cpp b/hphp/runtime/ext/curl/curl-multi-await.cpp index 0d82f7b03af..d647f718e47 100644 --- a/hphp/runtime/ext/curl/curl-multi-await.cpp +++ b/hphp/runtime/ext/curl/curl-multi-await.cpp @@ -133,15 +133,10 @@ int CurlMultiAwait::addLowHandles(req::ptr multi) { // This is a little hacky, but necessary given cURL's APIs int CurlMultiAwait::addHighHandles(req::ptr multi) { int count = 0; - auto easy_handles = multi->getEasyHandles(); - for (ArrayIter iter(easy_handles); iter; ++iter) { - Variant easy_handle = iter.second(); - auto easy = dyn_cast_or_null(easy_handle); - if (!easy) continue; + for (auto const& ch : multi->getEasyHandles()) { long sock; - if ((curl_easy_getinfo(easy->get(), - CURLINFO_LASTSOCKET, &sock) != CURLE_OK) || - (sock < FD_SETSIZE)) { + auto const code = curl_easy_getinfo(ch->get(), CURLINFO_LASTSOCKET, &sock); + if ((code != CURLE_OK) || (sock < FD_SETSIZE)) { continue; } // No idea which type of event it needs, ask for everything diff --git a/hphp/runtime/ext/curl/curl-multi-resource.cpp b/hphp/runtime/ext/curl/curl-multi-resource.cpp index 791370c4a42..bfed6bcd671 100644 --- a/hphp/runtime/ext/curl/curl-multi-resource.cpp +++ b/hphp/runtime/ext/curl/curl-multi-resource.cpp @@ -20,16 +20,25 @@ CurlMultiResource::CurlMultiResource() { } void CurlMultiResource::close() { - if (m_multi) { - curl_multi_cleanup(m_multi); - m_easyh.clear(); - m_multi = nullptr; - } + if (!m_multi) return; + removeEasyHandles(); + curl_multi_cleanup(m_multi); + m_multi = nullptr; } void CurlMultiResource::sweep() { - if (m_multi) { - curl_multi_cleanup(m_multi); + if (!m_multi) return; + removeEasyHandles(true); + curl_multi_cleanup(m_multi); +} + +void CurlMultiResource::removeEasyHandles(bool leak) { + auto index_to_remove = 0; + auto const size = m_easyh.size(); + for (auto i = 0; i < size; i++) { + assertx(index_to_remove < m_easyh.size()); + auto const code = remove(m_easyh[index_to_remove].get(), leak); + if (code != CURLM_OK) index_to_remove++; } } @@ -37,7 +46,7 @@ bool CurlMultiResource::setOption(int option, const Variant& value) { #if LIBCURL_VERSION_NUM <= 0x070f04 /* 7.15.4 */ return false; #endif - if (m_multi == nullptr) { + if (!m_multi) { return false; } @@ -64,45 +73,61 @@ bool CurlMultiResource::setOption(int option, const Variant& value) { return error == CURLM_OK; } -void CurlMultiResource::remove(req::ptr curle) { - assertx(m_easyh.isVec()); - auto index_to_remove = -1; - for (ArrayIter iter(m_easyh); iter; ++iter) { - if (cast(iter.second())->get(true) == curle->get()) { - assertx(tvIsInt(iter.nvFirst())); - index_to_remove = val(iter.nvFirst()).num; - break; - } - } - if (index_to_remove >= 0) { - assertx(m_easyh.size() > 0); - auto const last = safe_cast(m_easyh.size() - 1); - if (index_to_remove != last) { - m_easyh.set(index_to_remove, m_easyh[last]); +CURLMcode CurlMultiResource::add(CurlResource* curle) { + // Don't add the handle to our metadata if the add op fails. + if (!m_multi) return CURLM_BAD_HANDLE; + if (!curle->get()) return CURLM_BAD_EASY_HANDLE; + if (curle->m_multi) return CURLM_ADDED_ALREADY; + auto const code = curl_multi_add_handle(m_multi, curle->get()); + if (code != CURLM_OK) return code; + + curle->m_multi = this; + m_easyh.emplace_back(curle); + return CURLM_OK; +} + +CURLMcode CurlMultiResource::remove(CurlResource* curle, bool leak) { + // Don't remove the handle from our metadata if the remove op fails. + if (!m_multi) return CURLM_BAD_HANDLE; + if (!curle->get()) return CURLM_BAD_EASY_HANDLE; + if (curle->m_multi != this) return CURLM_OK; // we do this sometimes... + auto const code = curl_multi_remove_handle(m_multi, curle->get()); + if (code != CURLM_OK) return code; + + // Find the index to remove. + auto const index_to_remove = [&]{ + for (auto i = 0; i < m_easyh.size(); i++) { + if (m_easyh[i].get() == curle) return i; } - m_easyh.pop(); + always_assert(false); + }(); + + // Remove the easy handle, leaking it if requested. + curle->m_multi = nullptr; + if (leak) m_easyh[index_to_remove].detach(); + auto const last = m_easyh.size() - 1; + if (index_to_remove != last) { + m_easyh[index_to_remove] = std::move(m_easyh[last]); } + m_easyh.pop_back(); + return CURLM_OK; } Resource CurlMultiResource::find(CURL *cp) { - for (ArrayIter iter(m_easyh); iter; ++iter) { - if (cast(iter.second())->get(true) == cp) { - return iter.second().toResource(); - } + for (auto const& curl : m_easyh) { + if (curl->get() == cp) return Resource(curl.get()); } return Resource(); } void CurlMultiResource::setInExec(bool b) { - for (ArrayIter iter(m_easyh); iter; ++iter) { - auto const curl = cast(iter.second()); + for (auto& curl : m_easyh) { curl->m_in_exec = b; } } bool CurlMultiResource::anyInExec() const { - for (ArrayIter iter(m_easyh); iter; ++iter) { - auto const curl = cast(iter.second()); + for (auto const& curl : m_easyh) { if (curl->m_in_exec) return true; } return false; @@ -111,8 +136,7 @@ bool CurlMultiResource::anyInExec() const { void CurlMultiResource::check_exceptions() { SCOPE_EXIT { if (debug) { - for (ArrayIter iter(m_easyh); iter; ++iter) { - auto const curl = cast(iter.second()); + for (auto const& curl : m_easyh) { always_assert(!curl->m_exception); } } @@ -120,8 +144,7 @@ void CurlMultiResource::check_exceptions() { // If we exit unexpectedly, ensure we've released any queued exceptions. SCOPE_EXIT { - for (ArrayIter iter(m_easyh); iter; ++iter) { - auto const curl = cast(iter.second()); + for (auto const& curl : m_easyh) { if (auto const exn = curl->getAndClearException()) { if (!CurlResource::isPhpException(exn)) { delete CurlResource::getCppException(exn); @@ -132,8 +155,7 @@ void CurlMultiResource::check_exceptions() { Exception* cppException = nullptr; Object phpException; - for (ArrayIter iter(m_easyh); iter; ++iter) { - auto const curl = cast(iter.second()); + for (auto const& curl : m_easyh) { auto const nextException = curl->getAndClearException(); if (!nextException) continue; if (CurlResource::isPhpException(nextException)) { @@ -157,7 +179,7 @@ void CurlMultiResource::check_exceptions() { } CURLM* CurlMultiResource::get() { - if (m_multi == nullptr) { + if (!m_multi) { throw_null_pointer_exception(); } return m_multi; diff --git a/hphp/runtime/ext/curl/curl-multi-resource.h b/hphp/runtime/ext/curl/curl-multi-resource.h index 1c1599982f5..58de2f1c707 100644 --- a/hphp/runtime/ext/curl/curl-multi-resource.h +++ b/hphp/runtime/ext/curl/curl-multi-resource.h @@ -2,6 +2,7 @@ #include "hphp/runtime/ext/extension.h" +#include "hphp/util/compact-vector.h" #include "hphp/util/type-scan.h" #include @@ -13,6 +14,8 @@ namespace HPHP { struct CurlResource; struct CurlMultiResource : SweepableResourceData { + using EasyHandles = CompactVector>; + DECLARE_RESOURCE_ALLOCATION(CurlMultiResource) CLASSNAME_IS("curl_multi") const String& o_getClassNameHook() const override { return classnameof(); } @@ -23,10 +26,10 @@ struct CurlMultiResource : SweepableResourceData { void close(); bool setOption(int option, const Variant& value); - void add(const Resource& ch) { m_easyh.append(ch); } - const Array& getEasyHandles() const { return m_easyh; } + const EasyHandles& getEasyHandles() const { return m_easyh; } - void remove(req::ptr curle); + CURLMcode add(CurlResource* curle); + CURLMcode remove(CurlResource* curle, bool leak = false); Resource find(CURL *cp); CURLM* get(); @@ -36,10 +39,11 @@ struct CurlMultiResource : SweepableResourceData { bool anyInExec() const; private: + void removeEasyHandles(bool leak = false); + CURLM *m_multi; - // CURLM is a typedef to void TYPE_SCAN_IGNORE_FIELD(m_multi); - Array m_easyh = Array::CreateVec(); + EasyHandles m_easyh; }; ///////////////////////////////////////////////////////////////////////////// diff --git a/hphp/runtime/ext/curl/curl-resource.cpp b/hphp/runtime/ext/curl/curl-resource.cpp index 968196dbb40..dfdcb92d029 100644 --- a/hphp/runtime/ext/curl/curl-resource.cpp +++ b/hphp/runtime/ext/curl/curl-resource.cpp @@ -1,4 +1,5 @@ #include "hphp/runtime/ext/curl/curl-resource.h" +#include "hphp/runtime/ext/curl/curl-multi-resource.h" #include "hphp/runtime/ext/curl/curl-share-resource.h" #include "hphp/runtime/ext/curl/ext_curl.h" @@ -53,6 +54,7 @@ CurlResource::ToFree::~ToFree() { CurlResource::CurlResource(const String& url) : m_emptyPost(true), m_safeUpload(true) { m_cp = curl_easy_init(); + m_multi = nullptr; m_url = url; memset(m_error_str, 0, sizeof(m_error_str)); @@ -86,11 +88,8 @@ void CurlResource::sweep() { } void CurlResource::close() { - // If we try to close while we're in a callback, warn. if (m_in_callback) { - raise_warning( - "curl_close(): Attempt to close cURL in callback, ignored." - ); + raise_warning("curl_close(): Attempt to close cURL in callback, ignored."); return; } closeForSweep(); @@ -100,7 +99,9 @@ void CurlResource::close() { void CurlResource::closeForSweep() { assertx(!m_exception); + assertx(IMPLIES(m_multi, m_cp)); if (!m_cp) return; + if (m_multi) m_multi->remove(this, /*leak=*/true); curl_easy_cleanup(m_cp); m_cp = nullptr; } @@ -283,8 +284,8 @@ Variant CurlResource::getOption(long option) { return false; } -CURL* CurlResource::get(bool nullOkay /*=false*/) { - if (m_cp == nullptr && !nullOkay) { +CURL* CurlResource::get() { + if (!m_cp) { throw_null_pointer_exception(); } return m_cp; diff --git a/hphp/runtime/ext/curl/curl-resource.h b/hphp/runtime/ext/curl/curl-resource.h index 72485bea3f8..8ee2e76d48c 100644 --- a/hphp/runtime/ext/curl/curl-resource.h +++ b/hphp/runtime/ext/curl/curl-resource.h @@ -13,6 +13,7 @@ namespace HPHP { ///////////////////////////////////////////////////////////////////////////// // CurlResource +struct CurlMultiResource; struct CurlResource : SweepableResourceData { using ExceptionType = req::Optional>; @@ -71,7 +72,7 @@ struct CurlResource : SweepableResourceData { int getError() { return m_error_no; } String getErrorString() { return String(m_error_str, CopyString); } - CURL *get(bool nullOkay = false); + CURL *get(); void check_exception(); ExceptionType getAndClearException() { return std::move(m_exception); } @@ -124,8 +125,12 @@ struct CurlResource : SweepableResourceData { static CURLcode ssl_ctx_callback(CURL *curl, void *sslctx, void *parm); private: + friend struct CurlMultiResource; + CURL *m_cp; TYPE_SCAN_IGNORE_FIELD(m_cp); + CurlMultiResource* m_multi; + TYPE_SCAN_IGNORE_FIELD(m_multi); ExceptionType m_exception; char m_error_str[CURL_ERROR_SIZE + 1]; @@ -146,7 +151,6 @@ struct CurlResource : SweepableResourceData { bool m_in_exec{false}; bool m_emptyPost; bool m_safeUpload; - friend struct CurlMultiResource; }; ///////////////////////////////////////////////////////////////////////////// diff --git a/hphp/runtime/ext/curl/ext_curl.cpp b/hphp/runtime/ext/curl/ext_curl.cpp index fee651b6cbc..cef082a3fe4 100644 --- a/hphp/runtime/ext/curl/ext_curl.cpp +++ b/hphp/runtime/ext/curl/ext_curl.cpp @@ -446,17 +446,13 @@ Variant HHVM_FUNCTION(curl_multi_strerror, int64_t code) { Variant HHVM_FUNCTION(curl_multi_add_handle, const Resource& mh, const Resource& ch) { CHECK_MULTI_RESOURCE(curlm); - auto curle = cast(ch); - curlm->add(ch); - return curl_multi_add_handle(curlm->get(), curle->get()); + return curlm->add(cast(ch).get()); } Variant HHVM_FUNCTION(curl_multi_remove_handle, const Resource& mh, const Resource& ch) { CHECK_MULTI_RESOURCE(curlm); - auto curle = cast(ch); - curlm->remove(curle); - return curl_multi_remove_handle(curlm->get(), curle->get()); + return curlm->remove(cast(ch).get()); } Variant HHVM_FUNCTION(curl_multi_exec, const Resource& mh, -- 2.11.4.GIT