From 7d1db86f54b3a314ae648d58a5b1fd1c095586c4 Mon Sep 17 00:00:00 2001 From: Jim Harris Date: Mon, 28 Jan 2019 16:25:48 -0700 Subject: [PATCH] iscsi: properly handle partial keys This includes properly detecting when a key's name extends past the end of the valid data. Note that the unit tests were using sizeof() instead of strlen() since some of the strings contain NULL characters. This means that we should be subtracting one to account for the implicit null character at the end of the string. Note that the iSCSI spec only says that the key/value pair has to end with a null character - a key/value pair that is split across two PDUs will not have a NULL character at the end of the first PDU. Signed-off-by: Jim Harris Change-Id: Ie95d6dd3b9ffa6a3902a31771ac4edb482418cce Reviewed-on: https://review.gerrithub.io/c/442450 Reviewed-by: Shuhei Matsumoto Reviewed-by: Ben Walker Tested-by: SPDK CI Jenkins --- lib/iscsi/param.c | 28 +++++++++++++++++++++----- test/unit/lib/iscsi/param.c/param_ut.c | 14 ++++++++++++- 2 files changed, 36 insertions(+), 6 deletions(-) diff --git a/lib/iscsi/param.c b/lib/iscsi/param.c index 3a2264928..c6531f2e4 100644 --- a/lib/iscsi/param.c +++ b/lib/iscsi/param.c @@ -223,7 +223,9 @@ spdk_iscsi_parse_param(struct iscsi_param **params, const uint8_t *data, uint32_ int key_len, val_len; int max_len; - key_end = strchr(data, '='); + data_len = strnlen(data, data_len); + /* No such thing as strnchr so use memchr instead. */ + key_end = memchr(data, '=', data_len); if (!key_end) { SPDK_ERRLOG("'=' not found\n"); return -1; @@ -343,13 +345,29 @@ spdk_iscsi_parse_params(struct iscsi_param **params, const uint8_t *data, /* * reverse iterate the string from the tail not including '\0' - * index of last '\0' is len -1. */ - for (i = len - 2; data[i] != '\0' && i > 0; i--) { + for (i = len - 1; data[i] != '\0' && i > 0; i--) { ; } - *partial_parameter = xstrdup(&data[i == 0 ? 0 : i + 1]); - len = (i == 0 ? 0 : i + 1); + if (i != 0) { + /* We found a NULL character - don't copy it into the + * partial parameter. + */ + i++; + } + + *partial_parameter = calloc(1, len - i + 1); + if (*partial_parameter == NULL) { + SPDK_ERRLOG("could not allocate partial parameter\n"); + return -1; + } + memcpy(*partial_parameter, &data[i], len - i); + if (i == 0) { + /* No full parameters to parse - so return now. */ + return 0; + } else { + len = i - 1; + } } while (offset < len && data[offset] != '\0') { diff --git a/test/unit/lib/iscsi/param.c/param_ut.c b/test/unit/lib/iscsi/param.c/param_ut.c index f76b0f653..7d07cf7e8 100644 --- a/test/unit/lib/iscsi/param.c/param_ut.c +++ b/test/unit/lib/iscsi/param.c/param_ut.c @@ -188,7 +188,7 @@ list_negotiation_test(void) #define PARSE(strconst, partial_enabled, partial_text) \ data = strconst; \ - len = sizeof(strconst); \ + len = sizeof(strconst) - 1; \ rc = spdk_iscsi_parse_params(¶ms, data, len, partial_enabled, partial_text) #define EXPECT_VAL(key, expected_value) \ @@ -274,6 +274,18 @@ parse_valid_test(void) EXPECT_VAL("OOOOLL", "MMMM"); CU_ASSERT_PTR_NULL(partial_parameter); + partial_parameter = NULL; + data = "PartialKey="; + len = 7; + rc = spdk_iscsi_parse_params(¶ms, data, len, true, &partial_parameter); + CU_ASSERT(rc == 0); + CU_ASSERT_STRING_EQUAL(partial_parameter, "Partial"); + EXPECT_NULL("PartialKey"); + PARSE("Key=Value", false, &partial_parameter); + CU_ASSERT(rc == 0); + EXPECT_VAL("PartialKey", "Value"); + CU_ASSERT_PTR_NULL(partial_parameter); + spdk_iscsi_param_free(params); }