Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NetHandler - Use the right type to hold additional_accepts config value. #11659

Merged
merged 1 commit into from
Aug 20, 2024

Conversation

brbzull0
Copy link
Contributor

@brbzull0 brbzull0 commented Aug 7, 2024

proxy.config.net.additional_accepts is expected to use -1 but the code wasn't storing the value into a signed type, so changed from unsigned to signed as this is what's expected.

This PR also makes sure the records validity check passes.

Original PR -> #10098

Part of a fix for #11649

Changed from unsigned to signed as this is what's expected. This PR also
makes sure the records validity check passes.
@brbzull0 brbzull0 added Network Bug Records Records related code. labels Aug 7, 2024
@brbzull0 brbzull0 added this to the 10.1.0 milestone Aug 7, 2024
@brbzull0 brbzull0 self-assigned this Aug 7, 2024
@sharkleash
Copy link
Contributor

sharkleash commented Aug 12, 2024

Thanks for finding this, would the behavior be the same in this case? It is definitely a bug that additional_accepts isn't a signed int, but doesn't setting an unsigned int to -1, make it equal to 2^32 - 1 (INT32_MAX - 1)? In that case, there's a section of code in NetHandler.cc that adds 1 to that, making it overflow to 0, and then sets it to 2^32 - 1 anyway.

this is that code:
int config_value = additional_accepts.load(std::memory_order_relaxed) + 1;
return (config_value > 0 ? config_value : INT32_MAX - 1);

It's certainly a bug, but if the behavior is the same, maybe this doesn't need to go into 10.0.x?

@brbzull0
Copy link
Contributor Author

Thanks for finding this, would the behavior be the same in this case?

No bother, thanks for looking at it. Yes, same behavior.

It's certainly a bug, but if the behavior is the same, maybe this doesn't need to go into 10.0.x?

Well, it partialy depends on where #11667 lands, if #11667 lands in 10 then this should also land in 10, at least part of this, to avoid:

traffic_server ERROR: proxy.config.net.additional_accepts: Default value consistency check failed. default value=-1 pattern=^-1|[0-9]+$.

Thanks.

@brbzull0 brbzull0 merged commit 3a2177a into apache:master Aug 20, 2024
15 checks passed
@cmcfarlen cmcfarlen modified the milestones: 10.1.0, 10.0.0 Aug 20, 2024
@cmcfarlen
Copy link
Contributor

Cherry-picked to v10.0.x

cmcfarlen pushed a commit that referenced this pull request Aug 20, 2024
…ue. (#11659)

Changed from unsigned to signed as this is what's expected. This PR also
makes sure the records validity check passes.

(cherry picked from commit 3a2177a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Network Records Records related code.
Projects
Status: picked-10.0.0
Development

Successfully merging this pull request may close these issues.

4 participants