From 4a3ad8371c0cfdd56662d59e0cb471bf5360a4f4 Mon Sep 17 00:00:00 2001 From: Shuhei Matsumoto Date: Fri, 28 Jun 2019 08:49:28 +0900 Subject: [PATCH] iscsi: Use not malloc'ed but fixed size for initiator name Using malloc'ed string for string in iSCSI target has caused scan-build error. Maximum size of initiator name is already defined to be MAX_INITIATOR_NAME, and hence use fixed size string whose size is MAX_INITIATOR_NAME + 1 for spdk_iscsi_initiator_name::name. This will also reduce the potential malloc failure. Signed-off-by: Shuhei Matsumoto Change-Id: Ic6bc172125fc6c9c0896499704d2a9b522106da0 Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/459705 Tested-by: SPDK CI Jenkins Reviewed-by: Ben Walker Reviewed-by: Darek Stojaczyk --- lib/iscsi/init_grp.c | 15 ++---- lib/iscsi/init_grp.h | 3 +- test/unit/lib/iscsi/tgt_node.c/tgt_node_ut.c | 51 +++++++++----------- 3 files changed, 31 insertions(+), 38 deletions(-) diff --git a/lib/iscsi/init_grp.c b/lib/iscsi/init_grp.c index c7321f817..f50a2614e 100644 --- a/lib/iscsi/init_grp.c +++ b/lib/iscsi/init_grp.c @@ -77,13 +77,15 @@ iscsi_init_grp_add_initiator(struct spdk_iscsi_init_grp *ig, char *name) { struct spdk_iscsi_initiator_name *iname; char *p; + size_t len; if (ig->ninitiators >= MAX_INITIATOR) { SPDK_ERRLOG("> MAX_INITIATOR(=%d) is not allowed\n", MAX_INITIATOR); return -EPERM; } - if (strlen(name) > MAX_INITIATOR_NAME) { + len = strlen(name); + if (len > MAX_INITIATOR_NAME) { SPDK_ERRLOG("Initiator Name is larger than 223 bytes\n"); return -EINVAL; } @@ -93,18 +95,13 @@ iscsi_init_grp_add_initiator(struct spdk_iscsi_init_grp *ig, char *name) return -EEXIST; } - iname = malloc(sizeof(*iname)); + iname = calloc(1, sizeof(*iname)); if (iname == NULL) { SPDK_ERRLOG("malloc() failed for initiator name str\n"); return -ENOMEM; } - iname->name = strdup(name); - if (iname->name == NULL) { - SPDK_ERRLOG("strdup() failed for initiator name\n"); - free(iname); - return -ENOMEM; - } + memcpy(iname->name, name, len); /* Replace "ALL" by "ANY" if set */ p = strstr(iname->name, "ALL"); @@ -133,7 +130,6 @@ iscsi_init_grp_delete_initiator(struct spdk_iscsi_init_grp *ig, char *name) TAILQ_REMOVE(&ig->initiator_head, iname, tailq); ig->ninitiators--; - free(iname->name); free(iname); return 0; } @@ -168,7 +164,6 @@ iscsi_init_grp_delete_all_initiators(struct spdk_iscsi_init_grp *ig) TAILQ_FOREACH_SAFE(iname, &ig->initiator_head, tailq, tmp) { TAILQ_REMOVE(&ig->initiator_head, iname, tailq); ig->ninitiators--; - free(iname->name); free(iname); } } diff --git a/lib/iscsi/init_grp.h b/lib/iscsi/init_grp.h index dacb100e7..fbae1ec9c 100644 --- a/lib/iscsi/init_grp.h +++ b/lib/iscsi/init_grp.h @@ -36,9 +36,10 @@ #define SPDK_INIT_GRP_H #include "spdk/conf.h" +#include "iscsi/iscsi.h" struct spdk_iscsi_initiator_name { - char *name; + char name[MAX_INITIATOR_NAME + 1]; TAILQ_ENTRY(spdk_iscsi_initiator_name) tailq; }; diff --git a/test/unit/lib/iscsi/tgt_node.c/tgt_node_ut.c b/test/unit/lib/iscsi/tgt_node.c/tgt_node_ut.c index 50dbe1f31..b6c51c8c7 100644 --- a/test/unit/lib/iscsi/tgt_node.c/tgt_node_ut.c +++ b/test/unit/lib/iscsi/tgt_node.c/tgt_node_ut.c @@ -365,7 +365,7 @@ node_access_allowed(void) ig.tag = 1; ig.ninitiators = 1; - iname.name = "iqn.2017-10.spdk.io:0001"; + snprintf(iname.name, sizeof(iname.name), "iqn.2017-10.spdk.io:0001"); TAILQ_INIT(&ig.initiator_head); TAILQ_INSERT_TAIL(&ig.initiator_head, &iname, tailq); @@ -429,7 +429,7 @@ node_access_denied_by_empty_netmask(void) ig.tag = 1; ig.ninitiators = 1; - iname.name = "iqn.2017-10.spdk.io:0001"; + snprintf(iname.name, sizeof(iname.name), "iqn.2017-10.spdk.io:0001"); TAILQ_INIT(&ig.initiator_head); TAILQ_INSERT_TAIL(&ig.initiator_head, &iname, tailq); @@ -482,7 +482,7 @@ node_access_multi_initiator_groups_cases(void) struct spdk_iscsi_portal_grp pg; struct spdk_iscsi_portal portal; struct spdk_iscsi_init_grp ig1, ig2; - struct spdk_iscsi_initiator_name iname1, iname2; + struct spdk_iscsi_initiator_name iname1 = {}, iname2 = {}; struct spdk_iscsi_initiator_netmask imask1, imask2; struct spdk_scsi_dev scsi_dev; struct spdk_iscsi_pg_map *pg_map; @@ -505,7 +505,6 @@ node_access_multi_initiator_groups_cases(void) TAILQ_INIT(&ig1.netmask_head); ig1.ninitiators = 1; - iname1.name = NULL; TAILQ_INSERT_TAIL(&ig1.initiator_head, &iname1, tailq); ig1.nnetmasks = 1; @@ -518,7 +517,6 @@ node_access_multi_initiator_groups_cases(void) TAILQ_INIT(&ig2.netmask_head); ig2.ninitiators = 1; - iname2.name = NULL; TAILQ_INSERT_TAIL(&ig2.initiator_head, &iname2, tailq); ig2.nnetmasks = 1; @@ -557,7 +555,7 @@ node_access_multi_initiator_groups_cases(void) * | denied | - | - | - | denied | * +-------------------------------------------+---------+ */ - iname1.name = NO_IQN1; + snprintf(iname1.name, sizeof(iname1.name), NO_IQN1); result = spdk_iscsi_tgt_node_access(&conn, &tgtnode, iqn, addr); CU_ASSERT(result == false); @@ -573,7 +571,7 @@ node_access_multi_initiator_groups_cases(void) * | allowed | allowed | - | - | allowed | * +-------------------------------------------+---------+ */ - iname1.name = IQN1; + snprintf(iname1.name, sizeof(iname1.name), IQN1); imask1.mask = IP1; result = spdk_iscsi_tgt_node_access(&conn, &tgtnode, iqn, addr); @@ -590,9 +588,9 @@ node_access_multi_initiator_groups_cases(void) * | allowed | denied | denied | - | denied | * +-------------------------------------------+---------+ */ - iname1.name = IQN1; + snprintf(iname1.name, sizeof(iname1.name), IQN1); imask1.mask = IP2; - iname2.name = NO_IQN1; + snprintf(iname2.name, sizeof(iname2.name), NO_IQN1); result = spdk_iscsi_tgt_node_access(&conn, &tgtnode, iqn, addr); CU_ASSERT(result == false); @@ -608,9 +606,9 @@ node_access_multi_initiator_groups_cases(void) * | allowed | denied | allowed | allowed | allowed | * +-------------------------------------------+---------+ */ - iname1.name = IQN1; + snprintf(iname1.name, sizeof(iname1.name), IQN1); imask1.mask = IP2; - iname2.name = IQN1; + snprintf(iname2.name, sizeof(iname2.name), IQN1); imask2.mask = IP1; result = spdk_iscsi_tgt_node_access(&conn, &tgtnode, iqn, addr); @@ -627,9 +625,9 @@ node_access_multi_initiator_groups_cases(void) * | allowed | denied | allowed | denied | denied | * +---------------------------------------------+---------+ */ - iname1.name = IQN1; + snprintf(iname1.name, sizeof(iname1.name), IQN1); imask1.mask = IP2; - iname2.name = IQN1; + snprintf(iname2.name, sizeof(iname2.name), IQN1); imask2.mask = IP2; result = spdk_iscsi_tgt_node_access(&conn, &tgtnode, iqn, addr); @@ -646,9 +644,9 @@ node_access_multi_initiator_groups_cases(void) * | allowed | denied | not found | - | denied | * +---------------------------------------------+---------+ */ - iname1.name = IQN1; + snprintf(iname1.name, sizeof(iname1.name), IQN1); imask1.mask = IP2; - iname2.name = IQN2; + snprintf(iname2.name, sizeof(iname2.name), IQN2); result = spdk_iscsi_tgt_node_access(&conn, &tgtnode, iqn, addr); CU_ASSERT(result == false); @@ -664,8 +662,8 @@ node_access_multi_initiator_groups_cases(void) * | not found | - | denied | - | denied | * +---------------------------------------------+---------+ */ - iname1.name = IQN2; - iname2.name = NO_IQN1; + snprintf(iname1.name, sizeof(iname1.name), IQN2); + snprintf(iname2.name, sizeof(iname2.name), NO_IQN1); result = spdk_iscsi_tgt_node_access(&conn, &tgtnode, iqn, addr); CU_ASSERT(result == false); @@ -681,8 +679,8 @@ node_access_multi_initiator_groups_cases(void) * | not found | - | allowed | allowed | allowed | * +---------------------------------------------+---------+ */ - iname1.name = IQN2; - iname2.name = IQN1; + snprintf(iname1.name, sizeof(iname1.name), IQN2); + snprintf(iname2.name, sizeof(iname2.name), IQN1); imask2.mask = IP1; result = spdk_iscsi_tgt_node_access(&conn, &tgtnode, iqn, addr); @@ -699,8 +697,8 @@ node_access_multi_initiator_groups_cases(void) * | not found | - | allowed | denied | denied | * +---------------------------------------------+---------+ */ - iname1.name = IQN2; - iname2.name = IQN1; + snprintf(iname1.name, sizeof(iname1.name), IQN2); + snprintf(iname2.name, sizeof(iname2.name), IQN1); imask2.mask = IP2; result = spdk_iscsi_tgt_node_access(&conn, &tgtnode, iqn, addr); @@ -717,8 +715,8 @@ node_access_multi_initiator_groups_cases(void) * | not found | - | not found | - | denied | * +---------------------------------------------+---------+ */ - iname1.name = IQN2; - iname2.name = IQN2; + snprintf(iname1.name, sizeof(iname1.name), IQN2); + snprintf(iname2.name, sizeof(iname2.name), IQN2); result = spdk_iscsi_tgt_node_access(&conn, &tgtnode, iqn, addr); CU_ASSERT(result == false); @@ -734,7 +732,7 @@ allow_iscsi_name_multi_maps_case(void) struct spdk_iscsi_tgt_node tgtnode; struct spdk_iscsi_portal_grp pg1, pg2; struct spdk_iscsi_init_grp ig; - struct spdk_iscsi_initiator_name iname; + struct spdk_iscsi_initiator_name iname = {}; struct spdk_iscsi_pg_map *pg_map1, *pg_map2; struct spdk_scsi_dev scsi_dev; char *iqn; @@ -753,7 +751,6 @@ allow_iscsi_name_multi_maps_case(void) TAILQ_INIT(&ig.initiator_head); ig.ninitiators = 1; - iname.name = NULL; TAILQ_INSERT_TAIL(&ig.initiator_head, &iname, tailq); /* portal group initialization */ @@ -770,12 +767,12 @@ allow_iscsi_name_multi_maps_case(void) /* test for IG1 <-> PG1, PG2 case */ iqn = IQN1; - iname.name = IQN1; + snprintf(iname.name, sizeof(iname.name), IQN1); result = iscsi_tgt_node_allow_iscsi_name(&tgtnode, iqn); CU_ASSERT(result == true); - iname.name = IQN2; + snprintf(iname.name, sizeof(iname.name), IQN2); result = iscsi_tgt_node_allow_iscsi_name(&tgtnode, iqn); CU_ASSERT(result == false);