ngx_http_discard_request_body may read incomplete data?
Simon Liu
simohayha.bobo at gmail.com
Tue Sep 27 06:42:10 UTC 2011
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.nginx.org/pipermail/nginx-devel/attachments/20110927/65c45539/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: body.patch
Type: text/x-patch
Size: 1794 bytes
Desc: not available
URL: <http://mailman.nginx.org/pipermail/nginx-devel/attachments/20110927/65c45539/attachment-0001.bin>
More information about the nginx-devel
mailing list