ngx_http_discard_request_body may read incomplete data?
Simon Liu
simohayha.bobo at gmail.com
Tue Sep 27 12:40:12 UTC 2011
Thanks Maxim.
I ignore half-close connection, so fix this.
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,18 +477,37 @@
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_DONE || rc == NGX_OK) {
r->lingering_close = 0;
+ }
- } else {
+ if (rc == NGX_OK) {
+ if ((ngx_event_flags & NGX_USE_LEVEL_EVENT) && rev->active) {
+ if (ngx_del_event(rev, NGX_READ_EVENT, 0) != NGX_OK) {
+ return NGX_HTTP_INTERNAL_SERVER_ERROR;
+ }
+ }
+
+ return NGX_OK;
+ }
+
+ if (rc == NGX_AGAIN) {
r->count++;
r->discard_body = 1;
}
+ /* NGX_AGAIN or NGX_DONE */
+
+ if (ngx_handle_read_event(rev, 0) != NGX_OK) {
+ return NGX_HTTP_INTERNAL_SERVER_ERROR;
+ }
+
return NGX_OK;
}
@@ -527,7 +547,22 @@
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) {
+ if ((ngx_event_flags & NGX_USE_LEVEL_EVENT) && rev->active) {
+ if (ngx_del_event(rev, NGX_READ_EVENT, 0) != NGX_OK) {
+ c->error = 1;
+ ngx_http_finalize_request(r, NGX_ERROR);
+ return;
+ }
+ }
+ }
+
+ if (rc == NGX_OK || rc == NGX_DONE) {
r->discard_body = 0;
r->lingering_close = 0;
ngx_http_finalize_request(r, NGX_DONE);
@@ -570,7 +605,7 @@
for ( ;; ) {
if (r->headers_in.content_length_n == 0) {
r->read_event_handler = ngx_http_block_reading;
- return NGX_OK;
+ return NGX_DONE;
}
if (!r->connection->read->ready) {
@@ -585,7 +620,7 @@
if (n == NGX_ERROR) {
r->connection->error = 1;
- return NGX_OK;
+ return NGX_ERROR;
}
if (n == NGX_AGAIN) {
On Tue, Sep 27, 2011 at 4:04 PM, Maxim Dounin <mdounin at mdounin.ru> wrote:
> 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
>
> _______________________________________________
> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.nginx.org/pipermail/nginx-devel/attachments/20110927/a254b008/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: body.patch
Type: text/x-patch
Size: 2570 bytes
Desc: not available
URL: <http://mailman.nginx.org/pipermail/nginx-devel/attachments/20110927/a254b008/attachment-0001.bin>
More information about the nginx-devel
mailing list