From 44057fef968505933d7d4334f503447df34c79bf Mon Sep 17 00:00:00 2001 From: Joshua Rogers Date: Sat, 1 Nov 2025 22:37:23 +0000 Subject: [PATCH] Removed always-true ClientRequestContext::httpStateIsValid() (#2279) ... and related cbdata checks/comments. `ClientRequestContext::httpStateIsValid()` always returns true. We can prove that by contradiction: * `ClientRequestContext::httpStateIsValid()` return value is `cbdataReferenceValid(http)` return value. * `ClientRequestContext::http` points to a `ClientHttpRequest` object. * If `cbdataReferenceValid(http)` is false, then the corresponding `ClientHttpRequest` object pointed to by `http` has been destructed. * `ClientHttpRequest` destructor deletes its `calloutContext` data member (i.e. `ClientRequestContext`). Thus, if an `x->httpStateIsValid()` call returns false, then `x` object has already been deleted, and the call itself was buggy because it was dereferencing a pointer to a deleted object! Moreover, `cbdataReferenceValid(nullptr)` is always true, so calling `httpStateIsValid()` twice for a deleted `ClientHttpRequest` object does not actually protect the second caller (i.e. does not detect that `http` is gone) because the first call makes `ClientRequestContext::http` nil, making `httpStateIsValid()` true again! None of the above problems actually happen because `cbdataReferenceValid(http)` is never false inside `ClientRequestContext::httpStateIsValid()`. By the time it could have become false, `ClientRequestContext` object itself is already gone. Also removed a source code comment that falsely claimed that `ClientRequestContext` callback methods should check whether `ClientHttpRequest` object is still "valid". In reality, `ClientRequestContext` methods are not called after `ClientHttpRequest` object is destructed because they are protected by cbdata guards, and `ClientHttpRequest` destruction deletes `ClientRequestContext` object, resulting in those guards skipping any callbacks. The same comment also incorrectly implied that `ClientHttpRequest` first becomes "invalid" and then, _after_ the current callback is done, `ClientHttpRequest` destructor is called. In reality, `cbdataReferenceValid(x)` becomes false when `delete x` is called and, hence, when `x` destructor runs. --- src/ClientRequestContext.h | 1 - src/client_side_request.cc | 55 -------------------------------------- 2 files changed, 56 deletions(-) diff --git a/src/ClientRequestContext.h b/src/ClientRequestContext.h index a5ac37842b..6d26c1741f 100644 --- a/src/ClientRequestContext.h +++ b/src/ClientRequestContext.h @@ -33,7 +33,6 @@ public: ClientRequestContext(ClientHttpRequest *); ~ClientRequestContext() override; - bool httpStateIsValid(); void hostHeaderVerify(); void hostHeaderIpVerify(const ipcache_addrs *, const Dns::LookupDetails &); void hostHeaderVerifyFailed(const char *A, const char *B); diff --git a/src/client_side_request.cc b/src/client_side_request.cc index 6afcec7e25..3ef4dc80d4 100644 --- a/src/client_side_request.cc +++ b/src/client_side_request.cc @@ -106,11 +106,6 @@ static void checkFailureRatio(err_type, hier_code); ClientRequestContext::~ClientRequestContext() { - /* - * Release our "lock" on our parent, ClientHttpRequest, if we - * still have one - */ - cbdataReferenceDone(http); delete error; @@ -268,21 +263,6 @@ ClientHttpRequest::~ClientHttpRequest() dlinkDelete(&active, &ClientActiveRequests); } -bool -ClientRequestContext::httpStateIsValid() -{ - ClientHttpRequest *http_ = http; - - if (cbdataReferenceValid(http_)) - return true; - - http = nullptr; - - cbdataReferenceDone(http_); - - return false; -} - #if FOLLOW_X_FORWARDED_FOR /** * clientFollowXForwardedForCheck() checks the content of X-Forwarded-For: @@ -302,10 +282,6 @@ static void clientFollowXForwardedForCheck(Acl::Answer answer, void *data) { ClientRequestContext *calloutContext = (ClientRequestContext *) data; - - if (!calloutContext->httpStateIsValid()) - return; - ClientHttpRequest *http = calloutContext->http; HttpRequest *request = http->request; @@ -607,10 +583,6 @@ void clientAccessCheckDoneWrapper(Acl::Answer answer, void *data) { ClientRequestContext *calloutContext = (ClientRequestContext *) data; - - if (!calloutContext->httpStateIsValid()) - return; - calloutContext->clientAccessCheckDone(answer); } @@ -1024,10 +996,6 @@ void clientRedirectDoneWrapper(void *data, const Helper::Reply &result) { ClientRequestContext *calloutContext = (ClientRequestContext *)data; - - if (!calloutContext->httpStateIsValid()) - return; - calloutContext->clientRedirectDone(result); } @@ -1035,10 +1003,6 @@ void clientStoreIdDoneWrapper(void *data, const Helper::Reply &result) { ClientRequestContext *calloutContext = (ClientRequestContext *)data; - - if (!calloutContext->httpStateIsValid()) - return; - calloutContext->clientStoreIdDone(result); } @@ -1225,10 +1189,6 @@ static void checkNoCacheDoneWrapper(Acl::Answer answer, void *data) { ClientRequestContext *calloutContext = (ClientRequestContext *) data; - - if (!calloutContext->httpStateIsValid()) - return; - calloutContext->checkNoCacheDone(answer); } @@ -1321,18 +1281,12 @@ static void sslBumpAccessCheckDoneWrapper(Acl::Answer answer, void *data) { ClientRequestContext *calloutContext = static_cast(data); - - if (!calloutContext->httpStateIsValid()) - return; calloutContext->sslBumpAccessCheckDone(answer); } void ClientRequestContext::sslBumpAccessCheckDone(const Acl::Answer &answer) { - if (!httpStateIsValid()) - return; - const Ssl::BumpMode bumpMode = answer.allowed() ? static_cast(answer.kind) : Ssl::bumpSplice; http->sslBumpNeed(bumpMode); // for processRequest() to bump if needed @@ -1409,8 +1363,6 @@ SslBumpEstablish(const Comm::ConnectionPointer &, char *, size_t, Comm::Flag err { ClientHttpRequest *r = static_cast(data); debugs(85, 5, "responded to CONNECT: " << r << " ? " << errflag); - - assert(r && cbdataReferenceValid(r)); r->sslBumpEstablish(errflag); } @@ -1612,11 +1564,6 @@ ClientHttpRequest::clearRequest() * Note that ClientRequestContext is created before the first call * to doCallouts(). * - * If one of the callouts notices that ClientHttpRequest is no - * longer valid, it should call cbdataReferenceDone() so that - * ClientHttpRequest's reference count goes to zero and it will get - * deleted. ClientHttpRequest will then delete ClientRequestContext. - * * Note that we set the _done flags here before actually starting * the callout. This is strictly for convenience. */ @@ -1768,7 +1715,6 @@ ClientHttpRequest::doCallouts() } } - cbdataReferenceDone(calloutContext->http); delete calloutContext; calloutContext = nullptr; @@ -1872,7 +1818,6 @@ ClientHttpRequest::startAdaptation(const Adaptation::ServiceGroupPointer &g) void ClientHttpRequest::noteAdaptationAnswer(const Adaptation::Answer &answer) { - assert(cbdataReferenceValid(this)); // indicates bug clearAdaptation(virginHeadSource); assert(!adaptedBodySource); -- 2.47.3