Correct handling of unsigned HTTP/2 SETTINGS values - #713
arturobernalg wants to merge 1 commit into
Conversation
| 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
|
|
||
| if (connOutputWindow.get() > 0 && remoteSettingState == SettingsHandshake.ACKED) { | ||
| produceOutput(); | ||
| } else { |
There was a problem hiding this comment.
@arturobernalg Please be careful. You are reverting changes from a previous fix.
| final H2Stream stream = it.next(); | ||
| final int activeStreamId = stream.getId(); | ||
| if (streams.isSameSide(activeStreamId) && activeStreamId > processedLocalStreamId) { | ||
| if (!streams.isSameSide(activeStreamId) && activeStreamId > processedLocalStreamId) { |
There was a problem hiding this comment.
@arturobernalg Please be careful. You are reverting changes from a previous fix
| private int lowMark; | ||
|
|
||
| private volatile H2Config remoteConfig; | ||
| private volatile int remoteHeaderTableSize; |
There was a problem hiding this comment.
@arturobernalg Why do you need these variable at all? I do not get it. You can still use their signed int representation from H2Config and convert it to unsigned long only when doing some calculations.
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.