Hello!
On Tue, Mar 16, 2021 at 10:04:27PM +0300, geniuss99 wrote:
> src/http/modules/ngx_http_grpc_module.c | 16 +++++++++++++---
> 1 files changed, 13 insertions(+), 3 deletions(-)
>
>
> # HG changeset patch
> # User geniuss99 <geniuss.dev@gmail.com>
> # Date 1615921026 -10800
> # Tue Mar 16 21:57:06 2021 +0300
> # Node ID 13552d5b785104f9d137d956a6cbef25ec09b345
> # Parent ed1348e8e25381b3b1a2540289effcf7ccec6fd6
> gRPC: fixed bug when padding is used in DATA frame.
>
> As per RFC 7540 DATA frame MAY contain padding if the PADDED flag is set
> in "Type" field (see clause 6.1).
>
> When such frame is sent from an upstream which previously defined payload
> length via "Content-Length" header nginx fails with an error: "upstream sent
> response body larger than indicated content length".
>
> This happens because padding is not taken into account while comparing expected
> and received payload length.
Thanks for reporting this, looks like a bug introduced by me in
7680:39501ce97e29 (nginx 1.19.1).
Do you have any particular servers/gRPC libraries in mind which
actually use padding on DATA frames, and so nginx cannot talk with
them after introduction of the check in question?
>
> diff -r ed1348e8e253 -r 13552d5b7851 src/http/modules/ngx_http_grpc_module.c
> --- a/src/http/modules/ngx_http_grpc_module.c Thu Mar 11 20:49:39 2021 +0300
> +++ b/src/http/modules/ngx_http_grpc_module.c Tue Mar 16 21:57:06 2021 +0300
> @@ -2075,14 +2075,24 @@
> }
>
> if (ctx->length != -1) {
> - if ((off_t) ctx->rest > ctx->length) {
> + if (ctx->flags & NGX_HTTP_V2_PADDED_FLAG) {
> + if (b->pos < b->last) {
> + u_char pad_length = *b->pos;
> + size_t payload_data_length = ctx->rest - pad_length - 1; // frame_payload_size_bytes - pad_length_bytes - pad_length_field_1_byte
> +
> + ctx->length -= payload_data_length;
> + }
> +
> + } else {
> + ctx->length -= ctx->rest;
> + }
> +
> + if (ctx->length < 0) {
> ngx_log_error(NGX_LOG_ERR, r->connection->log, 0,
> "upstream sent response body larger "
> "than indicated content length");
> return NGX_ERROR;
> }
> -
> - ctx->length -= ctx->rest;
> }
>
> if (ctx->rest > ctx->recv_window) {
The fix looks suboptimal. Rather, the check should be moved to
the end of the function, where padding is already handled. I'll
take a look how to fix this properly.
(Note well that as per nginx style, local variables are defined at
the start of the function, "//" comments are not used, and maximum
line length is 80 characters. The "Contributing Changes"
article[1] have additional hints and links to appropriate part of
the development guide with the details.)
[1] http://nginx.org/en/docs/contributing_changes.html
--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
