On Wed, Oct 01, 2014 at 04:32:09AM +0400, Maxim Dounin wrote:
> Hello!
>
> On Tue, Sep 30, 2014 at 03:10:42PM -0700, Yichun Zhang (agentzh) wrote:
>
> > Hello!
> >
> > I've noticed that the code re-factoring in the resolver in nginx 1.7.5
> > introduces a new regression that can cause memory invalid reads when
> > --with-debug is used to build the nginx. The issue still exists in
> > nginx 1.7.6.
>
> [...]
>
> > ngx_log_debug2(NGX_LOG_DEBUG_EVENT, ev->log, 0,
> > "event timer del: %d: %M",
> > ngx_event_ident(ev->data), ev->timer.key);
> >
> > while ev->data here is the resolver node that has already been freed
> > up earlier in ngx_resolver_free_node.
>
> Yes, thanks, it's known (though mostly harmless) issue.
> Ruslan is looking into it.

Here's a series of two patches that fix the issue.

The first one fixes the use-after-free memory access.

The second one fixes debug event logging for resolver
timer events.

# HG changeset patch
# User Ruslan Ermilov <ru@nginx.com>
# Date 1416301613 -10800
# Tue Nov 18 12:06:53 2014 +0300
# Node ID e7406dc8f6e0e662d3c738ef193adf8da4b4dae0
# Parent f1d87edc493b80c743c9512ae815bb19c76faf65
Resolver: fixed use-after-free memory access.

In 954867a2f0a6, we switched to using resolver node as the
timer event data, so make sure we do not free resolver node
memory until the corresponding timer is deleted.

diff --git a/src/core/ngx_resolver.c b/src/core/ngx_resolver.c
--- a/src/core/ngx_resolver.c
+++ b/src/core/ngx_resolver.c
@@ -1568,8 +1568,6 @@ ngx_resolver_process_a(ngx_resolver_t *r

ngx_rbtree_delete(&r->name_rbtree, &rn->node);

- ngx_resolver_free_node(r, rn);
-
/* unlock name mutex */

while (next) {
@@ -1580,6 +1578,8 @@ ngx_resolver_process_a(ngx_resolver_t *r
ctx->handler(ctx);
}

+ ngx_resolver_free_node(r, rn);
+
return;
}

@@ -2143,8 +2143,6 @@ valid:

ngx_rbtree_delete(tree, &rn->node);

- ngx_resolver_free_node(r, rn);
-
/* unlock addr mutex */

while (next) {
@@ -2155,6 +2153,8 @@ valid:
ctx->handler(ctx);
}

+ ngx_resolver_free_node(r, rn);
+
return;
}

# HG changeset patch
# User Ruslan Ermilov <ru@nginx.com>
# Date 1416302028 -10800
# Tue Nov 18 12:13:48 2014 +0300
# Node ID 8c5ec1928063baf7bf0dc52db680856858808c62
# Parent e7406dc8f6e0e662d3c738ef193adf8da4b4dae0
Resolver: fixed debug event logging.

In 954867a2f0a6, we switched to using resolver node as the timer
event data. This broke debug event logging.

Emulate enough of ngx_connection_t in ngx_resolver_node_t for
ngx_event_ident() to work. Redo it similarly in ngx_resolver_t.
Removed now unused ngx_resolver_ctx_t.ident.

diff --git a/src/core/ngx_resolver.c b/src/core/ngx_resolver.c
--- a/src/core/ngx_resolver.c
+++ b/src/core/ngx_resolver.c
@@ -101,6 +101,10 @@ static ngx_resolver_node_t *ngx_resolver
#endif


+#define ngx_resolver_node(n) \
+ (ngx_resolver_node_t *) ((u_char *) n - offsetof(ngx_resolver_node_t, node))
+
+
ngx_resolver_t *
ngx_resolver_create(ngx_conf_t *cf, ngx_str_t *names, ngx_uint_t n)
{
@@ -156,7 +160,7 @@ ngx_resolver_create(ngx_conf_t *cf, ngx_
r->event->handler = ngx_resolver_resend_handler;
r->event->data = r;
r->event->log = &cf->cycle->new_log;
- r->ident = -1;
+ r->fd = NGX_INVALID_FILE;

r->resend_timeout = 5;
r->expire = 30;
@@ -288,7 +292,7 @@ ngx_resolver_cleanup_tree(ngx_resolver_t

while (tree->root != tree->sentinel) {

- rn = (ngx_resolver_node_t *) ngx_rbtree_min(tree->root, tree->sentinel);
+ rn = ngx_resolver_node(ngx_rbtree_min(tree->root, tree->sentinel));

ngx_queue_remove(&rn->queue);

@@ -666,7 +670,7 @@ ngx_resolve_name_locked(ngx_resolver_t *
ctx->event->handler = ngx_resolver_timeout_handler;
ctx->event->data = rn;
ctx->event->log = r->log;
- ctx->ident = -1;
+ rn->fd = NGX_INVALID_FILE;

ngx_add_timer(ctx->event, ctx->timeout);
}
@@ -859,7 +863,7 @@ ngx_resolve_addr(ngx_resolver_ctx_t *ctx
ctx->event->handler = ngx_resolver_timeout_handler;
ctx->event->data = rn;
ctx->event->log = r->log;
- ctx->ident = -1;
+ rn->fd = NGX_INVALID_FILE;

ngx_add_timer(ctx->event, ctx->timeout);

@@ -2290,7 +2294,7 @@ ngx_resolver_lookup_name(ngx_resolver_t

/* hash == node->key */

- rn = (ngx_resolver_node_t *) node;
+ rn = ngx_resolver_node(node);

rc = ngx_memn2cmp(name->data, rn->name, name->len, rn->nlen);

@@ -2329,7 +2333,7 @@ ngx_resolver_lookup_addr(ngx_resolver_t

/* addr == node->key */

- return (ngx_resolver_node_t *) node;
+ return ngx_resolver_node(node);
}

/* not found */
@@ -2365,7 +2369,7 @@ ngx_resolver_lookup_addr6(ngx_resolver_t

/* hash == node->key */

- rn = (ngx_resolver_node_t *) node;
+ rn = ngx_resolver_node(node);

rc = ngx_memcmp(addr, &rn->addr6, 16);

@@ -2403,8 +2407,8 @@ ngx_resolver_rbtree_insert_value(ngx_rbt

} else { /* node->key == temp->key */

- rn = (ngx_resolver_node_t *) node;
- rn_temp = (ngx_resolver_node_t *) temp;
+ rn = ngx_resolver_node(node);
+ rn_temp = ngx_resolver_node(temp);

p = (ngx_memn2cmp(rn->name, rn_temp->name, rn->nlen, rn_temp->nlen)
< 0) ? &temp->left : &temp->right;
@@ -2446,8 +2450,8 @@ ngx_resolver_rbtree_insert_addr6_value(n

} else { /* node->key == temp->key */

- rn = (ngx_resolver_node_t *) node;
- rn_temp = (ngx_resolver_node_t *) temp;
+ rn = ngx_resolver_node(node);
+ rn_temp = ngx_resolver_node(temp);

p = (ngx_memcmp(&rn->addr6, &rn_temp->addr6, 16)
< 0) ? &temp->left : &temp->right;
diff --git a/src/core/ngx_resolver.h b/src/core/ngx_resolver.h
--- a/src/core/ngx_resolver.h
+++ b/src/core/ngx_resolver.h
@@ -51,11 +51,15 @@ typedef void (*ngx_resolver_handler_pt)(


typedef struct {
- ngx_rbtree_node_t node;
+ /* PTR: resolved name, A: name to resolve */
+ u_char *name;
+
ngx_queue_t queue;

- /* PTR: resolved name, A: name to resolve */
- u_char *name;
+ /* ident must be after 3 pointers */
+ ngx_fd_t fd;
+
+ ngx_rbtree_node_t node;

#if (NGX_HAVE_INET6)
/* PTR: IPv6 address to resolve (IPv4 address is in rbtree node key) */
@@ -104,7 +108,7 @@ typedef struct {
ngx_log_t *log;

/* ident must be after 3 pointers */
- ngx_int_t ident;
+ ngx_fd_t fd;

/* simple round robin DNS peers balancer */
ngx_array_t udp_connections;
@@ -143,9 +147,6 @@ struct ngx_resolver_ctx_s {
ngx_resolver_t *resolver;
ngx_udp_connection_t *udp_connection;

- /* ident must be after 3 pointers */
- ngx_int_t ident;
-
ngx_int_t state;
ngx_str_t name;


_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel