Correct handling of unsigned HTTP/2 SETTINGS values - #713
arturobernalg wants to merge 1 commit into
Conversation
| final H2Param param = H2Param.valueOf(code); | ||
| if (param != null) { | ||
| validateSetting(param, value); | ||
| final int boundedValue = value < 0 ? Integer.MAX_VALUE : value; |
There was a problem hiding this comment.
@arturobernalg Are you sure this is the right thing to do? This just reduced 32 bit value to 31 bit and makes it impossible to distinguish with the highest bit set. Java signed long is all we have unless we want to use BigInteger to represent the setting value with full 32 bit range.
What does bounding of the setting value to 31 bit really give us?
There was a problem hiding this comment.
@arturobernalg Are you sure this is the right thing to do? This just reduced 32 bit value to 31 bit and makes it impossible to distinguish with the highest bit set. Java signed
longis all we have unless we want to use BigInteger to represent the setting value with full 32 bit range.What does bounding of the setting value to 31 bit really give us?
@ok2c Good point. I’ve made the unsigned conversion explicit and only clamp when mapping the wire value into the existing int-based H2Config.
| if (param != null) { | ||
| validateSetting(param, value); | ||
| final long unsignedValue = Integer.toUnsignedLong(value); | ||
| final int effectiveValue = (int) Math.min(unsignedValue, Integer.MAX_VALUE); |
There was a problem hiding this comment.
@arturobernalg This still does not look right. We should be using long value instead of truncated int unless that value is really known to hold 2^31−1 values only.
a358e5b to
c79252c
Compare
|
@arturobernalg I am honestly not sure I understand the problem you are trying to solve here. What is the problem with the setting value being represented by signed int? What is important that for any kind of arithmetic operations it need to be converted to long with Integer#toUnsignedLong |
@ok2c I conflated the signed representation with the unsigned interpretation; I’ll keep the raw 32-bit value as int and only use Integer.toUnsignedLong where numeric comparison or arithmetic is required. |
67efe43 to
1ed120d
Compare
2 participants
RFC 9113 defines SETTINGS values as unsigned 32-bit integers. Values with the high bit set are currently read as negative Java
intvalues and rejected forSETTINGS_HEADER_TABLE_SIZE,SETTINGS_MAX_CONCURRENT_STREAMS, andSETTINGS_MAX_HEADER_LIST_SIZE.Accept the full unsigned wire range for these settings and bound values above
Integer.MAX_VALUEto the internalH2Configrepresentation.SETTINGS_INITIAL_WINDOW_SIZEis intentionally unchanged: values above2^31-1continue to produceFLOW_CONTROL_ERROR.SETTINGS_MAX_FRAME_SIZEalso retains its RFC-defined range.RFC 9113 §2.2, §6.5.1 and §6.5.2.