From: Sven Eckelmann sven@narfation.org
Hi,
today I had the unpleasent experience to receive [1] to a lot of PDF files which seem to contain some reports of the Klocwork static code analysis tool. Most of the stuff was nonsense about memory/resource leaks (even when it stored in some kind of structure one line before). I gathered the only interesting portions and prepared some patches for them.
The last two patches are actually not really bug fixes but are about code which completely killed the static analyzer. I didn't like the code in the last patch (looked to complex for something relatively easy) and thus I've just replaced it.
WARNING: these things were only compile tested
Kind regards, Sven
[1] not unpleasent due to some issue reports. But going through several hundred false positives wasn't my goal for today. Someone at seems to get paid by false-positives which this Klocworks analyzer produces (and there are really some braindead ones - even clang's static analyzer looks like a genius against some of these reports)
Sven Eckelmann (10): batctl: Print dummy value when localtime failed batctl: Handle failure during hash_iterator allocation batctl: Handle allocation error for path_buff batctl: Handle nlmsg_alloc errors batctl: Handle nl_socket_alloc errors batctl: Handle nl_cb_alloc errors batctl: Free nl_sock on genl_ctrl_resolve error batctl: Free nl_sock when if_nametoindex failed batctl: tcpdump: Fix types for for TT v1 batctl: Simplify concatenation of pathnames
bat-hosts.c | 4 +--- functions.c | 8 ++------ hash.c | 3 +++ netlink.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++-------- sys.c | 15 +++++++++++++++ tcpdump.c | 9 +++++++-- 6 files changed, 83 insertions(+), 19 deletions(-)
localtime can return NULL when the local time could not be calculated. Accessing this NULL pointer is not allowed.
Fixes: 05f27bfcd302 ("add arp packets , change output") Signed-off-by: Sven Eckelmann sven.eckelmann@openmesh.com --- tcpdump.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/tcpdump.c b/tcpdump.c index 4ede76b..5eb99cf 100644 --- a/tcpdump.c +++ b/tcpdump.c @@ -104,7 +104,11 @@ static int print_time(void) gettimeofday(&tv, NULL); tm = localtime(&tv.tv_sec);
- printf("%02d:%02d:%02d.%06ld ", tm->tm_hour, tm->tm_min, tm->tm_sec, tv.tv_usec); + if (tm) + printf("%02d:%02d:%02d.%06ld ", tm->tm_hour, tm->tm_min, tm->tm_sec, tv.tv_usec); + else + printf("00:00:00.000000 "); + return 1; }
The iterator functions should not try to start the iteration when the iterator could not be allocated.
Signed-off-by: Sven Eckelmann sven.eckelmann@openmesh.com --- hash.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/hash.c b/hash.c index f4a7af7..0e4d1e3 100644 --- a/hash.c +++ b/hash.c @@ -120,6 +120,9 @@ struct hash_it_t *hash_iterate(struct hashtable_t *hash,
if (iter_in == NULL) { iter = debugMalloc(sizeof(struct hash_it_t), 301); + if (!iter) + return NULL; + iter->index = -1; iter->bucket = NULL; iter->prev_bucket = NULL;
Fixes: 5a1af99276b0 ("batctl: adapt batctl to new sysfs interface handling") Fixes: 306fcb4480c9 ("batctl: support for multiple mesh clouds") Fixes: af115c9acf44 ("batctl: support new gateway sysfs API") Fixes: 2c2cb260ad2e ("batctl: list supported and configured routing algorithms") Signed-off-by: Sven Eckelmann sven.eckelmann@openmesh.com --- sys.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/sys.c b/sys.c index 65b438c..c49b428 100644 --- a/sys.c +++ b/sys.c @@ -153,6 +153,11 @@ int handle_loglevel(char *mesh_iface, int argc, char **argv) }
path_buff = malloc(PATH_BUFF_LEN); + if (!path_buff) { + fprintf(stderr, "Error - could not allocate path buffer: out of memory ?\n"); + return EXIT_FAILURE; + } + snprintf(path_buff, PATH_BUFF_LEN, SYS_BATIF_PATH_FMT, mesh_iface);
if (argc != 1) { @@ -255,6 +260,11 @@ int handle_sys_setting(char *mesh_iface, int setting, int argc, char **argv)
/* prepare the classic path */ path_buff = malloc(PATH_BUFF_LEN); + if (!path_buff) { + fprintf(stderr, "Error - could not allocate path buffer: out of memory ?\n"); + return EXIT_FAILURE; + } + snprintf(path_buff, PATH_BUFF_LEN, SYS_BATIF_PATH_FMT, mesh_iface);
/* if the specified interface is a VLAN then change the path to point @@ -329,6 +339,11 @@ int handle_gw_setting(char *mesh_iface, int argc, char **argv) }
path_buff = malloc(PATH_BUFF_LEN); + if (!path_buff) { + fprintf(stderr, "Error - could not allocate path buffer: out of memory ?\n"); + return EXIT_FAILURE; + } + snprintf(path_buff, PATH_BUFF_LEN, SYS_BATIF_PATH_FMT, mesh_iface);
if (argc == 1) {
nlmsg_alloc may return NULL on errors. The processing has to be aborted when this happens.
Fixes: d8dd1ff1a0fe ("batctl: Use netlink to replace some of debugfs") Signed-off-by: Sven Eckelmann sven.eckelmann@openmesh.com --- netlink.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/netlink.c b/netlink.c index 7fb1ee1..e3d7892 100644 --- a/netlink.c +++ b/netlink.c @@ -302,6 +302,11 @@ static char *netlink_get_info(int ifindex, uint8_t nl_cmd, const char *header) return NULL;
msg = nlmsg_alloc(); + if (!msg) { + nl_socket_free(sock); + return NULL; + } + genlmsg_put(msg, NL_AUTO_PID, NL_AUTO_SEQ, family, 0, 0, BATADV_CMD_GET_MESH_INFO, 1);
@@ -399,6 +404,11 @@ int netlink_print_routing_algos(void) return -EOPNOTSUPP;
msg = nlmsg_alloc(); + if (!msg) { + last_err = -ENOMEM; + goto err_free_sock; + } + genlmsg_put(msg, NL_AUTO_PID, NL_AUTO_SEQ, family, 0, NLM_F_DUMP, BATADV_CMD_GET_ROUTING_ALGOS, 1);
@@ -415,6 +425,8 @@ int netlink_print_routing_algos(void) nl_cb_err(cb, NL_CB_CUSTOM, print_error, NULL);
nl_recvmsgs(sock, cb); + +err_free_sock: nl_socket_free(sock);
if (!last_err) @@ -1131,6 +1143,9 @@ static int netlink_print_common(char *mesh_iface, char *orig_iface, header);
msg = nlmsg_alloc(); + if (!msg) + continue; + genlmsg_put(msg, NL_AUTO_PID, NL_AUTO_SEQ, family, 0, NLM_F_DUMP, nl_cmd, 1);
nl_socket_alloc may return NULL on errors. The processing has to be aborted when this happens.
Fixes: d8dd1ff1a0fe ("batctl: Use netlink to replace some of debugfs") Signed-off-by: Sven Eckelmann sven.eckelmann@openmesh.com --- netlink.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/netlink.c b/netlink.c index e3d7892..d7aeb9a 100644 --- a/netlink.c +++ b/netlink.c @@ -295,6 +295,9 @@ static char *netlink_get_info(int ifindex, uint8_t nl_cmd, const char *header) };
sock = nl_socket_alloc(); + if (!sock) + return NULL; + genl_connect(sock);
family = genl_ctrl_resolve(sock, BATADV_NL_NAME); @@ -397,6 +400,9 @@ int netlink_print_routing_algos(void) };
sock = nl_socket_alloc(); + if (!sock) + return -ENOMEM; + genl_connect(sock);
family = genl_ctrl_resolve(sock, BATADV_NL_NAME); @@ -1104,6 +1110,9 @@ static int netlink_print_common(char *mesh_iface, char *orig_iface, int family;
sock = nl_socket_alloc(); + if (!sock) + return -ENOMEM; + genl_connect(sock);
family = genl_ctrl_resolve(sock, BATADV_NL_NAME);
nl_cb_alloc may return NULL on errors. The processing has to be aborted when this happens.
Fixes: d8dd1ff1a0fe ("batctl: Use netlink to replace some of debugfs") Signed-off-by: Sven Eckelmann sven.eckelmann@openmesh.com --- netlink.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/netlink.c b/netlink.c index d7aeb9a..b95063a 100644 --- a/netlink.c +++ b/netlink.c @@ -320,11 +320,15 @@ static char *netlink_get_info(int ifindex, uint8_t nl_cmd, const char *header) nlmsg_free(msg);
cb = nl_cb_alloc(NL_CB_DEFAULT); + if (!cb) + goto err_free_sock; + nl_cb_set(cb, NL_CB_VALID, NL_CB_CUSTOM, info_callback, &opts); nl_cb_err(cb, NL_CB_CUSTOM, print_error, NULL);
nl_recvmsgs(sock, cb);
+err_free_sock: nl_socket_free(sock);
return opts.remaining_header; @@ -425,6 +429,11 @@ int netlink_print_routing_algos(void) opts.remaining_header = strdup("Available routing algorithms:\n");
cb = nl_cb_alloc(NL_CB_DEFAULT); + if (!cb) { + last_err = -ENOMEM; + goto err_free_sock; + } + nl_cb_set(cb, NL_CB_VALID, NL_CB_CUSTOM, netlink_print_common_cb, &opts); nl_cb_set(cb, NL_CB_FINISH, NL_CB_CUSTOM, stop_callback, NULL); @@ -1134,9 +1143,14 @@ static int netlink_print_common(char *mesh_iface, char *orig_iface, } }
+ cb = nl_cb_alloc(NL_CB_DEFAULT); + if (!cb) { + last_err = -ENOMEM; + goto err_free_sock; + } + bat_hosts_init(read_opt);
- cb = nl_cb_alloc(NL_CB_DEFAULT); nl_cb_set(cb, NL_CB_VALID, NL_CB_CUSTOM, netlink_print_common_cb, &opts); nl_cb_set(cb, NL_CB_FINISH, NL_CB_CUSTOM, stop_callback, NULL); nl_cb_err(cb, NL_CB_CUSTOM, print_error, NULL); @@ -1181,6 +1195,7 @@ static int netlink_print_common(char *mesh_iface, char *orig_iface,
bat_hosts_free();
+err_free_sock: nl_socket_free(sock);
return last_err;
genl_ctrl_resolve may return NULL on errors. The code must then free the socket which was used to start the genl_ctrl_resolve and stop the function with an error code.
Fixes: d8dd1ff1a0fe ("batctl: Use netlink to replace some of debugfs") Signed-off-by: Sven Eckelmann sven.eckelmann@openmesh.com --- netlink.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/netlink.c b/netlink.c index b95063a..de81d08 100644 --- a/netlink.c +++ b/netlink.c @@ -301,8 +301,10 @@ static char *netlink_get_info(int ifindex, uint8_t nl_cmd, const char *header) genl_connect(sock);
family = genl_ctrl_resolve(sock, BATADV_NL_NAME); - if (family < 0) + if (family < 0) { + nl_socket_free(sock); return NULL; + }
msg = nlmsg_alloc(); if (!msg) { @@ -410,8 +412,10 @@ int netlink_print_routing_algos(void) genl_connect(sock);
family = genl_ctrl_resolve(sock, BATADV_NL_NAME); - if (family < 0) - return -EOPNOTSUPP; + if (family < 0) { + last_err = -EOPNOTSUPP; + goto err_free_sock; + }
msg = nlmsg_alloc(); if (!msg) { @@ -1125,8 +1129,10 @@ static int netlink_print_common(char *mesh_iface, char *orig_iface, genl_connect(sock);
family = genl_ctrl_resolve(sock, BATADV_NL_NAME); - if (family < 0) - return -EOPNOTSUPP; + if (family < 0) { + last_err = -EOPNOTSUPP; + goto err_free_sock; + }
ifindex = if_nametoindex(mesh_iface); if (!ifindex) {
The if_nametoindex can return an error. The code must then free the previously allocated nl_sock and stop the function with an error code.
Fixes: d8dd1ff1a0fe ("batctl: Use netlink to replace some of debugfs") Signed-off-by: Sven Eckelmann sven.eckelmann@openmesh.com --- netlink.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/netlink.c b/netlink.c index de81d08..7b97809 100644 --- a/netlink.c +++ b/netlink.c @@ -1137,7 +1137,8 @@ static int netlink_print_common(char *mesh_iface, char *orig_iface, ifindex = if_nametoindex(mesh_iface); if (!ifindex) { fprintf(stderr, "Interface %s is unknown\n", mesh_iface); - return -ENODEV; + last_err = -ENODEV; + goto err_free_sock; }
if (orig_iface) { @@ -1145,7 +1146,8 @@ static int netlink_print_common(char *mesh_iface, char *orig_iface, if (!hardifindex) { fprintf(stderr, "Interface %s is unknown\n", orig_iface); - return -ENODEV; + last_err = -ENODEV; + goto err_free_sock; } }
The num_entry and num_vlan variables are accessed (printed) as u16 variables and not like integers. They should therefore also be stored like that.
Signed-off-by: Sven Eckelmann sven.eckelmann@openmesh.com --- tcpdump.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tcpdump.c b/tcpdump.c index 5eb99cf..a1b057b 100644 --- a/tcpdump.c +++ b/tcpdump.c @@ -158,7 +158,8 @@ static void batctl_tvlv_parse_tt_v1(void *buff, ssize_t buff_len) { struct batadv_tvlv_tt_data *tvlv = buff; struct batadv_tvlv_tt_vlan_data *vlan; - int i, num_vlan, num_entry; + int i; + unsigned short num_vlan, num_entry; const char *type; size_t vlan_len;
The combination of strncpy and strncat is hard to read and it is rather easy to introduce some kind of problems when using that. The usage of snprintf simplifies it slightly.
Signed-off-by: Sven Eckelmann sven.eckelmann@openmesh.com --- bat-hosts.c | 4 +--- functions.c | 8 ++------ 2 files changed, 3 insertions(+), 9 deletions(-)
diff --git a/bat-hosts.c b/bat-hosts.c index b530243..54b0a18 100644 --- a/bat-hosts.c +++ b/bat-hosts.c @@ -194,9 +194,7 @@ void bat_hosts_init(int read_opt) if (!homedir) continue;
- strncpy(confdir, homedir, CONF_DIR_LEN); - confdir[CONF_DIR_LEN - 1] = '\0'; - strncat(confdir, &bat_hosts_path[i][1], CONF_DIR_LEN - strlen(confdir) - 1); + snprintf(confdir, CONF_DIR_LEN, "%s%s", homedir, &bat_hosts_path[i][1]); } else { strncpy(confdir, bat_hosts_path[i], CONF_DIR_LEN); confdir[CONF_DIR_LEN - 1] = '\0'; diff --git a/functions.c b/functions.c index 868e0ae..8bcf52d 100644 --- a/functions.c +++ b/functions.c @@ -208,9 +208,7 @@ int read_file(const char *dir, const char *fname, int read_opt, if (read_opt & USE_BAT_HOSTS) bat_hosts_init(read_opt);
- strncpy(full_path, dir, sizeof(full_path)); - full_path[sizeof(full_path) - 1] = '\0'; - strncat(full_path, fname, sizeof(full_path) - strlen(full_path) - 1); + snprintf(full_path, sizeof(full_path), "%s%s", dir, fname);
open: line = 0; @@ -349,9 +347,7 @@ int write_file(const char *dir, const char *fname, const char *arg1, char full_path[500]; ssize_t write_len;
- strncpy(full_path, dir, sizeof(full_path)); - full_path[sizeof(full_path) - 1] = '\0'; - strncat(full_path, fname, sizeof(full_path) - strlen(full_path) - 1); + snprintf(full_path, sizeof(full_path), "%s%s", dir, fname);
fd = open(full_path, O_WRONLY);
On Thursday, November 23, 2017 3:04:34 PM CET Sven Eckelmann wrote:
From: Sven Eckelmann sven@narfation.org
Hi,
today I had the unpleasent experience to receive [1] to a lot of PDF files which seem to contain some reports of the Klocwork static code analysis tool. Most of the stuff was nonsense about memory/resource leaks (even when it stored in some kind of structure one line before). I gathered the only interesting portions and prepared some patches for them.
The last two patches are actually not really bug fixes but are about code which completely killed the static analyzer. I didn't like the code in the last patch (looked to complex for something relatively easy) and thus I've just replaced it.
WARNING: these things were only compile tested
Kind regards, Sven
I'ved picked up the series into 6116097..cfaec23
Thank you! Simon
b.a.t.m.a.n@lists.open-mesh.org
-
Simon Wunderlich -
Sven Eckelmann