Skip to content

Commit

Permalink
Fix issues found by Coverity.
Browse files Browse the repository at this point in the history
Most of the problems found were resource leaks in error paths, some NULL
pointer dereferences that do not happen in practice, and a few other issues.
They have all been fixed now anyway.
  • Loading branch information
gsliepen committed May 6, 2014
1 parent 8d64561 commit e913f3f
Show file tree
Hide file tree
Showing 10 changed files with 94 additions and 42 deletions.
5 changes: 3 additions & 2 deletions src/avl_tree.c
Original file line number Diff line number Diff line change
Expand Up @@ -228,8 +228,7 @@ static void avl_rebalance(avl_tree_t *tree, avl_node_t *node)
gchild->left->parent = gchild;
gchild->right = child;

if(gchild->right)
gchild->right->parent = gchild;
gchild->right->parent = gchild;

*superparent = gchild;
gchild->parent = parent;
Expand Down Expand Up @@ -600,6 +599,8 @@ void avl_unlink_node(avl_tree_t *tree, avl_node_t *node)
balnode = parent;
} else {
subst = node->prev;
if(!subst) // This only happens if node is not actually in a tree at all.
abort();

if(subst == left) {
balnode = subst;
Expand Down
6 changes: 5 additions & 1 deletion src/graph.c
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,7 @@ void dump_graph(void) {

if(!file) {
logger(LOG_ERR, "Unable to open graph dump file %s: %s", filename, strerror(errno));
free(filename);
free(tmpname);
return;
}
Expand Down Expand Up @@ -367,7 +368,10 @@ void dump_graph(void) {
#ifdef HAVE_MINGW
unlink(filename);
#endif
rename(tmpname, filename);
if(rename(tmpname, filename))
logger(LOG_ERR, "Could not rename %s to %s: %s\n", tmpname, filename, strerror(errno));
free(tmpname);
}

free(filename);
}
13 changes: 8 additions & 5 deletions src/linux/device.c
Original file line number Diff line number Diff line change
Expand Up @@ -107,17 +107,21 @@ static bool setup_device(void) {
ifr.ifr_flags |= IFF_ONE_QUEUE;
#endif

if(iface)
if(iface) {
strncpy(ifr.ifr_name, iface, IFNAMSIZ);
ifr.ifr_name[IFNAMSIZ - 1] = 0;
}

if(!ioctl(device_fd, TUNSETIFF, &ifr)) {
strncpy(ifrname, ifr.ifr_name, IFNAMSIZ);
if(iface) free(iface);
ifrname[IFNAMSIZ - 1] = 0;
free(iface);
iface = xstrdup(ifrname);
} else if(!ioctl(device_fd, (('T' << 8) | 202), &ifr)) {
logger(LOG_WARNING, "Old ioctl() request was needed for %s", device);
strncpy(ifrname, ifr.ifr_name, IFNAMSIZ);
if(iface) free(iface);
ifrname[IFNAMSIZ - 1] = 0;
free(iface);
iface = xstrdup(ifrname);
} else
#endif
Expand All @@ -126,8 +130,7 @@ static bool setup_device(void) {
overwrite_mac = true;
device_info = "Linux ethertap device";
device_type = DEVICE_TYPE_ETHERTAP;
if(iface)
free(iface);
free(iface);
iface = xstrdup(strrchr(device, '/') ? strrchr(device, '/') + 1 : device);
}

Expand Down
7 changes: 7 additions & 0 deletions src/multicast_device.c
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ static bool setup_device(void) {
space = strchr(host, ' ');
if(!space) {
logger(LOG_ERR, "Port number required for %s", device_info);
free(host);
return false;
}

Expand All @@ -75,6 +76,7 @@ static bool setup_device(void) {
device_fd = socket(ai->ai_family, SOCK_DGRAM, IPPROTO_UDP);
if(device_fd < 0) {
logger(LOG_ERR, "Creating socket failed: %s", sockstrerror(sockerrno));
free(host);
return false;
}

Expand All @@ -88,6 +90,7 @@ static bool setup_device(void) {
if(bind(device_fd, ai->ai_addr, ai->ai_addrlen)) {
closesocket(device_fd);
logger(LOG_ERR, "Can't bind to %s %s: %s", host, port, sockstrerror(sockerrno));
free(host);
return false;
}

Expand All @@ -102,6 +105,7 @@ static bool setup_device(void) {
if(setsockopt(device_fd, IPPROTO_IP, IP_ADD_MEMBERSHIP, (void *)&mreq, sizeof mreq)) {
logger(LOG_ERR, "Cannot join multicast group %s %s: %s", host, port, sockstrerror(sockerrno));
closesocket(device_fd);
free(host);
return false;
}
#ifdef IP_MULTICAST_LOOP
Expand All @@ -123,6 +127,7 @@ static bool setup_device(void) {
if(setsockopt(device_fd, IPPROTO_IPV6, IPV6_JOIN_GROUP, (void *)&mreq, sizeof mreq)) {
logger(LOG_ERR, "Cannot join multicast group %s %s: %s", host, port, sockstrerror(sockerrno));
closesocket(device_fd);
free(host);
return false;
}
#ifdef IPV6_MULTICAST_LOOP
Expand All @@ -137,9 +142,11 @@ static bool setup_device(void) {
default:
logger(LOG_ERR, "Multicast for address family %hx unsupported", ai->ai_family);
closesocket(device_fd);
free(host);
return false;
}

free(host);
logger(LOG_INFO, "%s is a %s", device, device_info);

return true;
Expand Down
6 changes: 6 additions & 0 deletions src/net.c
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,12 @@ void terminate_connection(connection_t *c, bool report) {
closesocket(c->socket);

if(c->edge) {
if(!c->node) {
logger(LOG_ERR, "Connection to %s (%s) has an edge but node is NULL!", c->name, c->hostname);
// And that should never happen.
abort();
}

if(report && !tunnelserver)
send_del_edge(everyone, c->edge);

Expand Down
40 changes: 22 additions & 18 deletions src/net_setup.c
Original file line number Diff line number Diff line change
Expand Up @@ -165,23 +165,25 @@ static bool read_rsa_private_key(void) {
char *fname, *key, *pubkey;

if(get_config_string(lookup_config(config_tree, "PrivateKey"), &key)) {
if(!get_config_string(lookup_config(config_tree, "PublicKey"), &pubkey)) {
logger(LOG_ERR, "PrivateKey used but no PublicKey found!");
return false;
}
myself->connection->rsa_key = RSA_new();
// RSA_blinding_on(myself->connection->rsa_key, NULL);
if(BN_hex2bn(&myself->connection->rsa_key->d, key) != strlen(key)) {
logger(LOG_ERR, "Invalid PrivateKey for myself!");
free(key);
return false;
}
free(key);
if(!get_config_string(lookup_config(config_tree, "PublicKey"), &pubkey)) {
logger(LOG_ERR, "PrivateKey used but no PublicKey found!");
return false;
}
if(BN_hex2bn(&myself->connection->rsa_key->n, pubkey) != strlen(pubkey)) {
logger(LOG_ERR, "Invalid PublicKey for myself!");
free(pubkey);
return false;
}
BN_hex2bn(&myself->connection->rsa_key->e, "FFFF");
free(key);
free(pubkey);
BN_hex2bn(&myself->connection->rsa_key->e, "FFFF");
return true;
}

Expand All @@ -200,15 +202,12 @@ static bool read_rsa_private_key(void) {
#if !defined(HAVE_MINGW) && !defined(HAVE_CYGWIN)
struct stat s;

if(fstat(fileno(fp), &s)) {
logger(LOG_ERR, "Could not stat RSA private key file `%s': %s'",
fname, strerror(errno));
free(fname);
return false;
if(!fstat(fileno(fp), &s)) {
if(s.st_mode & ~0100700)
logger(LOG_WARNING, "Warning: insecure file permissions for RSA private key file `%s'!", fname);
} else {
logger(LOG_WARNING, "Could not stat RSA private key file `%s': %s'", fname, strerror(errno));
}

if(s.st_mode & ~0100700)
logger(LOG_WARNING, "Warning: insecure file permissions for RSA private key file `%s'!", fname);
#endif

myself->connection->rsa_key = PEM_read_RSAPrivateKey(fp, NULL, NULL, NULL);
Expand Down Expand Up @@ -299,10 +298,12 @@ char *get_name(void) {
if(!envname) {
if(strcmp(name + 1, "HOST")) {
fprintf(stderr, "Invalid Name: environment variable %s does not exist\n", name + 1);
free(name);
return false;
}
if(gethostname(hostname, sizeof hostname) || !*hostname) {
fprintf(stderr, "Could not get hostname: %s\n", strerror(errno));
free(name);
return false;
}
hostname[31] = 0;
Expand Down Expand Up @@ -385,8 +386,7 @@ static bool setup_myself(void) {
sockaddr2str(&sa, NULL, &myport);
}

get_config_string(lookup_config(config_tree, "Proxy"), &proxy);
if(proxy) {
if(get_config_string(lookup_config(config_tree, "Proxy"), &proxy)) {
if((space = strchr(proxy, ' ')))
*space++ = 0;

Expand Down Expand Up @@ -587,8 +587,7 @@ static bool setup_myself(void) {

/* Generate packet encryption key */

if(get_config_string
(lookup_config(config_tree, "Cipher"), &cipher)) {
if(get_config_string(lookup_config(config_tree, "Cipher"), &cipher)) {
if(!strcasecmp(cipher, "none")) {
myself->incipher = NULL;
} else {
Expand All @@ -599,6 +598,7 @@ static bool setup_myself(void) {
return false;
}
}
free(cipher);
} else
myself->incipher = EVP_bf_cbc();

Expand All @@ -624,9 +624,12 @@ static bool setup_myself(void) {

if(!myself->indigest) {
logger(LOG_ERR, "Unrecognized digest type!");
free(digest);
return false;
}
}

free(digest);
} else
myself->indigest = EVP_sha1();

Expand Down Expand Up @@ -690,6 +693,7 @@ static bool setup_myself(void) {
else if(!strcasecmp(type, "vde"))
devops = vde_devops;
#endif
free(type);
}

if(!devops.setup())
Expand Down
22 changes: 13 additions & 9 deletions src/net_socket.c
Original file line number Diff line number Diff line change
Expand Up @@ -87,20 +87,22 @@ static bool bind_to_interface(int sd) {
int status;
#endif /* defined(SOL_SOCKET) && defined(SO_BINDTODEVICE) */

if(!get_config_string (lookup_config (config_tree, "BindToInterface"), &iface))
if(!get_config_string(lookup_config (config_tree, "BindToInterface"), &iface))
return true;

#if defined(SOL_SOCKET) && defined(SO_BINDTODEVICE)
memset(&ifr, 0, sizeof(ifr));
strncpy(ifr.ifr_ifrn.ifrn_name, iface, IFNAMSIZ);
ifr.ifr_ifrn.ifrn_name[IFNAMSIZ - 1] = 0;
free(iface);

status = setsockopt(sd, SOL_SOCKET, SO_BINDTODEVICE, (void *)&ifr, sizeof(ifr));
if(status) {
logger(LOG_ERR, "Can't bind to interface %s: %s", iface,
strerror(errno));
logger(LOG_ERR, "Can't bind to interface %s: %s", ifr.ifr_ifrn.ifrn_name, strerror(errno));
return false;
}

free(iface);
#else /* if !defined(SOL_SOCKET) || !defined(SO_BINDTODEVICE) */
logger(LOG_WARNING, "%s not supported on this platform", "BindToInterface");
#endif
Expand Down Expand Up @@ -135,20 +137,21 @@ int setup_listen_socket(const sockaddr_t *sa) {
setsockopt(nfd, SOL_IPV6, IPV6_V6ONLY, (void *)&option, sizeof option);
#endif

if(get_config_string
(lookup_config(config_tree, "BindToInterface"), &iface)) {
if(get_config_string(lookup_config(config_tree, "BindToInterface"), &iface)) {
#if defined(SOL_SOCKET) && defined(SO_BINDTODEVICE)
struct ifreq ifr;

memset(&ifr, 0, sizeof(ifr));
strncpy(ifr.ifr_ifrn.ifrn_name, iface, IFNAMSIZ);
ifr.ifr_ifrn.ifrn_name[IFNAMSIZ - 1] = 0;
free(iface);

if(setsockopt(nfd, SOL_SOCKET, SO_BINDTODEVICE, (void *)&ifr, sizeof(ifr))) {
closesocket(nfd);
logger(LOG_ERR, "Can't bind to interface %s: %s", iface,
strerror(sockerrno));
logger(LOG_ERR, "Can't bind to interface %s: %s", ifr.ifr_ifrn.ifrn_name, strerror(sockerrno));
return -1;
}

#else
logger(LOG_WARNING, "%s not supported on this platform", "BindToInterface");
#endif
Expand Down Expand Up @@ -407,7 +410,6 @@ void do_outgoing_connection(connection_t *c) {

if(!proxytype) {
c->socket = socket(c->address.sa.sa_family, SOCK_STREAM, IPPROTO_TCP);
configure_tcp(c);
} else if(proxytype == PROXY_EXEC) {
do_outgoing_pipe(c, proxyhost);
} else {
Expand All @@ -416,14 +418,16 @@ void do_outgoing_connection(connection_t *c) {
goto begin;
ifdebug(CONNECTIONS) logger(LOG_INFO, "Using proxy at %s port %s", proxyhost, proxyport);
c->socket = socket(proxyai->ai_family, SOCK_STREAM, IPPROTO_TCP);
configure_tcp(c);
}

if(c->socket == -1) {
ifdebug(CONNECTIONS) logger(LOG_ERR, "Creating socket for %s failed: %s", c->hostname, sockstrerror(sockerrno));
goto begin;
}

if(proxytype != PROXY_EXEC)
configure_tcp(c);

#ifdef FD_CLOEXEC
fcntl(c->socket, F_SETFD, FD_CLOEXEC);
#endif
Expand Down
2 changes: 1 addition & 1 deletion src/pidfile.c
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ pid_t read_pid (const char *pidfile)
if(fscanf(f,"%20ld", &pid) != 1)
pid = 0;
fclose(f);
return pid;
return (pid_t)pid;
}

/* check_pid
Expand Down
2 changes: 1 addition & 1 deletion src/raw_socket_device.c
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ static bool setup_device(void) {
return false;
}

memset(&sa, '0', sizeof(sa));
memset(&sa, 0, sizeof(sa));
sa.sll_family = AF_PACKET;
sa.sll_protocol = htons(ETH_P_ALL);
sa.sll_ifindex = ifr.ifr_ifindex;
Expand Down
Loading

0 comments on commit e913f3f

Please sign in to comment.