[PATCH] gRPC: fixed bug when padding is used in DATA frame

[geniuss99]

> 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.

Feel free to modify the patch as you need. I merely fixed a bug at the
place of origin.



> (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.)

Yeah, I know. I just wanted to make the code clear and compact so that
you understand everything.
Maybe I shouldn't have named it a patch. You may consider it a
pseudocode then :)



> 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?

I use gRPC module to pass normal http requests to h2 upstreams, not gRPC ones.
Works like a charm.

So can't give you any gRPC servers for testing. You can observe the bug by
connecting to developers.google.com h2 server for example. They use padding
in DATA streams.

On Wed, Mar 17, 2021 at 2:45 AM Maxim Dounin <mdounin at mdounin.ru> wrote:
>
> 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 at 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 at nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel