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