]> git.ipfire.org Git - thirdparty/squid.git/commit
Remove mem_hdr::freeDataUpto() assertion (#1562)
authorAlex Rousskov <rousskov@measurement-factory.com>
Wed, 1 Nov 2023 03:16:12 +0000 (03:16 +0000)
committerSquid Anubis <squid-anubis@squid-cache.org>
Wed, 1 Nov 2023 03:16:15 +0000 (03:16 +0000)
commitc11ee3d0812e7041f395073cf3b2f368f1caf26d
tree6dbf28f59636629bc1c0caa09a9c13f505efe1ef
parent3a5c8b44ecc1fcbe03c4ff04bbeecad28eafd634
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).
src/stmem.cc