WCCP: Fix offset calculation when parsing incoming packets - #2480
WCCP: Fix offset calculation when parsing incoming packets#2480askolesnik wants to merge 1 commit into
Conversation
squid-anubis
added
the
M-failed-description
This comment was marked as resolved.
This comment was marked as resolved.
rousskov
left a comment
rousskov
left a comment
There was a problem hiding this comment.
@jert4803-droid, thank you for working on this bug fix.
Please adjust your PR branch to be based on the current official master branch. Without that adjustment, this PR modifies more than one thousand files, including (but not limited to) numerous unwanted Copyright line changes.
Please also add your entry to CONTRIBUTORS.
N.B. #970 contains the same code change, but that PR has been dormant since September 2023 (and no author activity since September 2022). I am not against merging this simple non-controversial fix first/now. This PR probably does not fix all Bug 5179 problems, but is a step forward.
rousskov
added
the
S-waiting-for-author
CheckFieldDataLength() computed the field header size with sizeof(header), where header is a pointer parameter, so it always evaluated to the size of a pointer rather than the size of the pointed-to FieldHeader struct; for the 4-byte header types (wccp2_item_header_t, wccp2_capability_info_header_t) this yields a wrong offset on any 64-bit build, shifting dataStart and the returned field size by 4 bytes and desynchronizing the parse of subsequent WCCPv2 packet items, while for wccp2_capability_element_t it only happens to be correct on platforms where pointers are 8 bytes; using sizeof(*header) fixes the computation for every instantiation regardless of pointer width. Signed-off-by: Andrey Kolesnik <akolesnik@astralinux.ru>
9b80208 to
c625171
Compare
|
@rousskov Thanks for the feedback ! |
rousskov
changed the title
squid-anubis
removed
the
M-failed-description
rousskov
left a comment
rousskov
left a comment
There was a problem hiding this comment.
Thank you for adjusting your PR. Please check whether update PR title/description match your changes. They will form a commit message when this PR is merged. Adjusted as needed.
rousskov
added
the
S-could-use-an-approval
|
@rousskov Yes, the title and description match the changes. Could you tell me if there are any requirements for the content of the title and description? Please provide recommendations or a link to information so I can take this into account for future changes. My original title and description didn't work. |
rousskov
added
M-cleared-for-merge
kinkie
added
the
backport-to-v7
Since 2021 commit 464223c, WCCPv2 packet parsing code was using wrong offsets due to a `CheckFieldDataLength()` bug: * `wccp2_item_header_t` and `wccp2_capability_info_header_t`: The function returned a wrong offset on any 64-bit build, shifting dataStart and the returned field size by 4 bytes and desynchronizing the parse of subsequent WCCPv2 packet items; * `wccp2_capability_element_t`: The function happened to return a correct offset only on platforms with 8-byte pointers. This change is a reference point for automated CONTRIBUTORS updates.
squid-anubis
added
the
M-waiting-staging-checks
squid-anubis
added
M-merged
squidadm
removed
the
backport-to-v7
|
queued for backport to v7 |
5 participants
Since 2021 commit 464223c, WCCPv2 packet parsing code was using
wrong offsets due to a
CheckFieldDataLength()bug:wccp2_item_header_tandwccp2_capability_info_header_t: Thefunction returned a wrong offset on any 64-bit build, shifting
dataStart and the returned field size by 4 bytes and desynchronizing
the parse of subsequent WCCPv2 packet items;
wccp2_capability_element_t: The function happened to return acorrect offset only on platforms with 8-byte pointers.
This change is a reference point for automated CONTRIBUTORS updates.