iscsi: fixed crash on invalid add_portal_group call

Fixed double free in spdk_rpc_add_portal_group()
spdk_iscsi_portal_create() now takes string arguments as const char* and makes internal copies of them.
This patch also fixes potential memory leak when id == NULL

Change-Id: I4d0efb101471fb2368ceb8ceecb0e40614e3585d
Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>

lib/iscsi/iscsi_rpc.c

@@ -742,12 +742,13 @@ spdk_rpc_add_portal_group(struct spdk_jsonrpc_server_conn *conn,
 	struct spdk_iscsi_portal *portal_list[MAX_PORTAL] = {};
 	struct spdk_json_write_ctx *w;
 	size_t i;
+	int rc = -1;
 
 	if (spdk_json_decode_object(params, rpc_portal_group_decoders,
 				    SPDK_COUNTOF(rpc_portal_group_decoders),
 				    &req)) {
 		SPDK_ERRLOG("spdk_json_decode_object failed\n");
-		goto invalid;
+		goto out;
 	}
 
 	for (i = 0; i < req.portal_list.num_portals; i++) {
@@ -755,34 +756,32 @@ spdk_rpc_add_portal_group(struct spdk_jsonrpc_server_conn *conn,
 				 req.portal_list.portals[i].port, 0);
 		if (portal_list[i] == NULL) {
 			SPDK_ERRLOG("portal_list allocation failed\n");
-			goto invalid;
+			goto out;
 		}
 	}
 
-	if (spdk_iscsi_portal_grp_create_from_portal_list(req.tag, portal_list,
+	rc = spdk_iscsi_portal_grp_create_from_portal_list(req.tag, portal_list,
-			req.portal_list.num_portals)) {
+			req.portal_list.num_portals);
+
+	if (rc < 0) {
 		SPDK_ERRLOG("create_from_portal_list failed\n");
-		goto invalid;
 	}
 
-	/*
+out:
-	 * Leave the host and port strings allocated here and don't call free_rpc_portal_group(),
+	if (rc > 0) {
-	 *  because the pointers are inserted directly into the portal group list.
+		if (id != NULL) {
-	 */
+			w = spdk_jsonrpc_begin_result(conn, id);
-
+			spdk_json_write_bool(w, true);
-	if (id == NULL) {
+			spdk_jsonrpc_end_result(conn, w);
-		return;
+		}
+	} else {
+		spdk_jsonrpc_send_error_response(conn, id, SPDK_JSONRPC_ERROR_INVALID_PARAMS, "Invalid parameters");
 	}
 
-	w = spdk_jsonrpc_begin_result(conn, id);
-	spdk_json_write_bool(w, true);
-	spdk_jsonrpc_end_result(conn, w);
-	return;
-
-invalid:
-	spdk_jsonrpc_send_error_response(conn, id, SPDK_JSONRPC_ERROR_INVALID_PARAMS, "Invalid parameters");
 	for (i = 0; i < req.portal_list.num_portals; i++) {
-		spdk_iscsi_portal_destroy(portal_list[i]);
+		if (portal_list[i] != NULL) {
+			spdk_iscsi_portal_destroy(portal_list[i]);
+		}
 	}
 	free_rpc_portal_group(&req);
 }

lib/iscsi/portal_grp.c

@@ -65,7 +65,7 @@ spdk_iscsi_portal_grp_open(struct spdk_iscsi_portal_grp *pg);
 
 /* Assumes caller allocated host and port strings on the heap */
 struct spdk_iscsi_portal *
-spdk_iscsi_portal_create(char *host, char *port, uint64_t cpumask)
+spdk_iscsi_portal_create(const char *host, const char *port, uint64_t cpumask)
 {
 	struct spdk_iscsi_portal *p = NULL;
 
@@ -77,8 +77,8 @@ spdk_iscsi_portal_create(char *host, char *port, uint64_t cpumask)
 		SPDK_ERRLOG("portal malloc error (%s, %s)\n", host, port);
 		return NULL;
 	}
-	p->host = host;
+	p->host = strdup(host);
-	p->port = port;
+	p->port = strdup(port);
 	p->cpumask = cpumask;
 	p->sock = -1;
 	p->group = NULL; /* set at a later time by caller */
@@ -106,7 +106,7 @@ spdk_iscsi_portal_create_from_configline(const char *portalstring,
 	const char *cpumask_str;
 	uint64_t cpumask = 0;
 
-	int n, len;
+	int n, len, rc = -1;
 	const char *p, *q;
 
 	if (portalstring == NULL) {
@@ -240,14 +240,12 @@ spdk_iscsi_portal_create_from_configline(const char *portalstring,
 		}
 	}
 
-	return 0;
+	rc = 0;
-
 error_out:
-	if (host != NULL)
+	free(host);
-		free(host);
+	free(port);
-	if (port != NULL)
+
-		free(port);
+	return rc;
-	return -1;
 }
 
 struct spdk_iscsi_portal_grp *
@@ -316,13 +314,13 @@ spdk_iscsi_portal_grp_create_from_portal_list(int tag,
 
 	if (num_portals > MAX_PORTAL) {
 		SPDK_ERRLOG("%d > MAX_PORTAL\n", num_portals);
-		goto error_out;
+		return -1;
 	}
 
 	pg = spdk_iscsi_portal_grp_create(tag);
 	if (!pg) {
 		SPDK_ERRLOG("portal group creation error (%d)\n", tag);
-		goto error_out;
+		return -1;
 	}
 
 	for (i = 0; i < num_portals; i++) {
@@ -336,28 +334,24 @@ spdk_iscsi_portal_grp_create_from_portal_list(int tag,
 		sock = spdk_sock_listen(p->host, port);
 		if (sock < 0) {
 			SPDK_ERRLOG("listen error %.64s:%d\n", p->host, port);
-			spdk_iscsi_portal_destroy(p);
 			count++;
 			continue;
 		}
 		p->sock = sock;
 		spdk_iscsi_portal_grp_add_portal(pg, p);
+		portal_list[i] = NULL;
 	}
 
-	/* if listening is failed on all the ports,
-	 * then do not register the portal group. */
 	if (count == num_portals) {
+		/* if listening is failed on all the ports,
+		 * then do not register the portal group. */
 		spdk_iscsi_portal_grp_destroy(pg);
-		goto error_out;
+		return -1;
+	} else {
+		/* Add portal group to the end of the pg list */
+		spdk_iscsi_portal_grp_register(pg);
+		return num_portals - count;
 	}
-
-	/* Add portal group to the end of the pg list */
-	spdk_iscsi_portal_grp_register(pg);
-
-	return 0;
-
-error_out:
-	return -1;
 }
 
 int

lib/iscsi/portal_grp.h

@@ -56,7 +56,7 @@ struct spdk_iscsi_portal_grp {
 
 /* SPDK iSCSI Portal Group management API */
 
-struct spdk_iscsi_portal *spdk_iscsi_portal_create(char *host, char *port,
+struct spdk_iscsi_portal *spdk_iscsi_portal_create(const char *host, const char *port,
 		uint64_t cpumask);
 void spdk_iscsi_portal_destroy(struct spdk_iscsi_portal *p);