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
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
