apache / trafficserver

Apache Traffic Serverâ„¢ is a fast, scalable and extensible HTTP/1.1 and HTTP/2 compliant caching proxy server.
https://trafficserver.apache.org/
Apache License 2.0
1.8k stars 798 forks source link

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

Closed brbzull0 closed 1 month ago

brbzull0 commented 1 month ago

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 -> https://github.com/apache/trafficserver/pull/10098

Part of a fix for https://github.com/apache/trafficserver/issues/11649

sharkleash commented 1 month ago

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 commented 1 month ago

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 https://github.com/apache/trafficserver/pull/11667 lands, if https://github.com/apache/trafficserver/pull/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.

cmcfarlen commented 1 month ago

Cherry-picked to v10.0.x