Commit 5b89e4a1 authored by Ziye Yang's avatar Ziye Yang Committed by Ben Walker
Browse files

iscsi, param: fix memory leak related issue 



issue: ASAN reported that param_ut free already
freed memory.

Reason: spdk_iscsi_negotiate_params does some
modifcation on params and may free old params
and create new params. To solve this issue,
we need to input **params but not *params in
spdk_iscsi_negotiate_params

Change-Id: I68658fd8e08f317343753620692f04e7b0b57577
Signed-off-by: default avatarZiye Yang <optimistyzy@gmail.com>
Reviewed-on: https://review.gerrithub.io/363670


Reviewed-by: default avatarDaniel Verkamp <daniel.verkamp@intel.com>
Tested-by: default avatarSPDK Automated Test System <sys_sgsw@intel.com>
Reviewed-by: default avatarBen Walker <benjamin.walker@intel.com>
parent b6a9493b

lib/iscsi/iscsi.c

+17 −15
Original line number Diff line number Diff line
@@ -2120,7 +2120,7 @@ spdk_iscsi_op_login_rsp_handle_t_bit(struct spdk_iscsi_conn *conn, 
 */

static int

spdk_iscsi_op_login_rsp_handle(struct spdk_iscsi_conn *conn,

			       struct spdk_iscsi_pdu *rsp_pdu, struct iscsi_param *params,

			       struct spdk_iscsi_pdu *rsp_pdu, struct iscsi_param **params,

			       int alloc_len)

{

	int rc = 0;
@@ -2145,7 +2145,7 @@ spdk_iscsi_op_login_rsp_handle(struct spdk_iscsi_conn *conn, 
	SPDK_TRACEDUMP(SPDK_TRACE_DEBUG, "Negotiated Params", rsp_pdu->data, rc);



	/* handle the CSG bit case */

	rc = spdk_iscsi_op_login_rsp_handle_csg_bit(conn, rsp_pdu, params,

	rc = spdk_iscsi_op_login_rsp_handle_csg_bit(conn, rsp_pdu, *params,

			alloc_len);

	if (rc < 0)

		return rc;
@@ -2163,6 +2163,7 @@ spdk_iscsi_op_login(struct spdk_iscsi_conn *conn, struct spdk_iscsi_pdu *pdu) 
	int rc;

	struct spdk_iscsi_pdu *rsp_pdu;

	struct iscsi_param *params = NULL;

	struct iscsi_param **params_p = &params;

	int alloc_len;

	int cid;
@@ -2174,10 +2175,10 @@ spdk_iscsi_op_login(struct spdk_iscsi_conn *conn, struct spdk_iscsi_pdu *pdu) 




	rsp_pdu = spdk_get_pdu();

	rc = spdk_iscsi_op_login_rsp_init(conn, pdu, rsp_pdu, &params,

	rc = spdk_iscsi_op_login_rsp_init(conn, pdu, rsp_pdu, params_p,

					  &alloc_len, &cid);

	if (rc == SPDK_ISCSI_LOGIN_ERROR_RESPONSE || rc == SPDK_ISCSI_LOGIN_ERROR_PARAMETER) {

		spdk_iscsi_op_login_response(conn, rsp_pdu, params);

		spdk_iscsi_op_login_response(conn, rsp_pdu, *params_p);

		return rc;

	}
@@ -2188,21 +2189,21 @@ spdk_iscsi_op_login(struct spdk_iscsi_conn *conn, struct spdk_iscsi_pdu *pdu) 
	}



	if (conn->state == ISCSI_CONN_STATE_INVALID) {

		rc = spdk_iscsi_op_login_phase_none(conn, rsp_pdu, params,

		rc = spdk_iscsi_op_login_phase_none(conn, rsp_pdu, *params_p,

						    alloc_len, cid);

		if (rc == SPDK_ISCSI_LOGIN_ERROR_RESPONSE || rc == SPDK_ISCSI_LOGIN_ERROR_PARAMETER) {

			spdk_iscsi_op_login_response(conn, rsp_pdu, params);

			spdk_iscsi_op_login_response(conn, rsp_pdu, *params_p);

			return rc;

		}

	}



	rc = spdk_iscsi_op_login_rsp_handle(conn, rsp_pdu, params, alloc_len);

	rc = spdk_iscsi_op_login_rsp_handle(conn, rsp_pdu, params_p, alloc_len);

	if (rc == SPDK_ISCSI_LOGIN_ERROR_RESPONSE) {

		spdk_iscsi_op_login_response(conn, rsp_pdu, params);

		spdk_iscsi_op_login_response(conn, rsp_pdu, *params_p);

		return rc;

	}



	rc = spdk_iscsi_op_login_response(conn, rsp_pdu, params);

	rc = spdk_iscsi_op_login_response(conn, rsp_pdu, *params_p);



	if (rc == 0) {

		conn->state = ISCSI_CONN_STATE_RUNNING;
@@ -2218,6 +2219,7 @@ static int 
spdk_iscsi_op_text(struct spdk_iscsi_conn *conn, struct spdk_iscsi_pdu *pdu)

{

	struct iscsi_param *params = NULL;

	struct iscsi_param **params_p = &params;

	struct spdk_iscsi_pdu *rsp_pdu;

	uint8_t *data;

	uint64_t lun;
@@ -2302,17 +2304,17 @@ spdk_iscsi_op_text(struct spdk_iscsi_conn *conn, struct spdk_iscsi_pdu *pdu) 
	memset(data, 0, alloc_len);



	/* negotiate parameters */

	data_len = spdk_iscsi_negotiate_params(conn, params,

	data_len = spdk_iscsi_negotiate_params(conn, params_p,

					       data, alloc_len, data_len);

	if (data_len < 0) {

		SPDK_ERRLOG("spdk_iscsi_negotiate_params() failed\n");

		spdk_iscsi_param_free(params);

		spdk_iscsi_param_free(*params_p);

		free(data);

		return -1;

	}



	/* sendtargets is special case */

	val = spdk_iscsi_param_get_val(params, "SendTargets");

	val = spdk_iscsi_param_get_val(*params_p, "SendTargets");

	if (val != NULL) {

		if (spdk_iscsi_param_eq_val(conn->sess->params,

					    "SessionType", "Discovery")) {
@@ -2393,7 +2395,7 @@ spdk_iscsi_op_text(struct spdk_iscsi_conn *conn, struct spdk_iscsi_pdu *pdu) 
	rc = spdk_iscsi_copy_param2var(conn);

	if (rc < 0) {

		SPDK_ERRLOG("spdk_iscsi_copy_param2var() failed\n");

		spdk_iscsi_param_free(params);

		spdk_iscsi_param_free(*params_p);

		return -1;

	}
@@ -2401,11 +2403,11 @@ spdk_iscsi_op_text(struct spdk_iscsi_conn *conn, struct spdk_iscsi_pdu *pdu) 
	rc = spdk_iscsi_check_values(conn);

	if (rc < 0) {

		SPDK_ERRLOG("iscsi_check_values() failed\n");

		spdk_iscsi_param_free(params);

		spdk_iscsi_param_free(*params_p);

		return -1;

	}



	spdk_iscsi_param_free(params);

	spdk_iscsi_param_free(*params_p);

	return 0;

}

lib/iscsi/iscsi.h

+1 −1
Original line number Diff line number Diff line
@@ -349,7 +349,7 @@ bool spdk_iscsi_is_deferred_free_pdu(struct spdk_iscsi_pdu *pdu); 


void spdk_iscsi_shutdown(void);

int spdk_iscsi_negotiate_params(struct spdk_iscsi_conn *conn,

				struct iscsi_param *params, uint8_t *data,

				struct iscsi_param **params_p, uint8_t *data,

				int alloc_len, int data_len);

int spdk_iscsi_copy_param2var(struct spdk_iscsi_conn *conn);

lib/iscsi/param.c

+7 −7
Original line number Diff line number Diff line
@@ -851,7 +851,7 @@ spdk_iscsi_negotiate_param_init(struct spdk_iscsi_conn *conn, 


int

spdk_iscsi_negotiate_params(struct spdk_iscsi_conn *conn,

			    struct iscsi_param *params, uint8_t *data, int alloc_len,

			    struct iscsi_param **params, uint8_t *data, int alloc_len,

			    int data_len)

{

	struct iscsi_param *param;
@@ -876,14 +876,14 @@ spdk_iscsi_negotiate_params(struct spdk_iscsi_conn *conn, 
		return total;

	}



	if (params == NULL) {

	if (*params == NULL) {

		/* no input */

		return total;

	}



	/* discovery? */

	discovery = 0;

	cur_param = spdk_iscsi_param_find(params, "SessionType");

	cur_param = spdk_iscsi_param_find(*params, "SessionType");

	if (cur_param == NULL) {

		cur_param = spdk_iscsi_param_find(conn->sess->params, "SessionType");

		if (cur_param == NULL) {
@@ -924,9 +924,9 @@ spdk_iscsi_negotiate_params(struct spdk_iscsi_conn *conn, 
	/* To adjust the location of FirstBurstLength location and put it to

	 *  the end, then we can always firstly determine the MaxBurstLength

	 */

	param = spdk_iscsi_param_find(params, "MaxBurstLength");

	param = spdk_iscsi_param_find(*params, "MaxBurstLength");

	if (param != NULL) {

		param = spdk_iscsi_param_find(params, "FirstBurstLength");

		param = spdk_iscsi_param_find(*params, "FirstBurstLength");



		/* check the existence of FirstBurstLength */

		if (param != NULL) {
@@ -934,13 +934,13 @@ spdk_iscsi_negotiate_params(struct spdk_iscsi_conn *conn, 
			if (param->next != NULL) {

				snprintf(in_val, ISCSI_TEXT_MAX_VAL_LEN + 1, "%s", param->val);

				type = param->type;

				spdk_iscsi_param_add(&params, "FirstBurstLength",

				spdk_iscsi_param_add(params, "FirstBurstLength",

						     in_val, NULL, type);

			}

		}

	}



	for (param = params; param != NULL; param = param->next) {

	for (param = *params; param != NULL; param = param->next) {

		struct iscsi_param *params_dst = conn->params;

		int add_param_value = 0;

		new_val = NULL;

test/lib/iscsi/param/param_ut.c

+5 −3
Original line number Diff line number Diff line
@@ -71,12 +71,14 @@ burst_length_param_negotation(int FirstBurstLength, int MaxBurstLength, 
	struct spdk_iscsi_sess sess;

	struct spdk_iscsi_conn conn;

	struct iscsi_param *params;

	struct iscsi_param **params_p;

	char data[8192];

	int rc;

	int total, len;



	total = 0;

	params = NULL;

	params_p = &params;



	memset(&sess, 0, sizeof(sess));

	memset(&conn, 0, sizeof(conn));
@@ -131,11 +133,11 @@ burst_length_param_negotation(int FirstBurstLength, int MaxBurstLength, 
	total++;



	/* store incoming parameters */

	rc = spdk_iscsi_parse_params(&params, data, total, false, NULL);

	rc = spdk_iscsi_parse_params(params_p, data, total, false, NULL);

	CU_ASSERT(rc == 0);



	/* negotiate parameters */

	rc = spdk_iscsi_negotiate_params(&conn, params,

	rc = spdk_iscsi_negotiate_params(&conn, params_p,

					 data, 8192, rc);

	CU_ASSERT(rc > 0);
@@ -148,7 +150,7 @@ burst_length_param_negotation(int FirstBurstLength, int MaxBurstLength, 


	spdk_iscsi_param_free(sess.params);

	spdk_iscsi_param_free(conn.params);

	spdk_iscsi_param_free(params);

	spdk_iscsi_param_free(*params_p);

}



static void

unittest.sh

+1 −1
Original line number Diff line number Diff line
@@ -69,7 +69,7 @@ $valgrind test/lib/scsi/scsi_bdev/scsi_bdev_ut 
$valgrind test/lib/scsi/scsi_nvme/scsi_nvme_ut



# TODO: fix valgrind warnings and add $valgrind to iSCSI tests

test/lib/iscsi/param/param_ut

$valgrind test/lib/iscsi/param/param_ut

$valgrind test/lib/iscsi/target_node/target_node_ut test/lib/iscsi/target_node/target_node.conf

test/lib/iscsi/pdu/pdu