[B.A.T.M.A.N.] [PATCH 01/10] batadv-vis: Avoid memory leak after failed realloc

[B.A.T.M.A.N.] [PATCH 01/10] batadv-vis: Avoid memory leak after failed realloc

[Sven Eckelmann]

realloc doesn't free the original buffer when the reallocation failed. An abort
of read_file without free'ing the buffer would leak it.

Signed-off-by: Sven Eckelmann <sven@narfation.org>

---
 vis/vis.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/vis/vis.c b/vis/vis.c
index 7a8e780..b51fede 100644
--- a/vis/vis.c
+++ b/vis/vis.c
@@ -40,7 +40,7 @@
 static char *read_file(char *fname)
 {
 	FILE *fp;
-	char *buf = NULL;
+	char *buf = NULL, *buf_tmp;
 	size_t size, ret;
 
 	fp = fopen(fname, "r");
@@ -51,10 +51,13 @@ static char *read_file(char *fname)
 	size = 0;
 	while (!feof(fp)) {
 
-		buf = realloc(buf, size + 4097);
-		if (!buf)
+		buf_tmp = realloc(buf, size + 4097);
+		if (!buf_tmp) {
+			free(buf);
 			return NULL;
+		}
 
+		buf = buf_tmp;
 		ret = fread(buf + size, 1, 4096, fp);
 		size += ret;
 	}
-- 
2.0.0.rc2

[B.A.T.M.A.N.] [PATCH 02/10] alfred: Force null termination of string after strncpy

[Sven Eckelmann]

strncpy doesn't terminate the string with a '\0' character when the length
of the destination memory location was shorter than the source string.
Accessing it again with string related functions isn't safe after such a
semi-failed copy and the caller has to handle it. The easiest way is to
always set the last character in the destination buffer to '\0' after the
strncpy was called.

Signed-off-by: Sven Eckelmann <sven@narfation.org>

---
 debugfs.c | 1 +
 netsock.c | 1 +
 server.c  | 1 +
 vis/vis.c | 1 +
 4 files changed, 4 insertions(+)

diff --git a/debugfs.c b/debugfs.c
index 1e9418d..adada7c 100644
--- a/debugfs.c
+++ b/debugfs.c
@@ -154,6 +154,7 @@ char *debugfs_mount(const char *mountpoint)
 
 	/* save the mountpoint */
 	strncpy(debugfs_mountpoint, mountpoint, sizeof(debugfs_mountpoint));
+	debugfs_mountpoint[sizeof(debugfs_mountpoint) - 1] = '\0';
 	debugfs_found = 1;
 
 	return debugfs_mountpoint;
diff --git a/netsock.c b/netsock.c
index 08d2959..8712c11 100644
--- a/netsock.c
+++ b/netsock.c
@@ -59,6 +59,7 @@ int netsock_open(struct globals *globals)
 
 	memset(&ifr, 0, sizeof(ifr));
 	strncpy(ifr.ifr_name, globals->interface, IFNAMSIZ);
+	ifr.ifr_name[IFNAMSIZ - 1] = '\0';
 	if (ioctl(sock, SIOCGIFINDEX, &ifr) == -1) {
 		fprintf(stderr, "can't get interface: %s\n", strerror(errno));
 		goto err;
diff --git a/server.c b/server.c
index fdd97d4..e4465dc 100644
--- a/server.c
+++ b/server.c
@@ -242,6 +242,7 @@ static void check_if_socket(struct globals *globals)
 
 	memset(&ifr, 0, sizeof(ifr));
 	strncpy(ifr.ifr_name, globals->interface, IFNAMSIZ);
+	ifr.ifr_name[IFNAMSIZ - 1] = '\0';
 	if (ioctl(sock, SIOCGIFINDEX, &ifr) == -1) {
 		fprintf(stderr, "can't get interface: %s, closing netsock\n",
 			strerror(errno));
diff --git a/vis/vis.c b/vis/vis.c
index b51fede..9031b27 100644
--- a/vis/vis.c
+++ b/vis/vis.c
@@ -102,6 +102,7 @@ static int get_if_mac(char *ifname, uint8_t *mac)
 	int sock, ret;
 
 	strncpy(ifr.ifr_name, ifname, IFNAMSIZ);
+	ifr.ifr_name[IFNAMSIZ - 1] = '\0';
 
 	if ((sock = socket(AF_INET, SOCK_STREAM, 0)) < 0) {
 		fprintf(stderr, "can't get interface: %s\n", strerror(errno));
-- 
2.0.0.rc2

[B.A.T.M.A.N.] [PATCH 03/10] batadv-vis: Avoid memory leak when tl parsing failed

[Sven Eckelmann]

The parsing function for the local translation table can fail to convert the
mac address and then jumps out of the function. This leaks the earlier
allocated v_entry.

Signed-off-by: Sven Eckelmann <sven@narfation.org>

---
 vis/vis.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/vis/vis.c b/vis/vis.c
index 9031b27..a5ac664 100644
--- a/vis/vis.c
+++ b/vis/vis.c
@@ -213,8 +213,10 @@ static int parse_transtable_local(struct globals *globals)
 					continue;
 
 				mac = str_to_mac(tptr);
-				if (!mac)
+				if (!mac) {
+					free(v_entry);
 					continue;
+				}
 
 				memcpy(v_entry->v.mac, mac, ETH_ALEN);
 				v_entry->v.ifindex = 255;
-- 
2.0.0.rc2

[B.A.T.M.A.N.] [PATCH 04/10] alfred: Use pre-allocated object for global variables

[Sven Eckelmann]

The object storing the global variables is a good way to keep track of all
data. But it is unnecessary to allocate an object on the heap for it. Using
an object with static storage allows better compiler optimization and makes
checking for actual memory leaks (unreachable memory) easier.

Signed-off-by: Sven Eckelmann <sven@narfation.org>

---
 gpsd/alfred-gpsd.c | 7 +++----
 main.c             | 6 ++----
 vis/vis.c          | 7 +++----
 3 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/gpsd/alfred-gpsd.c b/gpsd/alfred-gpsd.c
index b71a9b2..ef20f28 100644
--- a/gpsd/alfred-gpsd.c
+++ b/gpsd/alfred-gpsd.c
@@ -21,6 +21,8 @@
 
 #include "alfred-gpsd.h"
 
+static struct globals gpsd_globals;
+
 static int alfred_open_sock(struct globals *globals)
 {
 	struct sockaddr_un addr;
@@ -394,10 +396,7 @@ static struct globals *gpsd_init(int argc, char *argv[])
 		{NULL,		0,			NULL,	0},
 	};
 
-	globals = malloc(sizeof(*globals));
-	if (!globals)
-		return NULL;
-
+	globals = &gpsd_globals;
 	memset(globals, 0, sizeof(*globals));
 
 	globals->opmode = OPMODE_CLIENT;
diff --git a/main.c b/main.c
index 32fb9b8..d848589 100644
--- a/main.c
+++ b/main.c
@@ -27,6 +27,7 @@
 #include "alfred.h"
 #include "packet.h"
 
+static struct globals alfred_globals;
 
 static void alfred_usage(void)
 {
@@ -69,10 +70,7 @@ static struct globals *alfred_init(int argc, char *argv[])
 		{NULL,		0,			NULL,	0},
 	};
 
-	globals = malloc(sizeof(*globals));
-	if (!globals)
-		return NULL;
-
+	globals = &alfred_globals;
 	memset(globals, 0, sizeof(*globals));
 
 	globals->opmode = OPMODE_SLAVE;
diff --git a/vis/vis.c b/vis/vis.c
index a5ac664..31f60d7 100644
--- a/vis/vis.c
+++ b/vis/vis.c
@@ -37,6 +37,8 @@
 #include <unistd.h>
 #include "debugfs.h"
 
+static struct globals vis_globals;
+
 static char *read_file(char *fname)
 {
 	FILE *fp;
@@ -832,10 +834,7 @@ static struct globals *vis_init(int argc, char *argv[])
 		{NULL,		0,			NULL,	0},
 	};
 
-	globals = malloc(sizeof(*globals));
-	if (!globals)
-		return NULL;
-
+	globals = &vis_globals;
 	memset(globals, 0, sizeof(*globals));
 
 	globals->opmode = OPMODE_CLIENT;
-- 
2.0.0.rc2

[B.A.T.M.A.N.] [PATCH 05/10] batadv-vis: Avoid invalid access in orig_list

[Sven Eckelmann]

The orig list parsing tries to gather information from 4 columns for each line.
The second part of the parsing routine should only be started when all
columns could be found. Otherwise parts of the variables are uninitialized.
Dereferencing iface in such a situation can cause a segfault.

Signed-off-by: Sven Eckelmann <sven@narfation.org>

---
 vis/vis.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/vis/vis.c b/vis/vis.c
index 31f60d7..2928d65 100644
--- a/vis/vis.c
+++ b/vis/vis.c
@@ -350,7 +350,7 @@ static int parse_orig_list(struct globals *globals)
 			default: break;
 			}
 		}
-		if (tnum >= 4) {
+		if (tnum > 4) {
 			if (strcmp(dest, neigh) == 0) {
 				tq_val = strtol(tq, NULL, 10);
 				if (tq_val < 1 || tq_val > 255)
-- 
2.0.0.rc2

[B.A.T.M.A.N.] [PATCH 06/10] alfred-gpsd: Fix altitute verification check

[Sven Eckelmann]

Signed-off-by: Sven Eckelmann <sven@narfation.org>

---
 gpsd/alfred-gpsd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gpsd/alfred-gpsd.c b/gpsd/alfred-gpsd.c
index ef20f28..089f2af 100644
--- a/gpsd/alfred-gpsd.c
+++ b/gpsd/alfred-gpsd.c
@@ -370,7 +370,7 @@ static void gpsd_parse_location(struct globals *globals,
 		exit(EXIT_FAILURE);
 	}
 
-	if ((alt < -1000) || (lon > 9000)) {
+	if ((alt < -1000) || (alt > 9000)) {
 		/* No support for aircraft or submarines! */
 		printf("Invalid altitude\n");
 		gpsd_usage();
-- 
2.0.0.rc2

Re: [B.A.T.M.A.N.] [PATCH 06/10] alfred-gpsd: Fix altitute verification check

[Andrew Lunn]

On Sat, May 24, 2014 at 01:44:11PM +0200, Sven Eckelmann wrote:
> Signed-off-by: Sven Eckelmann <sven@narfation.org>

Duh!

Thanks Sven

Acked-by: Andrew Lunn <andrew@lunn.ch>

	  Andrew

> 
> ---
>  gpsd/alfred-gpsd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gpsd/alfred-gpsd.c b/gpsd/alfred-gpsd.c
> index ef20f28..089f2af 100644
> --- a/gpsd/alfred-gpsd.c
> +++ b/gpsd/alfred-gpsd.c
> @@ -370,7 +370,7 @@ static void gpsd_parse_location(struct globals *globals,
>  		exit(EXIT_FAILURE);
>  	}
>  
> -	if ((alt < -1000) || (lon > 9000)) {
> +	if ((alt < -1000) || (alt > 9000)) {
>  		/* No support for aircraft or submarines! */
>  		printf("Invalid altitude\n");
>  		gpsd_usage();
> -- 
> 2.0.0.rc2
> 

[B.A.T.M.A.N.] [PATCH 07/10] alfred: Use strncpy instead of strcpy for string copy

[Sven Eckelmann]

The data used in strcpy is partially provided by the user. This can be larger
than the destination buffer and thus overwrite data after the actual string
buffer. This can easily be avoided by using strncpy.

Signed-off-by: Sven Eckelmann <sven@narfation.org>

---
 debugfs.c          |  4 +++-
 gpsd/alfred-gpsd.c | 18 +++++++++++++++---
 unix_sock.c        |  6 ++++--
 vis/vis.c          |  3 ++-
 4 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/debugfs.c b/debugfs.c
index adada7c..4b8801a 100644
--- a/debugfs.c
+++ b/debugfs.c
@@ -78,7 +78,9 @@ static const char *debugfs_find_mountpoint(void)
 	while (*ptr) {
 		if (debugfs_valid_mountpoint(*ptr) == 0) {
 			debugfs_found = 1;
-			strcpy(debugfs_mountpoint, *ptr);
+			strncpy(debugfs_mountpoint, *ptr,
+				sizeof(debugfs_mountpoint));
+			debugfs_mountpoint[sizeof(debugfs_mountpoint) - 1] = 0;
 			return debugfs_mountpoint;
 		}
 		ptr++;
diff --git a/gpsd/alfred-gpsd.c b/gpsd/alfred-gpsd.c
index 089f2af..84a0ded 100644
--- a/gpsd/alfred-gpsd.c
+++ b/gpsd/alfred-gpsd.c
@@ -36,7 +36,8 @@ static int alfred_open_sock(struct globals *globals)
 
 	memset(&addr, 0, sizeof(addr));
 	addr.sun_family = AF_LOCAL;
-	strcpy(addr.sun_path, ALFRED_SOCK_PATH);
+	strncpy(addr.sun_path, ALFRED_SOCK_PATH, sizeof(addr.sun_path));
+	addr.sun_path[sizeof(addr.sun_path) - 1] = '\0';
 
 	if (connect(globals->unix_sock, (struct sockaddr *)&addr,
 		    sizeof(addr)) < 0) {
@@ -300,6 +301,10 @@ static void gpsd_read_gpsd(struct globals *globals)
 	size_t cnt;
 	bool eol = false;
 	char buf[4096];
+	const size_t tpv_size = sizeof(*globals->buf) -
+				sizeof(*globals->push) -
+				sizeof(struct alfred_data) -
+				sizeof(*globals->gpsd_data);
 
 	cnt = 0;
 	do {
@@ -328,7 +333,9 @@ static void gpsd_read_gpsd(struct globals *globals)
 
 #define STARTSWITH(str, prefix)	strncmp(str, prefix, sizeof(prefix)-1)==0
 	if (STARTSWITH(buf, "{\"class\":\"TPV\"")) {
-		strcpy(globals->gpsd_data->tpv, buf);
+		strncpy(globals->gpsd_data->tpv, buf, tpv_size);
+		globals->gpsd_data->tpv[tpv_size - 1] = '\0';
+
 		globals->gpsd_data->tpv_len =
 			htonl(strlen(globals->gpsd_data->tpv) + 1);
 	}
@@ -443,6 +450,10 @@ static int gpsd_server(struct globals *globals)
 	int max_fd, ret;
 	const size_t overhead = sizeof(*globals->push) +
 		sizeof(struct alfred_data);
+	const size_t tpv_size = sizeof(*globals->buf) -
+				sizeof(*globals->push) -
+				sizeof(struct alfred_data) -
+				sizeof(*globals->gpsd_data);
 	long interval;
 
 	globals->push = (struct alfred_push_data_v0 *) globals->buf;
@@ -456,7 +467,8 @@ static int gpsd_server(struct globals *globals)
 	globals->push->data->header.type = GPSD_PACKETTYPE;
 	globals->push->data->header.version = GPSD_PACKETVERSION;
 
-	strcpy(globals->gpsd_data->tpv, GPSD_INIT_TPV);
+	strncpy(globals->gpsd_data->tpv, GPSD_INIT_TPV, tpv_size);
+	globals->gpsd_data->tpv[tpv_size - 1] = '\0';
 	globals->gpsd_data->tpv_len =
 		htonl(strlen(globals->gpsd_data->tpv) + 1);
 
diff --git a/unix_sock.c b/unix_sock.c
index 8251c81..4553db5 100644
--- a/unix_sock.c
+++ b/unix_sock.c
@@ -50,7 +50,8 @@ int unix_sock_open_daemon(struct globals *globals, const char *path)
 
 	memset(&addr, 0, sizeof(addr));
 	addr.sun_family = AF_LOCAL;
-	strcpy(addr.sun_path, path);
+	strncpy(addr.sun_path, path, sizeof(addr.sun_path));
+	addr.sun_path[sizeof(addr.sun_path) - 1] = '\0';
 
 	if (bind(globals->unix_sock, (struct sockaddr *)&addr,
 		 sizeof(addr)) < 0) {
@@ -81,7 +82,8 @@ int unix_sock_open_client(struct globals *globals, const char *path)
 
 	memset(&addr, 0, sizeof(addr));
 	addr.sun_family = AF_LOCAL;
-	strcpy(addr.sun_path, path);
+	strncpy(addr.sun_path, path, sizeof(addr.sun_path));
+	addr.sun_path[sizeof(addr.sun_path) - 1] = '\0';
 
 	if (connect(globals->unix_sock, (struct sockaddr *)&addr,
 		    sizeof(addr)) < 0) {
diff --git a/vis/vis.c b/vis/vis.c
index 2928d65..f429942 100644
--- a/vis/vis.c
+++ b/vis/vis.c
@@ -168,7 +168,8 @@ static int alfred_open_sock(struct globals *globals)
 
 	memset(&addr, 0, sizeof(addr));
 	addr.sun_family = AF_LOCAL;
-	strcpy(addr.sun_path, ALFRED_SOCK_PATH);
+	strncpy(addr.sun_path, ALFRED_SOCK_PATH, sizeof(addr.sun_path));
+	addr.sun_path[sizeof(addr.sun_path) - 1] = '\0';
 
 	if (connect(globals->unix_sock, (struct sockaddr *)&addr,
 		    sizeof(addr)) < 0) {
-- 
2.0.0.rc2

[B.A.T.M.A.N.] [PATCH 08/10] alfred: Free hash iterator when breaking out of loop

[Sven Eckelmann]

The hash iterator is automatically allocated and freed by the hash_iterate
function. But when using break during the iteration loop, the caller has to
handle the free-operation of the hash_iterator to avoid memory leaks.

Signed-off-by: Sven Eckelmann <sven@narfation.org>

---
 hash.c      | 8 +++++++-
 hash.h      | 3 +++
 unix_sock.c | 1 +
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/hash.c b/hash.c
index e1fd299..4b3106e 100644
--- a/hash.c
+++ b/hash.c
@@ -70,6 +70,12 @@ void hash_destroy(struct hashtable_t *hash)
 	debugFree(hash, 1303);
 }
 
+/* free hash_it_t pointer when stopping hash_iterate early */
+void hash_iterate_free(struct hash_it_t *iter_in)
+{
+	debugFree(iter_in, 1304);
+}
+
 /* iterate though the hash. first element is selected with iter_in NULL.
  * use the returned iterator to access the elements until hash_it_t returns
  * NULL. */
@@ -149,7 +155,7 @@ struct hash_it_t *hash_iterate(struct hashtable_t *hash,
 	}
 
 	/* nothing to iterate over anymore */
-	debugFree(iter, 1304);
+	hash_iterate_free(iter);
 	return NULL;
 }
 
diff --git a/hash.h b/hash.h
index bb77f75..c9c8fb1 100644
--- a/hash.h
+++ b/hash.h
@@ -102,4 +102,7 @@ void hash_debug(struct hashtable_t *hash);
 struct hash_it_t *hash_iterate(struct hashtable_t *hash,
 			       struct hash_it_t *iter_in);
 
+/* free hash_it_t pointer when stopping hash_iterate early */
+void hash_iterate_free(struct hash_it_t *iter_in);
+
 #endif
diff --git a/unix_sock.c b/unix_sock.c
index 4553db5..3915553 100644
--- a/unix_sock.c
+++ b/unix_sock.c
@@ -192,6 +192,7 @@ static int unix_sock_req_data_reply(struct globals *globals, int client_sock,
 
 		if (write(client_sock, buf, sizeof(push->header) + len) < 0) {
 			ret = -1;
+			hash_iterate_free(hashit);
 			break;
 		}
 	}
-- 
2.0.0.rc2

[B.A.T.M.A.N.] [PATCH 09/10] alfred: Handle fcntl error return codes

[Sven Eckelmann]

fcntl doesn't return non-error return values when starting a F_GETFL operation.
These have to be handled or otherwise a garbage value is given to fcntl for
the F_SETFL operation.

Signed-off-by: Sven Eckelmann <sven@narfation.org>

---
 netsock.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/netsock.c b/netsock.c
index 8712c11..305983f 100644
--- a/netsock.c
+++ b/netsock.c
@@ -48,6 +48,7 @@ int netsock_open(struct globals *globals)
 	int sock;
 	struct sockaddr_in6 sin6;
 	struct ifreq ifr;
+	int ret;
 
 	globals->netsock = -1;
 
@@ -86,7 +87,18 @@ int netsock_open(struct globals *globals)
 		goto err;
 	}
 
-	fcntl(sock, F_SETFL, fcntl(sock, F_GETFL, 0) | O_NONBLOCK);
+	ret = fcntl(sock, F_GETFL, 0);
+	if (ret < 0) {
+		fprintf(stderr, "failed to get file status flags\n");
+		goto err;
+	}
+
+	ret = fcntl(sock, F_SETFL, ret | O_NONBLOCK);
+	if (ret < 0) {
+		fprintf(stderr, "failed to set file status flags\n");
+		goto err;
+	}
+
 	globals->netsock = sock;
 
 	return 0;
-- 
2.0.0.rc2

[B.A.T.M.A.N.] [PATCH 10/10] alfred: Fix length check for push_data

[Sven Eckelmann]

The client receives the push_data header and the header of a data_block when
it tries to parse the answer of an request. The remaining buffer size to store
the actual data has to remove these two headers from its available, original
buffer size. The read of the data would otherwise (potentially) overflow
the output buffer.

Signed-off-by: Sven Eckelmann <sven@narfation.org>

---
 client.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/client.c b/client.c
index 3670f4f..0283f57 100644
--- a/client.c
+++ b/client.c
@@ -38,6 +38,7 @@ int alfred_client_request_data(struct globals *globals)
 	struct alfred_tlv *tlv;
 	struct alfred_data *data;
 	int ret, len, data_len, i;
+	const size_t buf_data_len = sizeof(buf) - sizeof(*push) + sizeof(*data);
 
 	if (unix_sock_open_client(globals, ALFRED_SOCK_PATH))
 		return -1;
@@ -88,7 +89,7 @@ int alfred_client_request_data(struct globals *globals)
 		data_len = ntohs(data->header.length);
 
 		/* would it fit? it should! */
-		if (data_len > (int)(sizeof(buf) - sizeof(*push)))
+		if (data_len > (int)buf_data_len)
 			break;
 
 		/* read the data */
-- 
2.0.0.rc2

Re: [B.A.T.M.A.N.] [PATCH 01/10] batadv-vis: Avoid memory leak after failed realloc

[Simon Wunderlich]

> realloc doesn't free the original buffer when the reallocation failed. An
> abort of read_file without free'ing the buffer would leak it.
> 
> Signed-off-by: Sven Eckelmann <sven@narfation.org>

The whole series has been applied with two minor fixes:
 * fixed a typo: alfred-gpsd: Fix altitute verification check -> altitude
 * fixed sign of length calculation in alfred: Fix length check for push_data

Thanks!
    Simon