Skip to content

Fix -Wsign-compare on arm32#2432

Open
kinkie wants to merge 4 commits into
squid-cache:masterfrom
kinkie:fix-cachedigest-overflow-v2
Open

Fix -Wsign-compare on arm32#2432
kinkie wants to merge 4 commits into
squid-cache:masterfrom
kinkie:fix-cachedigest-overflow-v2

Conversation

@kinkie
Copy link
Copy Markdown
Contributor

@kinkie kinkie commented Jun 2, 2026

Due to ssize_t differences on 32/64 bit platforms, changes
to peerDigestSwapInMask in commit 556b91a cause
signedness comparison errors.
Refactor to be safe both on 32- and 64-bit platforms

@kinkie
Copy link
Copy Markdown
Contributor Author

kinkie commented Jun 2, 2026

Error message on gentoo/arm7l

src/peer_digest.cc: In function 'int peerDigestSwapInMask(void*, char*, ssize_t)':
src/peer_digest.cc:562:35: error: comparison of integer expressions of different signedness: 'uint32_t' {aka 'unsigned int'} and 'ssize_t' {aka 'int'} [-Werror=sign-compare]
  562 |     if (fetch->mask_offset + size > static_cast<ssize_t>(pd->cd->mask_size)) {
      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors

see https://github.com/kinkie/dockerfiles/actions/runs/26706938522/job/78709886696

Copy link
Copy Markdown
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for fixing this regression!

src/peer_digest.cc:562:35: error: comparison of integer expressions of different signedness: 'uint32_t' {aka 'unsigned int'} and 'ssize_t' {aka 'int'} [-Werror=sign-compare]

Refactor ... for 32-bit safety ... Due to ssize_t differences on 32/64 bit platforms ...

The jump from "expressions of different signedness" in the error message to "32-bit safety" feels unnecessary here because signedness is different on both 32- and 64-bit platforms AFAICT.

I suggest using something like this for the PR title: Fix -Wsign-compare on arm32.

Please mention the problematic commit SHA in the PR description (or title).

See 4efdc65 for an example.

Comment thread src/peer_digest.cc Outdated
*/
Assure(size >= 0);
if (fetch->mask_offset + size > static_cast<ssize_t>(pd->cd->mask_size)) {
if (static_cast<uint32_t>(size) > pd->cd->mask_size - fetch->mask_offset) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not cast a ssize_t variable to a usually smaller and strangely-precise uint32_t. Cast it to size_t.

The assertion we added above the if statement makes a size_t cast safe. Technically, it does not make the proposed uint32_t cast safe (although it is safe for other reasons).

Suggested change
if (static_cast<uint32_t>(size) > pd->cd->mask_size - fetch->mask_offset) {
if (static_cast<size_t>(size) > pd->cd->mask_size - fetch->mask_offset) {

The change from summation to subtraction is outside this PR scope, but I am not going to object to it because subtraction is slightly better here (than what was originally done in the problematic commit) -- it avoids a suspicion of overflow. I trust you have verified that subtracting is safe here (i.e. that the right hand side expression cannot underflow).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I trust you have verified that subtracting is safe here (i.e. that the right hand side expression cannot underflow).

I have and it looks safe, but if we want to be extra safe we can Assure on it, what do you think?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we want to be extra safe we can Assure on it, what do you think?

Your call, but I think an Assure would be a good idea here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding it. It won't hurt and it may help

@kinkie kinkie changed the title Refactor peerDigestSwapInMask for 32-bit safety Fix -Wsign-compare on arm32 Jun 2, 2026
@kinkie kinkie mentioned this pull request Jun 2, 2026
rousskov
rousskov previously approved these changes Jun 2, 2026
@kinkie kinkie requested a review from rousskov June 2, 2026 20:51
@kinkie kinkie added M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels S-could-use-an-approval An approval may speed this PR merger (but is not required) backport-to-v7 maintainer has approved these changes for v7 backporting labels Jun 2, 2026
@kinkie
Copy link
Copy Markdown
Contributor Author

kinkie commented Jun 2, 2026

@yadij would you mind fast tracking this PR? It's the only blocker I have for releasing 7.6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-to-v7 maintainer has approved these changes for v7 backporting M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels S-could-use-an-approval An approval may speed this PR merger (but is not required)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants