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 a8bf5d5..4b23756 100644 (file)
--- 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 9592f98..73dadc4 100644 (file)
--- 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 d418aaf..b4360a6 100644 (file)
--- 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 99fafcc..ad0185a 100644 (file)
--- 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 fd79fdc..60f46cb 100644 (file)
--- 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 249724d..9f25fb9 100644 (file)
--- 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 b889bca..cfcf1c3 100644 (file)
--- 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 6e24d5f..232c2c4 100644 (file)
--- 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 0b97118..c61e43c 100644 (file)
--- 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 7416d11..d15d2ff 100644 (file)
--- 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;
 }