Skip to content

Commit

Permalink
Fixing Issue #149
Browse files Browse the repository at this point in the history 
Segmentation fault in spine 1.2.10 due to IPv6 address using IPv4 ICMP ping
  • Loading branch information
@TheWitness
TheWitness committed Mar 21, 2020
1 parent fa36ba5 commit 5644db7
Show file tree
Hide file tree
Showing 5 changed files with 129 additions and 28 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
The Cacti Group | spine

1.2.11
-issue#149: Segmentation fault in spine 1.2.10 due to IPv6 address using IPv4 ICMP ping

1.2.10
-feature: release to match Cacti release

Expand Down
145 changes: 118 additions & 27 deletions ping.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,16 +65,22 @@ int ping_host(host_t *host, ping_t *ping) {
}

if (!strstr(host->hostname, "localhost")) {
if (host->ping_method == PING_ICMP) {
ping_result = ping_icmp(host, ping);
} else if (host->ping_method == PING_UDP) {
ping_result = ping_udp(host, ping);
} else if (host->ping_method == PING_TCP) {
ping_result = ping_tcp(host, ping);
if (get_address_type(host) == 1) {
if (host->ping_method == PING_ICMP) {
ping_result = ping_icmp(host, ping);
} else if (host->ping_method == PING_UDP) {
ping_result = ping_udp(host, ping);
} else if (host->ping_method == PING_TCP) {
ping_result = ping_tcp(host, ping);
}
} else if (host->availability_method == AVAIL_PING) {
snprintf(ping->ping_status, 50, "0.000");
snprintf(ping->ping_response, SMALL_BUFSIZE, "PING: Device is IPV6. Please use the SNMP ping options only.");
return HOST_DOWN;
}
} else {
snprintf(ping->ping_status, 50, "0.000");
snprintf(ping->ping_response, SMALL_BUFSIZE, "PING: Device does not require ping");
snprintf(ping->ping_response, SMALL_BUFSIZE, "PING: Device does not require ping.");
ping_result = HOST_UP;
}
}
Expand Down Expand Up @@ -318,6 +324,7 @@ int ping_icmp(host_t *host, ping_t *ping) {
break;
}
}

#if !(defined(__CYGWIN__) && !defined(SOLAR_PRIV))
if (hasCaps() != TRUE) {
if (seteuid(getuid()) == -1) {
Expand Down Expand Up @@ -841,6 +848,61 @@ int ping_tcp(host_t *host, ping_t *ping) {
}
}

/*! \fn int get_address_type(host_t *host)
* \brief determines using getaddrinfo the iptype and returns the iptype
*
* \return 1 - IPv4, 2 - IPv6, -1 - Unknown
*/
int get_address_type(host_t *host) {
struct addrinfo hints, *res;
char addrstr[255];
void *ptr;

memset(&hints, 0, sizeof(hints));

hints.ai_family = AF_UNSPEC;
hints.ai_socktype = SOCK_DGRAM;
hints.ai_flags = AI_CANONNAME | AI_ADDRCONFIG;
int error;

if ((error = getaddrinfo(host->hostname, NULL, &hints, &res)) != 0) {
SPINE_LOG(("WARNING: Unable to determine address info for %s (%s)", host->hostname, gai_strerror(error)));
if (res = NULL) {

This comment has been minimized.

Sign in to view

Copy link
@nuno-silva

nuno-silva Mar 22, 2020

Contributor

this is a bug right here @TheWitness (= instead of != perhaps?)

This comment has been minimized.

Sign in to view

Copy link
@TheWitness

TheWitness Mar 22, 2020

Author Member

Yea, that is wrong. Oops.
freeaddrinfo(res);
}
return -1;
}

while (res) {
inet_ntop(res->ai_family, res->ai_addr->sa_data, addrstr, 100);

switch(res->ai_family) {
case AF_INET:
ptr = &((struct sockaddr_in *) res->ai_addr)->sin_addr;
break;
case AF_INET6:
ptr = &((struct sockaddr_in6 *) res->ai_addr)->sin6_addr;
break;
}

inet_ntop(res->ai_family, ptr, addrstr, 100);

SPINE_LOG_HIGH(("Device[%d] IPv%d address %s (%s)\n", host->id, res->ai_family == PF_INET6 ? 6:4, addrstr, res->ai_canonname));

if (res->ai_family == PF_INET6) {
return SPINE_IPV6;
} else {
return SPINE_IPV4;
}

res = res->ai_next;
}

freeaddrinfo(res);

This comment has been minimized.

Sign in to view

Copy link
@nuno-silva

nuno-silva Mar 22, 2020

Contributor

@TheWitness this is only freeing up the last element of the linked list returned by getaddrinfo; you should use a temporary pointer to iterate the list instead of replacing the one returned by getaddrinfo. Example here: http://man7.org/linux/man-pages/man3/getaddrinfo.3.html

This comment has been minimized.

Sign in to view

Copy link
@nuno-silva

nuno-silva Mar 22, 2020

Contributor

also note that return SPINE_XXX; is leaking the memory allocated for the linked list. It's probably missing a freeaddrinfo before the return.

This comment has been minimized.

Sign in to view

Copy link
@TheWitness

TheWitness Mar 22, 2020

Author Member

Yea, I'm a little rusty at Ansi C. It's a hobby after all.

return 1;
}

/*! \fn int init_sockaddr(struct sockaddr_in *name, const char *hostname, unsigned short int port)
* \brief converts a hostname to an internet address
*
Expand All @@ -854,33 +916,62 @@ int init_sockaddr(struct sockaddr_in *name, const char *hostname, unsigned short
// Initialize the hints structure
memset(&hints, 0, sizeof hints);

// This should be AF_UNSPEC and we should work out if we are using
// AF_INET or AF_INET6 later, but without checking the rest of spine
// for IPv6 compatibility, lets first make it work with IPv4 only
hints.ai_family = AF_INET;
hints.ai_flags = AI_CANONNAME | AI_ADDRCONFIG;
retry_count = 0;
rv = 0;

while (1) {
while (TRUE) {
rv = getaddrinfo(hostname, NULL, &hints, &hostinfo);
if (rv) {
if (rv == TRY_AGAIN && retry_count < 3) {
if (hostinfo != NULL) {
freeaddrinfo(hostinfo);
}

retry_count++;
usleep(50000);
continue;
} else {
SPINE_LOG(("WARNING: Error resolving host %s (%s)", hostname, gai_strerror(rv)));
if (hostinfo != NULL) {
freeaddrinfo(hostinfo);
}
return FALSE;
}
} else {
if (rv == 0) {
break;
} else {
switch (rv) {
case EAI_AGAIN:
if (retry_count < 3) {
SPINE_LOG(("WARNING: EAGAIN received resolving after 3 retryies for host %s (%s)", hostname, gai_strerror(rv)));
if (hostinfo != NULL) {
freeaddrinfo(hostinfo);
}

retry_count++;
usleep(50000);
continue;
} else {
SPINE_LOG(("WARNING: Error resolving after 3 retryies for host %s (%s)", hostname, gai_strerror(rv)));
if (hostinfo != NULL) {
freeaddrinfo(hostinfo);
}
return FALSE;
}

break;
case EAI_FAIL:
SPINE_LOG(("WARNING: DNS Server reported permanent error for host %s (%s)", hostname, gai_strerror(rv)));
if (hostinfo != NULL) {
freeaddrinfo(hostinfo);
}
return FALSE;

break;
case EAI_MEMORY:
SPINE_LOG(("WARNING: Out of memory trying to resolve host %s (%s)", hostname, gai_strerror(rv)));
if (hostinfo != NULL) {
freeaddrinfo(hostinfo);
}
return FALSE;

break;
default:
SPINE_LOG(("WARNING: Unknown error while resolving host %s (%s)", hostname, gai_strerror(rv)));
if (hostinfo != NULL) {
freeaddrinfo(hostinfo);
}
return FALSE;

break;
}
}
}

Expand Down
1 change: 1 addition & 0 deletions ping.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,4 +142,5 @@ extern int ping_tcp(host_t *host, ping_t *ping);
extern char *remove_tcp_udp_from_hostname(char *hostname);
extern void update_host_status(int status, host_t *host, ping_t *ping, int availability_method);
extern int init_sockaddr(struct sockaddr_in *name, const char *hostname, unsigned short int port);
extern int get_address_type(host_t *host);
extern unsigned short int get_checksum(void* buf, int len);
5 changes: 4 additions & 1 deletion poller.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1631,7 +1631,10 @@ void poll_host(int host_id, int host_thread, int last_host_thread, int host_data

free(host);
free(reindex);
free(ping);

if (ping != NULL) {
free(ping);

This comment has been minimized.

Sign in to view

Copy link
@nuno-silva

nuno-silva Mar 22, 2020

Contributor

this check is unnecessary:

If ptr is a null pointer, no action shall occur.

http://man7.org/linux/man-pages/man3/free.3p.html

This comment has been minimized.

Sign in to view

Copy link
@TheWitness

TheWitness Mar 22, 2020

Author Member

I was getting a doublefree segfault here before.

This comment has been minimized.

Sign in to view

Copy link
@nuno-silva

nuno-silva Mar 23, 2020

Contributor

Weird. Is your OS POSIX-compliant?

Another possibility is this:

void *p = xxxx;

free(p); // once
free(p); // twice, will crash, because p is already freed but is not NULL

In this case, adding the NULL check will not fix the issue, but this will:

void *p = xxxx;

free(p); // once
p=NULL;
free(p); // twice, but it's OK because p is NULL
}

/* update poller_items table for next polling interval */
if (host_thread == last_host_thread) {
Expand Down
3 changes: 3 additions & 0 deletions spine.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@
#define LOCAL 0
#define REMOTE 1

#define SPINE_IPV4 1
#define SPINE_IPV6 2

#ifndef __GNUC__
# define __attribute__(x) /* NOTHING */
#endif
Expand Down

0 comments on commit 5644db7

Please sign in to comment.