Properly cleanup and free struct cfg and servers

5 files changed, 154 insertions, 118 deletions

diff --git a/minecctl/minecctl-rcon.c b/minecctl/minecctl-rcon.c
index 27c586c..96d3522 100644
--- a/minecctl/minecctl-rcon.c
+++ b/minecctl/minecctl-rcon.c
@@ -133,11 +133,8 @@ rcon_login(struct cfg *cfg, struct server *server)
 	send_msg(fd, buf, sizeof(buf), RCON_PACKET_LOGIN, server->rcon_password,
 		 &rtype, &reply);
 
-	/* An rcon password isn't exactly super-secret, but can't hurt */
 	explicit_bzero(buf, sizeof(buf));
-	explicit_bzero(server->rcon_password, strlen(server->rcon_password));
-	xfree(server->rcon_password);
-	server->rcon_password = NULL;
+	free_password(&server->rcon_password);
 
 	if (rtype == RCON_PACKET_LOGIN_OK)
 		verbose("%s: login ok", server->name);
@@ -236,20 +233,6 @@ get_one_status(int fd, char *buf, size_t len, const char *cmd,
 		return false;
 }
 
-static struct server *
-get_default_server(struct cfg *cfg)
-{
-	struct server *server;
-
-	server = list_first_entry_or_null(&cfg->servers, struct server, list);
-	if (!server)
-		die("No servers defined");
-
-	read_server_config(server);
-
-	return server;
-}
-
 void
 do_status(struct cfg *cfg) {
 	char buf[4096];
diff --git a/minecctl/minecctl.c b/minecctl/minecctl.c
index 19bcc1e..90939ee 100644
--- a/minecctl/minecctl.c
+++ b/minecctl/minecctl.c
@@ -32,19 +32,20 @@ dump_config()
 
 	info("Configuration");
 	info("=============");
-	info("password   : %s", cfg->password);
-	info("cfgdir     : %s", cfg->cfgdir);
-	info("addrstr    : %s", cfg->addrstr);
-	info("cmdstr     : %s", cfg->cmdstr);
-	info("cmd        : %p", cfg->cmd);
-	info("force stop : %s", cfg->force_stop ? "yes" : "no");
+	info("cfgdir        : %s", cfg->cfgdir);
+	info("rcon_password : %s", cfg->rcon_password);
+	info("rcon_addrstr  : %s", cfg->rcon_addrstr);
+	info("mc_addrstr    : %s", cfg->mc_addrstr);
+	info("cmdstr        : %s", cfg->cmdstr);
+	info("cmd           : %p", cfg->cmd);
+	info("force stop    : %s", cfg->force_stop ? "yes" : "no");
 
 	list_for_each_entry(server, &cfg->servers, list) {
 		info(" * server");
-		info("    name          : %s", server->name);
-		info("    filename      : %s", server->filename);
-		info("    rcon_password : %s", server->rcon_password);
-		info("    file_read     : %s", server->file_read ? "yes" : "no");
+		info("    name             : %s", server->name);
+		info("    filename         : %s", server->filename);
+		info("    rcon_password    : %s", server->rcon_password);
+		info("    file_read        : %s", server->file_read ? "yes" : "no");
 		list_for_each_entry(saddr, &server->rcon_addrs, list)
 			info("    * rcon addr   : %s", saddr->addrstr);
 		list_for_each_entry(saddr, &server->mc_addrs, list)
@@ -147,9 +148,9 @@ usage(bool no_error)
 	info("Usage: %s [OPTION...] COMMAND\n"
 	     "\n"
 	     "Valid options:\n"
-	     "  -p, --password=PWD       use PWD as the rcon password\n"
+	     "  -p, --rcon-password=PWD  use PWD when connecting via rcon\n"
 	     "                           (or use environment variable RCON_PASSWORD)\n"
-	     "  -a, --rcon-address=ADDR  connect to rcon at ADDR\n"
+	     "  -r, --rcon-address=ADDR  connect to rcon at ADDR\n"
 	     "                           (or use environment variable RCON_ADDRESS)\n"
 	     "  -m, --mc-address=ADDR    connect to Minecraft server at ADDR\n"
 	     "                           (only relevant for some commands, can also\n"
@@ -180,7 +181,7 @@ usage(bool no_error)
 }
 
 static struct server *
-allocate_server()
+server_new()
 {
 	struct server *server;
 
@@ -193,6 +194,28 @@ allocate_server()
 }
 
 static void
+server_free(struct server *server)
+{
+	struct saddr *saddr, *tmp;
+
+	xfree(server->name);
+	xfree(server->filename);
+	free_password(&server->rcon_password);
+
+	list_for_each_entry_safe(saddr, tmp, &server->rcon_addrs, list) {
+		list_del(&saddr->list);
+		xfree(saddr);
+	}
+
+	list_for_each_entry_safe(saddr, tmp, &server->mc_addrs, list) {
+		list_del(&saddr->list);
+		xfree(saddr);
+	}
+
+	xfree(server);
+}
+
+static void
 get_servers()
 {
 	struct dirent *dent;
@@ -211,7 +234,7 @@ get_servers()
 		if (!is_valid_server_config_filename(dent, NULL))
 			continue;
 
-		server = allocate_server();
+		server = server_new();
 		server->filename = xstrdup(dent->d_name);
 
 		suffix = strrchr(dent->d_name, '.');
@@ -262,43 +285,41 @@ create_server_from_cmdline_args()
 {
 	struct server *server;
 
-	if (!cfg->addrstr && !cfg->mcaddrstr)
+	if (!cfg->rcon_addrstr && !cfg->mc_addrstr)
 		return false;
 
-	server = allocate_server();
+	server = server_new();
 
-	if (cfg->addrstr) {
-		if (!str_to_addrs(cfg->addrstr, &server->rcon_addrs))
+	if (cfg->rcon_addrstr) {
+		if (!str_to_addrs(cfg->rcon_addrstr, &server->rcon_addrs))
 			goto error;
 
-		server->name = cfg->addrstr;
-		cfg->addrstr = NULL;
+		server->name = cfg->rcon_addrstr;
+		cfg->rcon_addrstr = NULL;
 	}
 
-	if (cfg->mcaddrstr) {
-		if (!str_to_addrs(cfg->mcaddrstr, &server->mc_addrs))
+	if (cfg->mc_addrstr) {
+		if (!str_to_addrs(cfg->mc_addrstr, &server->mc_addrs))
 			goto error;
 
 		if (!server->name)
-			server->name = cfg->mcaddrstr;
+			server->name = cfg->mc_addrstr;
 		else
-			xfree(cfg->mcaddrstr);
+			xfree(cfg->mc_addrstr);
 
-		cfg->mcaddrstr = NULL;
+		cfg->mc_addrstr = NULL;
 	}
 
-	if (cfg->password) {
-		server->rcon_password = cfg->password;
-		cfg->password = NULL;
+	if (cfg->rcon_password) {
+		server->rcon_password = cfg->rcon_password;
+		cfg->rcon_password = NULL;
 	}
 
 	list_add(&server->list, &cfg->servers);
 	return true;
 
 error:
-	/* FIXME: free addrs */
-	xfree(server->name);
-	xfree(server);
+	server_free(server);
 	return false;
 }
 
@@ -307,12 +328,28 @@ do_list(struct cfg *cfg)
 {
 	struct server *server;
 
+	/* server->filename check excludes servers created from cmdline */
 	list_for_each_entry(server, &cfg->servers, list)
-		info("%s", server->name);
+		if (server->filename)
+			info("%s", server->name);
+}
+
+struct server *
+get_default_server(struct cfg *cfg)
+{
+	struct server *server;
+
+	server = list_first_entry_or_null(&cfg->servers, struct server, list);
+	if (!server)
+		die("No servers defined");
+
+	read_server_config(server);
+
+	return server;
 }
 
 static void
-set_server(const char *name)
+set_default_server(const char *name)
 {
 	struct server *server;
 
@@ -332,12 +369,12 @@ set_server(const char *name)
 static inline void
 get_optional_server_arg(char * const **argv, bool more)
 {
-	if (!cfg->addrstr) {
+	if (!cfg->rcon_addrstr) {
 		if (!**argv) {
 			error("Missing arguments");
 			usage(false);
 		}
-		set_server(**argv);
+		set_default_server(**argv);
 		(*argv)++;
 	}
 
@@ -356,7 +393,7 @@ parse_command(char * const *argv)
 
 	/* Shorthand for console if address is set */
 	if (!*argv) {
-		if (!cfg->addrstr) {
+		if (!cfg->rcon_addrstr) {
 			error("Missing arguments");
 			usage(false);
 		}
@@ -457,17 +494,17 @@ parse_cmdline(int argc, char * const *argv)
 	while (true) {
 		int option_index = 0;
 		static struct option long_options[] = {
-			{ "password",	  required_argument,	0, 'p' },
-			{ "rcon-address", required_argument,	0, 'a' },
-			{ "mc-address",   required_argument,	0, 'A' },
-			{ "cfgdir",	  required_argument,	0, 'c' },
-			{ "verbose",	  no_argument,		0, 'v' },
-			{ "force",	  no_argument,		0, 'f' },
-			{ "help",	  no_argument,		0, 'h' },
-			{ 0,		  0,			0,  0  }
+			{ "rcon-password", required_argument,	0, 'p' },
+			{ "rcon-address",  required_argument,	0, 'r' },
+			{ "mc-address",    required_argument,	0, 'm' },
+			{ "cfgdir",	   required_argument,	0, 'c' },
+			{ "verbose",	   no_argument,		0, 'v' },
+			{ "force",	   no_argument,		0, 'f' },
+			{ "help",	   no_argument,		0, 'h' },
+			{ 0,		   0,			0,  0  }
 		};
 
-		c = getopt_long(argc, argv, ":p:a:m:c:fvh",
+		c = getopt_long(argc, argv, ":p:r:m:c:vfh",
 				long_options, &option_index);
 
 		if (c == -1)
@@ -475,23 +512,23 @@ parse_cmdline(int argc, char * const *argv)
 
 		switch (c) {
 		case 'p':
-			cfg->password = xstrdup(optarg);
+			cfg->rcon_password = xstrdup(optarg);
 			break;
-		case 'a':
-			cfg->addrstr = xstrdup(optarg);
+		case 'r':
+			cfg->rcon_addrstr = xstrdup(optarg);
 			break;
 		case 'm':
-			cfg->mcaddrstr = xstrdup(optarg);
+			cfg->mc_addrstr = xstrdup(optarg);
 			break;
 		case 'c':
 			cfg->cfgdir = optarg;
 			break;
-		case 'f':
-			cfg->force_stop = true;
-			break;
 		case 'v':
 			debug_mask |= DBG_VERBOSE;
 			break;
+		case 'f':
+			cfg->force_stop = true;
+			break;
 		case 'h':
 			usage(true);
 		default:
@@ -500,22 +537,22 @@ parse_cmdline(int argc, char * const *argv)
 		}
 	}
 
-	if (!cfg->password) {
+	if (!cfg->rcon_password) {
 		e = getenv("RCON_PASSWORD");
 		if (e)
-			cfg->password = xstrdup(e);
+			cfg->rcon_password = xstrdup(e);
 	}
 
-	if (!cfg->addrstr) {
+	if (!cfg->rcon_addrstr) {
 		e = getenv("RCON_ADDRESS");
 		if (e)
-			cfg->addrstr = xstrdup(e);
+			cfg->rcon_addrstr = xstrdup(e);
 	}
 
-	if (!cfg->mcaddrstr) {
+	if (!cfg->mc_addrstr) {
 		e = getenv("MC_ADDRESS");
 		if (e)
-			cfg->mcaddrstr = xstrdup(e);
+			cfg->mc_addrstr = xstrdup(e);
 	}
 }
 
@@ -523,6 +560,7 @@ int
 main(int argc, char **argv)
 {
 	int rv = EXIT_FAILURE;
+	struct server *server, *tmp;
 
 	debug_mask = DBG_ERROR | DBG_INFO;
 
@@ -542,7 +580,7 @@ main(int argc, char **argv)
 		goto out;
 	}
 
-	if (cfg->addrstr || cfg->mcaddrstr)
+	if (cfg->rcon_addrstr || cfg->mc_addrstr)
 		if (!create_server_from_cmdline_args())
 			goto out;
 	dump_config();
@@ -553,12 +591,12 @@ main(int argc, char **argv)
 	rv = EXIT_SUCCESS;
 
 out:
-	/* FIXME: Not enough cleanup */
-	if (cfg->password)
-		explicit_bzero(cfg->password, strlen(cfg->password));
-	xfree(cfg->password);
-	xfree(cfg->addrstr);
-	xfree(cfg->mcaddrstr);
+	list_for_each_entry_safe(server, tmp, &cfg->servers, list)
+		server_free(server);
+	free_password(&cfg->rcon_password);
+	xfree(cfg->rcon_addrstr);
+	xfree(cfg->mc_addrstr);
+	xfree(cfg->cmdstr);
 	xfree(cfg);
 	exit(rv);
 }
diff --git a/minecctl/minecctl.h b/minecctl/minecctl.h
index 604225d..9e3d79b 100644
--- a/minecctl/minecctl.h
+++ b/minecctl/minecctl.h
@@ -13,10 +13,10 @@ struct server {
 
 struct cfg {
 	/* command line arguments */
-	char *cfgdir;
-	char *password;
-	char *addrstr;
-	char *mcaddrstr;
+	const char *cfgdir;
+	char *rcon_password;
+	char *rcon_addrstr;
+	char *mc_addrstr;
 	char *cmdstr;
 	bool force_stop;
 
@@ -27,5 +27,7 @@ struct cfg {
 
 void read_server_config(struct server *server);
 
+struct server *get_default_server(struct cfg *cfg);
+
 #endif
 
diff --git a/minecctl/misc.c b/minecctl/misc.c
index 13b4377..bb33161 100644
--- a/minecctl/misc.c
+++ b/minecctl/misc.c
@@ -61,6 +61,41 @@ strv_join(char * const *strv)
 	return r;
 }
 
+int
+connect_any(struct list_head *addrs, bool may_fail)
+{
+	struct saddr *saddr;
+	bool connected = false;
+	int sfd;
+
+	if (list_empty(addrs))
+		die("No address to connect to");
+
+	list_for_each_entry(saddr, addrs, list) {
+		verbose("Attempting connection to %s", saddr->addrstr);
+		sfd = socket(saddr->storage.ss_family, SOCK_STREAM | SOCK_CLOEXEC, 0);
+		if (sfd < 0)
+			die("socket: %m");
+
+		socket_set_low_latency(sfd, true, true, true);
+
+		if (connect(sfd, (struct sockaddr *)&saddr->storage, saddr->addrlen) < 0) {
+			close(sfd);
+			continue;
+		}
+
+		connected = true;
+		break;
+	}
+
+	if (!connected && may_fail)
+		return -1;
+	else if (!connected)
+		die("Failed to connect to remote host");
+
+	return sfd;
+}
+
 char *
 ask_password()
 {
@@ -99,39 +134,15 @@ ask_password()
 	return password;
 }
 
-int
-connect_any(struct list_head *addrs, bool may_fail)
+void
+free_password(char **password)
 {
-	struct saddr *saddr;
-	bool connected = false;
-	int sfd;
-
-	if (list_empty(addrs))
-		die("No address to connect to");
-
-	list_for_each_entry(saddr, addrs, list) {
-		verbose("Attempting connection to %s", saddr->addrstr);
-		sfd = socket(saddr->storage.ss_family, SOCK_STREAM | SOCK_CLOEXEC, 0);
-		if (sfd < 0)
-			die("socket: %m");
-
-		socket_set_low_latency(sfd, true, true, true);
-
-		if (connect(sfd, (struct sockaddr *)&saddr->storage, saddr->addrlen) < 0) {
-			close(sfd);
-			continue;
-		}
-
-		connected = true;
-		break;
-	}
-
-	if (!connected && may_fail)
-		return -1;
-	else if (!connected)
-		die("Failed to connect to remote host");
+	if (!password || !*password)
+		return;
 
-	return sfd;
+	explicit_bzero(*password, strlen(*password));
+	xfree(*password);
+	*password = NULL;
 }
 
 void
diff --git a/minecctl/misc.h b/minecctl/misc.h
index ca36dab..155681e 100644
--- a/minecctl/misc.h
+++ b/minecctl/misc.h
@@ -11,4 +11,6 @@ int connect_any(struct list_head *addrs, bool may_fail);
 
 char *ask_password();
 
+void free_password(char **password);
+
 #endif