[PATCH] HTTP/2: fix flow control with padded DATA frames

[Piotr Sikora]

Hey Valentin,

>> @@ -802,33 +805,32 @@ ngx_http_v2_state_data(ngx_http_v2_conne
>>          }
>>
>>          h2c->state.padding = *pos++;
>> -        h2c->state.length--;
>> -
>> -        if (h2c->state.padding > h2c->state.length) {
>> +
>> +        if (h2c->state.padding >= size) {
>>              ngx_log_error(NGX_LOG_INFO, h2c->connection->log, 0,
>>                            "client sent padded DATA frame "
>>                            "with incorrect length: %uz, padding: %uz",
>> -                          h2c->state.length, h2c->state.padding);
>> +                          size, h2c->state.padding);
>>
>>              return ngx_http_v2_connection_error(h2c, NGX_HTTP_V2_SIZE_ERROR);
>>          }
>>
>> -        h2c->state.length -= h2c->state.padding;
>> +        h2c->state.length -= 1 + h2c->state.padding;
>>      }
>
> IMHO, the previous version of this fragment with explicit h2c->state.length
> decrement right after reading the padding size and "> h2c->state.length"
> condition is more readable.
>
> YMMV.

As was pointed out during internal code review, "h2c->state.padding >=
size" follows wording from RFC7540, i.e.

   If the length of the padding is the length of the
   frame payload or greater, the recipient MUST treat this as a
   connection error (Section 5.4.1) of type PROTOCOL_ERROR.

and matching code makes it easier to avoid off-by-one errors while
mentally translating this logic to the code.

Also, doing "h2c->state.padding > h2c->state.length" check and using
"size" value in the error log feels kind of weird.

Having said that, it's a matter of preference, so please explicitly
ask if you want me to change it back, and I'll send updated patch
shortly.

Best regards,
Piotr Sikora