import EuroLinux squid-4.15-7.module+el8.9.0+21530+59b09a5b.10
This commit is contained in:
parent
b5d3c5c00b
commit
06eaaacb31
@ -1,599 +0,0 @@
|
||||
From 4896d07bf753683a3dbba4210384b0d862ff2d11 Mon Sep 17 00:00:00 2001
|
||||
From: Eduard Bagdasaryan <eduard.bagdasaryan@measurement-factory.com>
|
||||
Date: Thu, 7 Dec 2023 16:47:08 +0000
|
||||
Subject: [PATCH 1/7] Break long store_client call chains with async calls
|
||||
(#1056)
|
||||
|
||||
The store_client class design created very long call chains spanning
|
||||
Squid-client and Squid-server processing and multiple transactions.
|
||||
These call chains also create ideal conditions for dangerous recursive
|
||||
relationships between communicating classes (a.k.a. "reentrancy" among
|
||||
Squid developers). For example, storeClientCopy() enters store_client
|
||||
and triggers disk I/O that triggers invokeHandlers() that re-enters the
|
||||
same store_client object and starts competing with the original
|
||||
storeClientCopy() processing state.
|
||||
|
||||
The official code prevented the worst recursion cases with three(!)
|
||||
boolean flags and time-based events abused to break some of the call
|
||||
chains, but that approach did not solve all of the problems while also
|
||||
losing transaction context information across time-based events.
|
||||
|
||||
This change effectively makes STCB storeClientCopy() callbacks
|
||||
asynchronous, eliminating the need for time-based events and one of the
|
||||
flags. It shortens many call chains and preserves transaction context.
|
||||
The remaining problems can and should be eliminated by converting
|
||||
store_client into AsyncJob, but those changes deserve a dedicated PR.
|
||||
|
||||
store_client orchestrates cooperation of multiple asynchronous players:
|
||||
|
||||
* Sink: A Store client requests a STCB callback via a
|
||||
storeClientCopy()/copy() call. A set _callback.callback_handler
|
||||
implies that the client is waiting for this callback.
|
||||
|
||||
* Source1: A Store disk reading subsystem activated by the storeRead()
|
||||
call "spontaneously" delivers response bytes via storeClientRead*()
|
||||
callbacks. The disk_io_pending flag implies waiting for them.
|
||||
|
||||
* Source2: Store memory subsystem activated by storeClientListAdd()
|
||||
"spontaneously" delivers response bytes via invokeHandlers().
|
||||
|
||||
* Source3: Store disk subsystem activated by storeSwapInStart()
|
||||
"spontaneously" notifies of EOF/error by calling noteSwapInDone().
|
||||
|
||||
* Source4: A store_client object owner may delete the object by
|
||||
"spontaneously" calling storeUnregister(). The official code was
|
||||
converting this event into an error-notifying callback.
|
||||
|
||||
We continue to answer each storeClientCopy() request with the first
|
||||
available information even though several SourceN calls are possible
|
||||
while we are waiting to complete the STCB callback. The StoreIOBuffer
|
||||
API and STCB recipients do not support data+error/eof combinations, and
|
||||
future code will move this wait to the main event loop anyway. This
|
||||
first-available approach means that the creation of the notifier call
|
||||
effectively ends answer processing -- store_client just waits for that
|
||||
call to fire so that it can relay the answer to still-synchronous STCB.
|
||||
When STCB itself becomes asynchronous, this logic will continue to work.
|
||||
|
||||
Also stopped calling STCB from storeUnregister(). Conceptually, the
|
||||
storeUnregister() and storeClientCopy() callers ought to represent the
|
||||
same get-content-from-Store task; there should be no need to notify that
|
||||
task about what it is doing. Technically, analysis of STCB callbacks
|
||||
showed that many such notifications would be dangerous (if they are or
|
||||
become reachable). At the time of the storeUnregister() call, the STCB
|
||||
callbacks are usually unset (e.g., when storeUnregister() is called from
|
||||
the destructor, after that object has finished copying -- a very common
|
||||
case) or do not do anything (useful).
|
||||
|
||||
Also removed callback_data from the Callback::pending() condition. It is
|
||||
conceptually wrong to require non-nil callback parameter, and it is
|
||||
never cleared separately from the callback_handler data member anyway.
|
||||
|
||||
Also hid copyInto into the private store_client section to make sure it
|
||||
is not modified while we are waiting to complete the STCB callback. This
|
||||
move required adding a couple of read-only wrapper methods like
|
||||
bytesWanted() and noteSwapInDone().
|
||||
|
||||
Also simplified error/EOF/bytes handling on copy()-STCB path using
|
||||
dedicated methods (e.g., store_client::callback() API is no longer
|
||||
mixing EOF and error signals).
|
||||
|
||||
Modified-by: Alex Burmashev <alexander.burmashev@oracle.com>
|
||||
Signed-off-by: Alex Burmashev <alexander.burmashev@oracle.com>
|
||||
---
|
||||
src/MemObject.cc | 6 +-
|
||||
src/StoreClient.h | 64 ++++++++++--
|
||||
src/store_client.cc | 177 ++++++++++++++++++++++-----------
|
||||
src/store_swapin.cc | 2 +-
|
||||
src/tests/stub_store_client.cc | 5 +-
|
||||
5 files changed, 186 insertions(+), 68 deletions(-)
|
||||
|
||||
diff --git a/src/MemObject.cc b/src/MemObject.cc
|
||||
index df7791f..4ba63cc 100644
|
||||
--- a/src/MemObject.cc
|
||||
+++ b/src/MemObject.cc
|
||||
@@ -196,8 +196,8 @@ struct LowestMemReader : public unary_function<store_client, void> {
|
||||
LowestMemReader(int64_t seed):current(seed) {}
|
||||
|
||||
void operator() (store_client const &x) {
|
||||
- if (x.memReaderHasLowerOffset(current))
|
||||
- current = x.copyInto.offset;
|
||||
+ if (x.getType() == STORE_MEM_CLIENT)
|
||||
+ current = std::min(current, x.readOffset());
|
||||
}
|
||||
|
||||
int64_t current;
|
||||
@@ -492,7 +492,7 @@ MemObject::mostBytesAllowed() const
|
||||
|
||||
#endif
|
||||
|
||||
- j = sc->delayId.bytesWanted(0, sc->copyInto.length);
|
||||
+ j = sc->bytesWanted();
|
||||
|
||||
if (j > jmax) {
|
||||
jmax = j;
|
||||
diff --git a/src/StoreClient.h b/src/StoreClient.h
|
||||
index 65472d8..457844a 100644
|
||||
--- a/src/StoreClient.h
|
||||
+++ b/src/StoreClient.h
|
||||
@@ -12,6 +12,7 @@
|
||||
#include "dlink.h"
|
||||
#include "StoreIOBuffer.h"
|
||||
#include "StoreIOState.h"
|
||||
+#include "base/AsyncCall.h"
|
||||
|
||||
typedef void STCB(void *, StoreIOBuffer); /* store callback */
|
||||
|
||||
@@ -39,14 +40,32 @@ class store_client
|
||||
public:
|
||||
store_client(StoreEntry *);
|
||||
~store_client();
|
||||
- bool memReaderHasLowerOffset(int64_t) const;
|
||||
+
|
||||
+ /// An offset into the stored response bytes, including the HTTP response
|
||||
+ /// headers (if any). Note that this offset does not include Store entry
|
||||
+ /// metadata, because it is not a part of the stored response.
|
||||
+ /// \retval 0 means the client wants to read HTTP response headers.
|
||||
+ /// \retval +N the response byte that the client wants to read next.
|
||||
+ /// \retval -N should not occur.
|
||||
+ // TODO: Callers do not expect negative offset. Verify that the return
|
||||
+ // value cannot be negative and convert to unsigned in this case.
|
||||
+ int64_t readOffset() const { return copyInto.offset; }
|
||||
+
|
||||
int getType() const;
|
||||
- void fail();
|
||||
- void callback(ssize_t len, bool error = false);
|
||||
+
|
||||
+ /// React to the end of reading the response from disk. There will be no
|
||||
+ /// more readHeader() and readBody() callbacks for the current storeRead()
|
||||
+ /// swapin after this notification.
|
||||
+ void noteSwapInDone(bool error);
|
||||
+
|
||||
void doCopy (StoreEntry *e);
|
||||
void readHeader(const char *buf, ssize_t len);
|
||||
void readBody(const char *buf, ssize_t len);
|
||||
+
|
||||
+ /// Request StoreIOBuffer-described response data via an asynchronous STCB
|
||||
+ /// callback. At most one outstanding request is allowed per store_client.
|
||||
void copy(StoreEntry *, StoreIOBuffer, STCB *, void *);
|
||||
+
|
||||
void dumpStats(MemBuf * output, int clientNumber) const;
|
||||
|
||||
int64_t cmp_offset;
|
||||
@@ -59,19 +78,29 @@ public:
|
||||
StoreIOState::Pointer swapin_sio;
|
||||
|
||||
struct {
|
||||
+ /// whether we are expecting a response to be swapped in from disk
|
||||
+ /// (i.e. whether async storeRead() is currently in progress)
|
||||
+ // TODO: a better name reflecting the 'in' scope of the flag
|
||||
bool disk_io_pending;
|
||||
+
|
||||
+ /// whether the store_client::doCopy()-initiated STCB sequence is
|
||||
+ /// currently in progress
|
||||
bool store_copying;
|
||||
- bool copy_event_pending;
|
||||
} flags;
|
||||
|
||||
#if USE_DELAY_POOLS
|
||||
DelayId delayId;
|
||||
+
|
||||
+ /// The maximum number of bytes the Store client can read/copy next without
|
||||
+ /// overflowing its buffer and without violating delay pool limits. Store
|
||||
+ /// I/O is not rate-limited, but we assume that the same number of bytes may
|
||||
+ /// be read from the Squid-to-server connection that may be rate-limited.
|
||||
+ int bytesWanted() const;
|
||||
+
|
||||
void setDelayId(DelayId delay_id);
|
||||
#endif
|
||||
|
||||
dlink_node node;
|
||||
- /* Below here is private - do no alter outside storeClient calls */
|
||||
- StoreIOBuffer copyInto;
|
||||
|
||||
private:
|
||||
bool moreToSend() const;
|
||||
@@ -83,9 +112,25 @@ private:
|
||||
bool startSwapin();
|
||||
bool unpackHeader(char const *buf, ssize_t len);
|
||||
|
||||
+ void fail();
|
||||
+ void callback(ssize_t);
|
||||
+ void noteCopiedBytes(size_t);
|
||||
+ void noteEof();
|
||||
+ void noteNews();
|
||||
+ void finishCallback();
|
||||
+ static void FinishCallback(store_client *);
|
||||
+
|
||||
int type;
|
||||
bool object_ok;
|
||||
|
||||
+ /// Storage and metadata associated with the current copy() request. Ought
|
||||
+ /// to be ignored when not answering a copy() request.
|
||||
+ StoreIOBuffer copyInto;
|
||||
+
|
||||
+ /// The number of bytes loaded from Store into copyInto while answering the
|
||||
+ /// current copy() request. Ought to be ignored when not answering.
|
||||
+ size_t copiedSize;
|
||||
+
|
||||
/* Until we finish stuffing code into store_client */
|
||||
|
||||
public:
|
||||
@@ -94,9 +139,16 @@ public:
|
||||
Callback ():callback_handler(NULL), callback_data(NULL) {}
|
||||
|
||||
Callback (STCB *, void *);
|
||||
+
|
||||
+ /// Whether the copy() answer is needed/expected (by the client) and has
|
||||
+ /// not been computed (by us). False during (asynchronous) answer
|
||||
+ /// delivery to the STCB callback_handler.
|
||||
bool pending() const;
|
||||
STCB *callback_handler;
|
||||
void *callback_data;
|
||||
+
|
||||
+ /// a scheduled asynchronous finishCallback() call (or nil)
|
||||
+ AsyncCall::Pointer notifier;
|
||||
} _callback;
|
||||
};
|
||||
|
||||
diff --git a/src/store_client.cc b/src/store_client.cc
|
||||
index 1b54f04..207c96b 100644
|
||||
--- a/src/store_client.cc
|
||||
+++ b/src/store_client.cc
|
||||
@@ -9,6 +9,7 @@
|
||||
/* DEBUG: section 90 Storage Manager Client-Side Interface */
|
||||
|
||||
#include "squid.h"
|
||||
+#include "base/AsyncCbdataCalls.h"
|
||||
#include "event.h"
|
||||
#include "globals.h"
|
||||
#include "HttpReply.h"
|
||||
@@ -39,17 +40,10 @@
|
||||
static StoreIOState::STRCB storeClientReadBody;
|
||||
static StoreIOState::STRCB storeClientReadHeader;
|
||||
static void storeClientCopy2(StoreEntry * e, store_client * sc);
|
||||
-static EVH storeClientCopyEvent;
|
||||
static bool CheckQuickAbortIsReasonable(StoreEntry * entry);
|
||||
|
||||
CBDATA_CLASS_INIT(store_client);
|
||||
|
||||
-bool
|
||||
-store_client::memReaderHasLowerOffset(int64_t anOffset) const
|
||||
-{
|
||||
- return getType() == STORE_MEM_CLIENT && copyInto.offset < anOffset;
|
||||
-}
|
||||
-
|
||||
int
|
||||
store_client::getType() const
|
||||
{
|
||||
@@ -104,22 +98,41 @@ storeClientListAdd(StoreEntry * e, void *data)
|
||||
return sc;
|
||||
}
|
||||
|
||||
+/// schedules asynchronous STCB call to relay disk or memory read results
|
||||
+/// \param outcome an error signal (if negative), an EOF signal (if zero), or the number of bytes read
|
||||
+void
|
||||
+store_client::callback(const ssize_t outcome)
|
||||
+{
|
||||
+ if (outcome > 0)
|
||||
+ return noteCopiedBytes(outcome);
|
||||
+
|
||||
+ if (outcome < 0)
|
||||
+ return fail();
|
||||
+
|
||||
+ noteEof();
|
||||
+}
|
||||
+/// finishCallback() wrapper; TODO: Add NullaryMemFunT for non-jobs.
|
||||
void
|
||||
-store_client::callback(ssize_t sz, bool error)
|
||||
+store_client::FinishCallback(store_client * const sc)
|
||||
{
|
||||
- size_t bSz = 0;
|
||||
+ sc->finishCallback();
|
||||
+}
|
||||
|
||||
- if (sz >= 0 && !error)
|
||||
- bSz = sz;
|
||||
+/// finishes a copy()-STCB sequence by synchronously calling STCB
|
||||
+void
|
||||
+store_client::finishCallback()
|
||||
+{
|
||||
+ Assure(_callback.callback_handler);
|
||||
+ Assure(_callback.notifier);
|
||||
|
||||
- StoreIOBuffer result(bSz, 0 ,copyInto.data);
|
||||
+ // callers are not ready to handle a content+error combination
|
||||
+ Assure(object_ok || !copiedSize);
|
||||
|
||||
- if (sz < 0 || error)
|
||||
- result.flags.error = 1;
|
||||
+ StoreIOBuffer result(copiedSize, copyInto.offset, copyInto.data);
|
||||
+ result.flags.error = object_ok ? 0 : 1;
|
||||
+ copiedSize = 0;
|
||||
|
||||
- result.offset = cmp_offset;
|
||||
- assert(_callback.pending());
|
||||
- cmp_offset = copyInto.offset + bSz;
|
||||
+ cmp_offset = result.offset + result.length;
|
||||
STCB *temphandler = _callback.callback_handler;
|
||||
void *cbdata = _callback.callback_data;
|
||||
_callback = Callback(NULL, NULL);
|
||||
@@ -131,18 +144,24 @@ store_client::callback(ssize_t sz, bool error)
|
||||
cbdataReferenceDone(cbdata);
|
||||
}
|
||||
|
||||
-static void
|
||||
-storeClientCopyEvent(void *data)
|
||||
+/// schedules asynchronous STCB call to relay a successful disk or memory read
|
||||
+/// \param bytesCopied the number of response bytes copied into copyInto
|
||||
+void
|
||||
+store_client::noteCopiedBytes(const size_t bytesCopied)
|
||||
{
|
||||
- store_client *sc = (store_client *)data;
|
||||
- debugs(90, 3, "storeClientCopyEvent: Running");
|
||||
- assert (sc->flags.copy_event_pending);
|
||||
- sc->flags.copy_event_pending = false;
|
||||
-
|
||||
- if (!sc->_callback.pending())
|
||||
- return;
|
||||
+ debugs(90, 5, bytesCopied);
|
||||
+ Assure(bytesCopied > 0);
|
||||
+ Assure(!copiedSize);
|
||||
+ copiedSize = bytesCopied;
|
||||
+ noteNews();
|
||||
+}
|
||||
|
||||
- storeClientCopy2(sc->entry, sc);
|
||||
+void
|
||||
+store_client::noteEof()
|
||||
+{
|
||||
+ debugs(90, 5, copiedSize);
|
||||
+ Assure(!copiedSize);
|
||||
+ noteNews();
|
||||
}
|
||||
|
||||
store_client::store_client(StoreEntry *e) :
|
||||
@@ -152,11 +171,11 @@ store_client::store_client(StoreEntry *e) :
|
||||
#endif
|
||||
entry(e),
|
||||
type(e->storeClientType()),
|
||||
- object_ok(true)
|
||||
+ object_ok(true),
|
||||
+ copiedSize(0)
|
||||
{
|
||||
flags.disk_io_pending = false;
|
||||
flags.store_copying = false;
|
||||
- flags.copy_event_pending = false;
|
||||
++ entry->refcount;
|
||||
|
||||
if (getType() == STORE_DISK_CLIENT) {
|
||||
@@ -272,17 +291,11 @@ static void
|
||||
storeClientCopy2(StoreEntry * e, store_client * sc)
|
||||
{
|
||||
/* reentrancy not allowed - note this could lead to
|
||||
- * dropped events
|
||||
+ * dropped notifications about response data availability
|
||||
*/
|
||||
|
||||
- if (sc->flags.copy_event_pending) {
|
||||
- return;
|
||||
- }
|
||||
-
|
||||
if (sc->flags.store_copying) {
|
||||
- sc->flags.copy_event_pending = true;
|
||||
- debugs(90, 3, "storeClientCopy2: Queueing storeClientCopyEvent()");
|
||||
- eventAdd("storeClientCopyEvent", storeClientCopyEvent, sc, 0.0, 0);
|
||||
+ debugs(90, 3, "prevented recursive copying for " << *e);
|
||||
return;
|
||||
}
|
||||
|
||||
@@ -295,21 +308,16 @@ storeClientCopy2(StoreEntry * e, store_client * sc)
|
||||
* if the peer aborts, we want to give the client(s)
|
||||
* everything we got before the abort condition occurred.
|
||||
*/
|
||||
- /* Warning: doCopy may indirectly free itself in callbacks,
|
||||
- * hence the lock to keep it active for the duration of
|
||||
- * this function
|
||||
- * XXX: Locking does not prevent calling sc destructor (it only prevents
|
||||
- * freeing sc memory) so sc may become invalid from C++ p.o.v.
|
||||
- */
|
||||
- CbcPointer<store_client> tmpLock = sc;
|
||||
- assert (!sc->flags.store_copying);
|
||||
sc->doCopy(e);
|
||||
- assert(!sc->flags.store_copying);
|
||||
}
|
||||
|
||||
void
|
||||
store_client::doCopy(StoreEntry *anEntry)
|
||||
{
|
||||
+ Assure(_callback.pending());
|
||||
+ Assure(!flags.disk_io_pending);
|
||||
+ Assure(!flags.store_copying);
|
||||
+
|
||||
assert (anEntry == entry);
|
||||
flags.store_copying = true;
|
||||
MemObject *mem = entry->mem_obj;
|
||||
@@ -321,7 +329,7 @@ store_client::doCopy(StoreEntry *anEntry)
|
||||
if (!moreToSend()) {
|
||||
/* There is no more to send! */
|
||||
debugs(33, 3, HERE << "There is no more to send!");
|
||||
- callback(0);
|
||||
+ noteEof();
|
||||
flags.store_copying = false;
|
||||
return;
|
||||
}
|
||||
@@ -382,6 +390,16 @@ store_client::startSwapin()
|
||||
}
|
||||
}
|
||||
|
||||
+void
|
||||
+store_client::noteSwapInDone(const bool error)
|
||||
+{
|
||||
+ Assure(_callback.pending());
|
||||
+ if (error)
|
||||
+ fail();
|
||||
+ else
|
||||
+ noteEof();
|
||||
+}
|
||||
+
|
||||
void
|
||||
store_client::scheduleRead()
|
||||
{
|
||||
@@ -421,7 +439,7 @@ store_client::scheduleMemRead()
|
||||
/* What the client wants is in memory */
|
||||
/* Old style */
|
||||
debugs(90, 3, "store_client::doCopy: Copying normal from memory");
|
||||
- size_t sz = entry->mem_obj->data_hdr.copy(copyInto);
|
||||
+ const auto sz = entry->mem_obj->data_hdr.copy(copyInto); // may be <= 0 per copy() API
|
||||
callback(sz);
|
||||
flags.store_copying = false;
|
||||
}
|
||||
@@ -493,7 +511,19 @@ store_client::readBody(const char *, ssize_t len)
|
||||
void
|
||||
store_client::fail()
|
||||
{
|
||||
+ debugs(90, 3, (object_ok ? "once" : "again"));
|
||||
+ if (!object_ok)
|
||||
+ return; // we failed earlier; nothing to do now
|
||||
+
|
||||
object_ok = false;
|
||||
+
|
||||
+ noteNews();
|
||||
+}
|
||||
+
|
||||
+/// if necessary and possible, informs the Store reader about copy() result
|
||||
+void
|
||||
+store_client::noteNews()
|
||||
+{
|
||||
/* synchronous open failures callback from the store,
|
||||
* before startSwapin detects the failure.
|
||||
* TODO: fix this inconsistent behaviour - probably by
|
||||
@@ -501,8 +531,20 @@ store_client::fail()
|
||||
* not synchronous
|
||||
*/
|
||||
|
||||
- if (_callback.pending())
|
||||
- callback(0, true);
|
||||
+ if (!_callback.callback_handler) {
|
||||
+ debugs(90, 5, "client lost interest");
|
||||
+ return;
|
||||
+ }
|
||||
+
|
||||
+ if (_callback.notifier) {
|
||||
+ debugs(90, 5, "earlier news is being delivered by " << _callback.notifier);
|
||||
+ return;
|
||||
+ }
|
||||
+
|
||||
+ _callback.notifier = asyncCall(90, 4, "store_client::FinishCallback", cbdataDialer(store_client::FinishCallback, this));
|
||||
+ ScheduleCallHere(_callback.notifier);
|
||||
+
|
||||
+ Assure(!_callback.pending());
|
||||
}
|
||||
|
||||
static void
|
||||
@@ -673,10 +715,12 @@ storeUnregister(store_client * sc, StoreEntry * e, void *data)
|
||||
++statCounter.swap.ins;
|
||||
}
|
||||
|
||||
- if (sc->_callback.pending()) {
|
||||
- /* callback with ssize = -1 to indicate unexpected termination */
|
||||
- debugs(90, 3, "store_client for " << *e << " has a callback");
|
||||
- sc->fail();
|
||||
+ if (sc->_callback.callback_handler || sc->_callback.notifier) {
|
||||
+ debugs(90, 3, "forgetting store_client callback for " << *e);
|
||||
+ // Do not notify: Callers want to stop copying and forget about this
|
||||
+ // pending copy request. Some would mishandle a notification from here.
|
||||
+ if (sc->_callback.notifier)
|
||||
+ sc->_callback.notifier->cancel("storeUnregister");
|
||||
}
|
||||
|
||||
#if STORE_CLIENT_LIST_DEBUG
|
||||
@@ -684,6 +728,8 @@ storeUnregister(store_client * sc, StoreEntry * e, void *data)
|
||||
|
||||
#endif
|
||||
|
||||
+ // XXX: We might be inside sc store_client method somewhere up the call
|
||||
+ // stack. TODO: Convert store_client to AsyncJob to make destruction async.
|
||||
delete sc;
|
||||
|
||||
assert(e->locked());
|
||||
@@ -740,6 +786,16 @@ StoreEntry::invokeHandlers()
|
||||
|
||||
if (sc->flags.disk_io_pending)
|
||||
continue;
|
||||
+ if (sc->flags.store_copying)
|
||||
+ continue;
|
||||
+
|
||||
+ // XXX: If invokeHandlers() is (indirectly) called from a store_client
|
||||
+ // method, then the above three conditions may not be sufficient to
|
||||
+ // prevent us from reentering the same store_client object! This
|
||||
+ // probably does not happen in the current code, but no observed
|
||||
+ // invariant prevents this from (accidentally) happening in the future.
|
||||
+
|
||||
+ // TODO: Convert store_client into AsyncJob; make this call asynchronous
|
||||
|
||||
storeClientCopy2(this, sc);
|
||||
}
|
||||
@@ -864,8 +920,8 @@ store_client::dumpStats(MemBuf * output, int clientNumber) const
|
||||
if (flags.store_copying)
|
||||
output->append(" store_copying", 14);
|
||||
|
||||
- if (flags.copy_event_pending)
|
||||
- output->append(" copy_event_pending", 19);
|
||||
+ if (_callback.notifier)
|
||||
+ output->append(" notifying", 10);
|
||||
|
||||
output->append("\n",1);
|
||||
}
|
||||
@@ -873,12 +929,19 @@ store_client::dumpStats(MemBuf * output, int clientNumber) const
|
||||
bool
|
||||
store_client::Callback::pending() const
|
||||
{
|
||||
- return callback_handler && callback_data;
|
||||
+ return callback_handler && !notifier;
|
||||
}
|
||||
|
||||
store_client::Callback::Callback(STCB *function, void *data) : callback_handler(function), callback_data (data) {}
|
||||
|
||||
#if USE_DELAY_POOLS
|
||||
+int
|
||||
+store_client::bytesWanted() const
|
||||
+{
|
||||
+ // TODO: To avoid using stale copyInto, return zero if !_callback.pending()?
|
||||
+ return delayId.bytesWanted(0, copyInto.length);
|
||||
+}
|
||||
+
|
||||
void
|
||||
store_client::setDelayId(DelayId delay_id)
|
||||
{
|
||||
diff --git a/src/store_swapin.cc b/src/store_swapin.cc
|
||||
index a05d7e3..cd32e94 100644
|
||||
--- a/src/store_swapin.cc
|
||||
+++ b/src/store_swapin.cc
|
||||
@@ -56,7 +56,7 @@ storeSwapInFileClosed(void *data, int errflag, StoreIOState::Pointer)
|
||||
|
||||
if (sc->_callback.pending()) {
|
||||
assert (errflag <= 0);
|
||||
- sc->callback(0, errflag ? true : false);
|
||||
+ sc->noteSwapInDone(errflag);
|
||||
}
|
||||
|
||||
++statCounter.swap.ins;
|
||||
diff --git a/src/tests/stub_store_client.cc b/src/tests/stub_store_client.cc
|
||||
index 2a13874..4a73863 100644
|
||||
--- a/src/tests/stub_store_client.cc
|
||||
+++ b/src/tests/stub_store_client.cc
|
||||
@@ -34,7 +34,10 @@ void storeLogOpen(void) STUB
|
||||
void storeDigestInit(void) STUB
|
||||
void storeRebuildStart(void) STUB
|
||||
void storeReplSetup(void) STUB
|
||||
-bool store_client::memReaderHasLowerOffset(int64_t anOffset) const STUB_RETVAL(false)
|
||||
+void store_client::noteSwapInDone(bool) STUB
|
||||
+#if USE_DELAY_POOLS
|
||||
+int store_client::bytesWanted() const STUB_RETVAL(0)
|
||||
+#endif
|
||||
void store_client::dumpStats(MemBuf * output, int clientNumber) const STUB
|
||||
int store_client::getType() const STUB_RETVAL(0)
|
||||
|
||||
--
|
||||
2.39.3
|
||||
|
@ -1,119 +0,0 @@
|
||||
From af18cb04f07555f49daef982c8c21459bfbe388c Mon Sep 17 00:00:00 2001
|
||||
From: Alex Rousskov <rousskov@measurement-factory.com>
|
||||
Date: Thu, 23 Nov 2023 18:27:24 +0000
|
||||
Subject: [PATCH 3/7] Bug 5309: frequent "lowestOffset () <= target_offset"
|
||||
assertion (#1561)
|
||||
|
||||
Recent commit 122a6e3 left store_client::readOffset() unchanged but
|
||||
should have adjusted it to match changed copyInto.offset semantics:
|
||||
Starting with that commit, storeClientCopy() callers supply HTTP
|
||||
response _body_ offset rather than HTTP response offset.
|
||||
....
|
||||
This bug decreased readOffset() values (by the size of stored HTTP
|
||||
response headers), effectively telling Store that we are not yet done
|
||||
with some of the MemObject/mem_hdr bytes. This bug could cause slightly
|
||||
higher transaction memory usage because the same response bytes are
|
||||
trimmed later. This bug should not have caused any assertions.
|
||||
....
|
||||
However, the old mem_hdr::freeDataUpto() code that uses readOffset() is
|
||||
also broken -- the assertion in that method only "works" when
|
||||
readOffset() returns values matching a memory node boundary. The smaller
|
||||
values returned by buggy readOffset() triggered buggy assertions.
|
||||
....
|
||||
This minimal fix removes the recent store_client::readOffset() bug
|
||||
described above. We will address old mem_hdr problems separately.
|
||||
|
||||
Modified-by: Alex Burmashev <alexander.burmashev@oracle.com>
|
||||
Signed-off-by: Alex Burmashev <alexander.burmashev@oracle.com>
|
||||
---
|
||||
src/MemObject.cc | 2 +-
|
||||
src/StoreClient.h | 19 ++++++++++---------
|
||||
src/store_client.cc | 13 +++++++++++++
|
||||
3 files changed, 24 insertions(+), 10 deletions(-)
|
||||
|
||||
diff --git a/src/MemObject.cc b/src/MemObject.cc
|
||||
index d7aaf5e..650d3fd 100644
|
||||
--- a/src/MemObject.cc
|
||||
+++ b/src/MemObject.cc
|
||||
@@ -197,7 +197,7 @@ struct LowestMemReader : public unary_function<store_client, void> {
|
||||
|
||||
void operator() (store_client const &x) {
|
||||
if (x.getType() == STORE_MEM_CLIENT)
|
||||
- current = std::min(current, x.readOffset());
|
||||
+ current = std::min(current, x.discardableHttpEnd());
|
||||
}
|
||||
|
||||
int64_t current;
|
||||
diff --git a/src/StoreClient.h b/src/StoreClient.h
|
||||
index 1d90e5a..0524776 100644
|
||||
--- a/src/StoreClient.h
|
||||
+++ b/src/StoreClient.h
|
||||
@@ -54,15 +54,8 @@ public:
|
||||
store_client(StoreEntry *);
|
||||
~store_client();
|
||||
|
||||
- /// An offset into the stored response bytes, including the HTTP response
|
||||
- /// headers (if any). Note that this offset does not include Store entry
|
||||
- /// metadata, because it is not a part of the stored response.
|
||||
- /// \retval 0 means the client wants to read HTTP response headers.
|
||||
- /// \retval +N the response byte that the client wants to read next.
|
||||
- /// \retval -N should not occur.
|
||||
- // TODO: Callers do not expect negative offset. Verify that the return
|
||||
- // value cannot be negative and convert to unsigned in this case.
|
||||
- int64_t readOffset() const { return copyInto.offset; }
|
||||
+ /// the client will not use HTTP response bytes with lower offsets (if any)
|
||||
+ auto discardableHttpEnd() const { return discardableHttpEnd_; }
|
||||
|
||||
int getType() const;
|
||||
|
||||
@@ -156,8 +149,16 @@ private:
|
||||
|
||||
/// Storage and metadata associated with the current copy() request. Ought
|
||||
/// to be ignored when not answering a copy() request.
|
||||
+ /// * copyInto.offset is the requested HTTP response body offset;
|
||||
+ /// * copyInto.data is the client-owned, client-provided result buffer;
|
||||
+ /// * copyInto.length is the size of the .data result buffer;
|
||||
+ /// * copyInto.flags are unused by this class.
|
||||
StoreIOBuffer copyInto;
|
||||
|
||||
+ // TODO: Convert to uint64_t after fixing mem_hdr::endOffset() and friends.
|
||||
+ /// \copydoc discardableHttpEnd()
|
||||
+ int64_t discardableHttpEnd_ = 0;
|
||||
+
|
||||
/// the total number of finishCallback() calls
|
||||
uint64_t answers;
|
||||
|
||||
diff --git a/src/store_client.cc b/src/store_client.cc
|
||||
index 1731c4c..383aac8 100644
|
||||
--- a/src/store_client.cc
|
||||
+++ b/src/store_client.cc
|
||||
@@ -122,6 +122,16 @@ store_client::finishCallback()
|
||||
result = parsingBuffer->packBack();
|
||||
result.flags.error = object_ok ? 0 : 1;
|
||||
|
||||
+ // TODO: Move object_ok handling above into this `if` statement.
|
||||
+ if (object_ok) {
|
||||
+ // works for zero hdr_sz cases as well; see also: nextHttpReadOffset()
|
||||
+ discardableHttpEnd_ = NaturalSum<int64_t>(entry->mem().baseReply().hdr_sz, result.offset, result.length).value();
|
||||
+ } else {
|
||||
+ // object_ok is sticky, so we will not be able to use any response bytes
|
||||
+ discardableHttpEnd_ = entry->mem().endOffset();
|
||||
+ }
|
||||
+ debugs(90, 7, "with " << result << "; discardableHttpEnd_=" << discardableHttpEnd_);
|
||||
+
|
||||
// no HTTP headers and no body bytes (but not because there was no space)
|
||||
atEof_ = !sendingHttpHeaders() && !result.length && copyInto.length;
|
||||
|
||||
@@ -220,6 +230,9 @@ store_client::copy(StoreEntry * anEntry,
|
||||
|
||||
parsingBuffer.emplace(copyInto);
|
||||
|
||||
+ discardableHttpEnd_ = nextHttpReadOffset();
|
||||
+ debugs(90, 7, "discardableHttpEnd_=" << discardableHttpEnd_);
|
||||
+
|
||||
static bool copying (false);
|
||||
assert (!copying);
|
||||
copying = true;
|
||||
--
|
||||
2.39.3
|
||||
|
@ -1,67 +0,0 @@
|
||||
From 422272d78399d5fb2fc340281611961fc7c528e7 Mon Sep 17 00:00:00 2001
|
||||
From: Alex Rousskov <rousskov@measurement-factory.com>
|
||||
Date: Thu, 23 Nov 2023 18:27:45 +0000
|
||||
Subject: [PATCH 4/7] Remove mem_hdr::freeDataUpto() assertion (#1562)
|
||||
|
||||
stmem.cc:98: "lowestOffset () <= target_offset"
|
||||
....
|
||||
The assertion is conceptually wrong: The given target_offset parameter
|
||||
may have any value; that value does not have to correlate with mem_hdr
|
||||
state in any way. It is freeDataUpto() job to preserve nodes at or above
|
||||
the given offset and (arguably optionally) remove nodes below it, but
|
||||
the assertion does not actually validate that freeDataUpdo() did that.
|
||||
....
|
||||
The old mem_hdr::freeDataUpto() assertion incorrectly assumed that,
|
||||
after zero or more unneeded memory nodes were freed, the remaining
|
||||
memory area never starts after the given target_offset parameter. That
|
||||
assumption fails in at least two use cases, both using target_offset
|
||||
values that do not belong to any existing or future mem_hdr node:
|
||||
....
|
||||
1. target_offset is points to the left of the first node. freeDataUpto()
|
||||
correctly keeps all memory nodes in such calls, but then asserts. For
|
||||
example, calling freeDataUpto(0) when mem_hdr has bytes [100,199)
|
||||
triggers this incorrect assertion.
|
||||
....
|
||||
2. target_offset is in the gap between two nodes. For example, calling
|
||||
freeDataUpto(2000) when mem_hdr contains two nodes: [0,1000) and
|
||||
[3000,3003) will trigger this assertion (as happened in Bug 5309).
|
||||
Such gaps are very common for HTTP 206 responses with a Content-Range
|
||||
header because such responses often specify a range that does not
|
||||
start with zero and create a gap after the node(s) with HTTP headers.
|
||||
....
|
||||
Bugs notwithstanding, it is unlikely that relevant calls exist today,
|
||||
but they certainly could be added, especially when freeDataUpto() stops
|
||||
preserving the last unused node. The current "avoid change to [some
|
||||
unidentified] part of code" hoarding excuse should not last forever.
|
||||
....
|
||||
Prior to commit 122a6e3, Squid did not (frequently) assert in gap cases:
|
||||
Callers first give target_offset 0 (which results in freeDataUpto()
|
||||
doing nothing, keeping the header node(s)) and then they give
|
||||
target_offset matching the beginning of the first body node (which
|
||||
results in freeDataUpto() freeing the header nodes(s) and increasing
|
||||
lowerOffset() from zero to target_offset). A bug in commit 122a6e3
|
||||
lowered target_offset a bit, placing target_offset in the gap and
|
||||
triggering frequent (and incorrect) assertions (Bug 5309).
|
||||
|
||||
Modified-by: Alex Burmashev <alexander.burmashev@oracle.com>
|
||||
Signed-off-by: Alex Burmashev <alexander.burmashev@oracle.com>
|
||||
---
|
||||
src/stmem.cc | 2 --
|
||||
1 file changed, 2 deletions(-)
|
||||
|
||||
diff --git a/src/stmem.cc b/src/stmem.cc
|
||||
index d117c15..b627005 100644
|
||||
--- a/src/stmem.cc
|
||||
+++ b/src/stmem.cc
|
||||
@@ -95,8 +95,6 @@ mem_hdr::freeDataUpto(int64_t target_offset)
|
||||
break;
|
||||
}
|
||||
|
||||
- assert (lowestOffset () <= target_offset);
|
||||
-
|
||||
return lowestOffset ();
|
||||
}
|
||||
|
||||
--
|
||||
2.39.3
|
||||
|
@ -1,210 +0,0 @@
|
||||
From 5df95b5923de244eaf2ddccf980d5f28d7114b1f Mon Sep 17 00:00:00 2001
|
||||
From: Alex Burmashev <alexander.burmashev@oracle.com>
|
||||
Date: Thu, 7 Dec 2023 18:01:47 +0000
|
||||
Subject: [PATCH 5/7] Backport Add Assure() as a replacement for problematic
|
||||
Must()
|
||||
|
||||
This is a partial backport of
|
||||
b9a1bbfbc531359a87647271a282edff9ccdd206
|
||||
b8ae064d94784934b3402e5db015246d1b1ca658
|
||||
|
||||
Needed for CVE CVE-2023-5824 fix
|
||||
|
||||
Signed-off-by: Alex Burmashev <alexander.burmashev@oracle.com>
|
||||
---
|
||||
src/HttpReply.cc | 1 +
|
||||
src/acl/Asn.cc | 1 +
|
||||
src/base/Assure.cc | 23 ++++++++++++++++++
|
||||
src/base/Assure.h | 51 ++++++++++++++++++++++++++++++++++++++++
|
||||
src/base/Makefile.am | 2 ++
|
||||
src/base/Makefile.in | 8 +++++--
|
||||
src/client_side_reply.cc | 1 +
|
||||
7 files changed, 85 insertions(+), 2 deletions(-)
|
||||
create mode 100644 src/base/Assure.cc
|
||||
create mode 100644 src/base/Assure.h
|
||||
|
||||
diff --git a/src/HttpReply.cc b/src/HttpReply.cc
|
||||
index af2bd4d..df5bcef 100644
|
||||
--- a/src/HttpReply.cc
|
||||
+++ b/src/HttpReply.cc
|
||||
@@ -10,6 +10,7 @@
|
||||
|
||||
#include "squid.h"
|
||||
#include "acl/AclSizeLimit.h"
|
||||
+#include "base/Assure.h"
|
||||
#include "acl/FilledChecklist.h"
|
||||
#include "base/EnumIterator.h"
|
||||
#include "globals.h"
|
||||
diff --git a/src/acl/Asn.cc b/src/acl/Asn.cc
|
||||
index ad450c0..bcedc82 100644
|
||||
--- a/src/acl/Asn.cc
|
||||
+++ b/src/acl/Asn.cc
|
||||
@@ -17,6 +17,7 @@
|
||||
#include "acl/SourceAsn.h"
|
||||
#include "acl/Strategised.h"
|
||||
#include "base/CharacterSet.h"
|
||||
+#include "base/Assure.h"
|
||||
#include "FwdState.h"
|
||||
#include "HttpReply.h"
|
||||
#include "HttpRequest.h"
|
||||
diff --git a/src/base/Assure.cc b/src/base/Assure.cc
|
||||
new file mode 100644
|
||||
index 0000000..b09b848
|
||||
--- /dev/null
|
||||
+++ b/src/base/Assure.cc
|
||||
@@ -0,0 +1,23 @@
|
||||
+/*
|
||||
+ * Copyright (C) 1996-2023 The Squid Software Foundation and contributors
|
||||
+ *
|
||||
+ * Squid software is distributed under GPLv2+ license and includes
|
||||
+ * contributions from numerous individuals and organizations.
|
||||
+ * Please see the COPYING and CONTRIBUTORS files for details.
|
||||
+ */
|
||||
+
|
||||
+#include "squid.h"
|
||||
+#include "base/Assure.h"
|
||||
+#include "base/TextException.h"
|
||||
+#include "sbuf/Stream.h"
|
||||
+
|
||||
+[[ noreturn ]] void
|
||||
+ReportAndThrow_(const int debugLevel, const char *description, const SourceLocation &location)
|
||||
+{
|
||||
+ const TextException ex(description, location);
|
||||
+ const auto label = debugLevel <= DBG_IMPORTANT ? "ERROR: Squid BUG: " : "";
|
||||
+ // TODO: Consider also printing the number of BUGs reported so far. It would
|
||||
+ // require GC, but we could even print the number of same-location reports.
|
||||
+ debugs(0, debugLevel, label << ex);
|
||||
+ throw ex;
|
||||
+}
|
||||
diff --git a/src/base/Assure.h b/src/base/Assure.h
|
||||
new file mode 100644
|
||||
index 0000000..650c204
|
||||
--- /dev/null
|
||||
+++ b/src/base/Assure.h
|
||||
@@ -0,0 +1,51 @@
|
||||
+/*
|
||||
+ * Copyright (C) 1996-2023 The Squid Software Foundation and contributors
|
||||
+ *
|
||||
+ * Squid software is distributed under GPLv2+ license and includes
|
||||
+ * contributions from numerous individuals and organizations.
|
||||
+ * Please see the COPYING and CONTRIBUTORS files for details.
|
||||
+ */
|
||||
+
|
||||
+#ifndef SQUID_SRC_BASE_ASSURE_H
|
||||
+#define SQUID_SRC_BASE_ASSURE_H
|
||||
+
|
||||
+#include "base/Here.h"
|
||||
+
|
||||
+/// Reports the description (at the given debugging level) and throws
|
||||
+/// the corresponding exception. Reduces compiled code size of Assure() and
|
||||
+/// Must() callers. Do not call directly; use Assure() instead.
|
||||
+/// \param description explains the condition (i.e. what MUST happen)
|
||||
+[[ noreturn ]] void ReportAndThrow_(int debugLevel, const char *description, const SourceLocation &);
|
||||
+
|
||||
+/// Calls ReportAndThrow() if needed. Reduces caller code duplication.
|
||||
+/// Do not call directly; use Assure() instead.
|
||||
+/// \param description c-string explaining the condition (i.e. what MUST happen)
|
||||
+#define Assure_(debugLevel, condition, description, location) \
|
||||
+ while (!(condition)) \
|
||||
+ ReportAndThrow_((debugLevel), (description), (location))
|
||||
+
|
||||
+#if !defined(NDEBUG)
|
||||
+
|
||||
+/// Like assert() but throws an exception instead of aborting the process. Use
|
||||
+/// this macro to detect code logic mistakes (i.e. bugs) where aborting the
|
||||
+/// current AsyncJob or a similar task is unlikely to jeopardize Squid service
|
||||
+/// integrity. For example, this macro is _not_ appropriate for detecting bugs
|
||||
+/// that indicate a dangerous global state corruption which may go unnoticed by
|
||||
+/// other jobs after the current job or task is aborted.
|
||||
+#define Assure(condition) \
|
||||
+ Assure2((condition), #condition)
|
||||
+
|
||||
+/// Like Assure() but allows the caller to customize the exception message.
|
||||
+/// \param description string literal describing the condition (i.e. what MUST happen)
|
||||
+#define Assure2(condition, description) \
|
||||
+ Assure_(0, (condition), ("assurance failed: " description), Here())
|
||||
+
|
||||
+#else
|
||||
+
|
||||
+/* do-nothing implementations for NDEBUG builds */
|
||||
+#define Assure(condition) ((void)0)
|
||||
+#define Assure2(condition, description) ((void)0)
|
||||
+
|
||||
+#endif /* NDEBUG */
|
||||
+
|
||||
+#endif /* SQUID_SRC_BASE_ASSURE_H */
|
||||
diff --git a/src/base/Makefile.am b/src/base/Makefile.am
|
||||
index 9b0f4cf..c22dd0e 100644
|
||||
--- a/src/base/Makefile.am
|
||||
+++ b/src/base/Makefile.am
|
||||
@@ -19,6 +19,8 @@ libbase_la_SOURCES = \
|
||||
AsyncJob.cc \
|
||||
AsyncJob.h \
|
||||
AsyncJobCalls.h \
|
||||
+ Assure.cc \
|
||||
+ Assure.h \
|
||||
ByteCounter.h \
|
||||
CbcPointer.h \
|
||||
CbDataList.h \
|
||||
diff --git a/src/base/Makefile.in b/src/base/Makefile.in
|
||||
index 90a4f5b..f43e098 100644
|
||||
--- a/src/base/Makefile.in
|
||||
+++ b/src/base/Makefile.in
|
||||
@@ -163,7 +163,7 @@ CONFIG_CLEAN_FILES =
|
||||
CONFIG_CLEAN_VPATH_FILES =
|
||||
LTLIBRARIES = $(noinst_LTLIBRARIES)
|
||||
libbase_la_LIBADD =
|
||||
-am_libbase_la_OBJECTS = AsyncCall.lo AsyncCallQueue.lo AsyncJob.lo \
|
||||
+am_libbase_la_OBJECTS = AsyncCall.lo AsyncCallQueue.lo AsyncJob.lo Assure.lo \
|
||||
CharacterSet.lo File.lo Here.lo RegexPattern.lo \
|
||||
RunnersRegistry.lo TextException.lo
|
||||
libbase_la_OBJECTS = $(am_libbase_la_OBJECTS)
|
||||
@@ -187,7 +187,7 @@ DEFAULT_INCLUDES =
|
||||
depcomp = $(SHELL) $(top_srcdir)/cfgaux/depcomp
|
||||
am__maybe_remake_depfiles = depfiles
|
||||
am__depfiles_remade = ./$(DEPDIR)/AsyncCall.Plo \
|
||||
- ./$(DEPDIR)/AsyncCallQueue.Plo ./$(DEPDIR)/AsyncJob.Plo \
|
||||
+ ./$(DEPDIR)/AsyncCallQueue.Plo ./$(DEPDIR)/AsyncJob.Plo ./$(DEPDIR)/Assure.Plo \
|
||||
./$(DEPDIR)/CharacterSet.Plo ./$(DEPDIR)/File.Plo \
|
||||
./$(DEPDIR)/Here.Plo ./$(DEPDIR)/RegexPattern.Plo \
|
||||
./$(DEPDIR)/RunnersRegistry.Plo ./$(DEPDIR)/TextException.Plo
|
||||
@@ -737,6 +737,8 @@ libbase_la_SOURCES = \
|
||||
AsyncJob.cc \
|
||||
AsyncJob.h \
|
||||
AsyncJobCalls.h \
|
||||
+ Assure.cc \
|
||||
+ Assure.h \
|
||||
ByteCounter.h \
|
||||
CbcPointer.h \
|
||||
CbDataList.h \
|
||||
@@ -830,6 +832,7 @@ distclean-compile:
|
||||
@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/AsyncCall.Plo@am__quote@ # am--include-marker
|
||||
@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/AsyncCallQueue.Plo@am__quote@ # am--include-marker
|
||||
@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/AsyncJob.Plo@am__quote@ # am--include-marker
|
||||
+@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/Assure.Plo@am__quote@ # am--include-marker
|
||||
@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/CharacterSet.Plo@am__quote@ # am--include-marker
|
||||
@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/File.Plo@am__quote@ # am--include-marker
|
||||
@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/Here.Plo@am__quote@ # am--include-marker
|
||||
@@ -1224,6 +1227,7 @@ maintainer-clean: maintainer-clean-am
|
||||
-rm -f ./$(DEPDIR)/AsyncCall.Plo
|
||||
-rm -f ./$(DEPDIR)/AsyncCallQueue.Plo
|
||||
-rm -f ./$(DEPDIR)/AsyncJob.Plo
|
||||
+ -rm -f ./$(DEPDIR)/Assure.Plo
|
||||
-rm -f ./$(DEPDIR)/CharacterSet.Plo
|
||||
-rm -f ./$(DEPDIR)/File.Plo
|
||||
-rm -f ./$(DEPDIR)/Here.Plo
|
||||
diff --git a/src/client_side_reply.cc b/src/client_side_reply.cc
|
||||
index 861f4b4..470f4bc 100644
|
||||
--- a/src/client_side_reply.cc
|
||||
+++ b/src/client_side_reply.cc
|
||||
@@ -12,6 +12,7 @@
|
||||
#include "acl/FilledChecklist.h"
|
||||
#include "acl/Gadgets.h"
|
||||
#include "anyp/PortCfg.h"
|
||||
+#include "base/Assure.h"
|
||||
#include "client_side_reply.h"
|
||||
#include "errorpage.h"
|
||||
#include "ETag.h"
|
||||
--
|
||||
2.39.3
|
||||
|
@ -1,200 +0,0 @@
|
||||
From c24b9507e35fa43ddb40211a50fae9d58a0381bc Mon Sep 17 00:00:00 2001
|
||||
From: Alex Burmashev <alexander.burmashev@oracle.com>
|
||||
Date: Mon, 27 Nov 2023 11:47:40 +0000
|
||||
Subject: [PATCH 6/7] Backport additional functions for SquidMath
|
||||
|
||||
This includes some cherry-picks from
|
||||
b308d7e2ad02ae6622f380d94d2303446f5831a9 and later commits.
|
||||
|
||||
This is needed for CVE-2023-5824 fix
|
||||
|
||||
Signed-off-by: Alex Burmashev <alexander.burmashev@oracle.com>
|
||||
---
|
||||
src/SquidMath.h | 164 ++++++++++++++++++++++++++++++++++++++++++++++++
|
||||
1 file changed, 164 insertions(+)
|
||||
|
||||
diff --git a/src/SquidMath.h b/src/SquidMath.h
|
||||
index c70acd1..e5b6e58 100644
|
||||
--- a/src/SquidMath.h
|
||||
+++ b/src/SquidMath.h
|
||||
@@ -8,7 +8,11 @@
|
||||
|
||||
#ifndef _SQUID_SRC_SQUIDMATH_H
|
||||
#define _SQUID_SRC_SQUIDMATH_H
|
||||
+#include "base/forward.h"
|
||||
+#include "base/TypeTraits.h"
|
||||
|
||||
+#include <limits>
|
||||
+#include <optional>
|
||||
/* Math functions we define locally for Squid */
|
||||
namespace Math
|
||||
{
|
||||
@@ -21,5 +25,165 @@ double doubleAverage(const double, const double, int, const int);
|
||||
|
||||
} // namespace Math
|
||||
|
||||
+// If Sum() performance becomes important, consider using GCC and clang
|
||||
+// built-ins like __builtin_add_overflow() instead of manual overflow checks.
|
||||
+
|
||||
+/// detects a pair of unsigned types
|
||||
+/// reduces code duplication in declarations further below
|
||||
+template <typename T, typename U>
|
||||
+using AllUnsigned = typename std::conditional<
|
||||
+ std::is_unsigned<T>::value && std::is_unsigned<U>::value,
|
||||
+ std::true_type,
|
||||
+ std::false_type
|
||||
+ >::type;
|
||||
+
|
||||
+// TODO: Replace with std::cmp_less() after migrating to C++20.
|
||||
+/// whether integer a is less than integer b, with correct overflow handling
|
||||
+template <typename A, typename B>
|
||||
+constexpr bool
|
||||
+Less(const A a, const B b) {
|
||||
+ // The casts below make standard C++ integer conversions explicit. They
|
||||
+ // quell compiler warnings about signed/unsigned comparison. The first two
|
||||
+ // lines exclude different-sign a and b, making the casts/comparison safe.
|
||||
+ using AB = typename std::common_type<A, B>::type;
|
||||
+ return
|
||||
+ (a >= 0 && b < 0) ? false :
|
||||
+ (a < 0 && b >= 0) ? true :
|
||||
+ /* (a >= 0) == (b >= 0) */ static_cast<AB>(a) < static_cast<AB>(b);
|
||||
+}
|
||||
+
|
||||
+/// ensure that T is supported by NaturalSum() and friends
|
||||
+template<typename T>
|
||||
+constexpr void
|
||||
+AssertNaturalType()
|
||||
+{
|
||||
+ static_assert(std::numeric_limits<T>::is_bounded, "std::numeric_limits<T>::max() is meaningful");
|
||||
+ static_assert(std::numeric_limits<T>::is_exact, "no silent loss of precision");
|
||||
+ static_assert(!std::is_enum<T>::value, "no silent creation of non-enumerated values");
|
||||
+}
|
||||
+
|
||||
+// TODO: Investigate whether this optimization can be expanded to [signed] types
|
||||
+// A and B when std::numeric_limits<decltype(A(0)+B(0))>::is_modulo is true.
|
||||
+/// This IncreaseSumInternal() overload is optimized for speed.
|
||||
+/// \returns a non-overflowing sum of the two unsigned arguments (or nothing)
|
||||
+/// \prec both argument types are unsigned
|
||||
+template <typename S, typename A, typename B, std::enable_if_t<AllUnsigned<A,B>::value, int> = 0>
|
||||
+std::optional<S>
|
||||
+IncreaseSumInternal(const A a, const B b) {
|
||||
+ // paranoid: AllUnsigned<A,B> precondition established that already
|
||||
+ static_assert(std::is_unsigned<A>::value, "AllUnsigned dispatch worked for A");
|
||||
+ static_assert(std::is_unsigned<B>::value, "AllUnsigned dispatch worked for B");
|
||||
+
|
||||
+ AssertNaturalType<S>();
|
||||
+ AssertNaturalType<A>();
|
||||
+ AssertNaturalType<B>();
|
||||
+
|
||||
+ // we should only be called by IncreaseSum(); it forces integer promotion
|
||||
+ static_assert(std::is_same<A, decltype(+a)>::value, "a will not be promoted");
|
||||
+ static_assert(std::is_same<B, decltype(+b)>::value, "b will not be promoted");
|
||||
+ // and without integer promotions, a sum of unsigned integers is unsigned
|
||||
+ static_assert(std::is_unsigned<decltype(a+b)>::value, "a+b is unsigned");
|
||||
+
|
||||
+ // with integer promotions ruled out, a or b can only undergo integer
|
||||
+ // conversion to the higher rank type (A or B, we do not know which)
|
||||
+ using AB = typename std::common_type<A, B>::type;
|
||||
+ static_assert(std::is_same<AB, A>::value || std::is_same<AB, B>::value, "no unexpected conversions");
|
||||
+ static_assert(std::is_same<AB, decltype(a+b)>::value, "lossless assignment");
|
||||
+ const AB sum = a + b;
|
||||
+
|
||||
+ static_assert(std::numeric_limits<AB>::is_modulo, "we can detect overflows");
|
||||
+ // 1. modulo math: overflowed sum is smaller than any of its operands
|
||||
+ // 2. the sum may overflow S (i.e. the return base type)
|
||||
+ // We do not need Less() here because we compare promoted unsigned types.
|
||||
+ return (sum >= a && sum <= std::numeric_limits<S>::max()) ?
|
||||
+ std::optional<S>(sum) : std::optional<S>();
|
||||
+}
|
||||
+
|
||||
+/// This IncreaseSumInternal() overload supports a larger variety of types.
|
||||
+/// \returns a non-overflowing sum of the two arguments (or nothing)
|
||||
+/// \returns nothing if at least one of the arguments is negative
|
||||
+/// \prec at least one of the argument types is signed
|
||||
+template <typename S, typename A, typename B, std::enable_if_t<!AllUnsigned<A,B>::value, int> = 0>
|
||||
+std::optional<S> constexpr
|
||||
+IncreaseSumInternal(const A a, const B b) {
|
||||
+ AssertNaturalType<S>();
|
||||
+ AssertNaturalType<A>();
|
||||
+ AssertNaturalType<B>();
|
||||
+
|
||||
+ // we should only be called by IncreaseSum() that does integer promotion
|
||||
+ static_assert(std::is_same<A, decltype(+a)>::value, "a will not be promoted");
|
||||
+ static_assert(std::is_same<B, decltype(+b)>::value, "b will not be promoted");
|
||||
+
|
||||
+ return
|
||||
+ // We could support a non-under/overflowing sum of negative numbers, but
|
||||
+ // our callers use negative values specially (e.g., for do-not-use or
|
||||
+ // do-not-limit settings) and are not supposed to do math with them.
|
||||
+ (a < 0 || b < 0) ? std::optional<S>() :
|
||||
+ // To avoid undefined behavior of signed overflow, we must not compute
|
||||
+ // the raw a+b sum if it may overflow. When A is not B, a or b undergoes
|
||||
+ // (safe for non-negatives) integer conversion in these expressions, so
|
||||
+ // we do not know the resulting a+b type AB and its maximum. We must
|
||||
+ // also detect subsequent casting-to-S overflows.
|
||||
+ // Overflow condition: (a + b > maxAB) or (a + b > maxS).
|
||||
+ // A is an integer promotion of S, so maxS <= maxA <= maxAB.
|
||||
+ // Since maxS <= maxAB, it is sufficient to just check: a + b > maxS,
|
||||
+ // which is the same as the overflow-safe condition here: maxS - a < b.
|
||||
+ // Finally, (maxS - a) cannot overflow because a is not negative and
|
||||
+ // cannot underflow because a is a promotion of s: 0 <= a <= maxS.
|
||||
+ Less(std::numeric_limits<S>::max() - a, b) ? std::optional<S>() :
|
||||
+ std::optional<S>(a + b);
|
||||
+}
|
||||
+
|
||||
+/// argument pack expansion termination for IncreaseSum<S, T, Args...>()
|
||||
+template <typename S, typename T>
|
||||
+std::optional<S>
|
||||
+IncreaseSum(const S s, const T t)
|
||||
+{
|
||||
+ // Force (always safe) integer promotions now, to give std::enable_if_t<>
|
||||
+ // promoted types instead of entering IncreaseSumInternal<AllUnsigned>(s,t)
|
||||
+ // but getting a _signed_ promoted value of s or t in s + t.
|
||||
+ return IncreaseSumInternal<S>(+s, +t);
|
||||
+}
|
||||
+
|
||||
+/// \returns a non-overflowing sum of the arguments (or nothing)
|
||||
+template <typename S, typename T, typename... Args>
|
||||
+std::optional<S>
|
||||
+IncreaseSum(const S sum, const T t, const Args... args) {
|
||||
+ if (const auto head = IncreaseSum(sum, t)) {
|
||||
+ return IncreaseSum(head.value(), args...);
|
||||
+ } else {
|
||||
+ // std::optional<S>() triggers bogus -Wmaybe-uninitialized warnings in GCC v10.3
|
||||
+ return std::nullopt;
|
||||
+ }
|
||||
+}
|
||||
+
|
||||
+/// \returns an exact, non-overflowing sum of the arguments (or nothing)
|
||||
+template <typename SummationType, typename... Args>
|
||||
+std::optional<SummationType>
|
||||
+NaturalSum(const Args... args) {
|
||||
+ return IncreaseSum<SummationType>(0, args...);
|
||||
+}
|
||||
+
|
||||
+/// Safely resets the given variable to NaturalSum() of the given arguments.
|
||||
+/// If the sum overflows, resets to variable's maximum possible value.
|
||||
+/// \returns the new variable value (like an assignment operator would)
|
||||
+template <typename S, typename... Args>
|
||||
+S
|
||||
+SetToNaturalSumOrMax(S &var, const Args... args)
|
||||
+{
|
||||
+ var = NaturalSum<S>(args...).value_or(std::numeric_limits<S>::max());
|
||||
+ return var;
|
||||
+}
|
||||
+
|
||||
+/// converts a given non-negative integer into an integer of a given type
|
||||
+/// without loss of information or undefined behavior
|
||||
+template <typename Result, typename Source>
|
||||
+Result
|
||||
+NaturalCast(const Source s)
|
||||
+{
|
||||
+ return NaturalSum<Result>(s).value();
|
||||
+}
|
||||
+
|
||||
+
|
||||
#endif /* _SQUID_SRC_SQUIDMATH_H */
|
||||
|
||||
--
|
||||
2.39.3
|
||||
|
@ -1,763 +0,0 @@
|
||||
From 37de4ce82f7f8906606d0625774d856ffd3a9453 Mon Sep 17 00:00:00 2001
|
||||
From: Alex Burmashev <alexander.burmashev@oracle.com>
|
||||
Date: Thu, 7 Dec 2023 20:51:39 +0000
|
||||
Subject: [PATCH] Adapt to older gcc, cleanup
|
||||
|
||||
Fix code that is not applicable to older codebase of squidv4.
|
||||
On top do some work to adapt code to older gcc.
|
||||
most of that is std::optional to std::pair conversion
|
||||
|
||||
Signed-off-by: Alex Burmashev <alexander.burmashev@oracle.com>
|
||||
---
|
||||
src/HttpReply.cc | 4 +-
|
||||
src/MemObject.h | 3 ++
|
||||
src/MemStore.cc | 6 +--
|
||||
src/SquidMath.h | 27 ++++++------
|
||||
src/Store.h | 3 ++
|
||||
src/StoreClient.h | 2 +-
|
||||
src/acl/Asn.cc | 14 +------
|
||||
src/base/Assure.cc | 8 ++++
|
||||
src/client_side_reply.cc | 64 ++++++++++++-----------------
|
||||
src/peer_digest.cc | 1 +
|
||||
src/store/ParsingBuffer.cc | 47 ++++++++++-----------
|
||||
src/store/ParsingBuffer.h | 2 +-
|
||||
src/store_client.cc | 84 +++++++++++++++++---------------------
|
||||
src/urn.cc | 2 +-
|
||||
14 files changed, 123 insertions(+), 144 deletions(-)
|
||||
|
||||
diff --git a/src/HttpReply.cc b/src/HttpReply.cc
|
||||
index df5bcef..21c62c2 100644
|
||||
--- a/src/HttpReply.cc
|
||||
+++ b/src/HttpReply.cc
|
||||
@@ -534,13 +534,13 @@ HttpReply::parseTerminatedPrefix(const char * const terminatedBuf, const size_t
|
||||
const bool eof = false; // TODO: Remove after removing atEnd from HttpHeader::parse()
|
||||
if (parse(terminatedBuf, bufSize, eof, &error)) {
|
||||
debugs(58, 7, "success after accumulating " << bufSize << " bytes and parsing " << hdr_sz);
|
||||
- Assure(pstate == Http::Message::psParsed);
|
||||
+ Assure(pstate == psParsed);
|
||||
Assure(hdr_sz > 0);
|
||||
Assure(!Less(bufSize, hdr_sz)); // cannot parse more bytes than we have
|
||||
return hdr_sz; // success
|
||||
}
|
||||
|
||||
- Assure(pstate != Http::Message::psParsed);
|
||||
+ Assure(pstate != psParsed);
|
||||
hdr_sz = 0;
|
||||
|
||||
if (error) {
|
||||
diff --git a/src/MemObject.h b/src/MemObject.h
|
||||
index ba6646f..5a7590a 100644
|
||||
--- a/src/MemObject.h
|
||||
+++ b/src/MemObject.h
|
||||
@@ -56,6 +56,9 @@ public:
|
||||
|
||||
void write(const StoreIOBuffer &buf);
|
||||
void unlinkRequest();
|
||||
+
|
||||
+ HttpReply &baseReply() const { return *_reply; }
|
||||
+
|
||||
HttpReply const *getReply() const;
|
||||
void replaceHttpReply(HttpReply *newrep);
|
||||
void stat (MemBuf * mb) const;
|
||||
diff --git a/src/MemStore.cc b/src/MemStore.cc
|
||||
index fe7af2f..6762c4f 100644
|
||||
--- a/src/MemStore.cc
|
||||
+++ b/src/MemStore.cc
|
||||
@@ -511,8 +511,8 @@ MemStore::copyFromShm(StoreEntry &e, const sfileno index, const Ipc::StoreMapAnc
|
||||
" from " << extra.page << '+' << prefixSize);
|
||||
|
||||
// parse headers if needed; they might span multiple slices!
|
||||
- auto &reply = e.mem().adjustableBaseReply();
|
||||
- if (reply.pstate != Http::Message::psParsed) {
|
||||
+ auto &reply = e.mem().baseReply();
|
||||
+ if (reply.pstate != psParsed) {
|
||||
httpHeaderParsingBuffer.append(sliceBuf.data, sliceBuf.length);
|
||||
if (reply.parseTerminatedPrefix(httpHeaderParsingBuffer.c_str(), httpHeaderParsingBuffer.length()))
|
||||
httpHeaderParsingBuffer = SBuf(); // we do not need these bytes anymore
|
||||
@@ -542,7 +542,7 @@ MemStore::copyFromShm(StoreEntry &e, const sfileno index, const Ipc::StoreMapAnc
|
||||
debugs(20, 5, "mem-loaded all " << e.mem_obj->endOffset() << '/' <<
|
||||
anchor.basics.swap_file_sz << " bytes of " << e);
|
||||
|
||||
- if (e.mem().adjustableBaseReply().pstate != Http::Message::psParsed)
|
||||
+ if (e.mem().baseReply().pstate != psParsed)
|
||||
throw TextException(ToSBuf("truncated mem-cached headers; accumulated: ", httpHeaderParsingBuffer.length()), Here());
|
||||
|
||||
// from StoreEntry::complete()
|
||||
diff --git a/src/SquidMath.h b/src/SquidMath.h
|
||||
index e5b6e58..538833b 100644
|
||||
--- a/src/SquidMath.h
|
||||
+++ b/src/SquidMath.h
|
||||
@@ -8,8 +8,6 @@
|
||||
|
||||
#ifndef _SQUID_SRC_SQUIDMATH_H
|
||||
#define _SQUID_SRC_SQUIDMATH_H
|
||||
-#include "base/forward.h"
|
||||
-#include "base/TypeTraits.h"
|
||||
|
||||
#include <limits>
|
||||
#include <optional>
|
||||
@@ -68,7 +66,7 @@ AssertNaturalType()
|
||||
/// \returns a non-overflowing sum of the two unsigned arguments (or nothing)
|
||||
/// \prec both argument types are unsigned
|
||||
template <typename S, typename A, typename B, std::enable_if_t<AllUnsigned<A,B>::value, int> = 0>
|
||||
-std::optional<S>
|
||||
+std::pair<S, bool>
|
||||
IncreaseSumInternal(const A a, const B b) {
|
||||
// paranoid: AllUnsigned<A,B> precondition established that already
|
||||
static_assert(std::is_unsigned<A>::value, "AllUnsigned dispatch worked for A");
|
||||
@@ -96,7 +94,7 @@ IncreaseSumInternal(const A a, const B b) {
|
||||
// 2. the sum may overflow S (i.e. the return base type)
|
||||
// We do not need Less() here because we compare promoted unsigned types.
|
||||
return (sum >= a && sum <= std::numeric_limits<S>::max()) ?
|
||||
- std::optional<S>(sum) : std::optional<S>();
|
||||
+ std::make_pair(sum, true) : std::make_pair(S(), false);
|
||||
}
|
||||
|
||||
/// This IncreaseSumInternal() overload supports a larger variety of types.
|
||||
@@ -104,7 +102,7 @@ IncreaseSumInternal(const A a, const B b) {
|
||||
/// \returns nothing if at least one of the arguments is negative
|
||||
/// \prec at least one of the argument types is signed
|
||||
template <typename S, typename A, typename B, std::enable_if_t<!AllUnsigned<A,B>::value, int> = 0>
|
||||
-std::optional<S> constexpr
|
||||
+std::pair<S, bool>
|
||||
IncreaseSumInternal(const A a, const B b) {
|
||||
AssertNaturalType<S>();
|
||||
AssertNaturalType<A>();
|
||||
@@ -118,7 +116,7 @@ IncreaseSumInternal(const A a, const B b) {
|
||||
// We could support a non-under/overflowing sum of negative numbers, but
|
||||
// our callers use negative values specially (e.g., for do-not-use or
|
||||
// do-not-limit settings) and are not supposed to do math with them.
|
||||
- (a < 0 || b < 0) ? std::optional<S>() :
|
||||
+ (a < 0 || b < 0) ? std::make_pair(S(), false) :
|
||||
// To avoid undefined behavior of signed overflow, we must not compute
|
||||
// the raw a+b sum if it may overflow. When A is not B, a or b undergoes
|
||||
// (safe for non-negatives) integer conversion in these expressions, so
|
||||
@@ -130,13 +128,13 @@ IncreaseSumInternal(const A a, const B b) {
|
||||
// which is the same as the overflow-safe condition here: maxS - a < b.
|
||||
// Finally, (maxS - a) cannot overflow because a is not negative and
|
||||
// cannot underflow because a is a promotion of s: 0 <= a <= maxS.
|
||||
- Less(std::numeric_limits<S>::max() - a, b) ? std::optional<S>() :
|
||||
- std::optional<S>(a + b);
|
||||
+ Less(std::numeric_limits<S>::max() - a, b) ? std::make_pair(S(), false) :
|
||||
+ std::make_pair(S(a + b), true);
|
||||
}
|
||||
|
||||
/// argument pack expansion termination for IncreaseSum<S, T, Args...>()
|
||||
template <typename S, typename T>
|
||||
-std::optional<S>
|
||||
+std::pair<S, bool>
|
||||
IncreaseSum(const S s, const T t)
|
||||
{
|
||||
// Force (always safe) integer promotions now, to give std::enable_if_t<>
|
||||
@@ -147,19 +145,20 @@ IncreaseSum(const S s, const T t)
|
||||
|
||||
/// \returns a non-overflowing sum of the arguments (or nothing)
|
||||
template <typename S, typename T, typename... Args>
|
||||
-std::optional<S>
|
||||
+std::pair<S, bool>
|
||||
IncreaseSum(const S sum, const T t, const Args... args) {
|
||||
- if (const auto head = IncreaseSum(sum, t)) {
|
||||
- return IncreaseSum(head.value(), args...);
|
||||
+ const auto head = IncreaseSum(sum, t);
|
||||
+ if (head.second) {
|
||||
+ return IncreaseSum(head.first, args...);
|
||||
} else {
|
||||
// std::optional<S>() triggers bogus -Wmaybe-uninitialized warnings in GCC v10.3
|
||||
- return std::nullopt;
|
||||
+ return std::make_pair(S(), false);
|
||||
}
|
||||
}
|
||||
|
||||
/// \returns an exact, non-overflowing sum of the arguments (or nothing)
|
||||
template <typename SummationType, typename... Args>
|
||||
-std::optional<SummationType>
|
||||
+std::pair<SummationType, bool>
|
||||
NaturalSum(const Args... args) {
|
||||
return IncreaseSum<SummationType>(0, args...);
|
||||
}
|
||||
diff --git a/src/Store.h b/src/Store.h
|
||||
index 3eb6b84..2475fe0 100644
|
||||
--- a/src/Store.h
|
||||
+++ b/src/Store.h
|
||||
@@ -49,6 +49,9 @@ public:
|
||||
StoreEntry();
|
||||
virtual ~StoreEntry();
|
||||
|
||||
+ MemObject &mem() { assert(mem_obj); return *mem_obj; }
|
||||
+ const MemObject &mem() const { assert(mem_obj); return *mem_obj; }
|
||||
+
|
||||
virtual HttpReply const *getReply() const;
|
||||
virtual void write (StoreIOBuffer);
|
||||
|
||||
diff --git a/src/StoreClient.h b/src/StoreClient.h
|
||||
index 0524776..ba5e669 100644
|
||||
--- a/src/StoreClient.h
|
||||
+++ b/src/StoreClient.h
|
||||
@@ -166,7 +166,7 @@ private:
|
||||
/// request. Buffer contents depends on the source and parsing stage; it may
|
||||
/// hold (parts of) swap metadata, HTTP response headers, and/or HTTP
|
||||
/// response body bytes.
|
||||
- std::optional<Store::ParsingBuffer> parsingBuffer;
|
||||
+ std::pair<Store::ParsingBuffer, bool> parsingBuffer = std::make_pair(Store::ParsingBuffer(), false);
|
||||
|
||||
StoreIOBuffer lastDiskRead; ///< buffer used for the last storeRead() call
|
||||
|
||||
diff --git a/src/acl/Asn.cc b/src/acl/Asn.cc
|
||||
index bcedc82..67e453f 100644
|
||||
--- a/src/acl/Asn.cc
|
||||
+++ b/src/acl/Asn.cc
|
||||
@@ -73,7 +73,7 @@ class ASState
|
||||
CBDATA_CLASS(ASState);
|
||||
|
||||
public:
|
||||
- ASState();
|
||||
+ ASState() = default;
|
||||
~ASState();
|
||||
|
||||
StoreEntry *entry;
|
||||
@@ -87,18 +87,6 @@ public:
|
||||
|
||||
CBDATA_CLASS_INIT(ASState);
|
||||
|
||||
-ASState::ASState() :
|
||||
- entry(NULL),
|
||||
- sc(NULL),
|
||||
- request(NULL),
|
||||
- as_number(0),
|
||||
- offset(0),
|
||||
- reqofs(0),
|
||||
- dataRead(false)
|
||||
-{
|
||||
- memset(reqbuf, 0, AS_REQBUF_SZ);
|
||||
-}
|
||||
-
|
||||
ASState::~ASState()
|
||||
{
|
||||
debugs(53, 3, entry->url());
|
||||
diff --git a/src/base/Assure.cc b/src/base/Assure.cc
|
||||
index b09b848..b4cf3e5 100644
|
||||
--- a/src/base/Assure.cc
|
||||
+++ b/src/base/Assure.cc
|
||||
@@ -11,6 +11,14 @@
|
||||
#include "base/TextException.h"
|
||||
#include "sbuf/Stream.h"
|
||||
|
||||
+std::ostream &
|
||||
+operator <<(std::ostream &os, const TextException &ex)
|
||||
+{
|
||||
+ ex.print(os);
|
||||
+ return os;
|
||||
+}
|
||||
+
|
||||
+
|
||||
[[ noreturn ]] void
|
||||
ReportAndThrow_(const int debugLevel, const char *description, const SourceLocation &location)
|
||||
{
|
||||
diff --git a/src/client_side_reply.cc b/src/client_side_reply.cc
|
||||
index 470f4bc..64fd489 100644
|
||||
--- a/src/client_side_reply.cc
|
||||
+++ b/src/client_side_reply.cc
|
||||
@@ -1142,8 +1142,8 @@ clientReplyContext::storeNotOKTransferDone() const
|
||||
MemObject *mem = http->storeEntry()->mem_obj;
|
||||
assert(mem != NULL);
|
||||
assert(http->request != NULL);
|
||||
-
|
||||
- if (mem->baseReply().pstate != Http::Message::psParsed)
|
||||
+ const auto expectedBodySize = mem->baseReply().content_length;
|
||||
+ if (mem->baseReply().pstate != psParsed)
|
||||
return 0;
|
||||
|
||||
/*
|
||||
@@ -1808,32 +1808,6 @@ clientReplyContext::SendMoreData(void *data, StoreIOBuffer result)
|
||||
context->sendMoreData (result);
|
||||
}
|
||||
|
||||
-/// Whether the given body area describes the start of our Client Stream buffer.
|
||||
-/// An empty area does.
|
||||
-bool
|
||||
-clientReplyContext::matchesStreamBodyBuffer(const StoreIOBuffer &their) const
|
||||
-{
|
||||
- // the answer is undefined for errors; they are not really "body buffers"
|
||||
- Assure(!their.flags.error);
|
||||
-
|
||||
- if (!their.length)
|
||||
- return true; // an empty body area always matches our body area
|
||||
-
|
||||
- if (their.data != next()->readBuffer.data) {
|
||||
- debugs(88, 7, "no: " << their << " vs. " << next()->readBuffer);
|
||||
- return false;
|
||||
- }
|
||||
-
|
||||
- return true;
|
||||
-}
|
||||
-
|
||||
-void
|
||||
-clientReplyContext::noteStreamBufferredBytes(const StoreIOBuffer &result)
|
||||
-{
|
||||
- Assure(matchesStreamBodyBuffer(result));
|
||||
- lastStreamBufferedBytes = result; // may be unchanged and/or zero-length
|
||||
-}
|
||||
-
|
||||
void
|
||||
clientReplyContext::makeThisHead()
|
||||
{
|
||||
@@ -2180,21 +2154,33 @@ clientReplyContext::sendMoreData (StoreIOBuffer result)
|
||||
sc->setDelayId(DelayId::DelayClient(http,reply));
|
||||
#endif
|
||||
|
||||
- /* handle headers */
|
||||
+ holdingBuffer = result;
|
||||
+ processReplyAccess();
|
||||
+ return;
|
||||
+}
|
||||
|
||||
- if (Config.onoff.log_mime_hdrs) {
|
||||
- size_t k;
|
||||
+/// Whether the given body area describes the start of our Client Stream buffer.
|
||||
+/// An empty area does.
|
||||
+bool
|
||||
+clientReplyContext::matchesStreamBodyBuffer(const StoreIOBuffer &their) const
|
||||
+{
|
||||
+ // the answer is undefined for errors; they are not really "body buffers"
|
||||
+ Assure(!their.flags.error);
|
||||
+ if (!their.length)
|
||||
+ return true; // an empty body area always matches our body area
|
||||
+ if (their.data != next()->readBuffer.data) {
|
||||
+ debugs(88, 7, "no: " << their << " vs. " << next()->readBuffer);
|
||||
+ return false;
|
||||
|
||||
- if ((k = headersEnd(buf, reqofs))) {
|
||||
- safe_free(http->al->headers.reply);
|
||||
- http->al->headers.reply = (char *)xcalloc(k + 1, 1);
|
||||
- xstrncpy(http->al->headers.reply, buf, k);
|
||||
- }
|
||||
}
|
||||
+ return true;
|
||||
+}
|
||||
|
||||
- holdingBuffer = result;
|
||||
- processReplyAccess();
|
||||
- return;
|
||||
+void
|
||||
+clientReplyContext::noteStreamBufferredBytes(const StoreIOBuffer &result)
|
||||
+{
|
||||
+ Assure(matchesStreamBodyBuffer(result));
|
||||
+ lastStreamBufferedBytes = result; // may be unchanged and/or zero-length
|
||||
}
|
||||
|
||||
/* Using this breaks the client layering just a little!
|
||||
diff --git a/src/peer_digest.cc b/src/peer_digest.cc
|
||||
index abfea4a..89ea73e 100644
|
||||
--- a/src/peer_digest.cc
|
||||
+++ b/src/peer_digest.cc
|
||||
@@ -588,6 +588,7 @@ peerDigestFetchReply(void *data, char *buf, ssize_t size)
|
||||
|
||||
return 0; // we consumed/used no buffered bytes
|
||||
}
|
||||
+}
|
||||
|
||||
int
|
||||
peerDigestSwapInCBlock(void *data, char *buf, ssize_t size)
|
||||
diff --git a/src/store/ParsingBuffer.cc b/src/store/ParsingBuffer.cc
|
||||
index e948fe2..affbe9e 100644
|
||||
--- a/src/store/ParsingBuffer.cc
|
||||
+++ b/src/store/ParsingBuffer.cc
|
||||
@@ -28,19 +28,19 @@ Store::ParsingBuffer::ParsingBuffer(StoreIOBuffer &initialSpace):
|
||||
const char *
|
||||
Store::ParsingBuffer::memory() const
|
||||
{
|
||||
- return extraMemory_ ? extraMemory_->rawContent() : readerSuppliedMemory_.data;
|
||||
+ return extraMemory_.second ? extraMemory_.first.rawContent() : readerSuppliedMemory_.data;
|
||||
}
|
||||
|
||||
size_t
|
||||
Store::ParsingBuffer::capacity() const
|
||||
{
|
||||
- return extraMemory_ ? (extraMemory_->length() + extraMemory_->spaceSize()) : readerSuppliedMemory_.length;
|
||||
+ return extraMemory_.second ? (extraMemory_.first.length() + extraMemory_.first.spaceSize()) : readerSuppliedMemory_.length;
|
||||
}
|
||||
|
||||
size_t
|
||||
Store::ParsingBuffer::contentSize() const
|
||||
{
|
||||
- return extraMemory_ ? extraMemory_->length() : readerSuppliedMemoryContentSize_;
|
||||
+ return extraMemory_.second ? extraMemory_.first.length() : readerSuppliedMemoryContentSize_;
|
||||
}
|
||||
|
||||
void
|
||||
@@ -56,10 +56,10 @@ Store::ParsingBuffer::appended(const char * const newBytes, const size_t newByte
|
||||
assert(memory() + contentSize() == newBytes); // the new bytes start in our space
|
||||
// and now we know that newBytes is not nil either
|
||||
|
||||
- if (extraMemory_)
|
||||
- extraMemory_->rawAppendFinish(newBytes, newByteCount);
|
||||
+ if (extraMemory_.second)
|
||||
+ extraMemory_.first.rawAppendFinish(newBytes, newByteCount);
|
||||
else
|
||||
- readerSuppliedMemoryContentSize_ = *IncreaseSum(readerSuppliedMemoryContentSize_, newByteCount);
|
||||
+ readerSuppliedMemoryContentSize_ = IncreaseSum(readerSuppliedMemoryContentSize_, newByteCount).first;
|
||||
|
||||
assert(contentSize() <= capacity()); // paranoid
|
||||
}
|
||||
@@ -68,8 +68,8 @@ void
|
||||
Store::ParsingBuffer::consume(const size_t parsedBytes)
|
||||
{
|
||||
Assure(contentSize() >= parsedBytes); // more conservative than extraMemory_->consume()
|
||||
- if (extraMemory_) {
|
||||
- extraMemory_->consume(parsedBytes);
|
||||
+ if (extraMemory_.second) {
|
||||
+ extraMemory_.first.consume(parsedBytes);
|
||||
} else {
|
||||
readerSuppliedMemoryContentSize_ -= parsedBytes;
|
||||
if (parsedBytes && readerSuppliedMemoryContentSize_)
|
||||
@@ -81,8 +81,8 @@ StoreIOBuffer
|
||||
Store::ParsingBuffer::space()
|
||||
{
|
||||
const auto size = spaceSize();
|
||||
- const auto start = extraMemory_ ?
|
||||
- extraMemory_->rawAppendStart(size) :
|
||||
+ const auto start = extraMemory_.second ?
|
||||
+ extraMemory_.first.rawAppendStart(size) :
|
||||
(readerSuppliedMemory_.data + readerSuppliedMemoryContentSize_);
|
||||
return StoreIOBuffer(spaceSize(), 0, start);
|
||||
}
|
||||
@@ -110,22 +110,23 @@ void
|
||||
Store::ParsingBuffer::growSpace(const size_t minimumSpaceSize)
|
||||
{
|
||||
const auto capacityIncreaseAttempt = IncreaseSum(contentSize(), minimumSpaceSize);
|
||||
- if (!capacityIncreaseAttempt)
|
||||
+ if (!capacityIncreaseAttempt.second)
|
||||
throw TextException(ToSBuf("no support for a single memory block of ", contentSize(), '+', minimumSpaceSize, " bytes"), Here());
|
||||
- const auto newCapacity = *capacityIncreaseAttempt;
|
||||
+ const auto newCapacity = capacityIncreaseAttempt.first;
|
||||
|
||||
if (newCapacity <= capacity())
|
||||
return; // already have enough space; no reallocation is needed
|
||||
|
||||
debugs(90, 7, "growing to provide " << minimumSpaceSize << " in " << *this);
|
||||
|
||||
- if (extraMemory_) {
|
||||
- extraMemory_->reserveCapacity(newCapacity);
|
||||
+ if (extraMemory_.second) {
|
||||
+ extraMemory_.first.reserveCapacity(newCapacity);
|
||||
} else {
|
||||
SBuf newStorage;
|
||||
newStorage.reserveCapacity(newCapacity);
|
||||
newStorage.append(readerSuppliedMemory_.data, readerSuppliedMemoryContentSize_);
|
||||
- extraMemory_ = std::move(newStorage);
|
||||
+ extraMemory_.first = std::move(newStorage);
|
||||
+ extraMemory_.second = true;
|
||||
}
|
||||
Assure(spaceSize() >= minimumSpaceSize);
|
||||
}
|
||||
@@ -133,14 +134,14 @@ Store::ParsingBuffer::growSpace(const size_t minimumSpaceSize)
|
||||
SBuf
|
||||
Store::ParsingBuffer::toSBuf() const
|
||||
{
|
||||
- return extraMemory_ ? *extraMemory_ : SBuf(content().data, content().length);
|
||||
+ return extraMemory_.second ? extraMemory_.first : SBuf(content().data, content().length);
|
||||
}
|
||||
|
||||
size_t
|
||||
Store::ParsingBuffer::spaceSize() const
|
||||
{
|
||||
- if (extraMemory_)
|
||||
- return extraMemory_->spaceSize();
|
||||
+ if (extraMemory_.second)
|
||||
+ return extraMemory_.first.spaceSize();
|
||||
|
||||
assert(readerSuppliedMemoryContentSize_ <= readerSuppliedMemory_.length);
|
||||
return readerSuppliedMemory_.length - readerSuppliedMemoryContentSize_;
|
||||
@@ -169,12 +170,12 @@ Store::ParsingBuffer::packBack()
|
||||
result.length = bytesToPack;
|
||||
Assure(result.data);
|
||||
|
||||
- if (!extraMemory_) {
|
||||
+ if (!extraMemory_.second) {
|
||||
// no accumulated bytes copying because they are in readerSuppliedMemory_
|
||||
debugs(90, 7, "quickly exporting " << result.length << " bytes via " << readerSuppliedMemory_);
|
||||
} else {
|
||||
- debugs(90, 7, "slowly exporting " << result.length << " bytes from " << extraMemory_->id << " back into " << readerSuppliedMemory_);
|
||||
- memmove(result.data, extraMemory_->rawContent(), result.length);
|
||||
+ debugs(90, 7, "slowly exporting " << result.length << " bytes from " << extraMemory_.first.id << " back into " << readerSuppliedMemory_);
|
||||
+ memmove(result.data, extraMemory_.first.rawContent(), result.length);
|
||||
}
|
||||
|
||||
return result;
|
||||
@@ -185,9 +186,9 @@ Store::ParsingBuffer::print(std::ostream &os) const
|
||||
{
|
||||
os << "size=" << contentSize();
|
||||
|
||||
- if (extraMemory_) {
|
||||
+ if (extraMemory_.second) {
|
||||
os << " capacity=" << capacity();
|
||||
- os << " extra=" << extraMemory_->id;
|
||||
+ os << " extra=" << extraMemory_.first.id;
|
||||
}
|
||||
|
||||
// report readerSuppliedMemory_ (if any) even if we are no longer using it
|
||||
diff --git a/src/store/ParsingBuffer.h b/src/store/ParsingBuffer.h
|
||||
index b8aa957..b473ac6 100644
|
||||
--- a/src/store/ParsingBuffer.h
|
||||
+++ b/src/store/ParsingBuffer.h
|
||||
@@ -112,7 +112,7 @@ private:
|
||||
|
||||
/// our internal buffer that takes over readerSuppliedMemory_ when the
|
||||
/// latter becomes full and more memory is needed
|
||||
- std::optional<SBuf> extraMemory_;
|
||||
+ std::pair<SBuf, bool> extraMemory_ = std::make_pair(SBuf(), false);
|
||||
};
|
||||
|
||||
inline std::ostream &
|
||||
diff --git a/src/store_client.cc b/src/store_client.cc
|
||||
index 383aac8..0236274 100644
|
||||
--- a/src/store_client.cc
|
||||
+++ b/src/store_client.cc
|
||||
@@ -10,6 +10,7 @@
|
||||
|
||||
#include "squid.h"
|
||||
#include "base/AsyncCbdataCalls.h"
|
||||
+#include "base/Assure.h"
|
||||
#include "event.h"
|
||||
#include "globals.h"
|
||||
#include "HttpReply.h"
|
||||
@@ -118,24 +119,14 @@ store_client::finishCallback()
|
||||
// pointers. Some other legacy code expects "correct" result.offset even
|
||||
// when there is no body to return. Accommodate all those expectations.
|
||||
auto result = StoreIOBuffer(0, copyInto.offset, nullptr);
|
||||
- if (object_ok && parsingBuffer && parsingBuffer->contentSize())
|
||||
- result = parsingBuffer->packBack();
|
||||
+ if (object_ok && parsingBuffer.second && parsingBuffer.first.contentSize())
|
||||
+ result = parsingBuffer.first.packBack();
|
||||
result.flags.error = object_ok ? 0 : 1;
|
||||
|
||||
- // TODO: Move object_ok handling above into this `if` statement.
|
||||
- if (object_ok) {
|
||||
- // works for zero hdr_sz cases as well; see also: nextHttpReadOffset()
|
||||
- discardableHttpEnd_ = NaturalSum<int64_t>(entry->mem().baseReply().hdr_sz, result.offset, result.length).value();
|
||||
- } else {
|
||||
- // object_ok is sticky, so we will not be able to use any response bytes
|
||||
- discardableHttpEnd_ = entry->mem().endOffset();
|
||||
- }
|
||||
- debugs(90, 7, "with " << result << "; discardableHttpEnd_=" << discardableHttpEnd_);
|
||||
-
|
||||
// no HTTP headers and no body bytes (but not because there was no space)
|
||||
atEof_ = !sendingHttpHeaders() && !result.length && copyInto.length;
|
||||
|
||||
- parsingBuffer.reset();
|
||||
+ parsingBuffer.second = false;
|
||||