rpms/squid
7
0
Fork 0
Code Issues Pull Requests Releases Activity

Merge branch 'c8-stream-4' into a8-stream-4

Browse Source
This commit is contained in:
Eduard Abdullin 2024-03-06 15:35:47 +03:00
commit 4533dd2e0d
18 changed files with 10059 additions and 1261 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

599
SOURCES/0001-Break-long-store_client-call-chains-with-async-calls.patch Normal file
View File

@ -0,0 +1,599 @@
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

1190
SOURCES/0001-Fix-incremental-parsing-of-chunked-quoted-extensions.patch
View File

File diff suppressed because it is too large Load Diff

3388
SOURCES/0002-Remove-serialized-HTTP-headers-from-storeClientCopy.patch Normal file
View File

File diff suppressed because it is too large Load Diff

119
SOURCES/0003-Bug-5309-frequent-lowestOffset-target_offset-asserti.patch Normal file
View File

@ -0,0 +1,119 @@
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

67
SOURCES/0004-Remove-mem_hdr-freeDataUpto-assertion-1562.patch Normal file
View File

@ -0,0 +1,67 @@
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

210
SOURCES/0005-Backport-Add-Assure-as-a-replacement-for-problematic.patch Normal file
View File

@ -0,0 +1,210 @@
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

200
SOURCES/0006-Backport-additional-functions-for-SquidMath.patch Normal file
View File

@ -0,0 +1,200 @@
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

763
SOURCES/0007-Adapt-to-older-gcc-cleanup.patch Normal file
View File

@ -0,0 +1,763 @@
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;
++answers;
STCB *temphandler = _callback.callback_handler;
@@ -228,7 +219,9 @@ store_client::copy(StoreEntry * anEntry,
// when we already can respond with HTTP headers.
Assure(!copyInto.offset || answeredOnce());
- parsingBuffer.emplace(copyInto);
+ parsingBuffer.first = Store::ParsingBuffer(copyInto);
+ parsingBuffer.second = true;
+
discardableHttpEnd_ = nextHttpReadOffset();
debugs(90, 7, "discardableHttpEnd_=" << discardableHttpEnd_);
@@ -454,14 +447,14 @@ store_client::canReadFromMemory() const
const auto &mem = entry->mem();
const auto memReadOffset = nextHttpReadOffset();
return mem.inmem_lo <= memReadOffset && memReadOffset < mem.endOffset() &&
- parsingBuffer->spaceSize();
+ parsingBuffer.first.spaceSize();
}
/// The offset of the next stored HTTP response byte wanted by the client.
int64_t
store_client::nextHttpReadOffset() const
{
- Assure(parsingBuffer);
+ Assure(parsingBuffer.second);
const auto &mem = entry->mem();
const auto hdr_sz = mem.baseReply().hdr_sz;
// Certain SMP cache manager transactions do not store HTTP headers in
@@ -469,7 +462,7 @@ store_client::nextHttpReadOffset() const
// In such cases, hdr_sz ought to be zero. In all other (known) cases,
// mem_hdr contains HTTP response headers (positive hdr_sz if parsed)
// followed by HTTP response body. This code math accommodates all cases.
- return NaturalSum<int64_t>(hdr_sz, copyInto.offset, parsingBuffer->contentSize()).value();
+ return NaturalSum<int64_t>(hdr_sz, copyInto.offset, parsingBuffer.first.contentSize()).first;
}
/// Copies at least some of the requested body bytes from MemObject memory,
@@ -478,13 +471,13 @@ store_client::nextHttpReadOffset() const
void
store_client::readFromMemory()
{
- Assure(parsingBuffer);
- const auto readInto = parsingBuffer->space().positionAt(nextHttpReadOffset());
+ Assure(parsingBuffer.second);
+ const auto readInto = parsingBuffer.first.space().positionAt(nextHttpReadOffset());
debugs(90, 3, "copying HTTP body bytes from memory into " << readInto);
const auto sz = entry->mem_obj->data_hdr.copy(readInto);
Assure(sz > 0); // our canReadFromMemory() precondition guarantees that
- parsingBuffer->appended(readInto.data, sz);
+ parsingBuffer.first.appended(readInto.data, sz);
}
void
@@ -497,7 +490,7 @@ store_client::fileRead()
flags.disk_io_pending = true;
// mem->swap_hdr_sz is zero here during initial read(s)
- const auto nextStoreReadOffset = NaturalSum<int64_t>(mem->swap_hdr_sz, nextHttpReadOffset()).value();
+ const auto nextStoreReadOffset = NaturalSum<int64_t>(mem->swap_hdr_sz, nextHttpReadOffset()).first;
// XXX: If fileRead() is called when we do not yet know mem->swap_hdr_sz,
// then we must start reading from disk offset zero to learn it: we cannot
@@ -522,10 +515,10 @@ store_client::fileRead()
// * performance effects of larger disk reads may be negative somewhere.
const decltype(StoreIOBuffer::length) maxReadSize = SM_PAGE_SIZE;
- Assure(parsingBuffer);
+ Assure(parsingBuffer.second);
// also, do not read more than we can return (via a copyInto.length buffer)
const auto readSize = std::min(copyInto.length, maxReadSize);
- lastDiskRead = parsingBuffer->makeSpace(readSize).positionAt(nextStoreReadOffset);
+ lastDiskRead = parsingBuffer.first.makeSpace(readSize).positionAt(nextStoreReadOffset);
debugs(90, 5, "into " << lastDiskRead);
storeRead(swapin_sio,
@@ -540,13 +533,12 @@ store_client::fileRead()
void
store_client::readBody(const char * const buf, const ssize_t lastIoResult)
{
- int parsed_header = 0;
Assure(flags.disk_io_pending);
flags.disk_io_pending = false;
assert(_callback.pending());
- Assure(parsingBuffer);
- debugs(90, 3, "got " << lastIoResult << " using " << *parsingBuffer);
+ Assure(parsingBuffer.second);
+ debugs(90, 3, "got " << lastIoResult << " using " << parsingBuffer.first);
if (lastIoResult < 0)
return fail();
@@ -560,7 +552,7 @@ store_client::readBody(const char * const buf, const ssize_t lastIoResult)
assert(lastDiskRead.data == buf);
lastDiskRead.length = lastIoResult;
- parsingBuffer->appended(buf, lastIoResult);
+ parsingBuffer.first.appended(buf, lastIoResult);
// we know swap_hdr_sz by now and were reading beyond swap metadata because
// readHead() would have been called otherwise (to read swap metadata)
@@ -589,13 +581,12 @@ store_client::handleBodyFromDisk()
if (!answeredOnce()) {
// All on-disk responses have HTTP headers. First disk body read(s)
// include HTTP headers that we must parse (if needed) and skip.
- const auto haveHttpHeaders = entry->mem_obj->baseReply().pstate == Http::Message::psParsed;
+ const auto haveHttpHeaders = entry->mem_obj->baseReply().pstate == psParsed;
if (!haveHttpHeaders && !parseHttpHeadersFromDisk())
return;
skipHttpHeadersFromDisk();
}
- const HttpReply *rep = entry->getReply();
noteNews();
}
@@ -626,8 +617,6 @@ store_client::maybeWriteFromDiskToMemory(const StoreIOBuffer &httpResponsePart)
}
}
-}
-
void
store_client::fail()
{
@@ -735,20 +724,20 @@ store_client::readHeader(char const *buf, ssize_t len)
if (!object_ok)
return;
- Assure(parsingBuffer);
- debugs(90, 3, "got " << len << " using " << *parsingBuffer);
+ Assure(parsingBuffer.second);
+ debugs(90, 3, "got " << len << " using " << parsingBuffer.first);
if (len < 0)
return fail();
- Assure(!parsingBuffer->contentSize());
- parsingBuffer->appended(buf, len);
+ Assure(!parsingBuffer.first.contentSize());
+ parsingBuffer.first.appended(buf, len);
if (!unpackHeader(buf, len)) {
fail();
return;
}
- parsingBuffer->consume(mem->swap_hdr_sz);
- maybeWriteFromDiskToMemory(parsingBuffer->content());
+ parsingBuffer.first.consume(mem->swap_hdr_sz);
+ maybeWriteFromDiskToMemory(parsingBuffer.first.content());
handleBodyFromDisk();
}
@@ -1020,8 +1009,9 @@ store_client::parseHttpHeadersFromDisk()
// cache a header that we cannot parse and get here. Same for MemStore.
debugs(90, DBG_CRITICAL, "ERROR: Cannot parse on-disk HTTP headers" <<
Debug::Extra << "exception: " << CurrentException <<
- Debug::Extra << "raw input size: " << parsingBuffer->contentSize() << " bytes" <<
- Debug::Extra << "current buffer capacity: " << parsingBuffer->capacity() << " bytes");
+ Debug::Extra << "raw input size: " << parsingBuffer.first.contentSize() << " bytes" <<
+ Debug::Extra << "current buffer capacity: " << parsingBuffer.first.capacity() << " bytes");
+
fail();
return false;
}
@@ -1032,10 +1022,10 @@ store_client::parseHttpHeadersFromDisk()
bool
store_client::tryParsingHttpHeaders()
{
- Assure(parsingBuffer);
+ Assure(parsingBuffer.second);
Assure(!copyInto.offset); // otherwise, parsingBuffer cannot have HTTP response headers
- auto &adjustableReply = entry->mem().adjustableBaseReply();
- if (adjustableReply.parseTerminatedPrefix(parsingBuffer->c_str(), parsingBuffer->contentSize()))
+ auto &adjustableReply = entry->mem().baseReply();
+ if (adjustableReply.parseTerminatedPrefix(parsingBuffer.first.c_str(), parsingBuffer.first.contentSize()))
return true;
// TODO: Optimize by checking memory as well. For simplicity sake, we
@@ -1052,12 +1042,12 @@ store_client::skipHttpHeadersFromDisk()
{
const auto hdr_sz = entry->mem_obj->baseReply().hdr_sz;
Assure(hdr_sz > 0); // all on-disk responses have HTTP headers
- if (Less(parsingBuffer->contentSize(), hdr_sz)) {
- debugs(90, 5, "discovered " << hdr_sz << "-byte HTTP headers in memory after reading some of them from disk: " << *parsingBuffer);
- parsingBuffer->consume(parsingBuffer->contentSize()); // skip loaded HTTP header prefix
+ if (Less(parsingBuffer.first.contentSize(), hdr_sz)) {
+ debugs(90, 5, "discovered " << hdr_sz << "-byte HTTP headers in memory after reading some of them from disk: " << parsingBuffer.first);
+ parsingBuffer.first.consume(parsingBuffer.first.contentSize()); // skip loaded HTTP header prefix
} else {
- parsingBuffer->consume(hdr_sz); // skip loaded HTTP headers
- const auto httpBodyBytesAfterHeader = parsingBuffer->contentSize(); // may be zero
+ parsingBuffer.first.consume(hdr_sz); // skip loaded HTTP headers
+ const auto httpBodyBytesAfterHeader = parsingBuffer.first.contentSize(); // may be zero
Assure(httpBodyBytesAfterHeader <= copyInto.length);
debugs(90, 5, "read HTTP body prefix: " << httpBodyBytesAfterHeader);
}
diff --git a/src/urn.cc b/src/urn.cc
index 9f5e89d..ad42b74 100644
--- a/src/urn.cc
+++ b/src/urn.cc
@@ -238,7 +238,7 @@ urnHandleReply(void *data, StoreIOBuffer result)
return;
}
-+ urnState->parsingBuffer.appended(result.data, result.length);
+ urnState->parsingBuffer.appended(result.data, result.length);
/* If we haven't received the entire object (urn), copy more */
if (!urnState->sc->atEof()) {
--
2.39.3

0
SOURCES/perl-requires-squid.sh Executable file → Normal file
View File

38
SOURCES/squid-4.15-CVE-2023-46724.patch Normal file
View File

@ -0,0 +1,38 @@
From 792ef23e6e1c05780fe17f733859eef6eb8c8be3 Mon Sep 17 00:00:00 2001
From: Andreas Weigel <andreas.weigel@securepoint.de>
Date: Wed, 18 Oct 2023 04:14:31 +0000
Subject: [PATCH] Fix validation of certificates with CN=* (#1523)
The bug was discovered and detailed by Joshua Rogers at
https://megamansec.github.io/Squid-Security-Audit/
where it was filed as "Buffer UnderRead in SSL CN Parsing".
---
src/anyp/Uri.cc | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/src/anyp/Uri.cc b/src/anyp/Uri.cc
index 77b6f0c92..a6a5d5d9e 100644
--- a/src/anyp/Uri.cc
+++ b/src/anyp/Uri.cc
@@ -173,6 +173,10 @@ urlInitialize(void)
assert(0 == matchDomainName("*.foo.com", ".foo.com", mdnHonorWildcards));
assert(0 != matchDomainName("*.foo.com", "foo.com", mdnHonorWildcards));
+ assert(0 != matchDomainName("foo.com", ""));
+ assert(0 != matchDomainName("foo.com", "", mdnHonorWildcards));
+ assert(0 != matchDomainName("foo.com", "", mdnRejectSubsubDomains));
+
/* more cases? */
}
@@ -756,6 +760,8 @@ matchDomainName(const char *h, const char *d, MatchDomainNameFlags flags)
return -1;
dl = strlen(d);
+ if (dl == 0)
+ return 1;
/*
* Start at the ends of the two strings and work towards the
--
2.25.1

3381
SOURCES/squid-4.15-CVE-2023-46728.patch Normal file
View File

File diff suppressed because it is too large Load Diff

1136
SOURCES/squid-4.15-CVE-2023-46846.patch
View File

File diff suppressed because it is too large Load Diff

22
SOURCES/squid-4.15-CVE-2023-46847.patch
View File

@ -1,28 +1,15 @@
From dc0e10bec3334053c1a5297e50dd7052ea18aef0 Mon Sep 17 00:00:00 2001
From: Alex Bason <nonsleepr@gmail.com>
Date: Sun, 15 Oct 2023 13:04:47 +0000
Subject: [PATCH] Fix stack buffer overflow when parsing Digest Authorization
(#1517)
The bug was discovered and detailed by Joshua Rogers at
https://megamansec.github.io/Squid-Security-Audit/digest-overflow.html
where it was filed as "Stack Buffer Overflow in Digest Authentication".
---
src/auth/digest/Config.cc | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/src/auth/digest/Config.cc b/src/auth/digest/Config.cc
index f00e2ba68..3c070d242 100644
index 6a9736f..0a883fa 100644
--- a/src/auth/digest/Config.cc
+++ b/src/auth/digest/Config.cc
@@ -827,11 +827,15 @@ Auth::Digest::Config::decode(char const *proxy_auth, const HttpRequest *request,
@@ -847,11 +847,15 @@ Auth::Digest::Config::decode(char const *proxy_auth, const char *aRequestRealm)
break;
case DIGEST_NC:
- if (value.size() != 8) {
+ if (value.size() == 8) {
+ // for historical reasons, the nc value MUST be exactly 8 bytes
+ static_assert(sizeof(digest_request->nc) == 8 + 1);
+ static_assert(sizeof(digest_request->nc) == 8 + 1, "bad nc buffer size");
+ xstrncpy(digest_request->nc, value.rawBuf(), value.size() + 1);
+ debugs(29, 9, "Found noncecount '" << digest_request->nc << "'");
+ } else {
@ -34,6 +21,3 @@ index f00e2ba68..3c070d242 100644
break;
case DIGEST_CNONCE:
--
2.25.1

38
SOURCES/squid-4.15-CVE-2023-49285.patch Normal file
View File

@ -0,0 +1,38 @@
commit deee944f9a12c9fd399ce52f3e2526bb573a9470
Author: Alex Rousskov <rousskov@measurement-factory.com>
Date: Wed Oct 25 19:41:45 2023 +0000
RFC 1123: Fix date parsing (#1538)
The bug was discovered and detailed by Joshua Rogers at
https://megamansec.github.io/Squid-Security-Audit/datetime-overflow.html
where it was filed as "1-Byte Buffer OverRead in RFC 1123 date/time
Handling".
Back port upstream patch
Signed-Off-By: tianyue.lan@oracle.com
---
lib/rfc1123.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/lib/rfc1123.c b/lib/rfc1123.c
index 2d889cc..add63f0 100644
--- a/lib/rfc1123.c
+++ b/lib/rfc1123.c
@@ -50,7 +50,13 @@ make_month(const char *s)
char month[3];
month[0] = xtoupper(*s);
+ if (!month[0])
+ return -1; // protects *(s + 1) below
+
month[1] = xtolower(*(s + 1));
+ if (!month[1])
+ return -1; // protects *(s + 2) below
+
month[2] = xtolower(*(s + 2));
for (i = 0; i < 12; i++)
--
2.39.3

88
SOURCES/squid-4.15-CVE-2023-49286.patch Normal file
View File

@ -0,0 +1,88 @@
commit 6014c6648a2a54a4ecb7f952ea1163e0798f9264
Author: Alex Rousskov <rousskov@measurement-factory.com>
Date: Fri Oct 27 21:27:20 2023 +0000
Exit without asserting when helper process startup fails (#1543)
... to dup() after fork() and before execvp().
Assertions are for handling program logic errors. Helper initialization
code already handled system call errors correctly (i.e. by exiting the
newly created helper process with an error), except for a couple of
assert()s that could be triggered by dup(2) failures.
This bug was discovered and detailed by Joshua Rogers at
https://megamansec.github.io/Squid-Security-Audit/ipc-assert.html
where it was filed as 'Assertion in Squid "Helper" Process Creator'.
Back port upstream patch
Signed-Off-By: tianyue.lan@oracle.com
---
src/ipc.cc | 32 ++++++++++++++++++++++++++------
1 file changed, 26 insertions(+), 6 deletions(-)
diff --git a/src/ipc.cc b/src/ipc.cc
index e92a27f..3ddae70 100644
--- a/src/ipc.cc
+++ b/src/ipc.cc
@@ -19,6 +19,11 @@
#include "SquidConfig.h"
#include "SquidIpc.h"
#include "tools.h"
+#include <cstdlib>
+
+#if HAVE_UNISTD_H
+#include <unistd.h>
+#endif
static const char *hello_string = "hi there\n";
#ifndef HELLO_BUF_SZ
@@ -365,6 +370,22 @@ ipcCreate(int type, const char *prog, const char *const args[], const char *name
}
PutEnvironment();
+
+ // A dup(2) wrapper that reports and exits the process on errors. The
+ // exiting logic is only suitable for this child process context.
+ const auto dupOrExit = [prog,name](const int oldFd) {
+ const auto newFd = dup(oldFd);
+ if (newFd < 0) {
+ const auto savedErrno = errno;
+ debugs(54, DBG_CRITICAL, "ERROR: Helper process initialization failure: " << name <<
+ Debug::Extra << "helper (CHILD) PID: " << getpid() <<
+ Debug::Extra << "helper program name: " << prog <<
+ Debug::Extra << "dup(2) system call error for FD " << oldFd << ": " << xstrerr(savedErrno));
+ _exit(EXIT_FAILURE);
+ }
+ return newFd;
+ };
+
/*
* This double-dup stuff avoids problems when one of
* crfd, cwfd, or debug_log are in the rage 0-2.
@@ -372,17 +393,16 @@ ipcCreate(int type, const char *prog, const char *const args[], const char *name
do {
/* First make sure 0-2 is occupied by something. Gets cleaned up later */
- x = dup(crfd);
- assert(x > -1);
- } while (x < 3 && x > -1);
+ x = dupOrExit(crfd);
+ } while (x < 3);
close(x);
- t1 = dup(crfd);
+ t1 = dupOrExit(crfd);
- t2 = dup(cwfd);
+ t2 = dupOrExit(cwfd);
- t3 = dup(fileno(debug_log));
+ t3 = dupOrExit(fileno(debug_log));
assert(t1 > 2 && t2 > 2 && t3 > 2);
--
2.39.3

25
SOURCES/squid-4.15.tar.xz.asc
View File

@ -1,25 +0,0 @@
File: squid-4.15.tar.xz
Date: Mon 10 May 2021 10:50:22 UTC
Size: 2454176
MD5 : a593de9dc888dfeca4f1f7db2cd7d3b9
SHA1: 60bda34ba39657e2d870c8c1d2acece8a69c3075
Key : CD6DBF8EF3B17D3E <squid3@treenet.co.nz>
B068 84ED B779 C89B 044E 64E3 CD6D BF8E F3B1 7D3E
keyring = http://www.squid-cache.org/pgp.asc
keyserver = pool.sks-keyservers.net
-----BEGIN PGP SIGNATURE-----
iQIzBAABCgAdFiEEsGiE7bd5yJsETmTjzW2/jvOxfT4FAmCZD/UACgkQzW2/jvOx
fT6zZg/+N8JMIYpmVJ7jm4lF0Ub2kEHGTOrc+tnlA3LGnlMQuTm61+BYk58g0SKW
96NbJ0cycW215Q34L+Y0tWuxEbIU01vIc3AA7rQd0LKy+fQU0OtBuhk5Vf4bKilW
uHEVIQZs9HmY6bqC+kgtCf49tVZvR8FZYNuilg/68+i/pQdwaDDmVb+j2oF7w+y2
dgkTFWtM5NTL6bqUVC0E7lLFPjzMefKfxkkpWFdV/VrAhU25jN24kpnjcfotQhdW
LDFy5okduz3ljso9pBYJfLeMXM1FZPpceC91zj32x3tcUyrD3yIoXob58rEKvfe4
RDXN4SuClsNe4UQ4oNoGIES9XtaYlOzPR1PlbqPUrdp1cDnhgLJ+1fkAixlMqCml
wuI1VIKSEY+nvRzQzFHnXJK9otV8QwMF76AHaytO9y+X6JuZmu/CcV1pq61qY9qv
t1/8z99wWSxpu17zthZgq64J225GF/hkBedaFlYoS5k5YUMDLPlRSCC0yPmb8JBF
Cns5i/aq2PmOx2ZhQ2RQIF416J3HK8Galw8ytFOjnEcn4ux9yzKNjL38p4+PJJA0
7GCMAqYYNjok3LSkGbiR7cPgbHnkqRfYbPFLMj4FtruoFlZ9L5MIU3oFvqA3ZR6l
Az6LaKLsAYPUmukAOPUSIrqpKXZHc7hdBWkT+7RYA4qaoU+9oIo=
=1Re1
-----END PGP SIGNATURE-----

0
SOURCES/squid.nm Executable file → Normal file
View File

56
SPECS/squid.spec
View File

@ -2,7 +2,7 @@
Name: squid
Version: 4.15
Release: 7%{?dist}.1.alma.1
Release: 7%{?dist}.5
Summary: The Squid proxy caching server
Epoch: 7
# See CREDITS for breakdown of non GPLv2+ code
@ -41,9 +41,6 @@ Patch209: squid-4.15-ftp-filename-extraction.patch
# https://bugzilla.redhat.com/show_bug.cgi?id=2076717
Patch210: squid-4.15-halfclosed.patch
# OL squid-4.15-6.0.1.module+el8.8.0+90059+985ac402
Patch211: 0001-Fix-incremental-parsing-of-chunked-quoted-extensions.patch
# Security fixes
# https://bugzilla.redhat.com/show_bug.cgi?id=1941506
Patch300: squid-4.15-CVE-2021-28116.patch
@ -51,11 +48,26 @@ Patch300: squid-4.15-CVE-2021-28116.patch
Patch301: squid-4.15-CVE-2021-46784.patch
# https://bugzilla.redhat.com/show_bug.cgi?id=2129771
Patch302: squid-4.15-CVE-2022-41318.patch
# OL squid-4.15-6.0.1.module+el8.8.0+90059+985ac402
# https://bugzilla.redhat.com/show_bug.cgi?id=2245910
# +backported: https://github.com/squid-cache/squid/commit/417da4006cf5c97d44e74431b816fc58fec9e270
Patch303: squid-4.15-CVE-2023-46846.patch
# OL squid-4.15-6.0.1.module+el8.8.0+90059+985ac402
# https://bugzilla.redhat.com/show_bug.cgi?id=2245916
Patch304: squid-4.15-CVE-2023-46847.patch
#Oracle patches
Patch1001: 0001-Break-long-store_client-call-chains-with-async-calls.patch
Patch1002: 0002-Remove-serialized-HTTP-headers-from-storeClientCopy.patch
Patch1003: 0003-Bug-5309-frequent-lowestOffset-target_offset-asserti.patch
Patch1004: 0004-Remove-mem_hdr-freeDataUpto-assertion-1562.patch
Patch1005: 0005-Backport-Add-Assure-as-a-replacement-for-problematic.patch
Patch1006: 0006-Backport-additional-functions-for-SquidMath.patch
Patch1007: 0007-Adapt-to-older-gcc-cleanup.patch
Patch1008: squid-4.15-CVE-2023-46724.patch
Patch1009: squid-4.15-CVE-2023-46728.patch
Patch1010: squid-4.15-CVE-2023-49285.patch
Patch1011: squid-4.15-CVE-2023-49286.patch
Requires: bash >= 2.0
Requires(pre): shadow-utils
Requires(post): systemd
@ -117,7 +129,6 @@ lookup program (dnsserver), a program for retrieving FTP data
%patch208 -p1 -b .convert-ipv4
%patch209 -p1 -b .ftp-fn-extraction
%patch210 -p1 -b .halfclosed
%patch211 -p1 -b .fix-incremental-parsing
# Security patches
%patch300 -p1 -b .CVE-2021-28116
@ -126,6 +137,20 @@ lookup program (dnsserver), a program for retrieving FTP data
%patch303 -p1 -b .CVE-2023-46846
%patch304 -p1 -b .CVE-2023-46847
# Oracle patches
%patch1001 -p1
%patch1002 -p1
%patch1003 -p1
%patch1004 -p1
%patch1005 -p1
%patch1006 -p1
%patch1007 -p1
%patch1008 -p1
%patch1009 -p1
%patch1010 -p1
%patch1011 -p1
# https://bugzilla.redhat.com/show_bug.cgi?id=1679526
# Patch in the vendor documentation and used different location for documentation
sed -i 's|@SYSCONFDIR@/squid.conf.documented|%{_pkgdocdir}/squid.conf.documented|' src/squid.8.in
@ -341,9 +366,20 @@ fi
%changelog
* Wed Nov 15 2023 Eduard Abdullin <eabdullin@almalinux.org> - 4.15-7.1
- CVE-2023-46846 RFC 9112: Improve HTTP chunked encoding compliance
- CVE-2023-46847 Fix stack buffer overflow when parsing Digest Authorization
* Wed Jan 03 2024 Tianyue Lan <tianyue.lan@oracle.com> - 7:4.15-7.5
- Fix squid: Denial of Service in SSL Certificate validation (CVE-2023-46724)
- Fix squid: NULL pointer dereference in the gopher protocol code (CVE-2023-46728)
- Fix squid: Buffer over-read in the HTTP Message processing feature (CVE-2023-49285)
- Fix squid: Incorrect Check of Function Return Value In Helper Process management(CVE-2023-49286)
* Sun Dec 09 2023 Alex Burmashev <alexander.burmashev@oracle.com> - 7:4.15-7.3
- Fix squid: DoS against HTTP and HTTPS (CVE-2023-5824)
* Mon Oct 30 2023 Luboš Uhliarik <luhliari@redhat.com> - 7:4.15-7.1
- Resolves: RHEL-14801 - squid: squid: Denial of Service in HTTP Digest
Authentication
- Resolves: RHEL-14776 - squid: squid: Request/Response smuggling in HTTP/1.1
and ICAP
* Wed Aug 16 2023 Luboš Uhliarik <luhliari@redhat.com> - 7:4.15-7
- Resolves: #2076717 - Crash with half_closed_client on