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; }
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));
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;
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;
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)
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();
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
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) {
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; } }
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;
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 */
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
b.a.t.m.a.n@lists.open-mesh.org
-
Andrew Lunn -
Simon Wunderlich -
Sven Eckelmann