Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nginx h2 patch for 1.14.0 and fix spdy post bug #97

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

favortel
Copy link

@favortel favortel commented Jun 11, 2018

Nginx SPDY POST request will cause nginx segment fault in 1.14.0 and nginx-1.13.12

src/http/ngx_http_spdy.c:ngx_http_spdy_state_read_data
buf->last = ngx_cpymem(buf->last, pos, size);

src/http/ngx_http_spdy.c:ngx_http_spdy_read_request_body

+    if (!r->request_body && ngx_http_spdy_init_request_body(r) != NGX_OK) {
+        stream->skip_data = NGX_SPDY_DATA_INTERNAL_ERROR;
+        return NGX_HTTP_INTERNAL_SERVER_ERROR;
+    }
...

Cause problem in nginx-1.13.12 and 1.14.0, r->request_body has been inited BEFORE calling ngx_http_spdy_read_request_body in src/http/ngx_http_request_body.c.
So ngx_http_spdy_init_request_body will not run, and r->request_body->buf will be 0

If the patch for src/http/ngx_http_request_body.c still AFTER these codes:

 #if (NGX_HTTP_V2)
     if (r->stream) {
         rc = ngx_http_v2_read_request_body(r);
         goto done;
     }
 #endif

The r->request_body has been inited beforce ngx_http_spdy_read_request_body called,
ngx_http_spdy_init_request_body will not run, and r->request_body->buf will be 0
https://github.com/cloudflare/sslconfig/blob/master/patches/nginx__1.13.0_http2_spdy.patch

 src/http/ngx_http_request_body.c
@@ -84,6 +84,12 @@ ngx_http_read_client_request_body(ngx_http_request_t *r,
         goto done;
     }
 #endif
+#if (NGX_HTTP_SPDY)
+    if (r->spdy_stream) {
+        rc = ngx_http_spdy_read_request_body(r, post_handler);
+        goto done;
+    }
+#endif

http://lxr.nginx.org/source/src/http/ngx_http_request_body.c 0070-0073

0054     rb = ngx_pcalloc(r->pool, sizeof(ngx_http_request_body_t));
0055     if (rb == NULL) {
0056         rc = NGX_HTTP_INTERNAL_SERVER_ERROR;
0057         goto done;
0058     }
0059 
0060     /*
0061      * set by ngx_pcalloc():
0062      *
0063      *     rb->bufs = NULL;
0064      *     rb->buf = NULL;
0065      *     rb->free = NULL;
0066      *     rb->busy = NULL;
0067      *     rb->chunked = NULL;
0068      */
0069 
0070     rb->rest = -1;
0071     rb->post_handler = post_handler;
0072 
0073     r->request_body = rb;
0074 
0075     if (r->headers_in.content_length_n < 0 && !r->headers_in.chunked) {
0076         r->request_body_no_buffering = 0;
0077         post_handler(r);
0078         return NGX_OK;
0079     }
0080 
0081 #if (NGX_HTTP_V2)
0082     if (r->stream) {
0083         rc = ngx_http_v2_read_request_body(r);
0084         goto done;
0085     }
0086 #endif

Which is different with nginx 1.12.2
http://lxr.nginx.org/source/src/http/ngx_http_request_body.c?v=nginx-1.12.2

0029 ngx_int_t
0030 ngx_http_read_client_request_body(ngx_http_request_t *r,
0031     ngx_http_client_body_handler_pt post_handler)
0032 {
0033     size_t                     preread;
0034     ssize_t                    size;
0035     ngx_int_t                  rc;
0036     ngx_buf_t                 *b;
0037     ngx_chain_t                out;
0038     ngx_http_request_body_t   *rb;
0039     ngx_http_core_loc_conf_t  *clcf;
0040 
0041     r->main->count++;
0042 
0043     if (r != r->main || r->request_body || r->discard_body) {
0044         r->request_body_no_buffering = 0;
0045         post_handler(r);
0046         return NGX_OK;
0047     }
0048 
0049 #if (NGX_HTTP_V2)
0050     if (r->stream) {
0051         rc = ngx_http_v2_read_request_body(r, post_handler);
0052         goto done;
0053     }
0054 #endif
...
...
0061     rb = ngx_pcalloc(r->pool, sizeof(ngx_http_request_body_t));
0062     if (rb == NULL) {
0063         rc = NGX_HTTP_INTERNAL_SERVER_ERROR;
0064         goto done;
0065     }
0066 
0067     /*
0068      * set by ngx_pcalloc():
0069      *
0070      *     rb->bufs = NULL;
0071      *     rb->buf = NULL;
0072      *     rb->free = NULL;
0073      *     rb->busy = NULL;
0074      *     rb->chunked = NULL;
0075      */
0076 
0077     rb->rest = -1;
0078     rb->post_handler = post_handler;
0079 
0080     r->request_body = rb;

So, I use "r->request_body->buf" replace "r->request_body"

+    if (!r->request_body->buf && ngx_http_spdy_init_request_body(r) != NGX_OK) {
+        stream->skip_data = NGX_SPDY_DATA_INTERNAL_ERROR;
+        return NGX_HTTP_INTERNAL_SERVER_ERROR;
+    }

Or you can move the following code BEFORE r->request_body has been inited

+#if (NGX_HTTP_SPDY)
+    if (r->spdy_stream) {
+        rc = ngx_http_spdy_read_request_body(r, post_handler);
+        goto done;
+    }
+#endif

Nginx spdy post request will cause  nginx segment fault in 1.14.0 and nginx-1.13.12
src/http/ngx_http_spdy.c:ngx_http_spdy_read_request_body

+    case NGX_SPDY_DATA_INTERNAL_ERROR:
+        return NGX_HTTP_INTERNAL_SERVER_ERROR;
+    }
+
// Cause problem in nginx-1.13.12 and 1.14.0
// r->request_body has been inited BEFORE  calling  ngx_http_spdy_read_request_body
// ngx_http_spdy_init_request_body will not run, and r->request_body->buf will be 0
+    if (!r->request_body && ngx_http_spdy_init_request_body(r) != NGX_OK) {
+        stream->skip_data = NGX_SPDY_DATA_INTERNAL_ERROR;
+        return NGX_HTTP_INTERNAL_SERVER_ERROR;
+    }
...

//IF patch for src/http/ngx_http.h still AFTER NGX_HTTP_V2
 #if (NGX_HTTP_V2)
src/http/ngx_http_request_body.c
@@ -84,6 +84,12 @@ ngx_http_read_client_request_body(ngx_http_request_t *r,
         goto done;
     }
 #endif
+#if (NGX_HTTP_SPDY)
+    if (r->spdy_stream) {
+        rc = ngx_http_spdy_read_request_body(r, post_handler);
+        goto done;
+    }
+#endif
http://lxr.nginx.org/source/src/http/ngx_http_request_body.c
0054     rb = ngx_pcalloc(r->pool, sizeof(ngx_http_request_body_t));
0055     if (rb == NULL) {
0056         rc = NGX_HTTP_INTERNAL_SERVER_ERROR;
0057         goto done;
0058     }
0059 
0060     /*
0061      * set by ngx_pcalloc():
0062      *
0063      *     rb->bufs = NULL;
0064      *     rb->buf = NULL;
0065      *     rb->free = NULL;
0066      *     rb->busy = NULL;
0067      *     rb->chunked = NULL;
0068      */
0069 
0070     rb->rest = -1;
0071     rb->post_handler = post_handler;
0072 
0073     r->request_body = rb;
0074 
0075     if (r->headers_in.content_length_n < 0 && !r->headers_in.chunked) {
0076         r->request_body_no_buffering = 0;
0077         post_handler(r);
0078         return NGX_OK;
0079     }
0080 
0081 #if (NGX_HTTP_V2)
0082     if (r->stream) {
0083         rc = ngx_http_v2_read_request_body(r);
0084         goto done;
0085     }
0086 #endif


So, should use 
+    if (!r->request_body->buf && ngx_http_spdy_init_request_body(r) != NGX_OK) {
+        stream->skip_data = NGX_SPDY_DATA_INTERNAL_ERROR;
+        return NGX_HTTP_INTERNAL_SERVER_ERROR;
+    }

OR move the code BEFORE r->request_body has been inited
+#if (NGX_HTTP_SPDY)
+    if (r->spdy_stream) {
+        rc = ngx_http_spdy_read_request_body(r, post_handler);
+        goto done;
+    }
+#endif
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant