From 422272d78399d5fb2fc340281611961fc7c528e7 Mon Sep 17 00:00:00 2001 From: Alex Rousskov 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 Signed-off-by: Alex Burmashev --- 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