Code Review - Fix for OpenSocket() causing a double free() of previously free()'d memory

[GoogleCodeExporter]

Purpose of code changes on this branch:

Dominic's update to OpenSocket(...) code was setting fai->ai_next = ai_ipv4;. 
This will sometimes create a loop within the addrinfo's. freeaddrinfo() will 
follow this loop and try to double free() data. OpenSocket() would also get 
stuck in this loop if it fails to open any sockets.

I've implemented lists of addresses now as per the TODO, so this will now check 
all address rather then just the first IPv4 and IPv6.

I've tested this with a mix of IPv4 and IPv6 settings on the client and server 
and it seems to work as expected.

After the review, I'll merge this branch into:
/trunk

I've already merged this into ndt-web10g.

Original issue reported on code.google.com by rsanger...@gmail.com on 14 Apr 2013 at 3:27

Attachments:

• OpenSocket-Fix.diff

[GoogleCodeExporter]

The code is rather confusing. Is the goal to get two list of addrinfo's, the v6 
addresses, and the v4 addresses? If so, why can't you create two lists, one v4 
and one v6, doing something like:

addrinfo *ipv4_list = NULL;
addrinfo *ipv6_list = NULL;
ai = fai;
while (ai != NULL) {
    addrinfo *next = ai->ai_next;

    if (ai->ai_family == AF_INET) {
        ai->ai_next = ipv4_list;
        ipv4_list = ai;
    }
    // add similar for AF_INET6

    ai = next;
}

To get the sorted list of all v4 and v6 addresses, you just track the tail of 
the v6 linked list, and set its ai_next pointer to the v4 list.

Original comment by AaronMat...@gmail.com on 15 Apr 2013 at 3:46

[GoogleCodeExporter]

Yes the goal is to get two lists of addrinfo's for v6 and v4. The v6 is then 
linked to v4 as a fallback so ndt falls back to only v4 (A v6 socket can do 
both v4 and v6).

In theory we can make these lists by setting ai_next and as long as every 
element ends up in a list and the tails get set to NULL we cannot possibly have 
a loop. But the library implementation of getaddrinfo returns a list of 
individually malloc()'d nodes. If the first element is a v4 address then only 
v4's will be linked after it and freeaddrinfo() would leak all the v6's. We 
could make sure we call freeaddrinfo() on the head of the v6 (which will have 
the v4's after it) but then freeaddrinfo() is being called with a different 
memory address which would fail in a library that implemented the getaddrinfo() 
list in a single contiguous piece of memory.

So I think we have to consider the list returned by getaddrinfo() read-only and 
avoid touching ai_next. Which is why I've needed use struct ai_node's to point 
to the actual addrinfo's.

Original comment by rsanger...@gmail.com on 15 Apr 2013 at 6:26

[GoogleCodeExporter]

I think a clearer solution would probably look something like:

break out most of the existing OpenSocket function into an internal one that 
takes the family and whether to instaniate v4-to-v6 as options.

Then have OpenSocket do:

return = NULL;

if (require v4) {
     return = __OpenSocket(addr, AF_INET, 0);
}
else if (require v6) {
    return = __OpenSocket(addr, AF_INET6, 0);
}
else {
    return = __OpenSocket(addr, AF_INET6, 1);
    if (!return) {
        return = __OpenSocket(addr, AF_INET, 0);
    }
}

This disentangles the odd logic of v4/v6 choice from the actual instantiation 
of socket, and would allow some comments explaining the choice in the 
OpenSocket function.

Original comment by AaronMat...@gmail.com on 19 Apr 2013 at 5:05

[GoogleCodeExporter]

I committed a change similar to what I described above into trunk. Take a look 
at src/network.c to see what I did. I added an "int family" to the OpenSocket 
parameters (only caller was CreateListenSocket...). I then have 
CreateListenSocket call it first for v6 (assuming AF_INET6 is around), and then 
if that doesn't work (or it's v4 only, or v6 isn't available), it tries v4. 
I've tested locally, but it could use some further testing. However, since 
bwctl does almost the same thing, I'm pretty confident in it.

Original comment by AaronMat...@gmail.com on 21 Apr 2013 at 7:28

[GoogleCodeExporter]

Looks good to me. A lot less code this way which is nice.

I've tested this myself and this works as expected.

Original comment by rsanger...@gmail.com on 24 Apr 2013 at 1:04

• Changed state: Fixed