68 lines
3.0 KiB
Diff
68 lines
3.0 KiB
Diff
|
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
|
||
|
|
|