From: Guus Sliepen Date: Tue, 6 May 2014 19:34:26 +0000 (+0200) Subject: Fix issues found by Coverity. X-Git-Tag: release-1.0.24~8 X-Git-Url: https://www.tinc-vpn.org/git/browse?p=tinc;a=commitdiff_plain;h=e913f3f232e4809b7218d081ab9f94cef1c94ac3 Fix issues found by Coverity. 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. --- diff --git a/src/avl_tree.c b/src/avl_tree.c index a8bf5d5f..4b237560 100644 --- a/src/avl_tree.c +++ b/src/avl_tree.c @@ -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; @@ -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; diff --git a/src/graph.c b/src/graph.c index 9592f980..73dadc47 100644 --- a/src/graph.c +++ b/src/graph.c @@ -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; } @@ -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); } diff --git a/src/linux/device.c b/src/linux/device.c index d418aaf6..b4360a6a 100644 --- a/src/linux/device.c +++ b/src/linux/device.c @@ -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 @@ -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); } diff --git a/src/multicast_device.c b/src/multicast_device.c index 99fafcc8..ad0185a3 100644 --- a/src/multicast_device.c +++ b/src/multicast_device.c @@ -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; } @@ -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; } @@ -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; } @@ -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 @@ -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 @@ -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; diff --git a/src/net.c b/src/net.c index fd79fdc7..60f46cbb 100644 --- a/src/net.c +++ b/src/net.c @@ -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); diff --git a/src/net_setup.c b/src/net_setup.c index 249724d3..9f25fb95 100644 --- a/src/net_setup.c +++ b/src/net_setup.c @@ -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; } @@ -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); @@ -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; @@ -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; @@ -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 { @@ -599,6 +598,7 @@ static bool setup_myself(void) { return false; } } + free(cipher); } else myself->incipher = EVP_bf_cbc(); @@ -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(); @@ -690,6 +693,7 @@ static bool setup_myself(void) { else if(!strcasecmp(type, "vde")) devops = vde_devops; #endif + free(type); } if(!devops.setup()) diff --git a/src/net_socket.c b/src/net_socket.c index b889bca6..cfcf1c39 100644 --- a/src/net_socket.c +++ b/src/net_socket.c @@ -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 @@ -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 @@ -407,7 +410,6 @@ begin: 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 { @@ -416,7 +418,6 @@ begin: 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) { @@ -424,6 +425,9 @@ begin: goto begin; } + if(proxytype != PROXY_EXEC) + configure_tcp(c); + #ifdef FD_CLOEXEC fcntl(c->socket, F_SETFD, FD_CLOEXEC); #endif diff --git a/src/pidfile.c b/src/pidfile.c index 6e24d5f0..232c2c42 100644 --- a/src/pidfile.c +++ b/src/pidfile.c @@ -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 diff --git a/src/raw_socket_device.c b/src/raw_socket_device.c index 0b971189..c61e43c9 100644 --- a/src/raw_socket_device.c +++ b/src/raw_socket_device.c @@ -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; diff --git a/src/tincd.c b/src/tincd.c index 7416d11b..d15d2ffa 100644 --- a/src/tincd.c +++ b/src/tincd.c @@ -162,6 +162,11 @@ static bool parse_options(int argc, char **argv) { break; case 'c': /* config file */ + if(confbase) { + fprintf(stderr, "Only one configuration directory can be given.\n"); + usage(true); + return false; + } confbase = xstrdup(optarg); break; @@ -229,6 +234,11 @@ static bool parse_options(int argc, char **argv) { case 'n': /* net name given */ /* netname "." is special: a "top-level name" */ + if(netname) { + fprintf(stderr, "Only one netname can be given.\n"); + usage(true); + return false; + } netname = strcmp(optarg, ".") != 0 ? xstrdup(optarg) : NULL; break; @@ -281,11 +291,22 @@ static bool parse_options(int argc, char **argv) { use_logfile = true; if(!optarg && optind < argc && *argv[optind] != '-') optarg = argv[optind++]; - if(optarg) + if(optarg) { + if(logfilename) { + fprintf(stderr, "Only one logfile can be given.\n"); + usage(true); + return false; + } logfilename = xstrdup(optarg); + } break; case 5: /* write PID to a file */ + if(pidfilename) { + fprintf(stderr, "Only one pidfile can be given.\n"); + usage(true); + return false; + } pidfilename = xstrdup(optarg); break; @@ -350,7 +371,6 @@ static void indicator(int a, int b, void *p) { static bool keygen(int bits) { RSA *rsa_key; FILE *f; - char *name = get_name(); char *pubname, *privname; fprintf(stderr, "Generating %d bits keys:\n", bits); @@ -378,10 +398,14 @@ static bool keygen(int bits) { PEM_write_RSAPrivateKey(f, rsa_key, NULL, NULL, 0, NULL, NULL); fclose(f); - if(name) + char *name = get_name(); + + if(name) { xasprintf(&pubname, "%s/hosts/%s", confbase, name); - else + free(name); + } else { xasprintf(&pubname, "%s/rsa_key.pub", confbase); + } f = ask_and_open(pubname, "public RSA key"); free(pubname); @@ -392,7 +416,6 @@ static bool keygen(int bits) { fputc('\n', f); PEM_write_RSAPublicKey(f, rsa_key); fclose(f); - free(name); return true; }