Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#187 closed defect (fixed)

incorrect handling of bind option on wildcard listen addresses

Reported by: Roman Odaisky Owned by: somebody
Priority: minor Milestone:
Component: nginx-core Version: 1.3.x
Keywords: Cc:
uname -a: N/A
nginx -V: nginx/1.2.1 — fix tested;
nginx/1.3.3 — code unchanged, issue presumed to persist.

Description

Given the configuration

listen 10.2.2.2:80;
...
listen *:80;

or alternatively,

listen [2001:db8::42]:80;
...
listen [::]:80;

or any other configuration where a wildcard listen directive is present along with non-wildcard entries for the same port, nginx may fail to perform bind() correctly only once, depending on the order in which it encounters the directives. This is due to faulty sorting in ngx_http_optimize_servers according to ngx_http_cmp_conf_addrs criterion, which is not guaranteed to place the wildcard entry/entries at the end of the list as expected by ngx_http_init_listening. Eventually the problem causes EADDRINUSE upon startup due to multiple bind() calls. The following simple patch fixes the problem, presuming the wildcard field always equals 0 or 1.

  • src/http/ngx_http.c

    a b  
    16081608    first = (ngx_http_conf_addr_t *) one;
    16091609    second = (ngx_http_conf_addr_t *) two;
    16101610
    1611     if (first->opt.wildcard) {
     1611    if (first->opt.wildcard != second->opt.wildcard) {
    16121612        /* a wildcard address must be the last resort, shift it to the end */
    1613         return 1;
     1613        return (int)first->opt.wildcard - (int)second->opt.wildcard;
    16141614    }
    16151615
    16161616    if (first->opt.bind && !second->opt.bind) {

--
HTH
Roman Odaisky

Change History (9)

comment:1 by Ruslan Ermilov, 12 years ago

I can't reproduce this. Is this actually exploitable?

ngx_sort() does an "insertion sort", and even though the comparator may erroneously report that "first == *" where "*" is a wildcard address, "*" will eventually be checked via the "first" parameter, and moved appropriately.

comment:2 by Roman Odaisky, 12 years ago

Most definitely. I encountered this in an actual setup. Minimal example to reproduce:

server {
    listen 127.0.0.1:58080;
}

server {
    listen 58080 sndbuf=42;
}

Swap the entries in the configuration file and everything works correctly. Otherwise, the comparator is invoked exactly once, sees the bind option on the right side, fails to see the wildcard option on the right side, and erroneously returns -1, causing swapping of the entries.

The problem is triggered when addresses in listen directives differ and when socket options are present. I would guess this is rare with IPv4, but will be commonplace with IPv6 (due to the ipv6only=on socket option and multitude of available addresses).

The usefulness of IP-based virtual hosts is obvious, DNS flexibility.

comment:3 by Maxim Dounin, 12 years ago

Ok, thanks, the problem is clear. It's in fact due to bind option (in your above config - implicitly set by "sndbuf" parameter) was not ignored for wildcard addresses, and caused separate bind on wildcard address. I would suggest the following patch, which looks more logical for me:

# HG changeset patch
# User Maxim Dounin <mdounin@mdounin.ru>
# Date 1342179498 -14400
# Node ID c2f751648432a57fe0e9c88ea1ca313f5fa5c8d8
# Parent  5f74b296240eceebefa97009e9f1e1f62cf4e80a
Ignore bind option on wildcard addresses (ticket #187).

Without this patch config

    server { listen 127.0.0.1:58080; }
    server { listen 58080 sndbuf=42; }

resulted in two binds, while the opposite order

    server { listen 58080 sndbuf=42; }
    server { listen 127.0.0.1:58080; }

in only wildcard one.  This happened due to the "bind" option set by
the "sndbuf" parameter, which caused wrong order after sorting with
ngx_http_cmp_conf_addrs().

diff --git a/src/http/ngx_http.c b/src/http/ngx_http.c
--- a/src/http/ngx_http.c
+++ b/src/http/ngx_http.c
@@ -1618,7 +1618,7 @@ ngx_http_cmp_conf_addrs(const void *one,
         return -1;
     }
 
-    if (!first->opt.bind && second->opt.bind) {
+    if (!first->opt.bind && second->opt.bind && !second->opt.wildcard) {
         /* shift explicit bind()ed addresses to the start */
         return 1;
     }

comment:4 by Maxim Dounin, 12 years ago

Priority: majorminor
Status: newaccepted
Summary: [patch] Failure to detect wildcard listeningincorrect handling of bind option on wildcard listen addresses

comment:5 by Roman Odaisky, 12 years ago

This makes the comparator fail the strict antisymmetry property (cmp(a, b) = -1 <=> cmp(b, a) = 1), which has the potential to break the sorting in subtle ways (do you think it was easy to discover the problem in the first place?).

My patch, the only function of which is restoring antisymmetry on the wildcard comparison, causes the entries to be sorted as the code originally intended:

Non-wildcard, bind
Non-wildcard, non-bind
Wildcard, bind
Wildcard, non-bind

(in other words, ORDER BY wildcard ASC, bind DESC), which, if I understand the code correctly, is right. If another ordering is required, then the proper procedure is to specify the ordering, then implement the comparator to satisfy strict weak ordering requirements (the comparator got this right with the bind option but not with the wildcard option).

So I suggest either the original patch, or to handle the wildcard option like bind:

  • src/http/ngx_http.c

    a b  
    16081608    first = (ngx_http_conf_addr_t *) one;
    16091609    second = (ngx_http_conf_addr_t *) two;
    16101610
    1611     if (first->opt.wildcard) {
     1611    if (first->opt.wildcard && !second->opt.wildcard) {
    16121612        /* a wildcard address must be the last resort, shift it to the end */
    16131613        return 1;
    16141614    }
    16151615
     1616    if (!first->opt.wildcard && second->opt.wildcard) {
     1617        return -1;
     1618    }
     1619
    16161620    if (first->opt.bind && !second->opt.bind) {
    16171621        /* shift explicit bind()ed addresses to the start */
    16181622        return -1;

comment:6 by Ruslan Ermilov, 12 years ago

Resolution: fixed
Status: acceptedclosed

In 4756/nginx:

(The changeset message doesn't reference this ticket)

comment:7 by Maxim Dounin, 12 years ago

In 4793/nginx:

(The changeset message doesn't reference this ticket)

comment:8 by Ruslan Ermilov, 12 years ago

In 4818/nginx:

(The changeset message doesn't reference this ticket)

comment:9 by Maxim Dounin, 12 years ago

In 4854/nginx:

(The changeset message doesn't reference this ticket)

Note: See TracTickets for help on using tickets.