From fda5a009c74433233b882b67e8039916b57b26ab Mon Sep 17 00:00:00 2001 From: Wenchao Hao Date: Wed, 9 Dec 2020 14:43:18 +0800 Subject: idbm: Fix memory leak and NULL pointer dereference in idbm_rec_update_param() Three memory issues in this function: 1. did not verify return value of calloc(), strdup() or malloc(); 2. memory leak of pointer tmp_value caused strsep() tmp_value would change after strsep() and free(tmp_value) would cause memory leak; 3. memory leak of pointer "found", the memory was allocated but did not freed Signed-off-by: Wenchao Hao --- usr/idbm.c | 30 ++++++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-) diff --git a/usr/idbm.c b/usr/idbm.c index f8b50f1..10eb8e2 100644 --- a/usr/idbm.c +++ b/usr/idbm.c @@ -1014,8 +1014,8 @@ int idbm_rec_update_param(recinfo_t *info, char *name, char *value, int i; int passwd_done = 0; char passwd_len[8]; - char *tmp_value, *token; - bool *found; + char *tmp_value, *token, *tmp; + bool *found = NULL; int *tmp_data; setup_passwd_len: @@ -1079,12 +1079,25 @@ setup_passwd_len: if (!info[i].data) continue; tbl = (void *)info[i].opts[0]; - /* strsep is destructive, make a copy to work with */ + /* + * strsep is destructive, make a copy to work with + * tmp_value would be modified in strsep() too, so + * here make a copy of tmp_value to tmp + */ tmp_value = strdup(value); + if (!tmp_value) + return ISCSI_ERR_NOMEM; + tmp = tmp_value; + k = 0; tmp_data = malloc(info[i].data_len); + if (!tmp_data) + goto free_tmp; memset(tmp_data, ~0, info[i].data_len); + found = calloc(info[i].numopts, sizeof(bool)); + if (!found) + goto free_tmp_data; next_token: while ((token = strsep(&tmp_value, ", \n"))) { if (!strlen(token)) @@ -1113,7 +1126,7 @@ next_token: while ((token = strsep(&tmp_value, ", \n"))) { " for '%s'", token, info[i].name); } memcpy(info[i].data, tmp_data, info[i].data_len); - free(tmp_value); + free(tmp); free(tmp_data); tmp_value = NULL; tmp_data = NULL; @@ -1135,8 +1148,17 @@ next_token: while ((token = strsep(&tmp_value, ", \n"))) { return ISCSI_ERR_INVAL; +free_tmp_data: + free(tmp_data); + +free_tmp: + free(tmp); + return ISCSI_ERR_NOMEM; + updated: strlcpy((char*)info[i].value, value, VALUE_MAXVAL); + if (found) + free(found); #define check_password_param(_param) \ if (!passwd_done && !strcmp(#_param, name)) { \ -- cgit v1.2.1 From 66961454632ce954b902b8a41b48c453b656973a Mon Sep 17 00:00:00 2001 From: Wenchao Hao Date: Thu, 10 Dec 2020 17:01:32 +0800 Subject: libopeniscsiusr: Fix memory leak in iscsi_nodes_get() If _scandir() gets 0 node, *node_count is 0, while calloc(*node_count, ...) might return a valid pointer although *node_count is 0. The memory allocated by calloc() would be freed in iscsi_nodes_free() where did not perform free operation if node_count is zero. So memory leak might occur if _scandir() get 0 node. Signed-off-by: Wenchao Hao --- libopeniscsiusr/node.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/libopeniscsiusr/node.c b/libopeniscsiusr/node.c index 6bec201..0bf357b 100644 --- a/libopeniscsiusr/node.c +++ b/libopeniscsiusr/node.c @@ -109,6 +109,15 @@ int iscsi_nodes_get(struct iscsi_context *ctx, struct iscsi_node ***nodes, _good(_scandir(ctx, NODE_CONFIG_DIR, &namelist, &n), rc, out); _debug(ctx, "Got %d target from %s nodes folder", n, NODE_CONFIG_DIR); + /* + * If continue with n == 0, calloc() might return a memory which failed + * to be freed in iscsi_nodes_free() + * + * So here just goto out to exit if n == 0 + */ + if (n == 0) + goto out; + *node_count = n & UINT32_MAX; *nodes = (struct iscsi_node **) calloc(*node_count, sizeof(struct iscsi_node *)); -- cgit v1.2.1 From e73749f34fdbf6862f43939fb958fca0ba4caadb Mon Sep 17 00:00:00 2001 From: Wenchao Hao Date: Wed, 16 Dec 2020 19:48:37 +0800 Subject: libopeniscsiusr: Fix memory leak in iscsi_sessions_get() If _iscsi_sids_get() gets 0 session, *session_count is 0, while calloc(*session_count, ...) might return a valid pointer although *session_count is 0. The memory allocated by calloc() would be freed in iscsi_session_free() where did not perform free operation if session_count is zero. So memory leak would occur if _iscsi_sids_get() gets 0 session, this patch just goto out on if _iscsi_sids_get() gets 0 session to avoid calloc() being called. Signed-off-by: Wenchao Hao --- libopeniscsiusr/session.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/libopeniscsiusr/session.c b/libopeniscsiusr/session.c index 98601dc..7ace4d6 100644 --- a/libopeniscsiusr/session.c +++ b/libopeniscsiusr/session.c @@ -256,6 +256,8 @@ int iscsi_sessions_get(struct iscsi_context *ctx, *session_count = 0; _good(_iscsi_sids_get(ctx, &sids, session_count), rc ,out); + if (!*session_count) + goto out; *sessions = calloc (*session_count, sizeof(struct iscsi_session *)); _alloc_null_check(ctx, *sessions, rc, out); -- cgit v1.2.1 From b24f8ff48e2285e42d151f73e464531c49a9509e Mon Sep 17 00:00:00 2001 From: Wenchao Hao Date: Tue, 29 Dec 2020 20:30:25 +0800 Subject: iscsiadm: Fix memory leak in iscsiadm Memory allocated by iscsi_context_new() would not be freed if error occurred during parameters parser stage and goto free_ifaces is used to jump to resource clean. Since all resource clean is performed after verified, so change all goto free_ifaces to goto out where handles resource better. Signed-off-by: Wenchao Hao --- libopeniscsiusr/context.c | 6 +++++- usr/iscsiadm.c | 27 +++++++++++++-------------- 2 files changed, 18 insertions(+), 15 deletions(-) diff --git a/libopeniscsiusr/context.c b/libopeniscsiusr/context.c index fe92155..c5e869f 100644 --- a/libopeniscsiusr/context.c +++ b/libopeniscsiusr/context.c @@ -55,8 +55,12 @@ struct iscsi_context *iscsi_context_new(void) void iscsi_context_free(struct iscsi_context *ctx) { - if (ctx != NULL) + if (ctx == NULL) + return; + + if (ctx->db) _idbm_free(ctx->db); + free(ctx); } diff --git a/usr/iscsiadm.c b/usr/iscsiadm.c index ea1643b..3987168 100644 --- a/usr/iscsiadm.c +++ b/usr/iscsiadm.c @@ -3627,7 +3627,7 @@ main(int argc, char **argv) "Priority must be greater than or " "equal to zero.", killiscsid); rc = ISCSI_ERR_INVAL; - goto free_ifaces; + goto out; } break; case 't': @@ -3639,7 +3639,7 @@ main(int argc, char **argv) log_error("can not recognize operation: '%s'", optarg); rc = ISCSI_ERR_INVAL; - goto free_ifaces; + goto out; } break; case 'n': @@ -3651,7 +3651,7 @@ main(int argc, char **argv) case 'H': host_no = parse_host_info(optarg, &rc); if (rc) - goto free_ifaces; + goto out; break; case 'r': sid = iscsi_sysfs_get_sid_from_path(optarg); @@ -3659,7 +3659,7 @@ main(int argc, char **argv) log_error("invalid sid '%s'", optarg); rc = ISCSI_ERR_INVAL; - goto free_ifaces; + goto out; } break; case 'R': @@ -3710,7 +3710,7 @@ main(int argc, char **argv) mode = str_to_mode(optarg); rc = verify_mode_params(argc, argv, mode); if (ISCSI_SUCCESS != rc) - goto free_ifaces; + goto out; break; case 'C': sub_mode = str_to_submode(optarg); @@ -3739,11 +3739,11 @@ main(int argc, char **argv) printf("Invalid iface name %s. Must be from " "1 to %d characters.\n", optarg, ISCSI_MAX_IFACE_LEN - 1); - goto free_ifaces; + goto out; } else if (!iface || rc) { printf("Could not add iface %s.", optarg); rc = ISCSI_ERR_INVAL; - goto free_ifaces; + goto out; } list_add_tail(&iface->list, &ifaces); @@ -3760,7 +3760,7 @@ main(int argc, char **argv) log_error("Invalid index %s. %s.", optarg, strerror(errno)); rc = ISCSI_ERR_INVAL; - goto free_ifaces; + goto out; } break; case 'A': @@ -3778,7 +3778,7 @@ main(int argc, char **argv) if (!param) { log_error("Cannot allocate memory for params."); rc = ISCSI_ERR_NOMEM; - goto free_ifaces; + goto out; } list_add_tail(¶m->list, ¶ms); name = NULL; @@ -3789,12 +3789,12 @@ main(int argc, char **argv) if (optopt) { log_error("unrecognized character '%c'", optopt); rc = ISCSI_ERR_INVAL; - goto free_ifaces; + goto out; } if (killiscsid >= 0) { kill_iscsid(killiscsid, timeout); - goto free_ifaces; + goto out; } if (mode < 0) @@ -3802,14 +3802,14 @@ main(int argc, char **argv) if (mode == MODE_FW) { rc = exec_fw_op(NULL, NULL, info_level, do_login, op); - goto free_ifaces; + goto out; } increase_max_files(); if (idbm_init(get_config_file)) { log_warning("exiting due to idbm configuration error"); rc = ISCSI_ERR_IDBM; - goto free_ifaces; + goto out; } switch (mode) { @@ -4070,7 +4070,6 @@ out: free(rec); iscsi_sessions_free(ses, se_count); idbm_terminate(); -free_ifaces: list_for_each_entry_safe(iface, tmp, &ifaces, list) { list_del(&iface->list); free(iface); -- cgit v1.2.1