Remove mem_hdr::freeDataUpto() assertion#1562
Closed
rousskov wants to merge 1 commit intosquid-cache:masterfrom
Closed
Remove mem_hdr::freeDataUpto() assertion#1562rousskov wants to merge 1 commit intosquid-cache:masterfrom
rousskov wants to merge 1 commit intosquid-cache:masterfrom
Conversation
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).
kinkie
approved these changes
Oct 31, 2023
Contributor
kinkie
left a comment
There was a problem hiding this comment.
Thanks for the ample context!
yadij
approved these changes
Oct 31, 2023
squid-anubis
pushed a commit
that referenced
this pull request
Nov 1, 2023
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).
Merged
squidadm
pushed a commit
to squidadm/squid
that referenced
this pull request
Nov 1, 2023
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).
yadij
pushed a commit
that referenced
this pull request
Nov 1, 2023
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).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
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.
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).