From a95f34136159b551d09a410c853174479989f132 Mon Sep 17 00:00:00 2001 From: Jim Harris Date: Mon, 28 Jan 2019 15:35:07 -0700 Subject: [PATCH] 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: Jim Harris Change-Id: Ic3706639ff6c4f8f344fd58c88ec11e247ea654c Reviewed-on: https://review.gerrithub.io/c/442449 Reviewed-by: Shuhei Matsumoto Reviewed-by: Ben Walker Tested-by: SPDK CI Jenkins --- lib/iscsi/param.c | 25 +++++++++++++++++-------- test/unit/lib/iscsi/param.c/param_ut.c | 12 ++++++++++++ 2 files changed, 29 insertions(+), 8 deletions(-) diff --git a/lib/iscsi/param.c b/lib/iscsi/param.c index b97544641..3a2264928 100644 --- a/lib/iscsi/param.c +++ b/lib/iscsi/param.c @@ -215,11 +215,11 @@ spdk_iscsi_param_set_int(struct iscsi_param *params, const char *key, uint32_t v * data = "KEY=VAL" */ 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; } diff --git a/test/unit/lib/iscsi/param.c/param_ut.c b/test/unit/lib/iscsi/param.c/param_ut.c index e96fe419d..f76b0f653 100644 --- a/test/unit/lib/iscsi/param.c/param_ut.c +++ b/test/unit/lib/iscsi/param.c/param_ut.c @@ -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(¶ms, data, len, false, NULL); + EXPECT_VAL("MaxRecvDataSegmentLength", "8192"); + CU_ASSERT(rc == 0); spdk_iscsi_param_free(params); }