Commit a95f3413 authored by Jim Harris's avatar Jim Harris
Browse files

iscsi: add data_len parameter to spdk_iscsi_parse_param 



Since params are parsed directly from the PDU's data
buffer, we need to know the end of the valid data.  Otherwise
previous PDUs that used this same data buffer may have left
non-zero characters just after the end of the text associated
with a LOGIN or TEXT PDU.

Found this bug while debugging an intermittent Calsoft test
failure.  Added a unit test to reproduce the original issue,
which now verifies that it is fixed.

Signed-off-by: default avatarJim Harris <james.r.harris@intel.com>
Change-Id: Ic3706639ff6c4f8f344fd58c88ec11e247ea654c
Reviewed-on: https://review.gerrithub.io/c/442449


Reviewed-by: default avatarShuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Reviewed-by: default avatarBen Walker <benjamin.walker@intel.com>
Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
parent 385f42eb

lib/iscsi/param.c

+17 −8
Original line number Diff line number Diff line
@@ -215,11 +215,11 @@ spdk_iscsi_param_set_int(struct iscsi_param *params, const char *key, uint32_t v 
 * data = "KEY=VAL<NUL>"

 */

static int

spdk_iscsi_parse_param(struct iscsi_param **params, const uint8_t *data)

spdk_iscsi_parse_param(struct iscsi_param **params, const uint8_t *data, uint32_t data_len)

{

	int rc;

	uint8_t *key_copy;

	const uint8_t *key_end, *val;

	uint8_t *key_copy, *val_copy;

	const uint8_t *key_end;

	int key_len, val_len;

	int max_len;
@@ -257,8 +257,7 @@ spdk_iscsi_parse_param(struct iscsi_param **params, const uint8_t *data) 
		return -1;

	}



	val = key_end + 1; /* +1 to skip over the '=' */

	val_len = strlen(val);

	val_len = strnlen(key_end + 1, data_len - key_len - 1);

	/*

	 * RFC 3720 5.1

	 * If not otherwise specified, the maximum length of a simple-value
@@ -277,7 +276,17 @@ spdk_iscsi_parse_param(struct iscsi_param **params, const uint8_t *data) 
		return -1;

	}



	rc = spdk_iscsi_param_add(params, key_copy, val, NULL, 0);

	val_copy = calloc(1, val_len + 1);

	if (val_copy == NULL) {

		SPDK_ERRLOG("Could not allocate value string\n");

		free(key_copy);

		return -1;

	}



	memcpy(val_copy, key_end + 1, val_len);



	rc = spdk_iscsi_param_add(params, key_copy, val_copy, NULL, 0);

	free(val_copy);

	free(key_copy);

	if (rc < 0) {

		SPDK_ERRLOG("iscsi_param_add() failed\n");
@@ -313,7 +322,7 @@ spdk_iscsi_parse_params(struct iscsi_param **params, const uint8_t *data, 
		if (!p) {

			return -1;

		}

		rc = spdk_iscsi_parse_param(params, p);

		rc = spdk_iscsi_parse_param(params, p, i + strlen(*partial_parameter));

		free(p);

		if (rc < 0) {

			return -1;
@@ -344,7 +353,7 @@ spdk_iscsi_parse_params(struct iscsi_param **params, const uint8_t *data, 
	}



	while (offset < len && data[offset] != '\0') {

		rc = spdk_iscsi_parse_param(params, data + offset);

		rc = spdk_iscsi_parse_param(params, data + offset, len - offset);

		if (rc < 0) {

			return -1;

		}

test/unit/lib/iscsi/param.c/param_ut.c

+12 −0
Original line number Diff line number Diff line
@@ -349,6 +349,18 @@ parse_invalid_test(void) 
	CU_ASSERT(rc != 0);

	EXPECT_VAL("B", "BB");



	/* Test where data buffer has non-NULL characters past the end of

	 * the valid data region.  This can happen with SPDK iSCSI target,

	 * since data buffers are reused and we do not zero the data buffers

	 * after they are freed since it would be too expensive.  Added as

	 * part of fixing an intermittent Calsoft failure that triggered this

	 * bug.

	 */

	data = "MaxRecvDataSegmentLength=81928";

	len = strlen(data) - 1;

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

	EXPECT_VAL("MaxRecvDataSegmentLength", "8192");

	CU_ASSERT(rc == 0);

	spdk_iscsi_param_free(params);

}