bugfix: Fix handling of new list elements

[oxpa]

ngx_list_push return an uninitialized element. Each of them needs 'next' element set to either zero or valid pointer.

The problem become apparent after #2063 : it fixes variable handling but leaves problems with uninitialized 'next' elements.

I managed to improve at least two tests to make the problem reveal itself on linux.
Adding MALLOC_PERTURB_ forces glib to initialize allocated memory with a non-zero value. This allows code to fail quicker: you don't have to wait till nginx pool reuses a chunk.

As I'm not really familiar with the code, so I might have missed a place or two more with similar problem.
Also, I guess, it makes sense to add MALLOC_PERTURB_ to all tests.

I hereby granted the copyright of the changes in this pull request
to the authors of this lua-nginx-module project.

[zhuizhuhaomeng]

@oxpa
you can add MALLOC_PERTURB_= 9; to the .travis.yml

[oxpa]

Makes sense, will do

[zhuizhuhaomeng]

@oxpa got the following error.

the header type is ngx_table_elt_t for the nginx older than 1.23 which does not have the next member

/home/travis/build/openresty/lua-nginx-module/src/ngx_http_lua_control.c:283:6: error: ‘ngx_table_elt_t {aka struct <anonymous>}’ has no member named ‘next’
     h->next = NULL;
      ^~

https://app.travis-ci.com/github/openresty/lua-nginx-module/jobs/599726670#L2356

[oxpa]

Sorry, didn't check this. Will fix and check it works pre-1.23.0.
Thank you.

[oxpa]

I have pushed the commit but I'm yet to test it.

[ssdr]

Maybe same problem in ngx_http_lua_builtin_multi_header()， where we should set next to NULL when override flag set.

[oxpa]

The code doesn't create a new element but rather goes through existing in a list and possibly updates their values. This element was created elsewhere and should already have proper 'next' value.

[ssdr]

Each element's next value may be not 'proper', since it's not NULL and it's 'hash' value is 0, this may cause memory access issue in ngx_http_variable_header_internal in ngx_http_variables.c.

[oxpa]

I still don't see a problem. The 'h->next' value is either NULL or a valid pointer to a next element.
In the 'override old values' block of code we assign all hash values of a headers list to 0. We then assign a value to the first one. And possibly leave a list of empty values attached to this changed value.
In the ngx_http_variables.c we will copy the first element, skip all others with hash==0 and break out of for() loop on reaching the end of the list.
It looks to me like it is exactly how it's meant to work. I see no problems with memory here. We do clear out values inside the list but we keep the list properly linked.

[ssdr]

Forgive me pasting the source code.

static ngx_int_t
ngx_http_variable_headers_internal(ngx_http_request_t *r,
    ngx_http_variable_value_t *v, uintptr_t data, u_char sep)
{
    size_t            len;
    u_char           *p;
    ngx_table_elt_t  *h, *th;

    h = *(ngx_table_elt_t **) ((char *) r + data);

    len = 0;

    for (th = h; th; th = th->next) {

        if (th->hash == 0) { 
            continue;
        }

        len += th->value.len + 2;       /* variable data size 'len' caculating */
    }

    if (len == 0) {
        v->not_found = 1;
        return NGX_OK;
    }

    len -= 2;     /* not including trailing sep and space */

    v->valid = 1;
    v->no_cacheable = 0;
    v->not_found = 0;

    if (h->next == NULL) {
        v->len = h->value.len;
        v->data = h->value.data;

        return NGX_OK;
    }

    p = ngx_pnalloc(r->pool, len);
    if (p == NULL) {
        return NGX_ERROR;
    }

    v->len = len;
    v->data = p;

    for (th = h; th; th = th->next) {

        if (th->hash == 0) {
            continue;
        }

        p = ngx_copy(p, th->value.data, th->value.len);

        if (th->next == NULL) {.   /* th->next has valid value while th->hash is 0 */
            break;
        }

        *p++ = sep; *p++ = ' ';    /* here is the problem, since the variable data size is 'len', not including
                                     extra sep and space, this will lead heap-buffer-overflow problem */
    }

    return NGX_OK;
}

[oxpa]

@ssdr while I'm not sure how to trigger this, the overall idea seems correct.
Does it also mean that we can't just set a random element in a list of headers? We should always have a continuous tail rather than a randomly assigned list.
And this makes quite a lot of places in lua modules a bit questionable: any place where the first hash == 0 element of a list is assigned potentially causes this problem, right? So even the same ngx_http_set_builtin_multi_header in the hv->no_override block may cause this issue if nginx received duplicate lines and ignored some of them, right?

Let me have a think. Though this seems a bit more complex than I can handle =)

[oxpa]

or even not 'continuous tail' but rather 'the last header value should be the last element in a list'. This seems proper wording.

[zhuizhuhaomeng]

If there are any bug there, we should try to add a test case to cover it.

[ssdr]

Another solution: Since h->next is valid value and it's hash might be 0, maybe ngx_http_variable_headers_internal needs some adaptions.

...
        for (th = h; th; th = th->next) {

        if (th->hash == 0) {
            continue;
        }

        p = ngx_copy(p, th->value.data, th->value.len);

        if (th->next == NULL) {
            break;
        }

        *p++ = sep; *p++ = ' ';    // maybe here needs some conditions, since th->next->hash might be 0
    }
...

[ssdr]

There have been a test case in 016-resp-header.t TEST 34, use openresty-asan binary will lead test case failed.

Non-asan binary needs gdb to check it:

=== TEST 34: set multi values to cache-control and override it with a single value
--- config
    location /lua {
        content_by_lua '
            ngx.header.cache_control = { "private", "no-store" }
            ngx.header.cache_control = { "no-cache" }
            ngx.say("Cache-Control: ", ngx.var.sent_http_cache_control)
            ngx.say("Cache-Control: ", ngx.header.cache_control)
        ';
    }
--- request
    GET /lua
--- response_headers
Cache-Control: no-cache
--- response_body
Cache-Control: no-cache
Cache-Control: no-cache

[oxpa]

I don't think editing nginx sources would be the right solution: there may be other places where this problem may arise
Nullifying the 'next' field seems like loosing memory (even though it is allocated from a pool).
So I changed the code to add a new element to the end of a list if needed. Please, have a look at #2182 . If it solves the issue with output headers, I will do similar change for input headers as well.
Thank you.

[ssdr]

fixed in nginx: https://hg.nginx.org/nginx/rev/b71e69247483