ngx_http_discard_request_body may read incomplete data?

Maxim Dounin mdounin at mdounin.ru
Tue Sep 27 08:04:02 UTC 2011


Hello!

On Tue, Sep 27, 2011 at 02:48:15PM +0800, Simon Liu wrote:

> change: use ngx_int_t replace int.
> 
> Index: src/http/ngx_http_request_body.c
> ===================================================================
> --- src/http/ngx_http_request_body.c    (revision 4146)
> +++ src/http/ngx_http_request_body.c    (working copy)
> @@ -438,6 +438,7 @@
>  ngx_http_discard_request_body(ngx_http_request_t *r)
>  {
>      ssize_t       size;
> +    ngx_int_t     rc;
>      ngx_event_t  *rev;
> 
>      if (r != r->main || r->discard_body) {
> @@ -476,11 +477,13 @@
> 
>      r->read_event_handler = ngx_http_discarded_request_body_handler;
> 
> -    if (ngx_handle_read_event(rev, 0) != NGX_OK) {
> -        return NGX_HTTP_INTERNAL_SERVER_ERROR;
> +    rc = ngx_http_read_discarded_request_body(r);
> +
> +    if (rc == NGX_ERROR) {
> +        return NGX_HTTP_CLIENT_CLOSED_REQUEST;

I don't think that returing NGX_HTTP_CLIENT_CLOSED_REQUEST here is 
a good idea.  There are clients out there which half-close 
connection after sending request, and this would break such 
clients.

Maxim Dounin

>      }
> 
> -    if (ngx_http_read_discarded_request_body(r) == NGX_OK) {
> +    if (rc == NGX_OK) {
>          r->lingering_close = 0;
> 
>      } else {
> @@ -488,6 +491,10 @@
>          r->discard_body = 1;
>      }
> 
> +    if (ngx_handle_read_event(rev, 0) != NGX_OK) {
> +        return NGX_HTTP_INTERNAL_SERVER_ERROR;
> +    }
> +
>      return NGX_OK;
>  }
> 
> @@ -527,6 +534,11 @@
> 
>      rc = ngx_http_read_discarded_request_body(r);
> 
> +    if (rc == NGX_ERROR) {
> +        ngx_http_finalize_request(r, NGX_HTTP_CLIENT_CLOSED_REQUEST);
> +        return;
> +    }
> +
>      if (rc == NGX_OK) {
>          r->discard_body = 0;
>          r->lingering_close = 0;
> @@ -583,19 +595,15 @@
> 
>          n = r->connection->recv(r->connection, buffer, size);
> 
> -        if (n == NGX_ERROR) {
> +        if (n == 0 || n == NGX_ERROR) {
>              r->connection->error = 1;
> -            return NGX_OK;
> +            return NGX_ERROR;
>          }
> 
>          if (n == NGX_AGAIN) {
>              return NGX_AGAIN;
>          }
> 
> -        if (n == 0) {
> -            return NGX_OK;
> -        }
> -
>          r->headers_in.content_length_n -= n;
>      }
>  }
> 
> 
> On Tue, Sep 27, 2011 at 2:42 PM, Simon Liu <simohayha.bobo at gmail.com> wrote:
> 
> > Sorry, I have mistake.
> >
> > This is correct patch:
> >
> >
> > Index: src/http/ngx_http_request_body.c
> > ===================================================================
> > --- src/http/ngx_http_request_body.c    (revision 4146)
> > +++ src/http/ngx_http_request_body.c    (working copy)
> > @@ -437,6 +437,7 @@
> >  ngx_int_t
> >  ngx_http_discard_request_body(ngx_http_request_t *r)
> >  {
> > +    int           rc;
> >      ssize_t       size;
> >      ngx_event_t  *rev;
> >
> > @@ -476,11 +477,13 @@
> >
> >
> >      r->read_event_handler = ngx_http_discarded_request_body_handler;
> >
> > -    if (ngx_handle_read_event(rev, 0) != NGX_OK) {
> > -        return NGX_HTTP_INTERNAL_SERVER_ERROR;
> > +    rc = ngx_http_read_discarded_request_body(r);
> > +
> > +    if (rc == NGX_ERROR) {
> > +        return NGX_HTTP_CLIENT_CLOSED_REQUEST;
> >      }
> >
> > -    if (ngx_http_read_discarded_request_body(r) == NGX_OK) {
> > +    if (rc == NGX_OK) {
> >          r->lingering_close = 0;
> >
> >      } else {
> > @@ -488,6 +491,10 @@
> >
> >          r->discard_body = 1;
> >      }
> >
> > +    if (ngx_handle_read_event(rev, 0) != NGX_OK) {
> > +        return NGX_HTTP_INTERNAL_SERVER_ERROR;
> > +    }
> > +
> >      return NGX_OK;
> >  }
> >
> > @@ -527,6 +534,11 @@
> >
> >
> >      rc = ngx_http_read_discarded_request_body(r);
> >
> > +    if (rc == NGX_ERROR) {
> > +        ngx_http_finalize_request(r, NGX_HTTP_CLIENT_CLOSED_REQUEST);
> > +        return;
> > +    }
> > +
> >      if (rc == NGX_OK) {
> >          r->discard_body = 0;
> >          r->lingering_close = 0;
> > @@ -583,19 +595,15 @@
> >
> >
> >          n = r->connection->recv(r->connection, buffer, size);
> >
> > -        if (n == NGX_ERROR) {
> > +        if (n == 0 || n == NGX_ERROR) {
> >              r->connection->error = 1;
> > -            return NGX_OK;
> > +            return NGX_ERROR;
> >          }
> >
> >          if (n == NGX_AGAIN) {
> >              return NGX_AGAIN;
> >          }
> >
> > -        if (n == 0) {
> > -            return NGX_OK;
> > -        }
> > -
> >          r->headers_in.content_length_n -= n;
> >      }
> >  }
> >
> >
> >
> >  On Tue, Sep 27, 2011 at 2:33 PM, Simon Liu <simohayha.bobo at gmail.com>wrote:
> >
> >> Thanks Maxim.
> >>
> >> This is new patch, I think this may be better.
> >>
> >> I think ngx_handle_read_event can't be avoid(in
> >> ngx_http_discard_request_body), because it doesn't known whether or not
> >> need to add read event in ngx_http_discard_request_body. So there  need
> >> ngx_handle_read_event to judge.
> >>
> >>
> >> Index: src/http/ngx_http_request_body.c
> >> ===================================================================
> >> --- src/http/ngx_http_request_body.c    (revision 4146)
> >> +++ src/http/ngx_http_request_body.c    (working copy)
> >>  @@ -476,11 +476,13 @@
> >>
> >>
> >>      r->read_event_handler = ngx_http_discarded_request_body_handler;
> >>
> >> -    if (ngx_handle_read_event(rev, 0) != NGX_OK) {
> >> -        return NGX_HTTP_INTERNAL_SERVER_ERROR;
> >> +    rc = ngx_http_read_discarded_request_body(r);
> >> +
> >> +    if (rc == NGX_ERROR) {
> >> +        return NGX_HTTP_CLIENT_CLOSED_REQUEST;
> >>      }
> >>
> >> -    if (ngx_http_read_discarded_request_body(r) == NGX_OK) {
> >> +    if (rc == NGX_OK) {
> >>          r->lingering_close = 0;
> >>
> >>      } else {
> >> @@ -488,6 +490,10 @@
> >>
> >>          r->discard_body = 1;
> >>      }
> >>
> >> +    if (ngx_handle_read_event(rev, 0) != NGX_OK) {
> >> +        return NGX_HTTP_INTERNAL_SERVER_ERROR;
> >> +    }
> >> +
> >>      return NGX_OK;
> >>  }
> >>
> >> @@ -527,6 +533,11 @@
> >>
> >>      rc = ngx_http_read_discarded_request_body(r);
> >>
> >> +    if (rc == NGX_ERROR) {
> >> +        ngx_http_finalize_request(r, NGX_HTTP_CLIENT_CLOSED_REQUEST);
> >> +        return;
> >> +    }
> >> +
> >>      if (rc == NGX_OK) {
> >>          r->discard_body = 0;
> >>          r->lingering_close = 0;
> >> @@ -583,19 +594,15 @@
> >>
> >>          n = r->connection->recv(r->connection, buffer, size);
> >>
> >> -        if (n == NGX_ERROR) {
> >> +        if (n == 0 || n == NGX_ERROR) {
> >>              r->connection->error = 1;
> >> -            return NGX_OK;
> >> +            return NGX_ERROR;
> >>          }
> >>
> >>          if (n == NGX_AGAIN) {
> >>              return NGX_AGAIN;
> >>          }
> >>
> >> -        if (n == 0) {
> >> -            return NGX_OK;
> >> -        }
> >> -
> >>          r->headers_in.content_length_n -= n;
> >>
> >>      }
> >>  }
> >>
> >>
> >> On Mon, Sep 26, 2011 at 11:50 PM, Maxim Dounin <mdounin at mdounin.ru>wrote:
> >>
> >>> Hello!
> >>>
> >>> On Mon, Sep 26, 2011 at 11:34:42PM +0800, Simon Liu wrote:
> >>>
> >>> > Thanks .
> >>> >
> >>> > This is patch:
> >>> >
> >>> > Index: src/http/ngx_http_request_body.c
> >>> > ===================================================================
> >>> > --- src/http/ngx_http_request_body.c    (revision 4146)
> >>> > +++ src/http/ngx_http_request_body.c    (working copy)
> >>> > @@ -476,10 +476,6 @@
> >>> >
> >>> >      r->read_event_handler = ngx_http_discarded_request_body_handler;
> >>> >
> >>> > -    if (ngx_handle_read_event(rev, 0) != NGX_OK) {
> >>> > -        return NGX_HTTP_INTERNAL_SERVER_ERROR;
> >>> > -    }
> >>> > -
> >>> >      if (ngx_http_read_discarded_request_body(r) == NGX_OK) {
> >>> >          r->lingering_close = 0;
> >>> >
> >>> > @@ -488,6 +484,10 @@
> >>> >          r->discard_body = 1;
> >>> >      }
> >>> >
> >>> > +    if (ngx_handle_read_event(rev, 0) != NGX_OK) {
> >>> > +        return NGX_HTTP_INTERNAL_SERVER_ERROR;
> >>> > +    }
> >>> > +
> >>> >      return NGX_OK;
> >>> >  }
> >>>
> >>> It's better to avoid ngx_handle_read_event() altogether in
> >>> case ngx_http_read_discarded_request_body() returns NGX_OK (i.e.
> >>> everything is read, no futher actions required).
> >>>
> >>> It would be a bit more readable and slightly more optimal.
> >>>
> >>> Maxim Dounin
> >>>
> >>> >
> >>> >
> >>> > On Mon, Sep 26, 2011 at 11:08 PM, Maxim Dounin <mdounin at mdounin.ru>
> >>> wrote:
> >>> >
> >>> > > Hello!
> >>> > >
> >>> > > On Mon, Sep 26, 2011 at 10:42:20PM +0800, Simon Liu wrote:
> >>> > >
> >>> > > > Hi all.
> >>> > > >
> >>> > > > My nginx version is svn trunk.
> >>> > > >
> >>> > > > Nginx will call ngx_handle_read_event, and then
> >>> > > > call ngx_http_read_discarded_request_body In
> >>> > > ngx_http_discard_request_body .
> >>> > > >  So this will cause client's body read  incomplete data , because
> >>> > > > ngx_handle_read_event
> >>> > > > may delete read event when use level event.
> >>> > > >
> >>> > > > ngx_handle_read_event:
> >>> > > >
> >>> > > > else if (ngx_event_flags & NGX_USE_LEVEL_EVENT) {
> >>> > > >
> >>> > > >         /* select, poll, /dev/poll */
> >>> > > > .................................................................
> >>> > > >
> >>> > > >         if (rev->active && (rev->ready || (flags &
> >>> NGX_CLOSE_EVENT))) {
> >>> > > >             if (ngx_del_event(rev, NGX_READ_EVENT, NGX_LEVEL_EVENT
> >>> |
> >>> > > flags)
> >>> > > >                 == NGX_ERROR)
> >>> > > >             {
> >>> > > >                 return NGX_ERROR;
> >>> > > >             }
> >>> > > >
> >>> > > >             return NGX_OK;
> >>> > > >         }
> >>> > > >
> >>> > > >
> >>> > > > ngx_http_discard_request_body:
> >>> > > >
> >>> > > >     r->read_event_handler =
> >>> ngx_http_discarded_request_body_handler;
> >>> > > >
> >>> > > >     if (ngx_handle_read_event(rev, 0) != NGX_OK) {
> >>> > > >         return NGX_HTTP_INTERNAL_SERVER_ERROR;
> >>> > > >     }
> >>> > > >
> >>> > > >     if (ngx_http_read_discarded_request_body(r) == NGX_OK) {
> >>> > > >         r->lingering_close = 0;
> >>> > > >
> >>> > > >
> >>> > > > thanks!
> >>> > >
> >>> > > Ack, ngx_handle_read_event() should be after
> >>> > > ngx_http_read_discarded_request_body() call, like it's done in
> >>> > > ngx_http_discarded_request_body_handler().
> >>> > >
> >>> > > Care to provide patch?
> >>> > >
> >>> > > Maxim Dounin
> >>> > >
> >>> > > _______________________________________________
> >>> > > nginx-devel mailing list
> >>> > > nginx-devel at nginx.org
> >>> > > http://mailman.nginx.org/mailman/listinfo/nginx-devel
> >>> > >
> >>>
> >>> > Index: src/http/ngx_http_request_body.c
> >>> > ===================================================================
> >>> > --- src/http/ngx_http_request_body.c  (revision 4146)
> >>> > +++ src/http/ngx_http_request_body.c  (working copy)
> >>> > @@ -476,10 +476,6 @@
> >>> >
> >>> >      r->read_event_handler = ngx_http_discarded_request_body_handler;
> >>> >
> >>> > -    if (ngx_handle_read_event(rev, 0) != NGX_OK) {
> >>> > -        return NGX_HTTP_INTERNAL_SERVER_ERROR;
> >>> > -    }
> >>> > -
> >>> >      if (ngx_http_read_discarded_request_body(r) == NGX_OK) {
> >>> >          r->lingering_close = 0;
> >>> >
> >>> > @@ -488,6 +484,10 @@
> >>> >          r->discard_body = 1;
> >>> >      }
> >>> >
> >>> > +    if (ngx_handle_read_event(rev, 0) != NGX_OK) {
> >>> > +        return NGX_HTTP_INTERNAL_SERVER_ERROR;
> >>> > +    }
> >>> > +
> >>> >      return NGX_OK;
> >>> >  }
> >>> >
> >>>
> >>> > _______________________________________________
> >>> > nginx-devel mailing list
> >>> > nginx-devel at nginx.org
> >>> > http://mailman.nginx.org/mailman/listinfo/nginx-devel
> >>>
> >>> _______________________________________________
> >>> nginx-devel mailing list
> >>> nginx-devel at nginx.org
> >>> http://mailman.nginx.org/mailman/listinfo/nginx-devel
> >>>
> >>
> >>
> >>
> >> --
> >> douban:www.douban.com/people/mustang/
> >>
> >>
> >> blog: www.pagefault.info
> >>
> >> twitter: www.twitter.com/minibobo
> >>
> >> weibo:  www.weibo.com/diaoliang
> >>
> >>
> >
> >
> > --
> > 博观约取
> >
> > 豆瓣:www.douban.com/people/mustang/
> >
> >
> > blog: www.pagefault.info
> >
> > twitter: www.twitter.com/minibobo
> >
> > sina 微博:  www.weibo.com/diaoliang
> >
> >
> 
> 
> -- 
> 
> douban:www.douban.com/people/mustang/
> 
> blog: www.pagefault.info
> 
> twitter: www.twitter.com/minibobo
> 
> sina:  www.weibo.com/diaoliang

> Index: src/http/ngx_http_request_body.c
> ===================================================================
> --- src/http/ngx_http_request_body.c	(revision 4146)
> +++ src/http/ngx_http_request_body.c	(working copy)
> @@ -438,6 +438,7 @@
>  ngx_http_discard_request_body(ngx_http_request_t *r)
>  {
>      ssize_t       size;
> +    ngx_int_t     rc;
>      ngx_event_t  *rev;
>  
>      if (r != r->main || r->discard_body) {
> @@ -476,11 +477,13 @@
>  
>      r->read_event_handler = ngx_http_discarded_request_body_handler;
>  
> -    if (ngx_handle_read_event(rev, 0) != NGX_OK) {
> -        return NGX_HTTP_INTERNAL_SERVER_ERROR;
> +    rc = ngx_http_read_discarded_request_body(r);
> +
> +    if (rc == NGX_ERROR) {
> +        return NGX_HTTP_CLIENT_CLOSED_REQUEST;
>      }
>  
> -    if (ngx_http_read_discarded_request_body(r) == NGX_OK) {
> +    if (rc == NGX_OK) {
>          r->lingering_close = 0;
>  
>      } else {
> @@ -488,6 +491,10 @@
>          r->discard_body = 1;
>      }
>  
> +    if (ngx_handle_read_event(rev, 0) != NGX_OK) {
> +        return NGX_HTTP_INTERNAL_SERVER_ERROR;
> +    }
> +
>      return NGX_OK;
>  }
>  
> @@ -527,6 +534,11 @@
>  
>      rc = ngx_http_read_discarded_request_body(r);
>  
> +    if (rc == NGX_ERROR) {
> +        ngx_http_finalize_request(r, NGX_HTTP_CLIENT_CLOSED_REQUEST);
> +        return;
> +    }
> +
>      if (rc == NGX_OK) {
>          r->discard_body = 0;
>          r->lingering_close = 0;
> @@ -583,19 +595,15 @@
>  
>          n = r->connection->recv(r->connection, buffer, size);
>  
> -        if (n == NGX_ERROR) {
> +        if (n == 0 || n == NGX_ERROR) {
>              r->connection->error = 1;
> -            return NGX_OK;
> +            return NGX_ERROR;
>          }
>  
>          if (n == NGX_AGAIN) {
>              return NGX_AGAIN;
>          }
>  
> -        if (n == 0) {
> -            return NGX_OK;
> -        }
> -
>          r->headers_in.content_length_n -= n;
>      }
>  }

> _______________________________________________
> nginx-devel mailing list
> nginx-devel at nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel



More information about the nginx-devel mailing list