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