New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

getaddrinfo fails to parse IPv6 address on some systems #34

Closed
alistairking opened this Issue Jan 8, 2017 · 3 comments

Comments

Projects
None yet
2 participants
@alistairking
Member

alistairking commented Jan 8, 2017

As reported by @vgiotsas

It seems that the way we use getaddrinfo (https://github.com/CAIDA/bgpstream/blob/master/lib/utils/bgpstream_utils_addr.c#L209) will fail to parse an IPv6 address on (some?) IPv6-only systems. Probably omitting AI_ADDRCONFIG from hints would fix this, but it seems there can be issues with getaddrinfo (see https://blog.powerdns.com/2014/05/21/a-surprising-discovery-on-converting-ipv6-addresses-we-no-longer-prefer-getaddrinfo/), so we may want to consider switching to inet_pton...

@alistairking

This comment has been minimized.

Show comment
Hide comment
@alistairking

alistairking Aug 23, 2017

Member

I just personally ran into this problem on one of our nodes, and whipped up the following fix. It'll need to be tested more, but it seems to work...

bgpstream_addr_storage_t *bgpstream_str2addr(char *addr_str,
                                             bgpstream_addr_storage_t *addr)
{
  if (addr_str == NULL || addr == NULL) {
    return NULL;
  }

  if (strchr(addr_str, ':') != NULL) {
    /* this looks like it will be an IPv6 address */
    if (inet_pton(AF_INET6, addr_str, &addr->ipv6) != 1) {
      fprintf(stderr, "ERROR: Could not parse address string %s\n", addr_str);
      return NULL;
    }
    addr->version = BGPSTREAM_ADDR_VERSION_IPV6;
  } else {
    /* probably a v4 address */
    if (inet_pton(AF_INET, addr_str, &addr->ipv4) != 1) {
      fprintf(stderr, "ERROR: Could not parse address string %s\n", addr_str);
      return NULL;
    }
    addr->version = BGPSTREAM_ADDR_VERSION_IPV4;
  }

  return addr;
}
Member

alistairking commented Aug 23, 2017

I just personally ran into this problem on one of our nodes, and whipped up the following fix. It'll need to be tested more, but it seems to work...

bgpstream_addr_storage_t *bgpstream_str2addr(char *addr_str,
                                             bgpstream_addr_storage_t *addr)
{
  if (addr_str == NULL || addr == NULL) {
    return NULL;
  }

  if (strchr(addr_str, ':') != NULL) {
    /* this looks like it will be an IPv6 address */
    if (inet_pton(AF_INET6, addr_str, &addr->ipv6) != 1) {
      fprintf(stderr, "ERROR: Could not parse address string %s\n", addr_str);
      return NULL;
    }
    addr->version = BGPSTREAM_ADDR_VERSION_IPV6;
  } else {
    /* probably a v4 address */
    if (inet_pton(AF_INET, addr_str, &addr->ipv4) != 1) {
      fprintf(stderr, "ERROR: Could not parse address string %s\n", addr_str);
      return NULL;
    }
    addr->version = BGPSTREAM_ADDR_VERSION_IPV4;
  }

  return addr;
}

alistairking added a commit to CAIDA/libbgpstream that referenced this issue Jul 24, 2018

Fix `bgpstream_str2addr` IPv6 parsing
This seems to solve the problem described at CAIDA/bgpstream#34

@digizeph is going to do a full review of this problem and fix, but for now this should be good enough.
@digizeph

This comment has been minimized.

Show comment
Hide comment
@digizeph

digizeph Jul 28, 2018

Contributor

getaddrinfo does DNS lookups. It seems that in multi-threading workflow, it can cause problem, likely because of the DNS lookups.

"The getaddrinfo() function combines the functionality provided by the gethostbyname(3) and getservbyname(3) functions into a single interface"

The domain name queries carried out by gethostbyname() and
       gethostbyaddr() rely on the Name Service Switch (nsswitch.conf(5))
       configured sources or a local name server (named(8)).  The default
       action is to query the Name Service Switch (nsswitch.conf(5))
       configured sources, failing that, a local name server (named(8)).

The usage of Name Service Switch was pointed out to be problematic in here and here.

Addding AI_NUMERICHOST flag could potentially resolve the problem, but I don't think getaddrinfo is the right tool to use here.

inet_pton() does not do anything "complicated", thus looks safer to use (https://beej.us/guide/bgnet/html/multi/inet_ntopman.html, "These functions don't do DNS lookups—you'll need getaddrinfo() for that.")

In any case, we don't seem to use DNS at all in the context, thus using inet_pton() is better even without the crashing issue. The revised wrapper function also looks good.

Contributor

digizeph commented Jul 28, 2018

getaddrinfo does DNS lookups. It seems that in multi-threading workflow, it can cause problem, likely because of the DNS lookups.

"The getaddrinfo() function combines the functionality provided by the gethostbyname(3) and getservbyname(3) functions into a single interface"

The domain name queries carried out by gethostbyname() and
       gethostbyaddr() rely on the Name Service Switch (nsswitch.conf(5))
       configured sources or a local name server (named(8)).  The default
       action is to query the Name Service Switch (nsswitch.conf(5))
       configured sources, failing that, a local name server (named(8)).

The usage of Name Service Switch was pointed out to be problematic in here and here.

Addding AI_NUMERICHOST flag could potentially resolve the problem, but I don't think getaddrinfo is the right tool to use here.

inet_pton() does not do anything "complicated", thus looks safer to use (https://beej.us/guide/bgnet/html/multi/inet_ntopman.html, "These functions don't do DNS lookups—you'll need getaddrinfo() for that.")

In any case, we don't seem to use DNS at all in the context, thus using inet_pton() is better even without the crashing issue. The revised wrapper function also looks good.

digizeph added a commit to digizeph/bgpstream that referenced this issue Jul 28, 2018

alistairking added a commit that referenced this issue Jul 31, 2018

Merge pull request #69 from digizeph/master
v1.2.1, applied fix for #34
@digizeph

This comment has been minimized.

Show comment
Hide comment
@digizeph

digizeph Aug 1, 2018

Contributor

the issue should be fixed by #69 .

Contributor

digizeph commented Aug 1, 2018

the issue should be fixed by #69 .

@digizeph digizeph closed this Aug 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment