bugfix: removing a request header might lead to memory corruptions. thanks Bjørnar Ness for the report.

Author: agentzh

src/ngx_http_lua_common.h

@@ -30,6 +30,8 @@
 #define MD5_DIGEST_LENGTH 16
 #endif
 
+#define ngx_http_lua_assert(a)  assert(a)
+
 /* Nginx HTTP Lua Inline tag prefix */
 
 #define NGX_HTTP_LUA_INLINE_TAG "nhli_"

src/ngx_http_lua_headers_in.c

@@ -171,13 +171,22 @@ ngx_http_set_header_helper(ngx_http_request_t *r, ngx_http_lua_header_val_t *hv,
                 rc = ngx_http_lua_rm_header_helper(&r->headers_in.headers,
                                                    part, i);
 
+                ngx_http_lua_assert(!(r->headers_in.headers.part.next == NULL
+                                      && r->headers_in.headers.last
+                                         != &r->headers_in.headers.part));
+
+                dd("rm header: rc=%d", (int) rc);
+
                 if (rc == NGX_OK) {
+
                     if (output_header) {
                         *output_header = NULL;
                     }
 
                     goto retry;
                 }
+
+                return NGX_ERROR;
             }
 
             h[i].value = *value;
@@ -651,42 +660,76 @@ ngx_http_lua_rm_header_helper(ngx_list_t *l, ngx_list_part_t *cur,
        (int) data[i].value.len, data[i].value.data);
 
     if (i == 0) {
+        dd("first entry in the part");
         cur->elts = (char *) cur->elts + l->size;
         cur->nelts--;
 
         if (cur == l->last) {
+            dd("being the last part");
             if (cur->nelts == 0) {
 #if 1
                 part = &l->part;
-                while (part->next != cur) {
-                    if (part->next == NULL) {
-                        return NGX_ERROR;
+                dd("cur=%p, part=%p, part next=%p, last=%p",
+                   cur, part, part->next, l->last);
+
+                if (part == cur) {
+                    cur->elts = (char *) cur->elts - l->size;
+                    /* do nothing */
+
+                } else {
+                    while (part->next != cur) {
+                        if (part->next == NULL) {
+                            return NGX_ERROR;
+                        }
+                        part = part->next;
                     }
-                    part = part->next;
-                }
 
-                l->last = part;
-                part->next = NULL;
-                l->nalloc = part->nelts;
+                    l->last = part;
+                    part->next = NULL;
+                    dd("part nelts: %d", (int) part->nelts);
+                    l->nalloc = part->nelts;
+                }
 #endif
 
             } else {
-                l->nalloc = cur->nelts;
+                l->nalloc--;
+                dd("nalloc decreased: %d", (int) l->nalloc);
             }
 
             return NGX_OK;
         }
 
         if (cur->nelts == 0) {
+            dd("current part is empty");
             part = &l->part;
-            while (part->next != cur) {
-                if (part->next == NULL) {
-                    return NGX_ERROR;
+            if (part == cur) {
+                ngx_http_lua_assert(cur->next != NULL);
+
+                dd("remove 'cur' from the list by rewriting 'cur': "
+                   "l->last: %p, cur: %p, cur->next: %p, part: %p",
+                   l->last, cur, cur->next, part);
+
+                if (l->last == cur->next) {
+                    dd("last is cur->next");
+                    l->part = *(cur->next);
+                    l->last = part;
+                    l->nalloc = part->nelts;
+
+                } else {
+                    l->part = *(cur->next);
                 }
-                part = part->next;
-            }
 
-            part->next = cur->next;
+            } else {
+                dd("remove 'cur' from the list");
+                while (part->next != cur) {
+                    if (part->next == NULL) {
+                        return NGX_ERROR;
+                    }
+                    part = part->next;
+                }
+
+                part->next = cur->next;
+            }
 
             return NGX_OK;
         }
@@ -700,7 +743,7 @@ ngx_http_lua_rm_header_helper(ngx_list_t *l, ngx_list_part_t *cur,
         cur->nelts--;
 
         if (cur == l->last) {
-            l->nalloc = cur->nelts;
+            l->nalloc--;
         }
 
         return NGX_OK;

t/028-req-header.t

@@ -9,7 +9,7 @@ use t::TestNginxLua;
 
 repeat_each(2);
 
-plan tests => repeat_each() * (2 * blocks() + 19);
+plan tests => repeat_each() * (2 * blocks() + 20);
 
 #no_diff();
 #no_long_string();
@@ -1346,3 +1346,60 @@ GET /bar
 host var: agentzh.org
 http_host var: agentZH.org:1984
 
+
+
+=== TEST 44: clear all and re-insert
+--- config
+    location = /t {
+        content_by_lua '
+            local headers = ngx.req.get_headers(100, true)
+            local n = 0
+            for header, _ in pairs(headers) do
+                n = n + 1
+                ngx.req.clear_header(header)
+            end
+            ngx.say("got ", n, " headers")
+            local i = 0
+            for header, value in pairs(headers) do
+                i = i + 1
+                print("1: reinsert header ", header, ": ", i)
+                ngx.req.set_header(header, value)
+            end
+            local headers = ngx.req.get_headers(100, true)
+            n = 0
+            for header, _ in pairs(headers) do
+                n = n + 1
+                ngx.req.clear_header(header)
+            end
+            ngx.say("got ", n, " headers")
+            -- do return end
+            local i = 0
+            for header, value in pairs(headers) do
+                i = i + 1
+                if i > 8 then
+                    break
+                end
+                print("2: reinsert header ", header, ": ", i)
+                ngx.req.set_header(header, value)
+            end
+        ';
+    }
+
+--- raw_request eval
+"GET /t HTTP/1.1\r
+Host: localhost\r
+Connection: close\r
+Cache-Control: max-age=0\r
+Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,*/*;q=0.8\r
+User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/30.0.1599.101 Safari/537.36\r
+Accept-Encoding: gzip,deflate,sdch\r
+Accept-Language: en-US,en;q=0.8\r
+Cookie: test=cookie;\r
+\r
+"
+--- response_body
+got 8 headers
+got 8 headers
+--- no_error_log
+[error]
+