libnvme, nvme: guarantee terminated discovery log entry strings

libnvme/src/nvme/fabrics.c

@@ -77,20 +77,6 @@ static inline void cleanup_ctrl_params(struct libnvme_ctrl_params *params)
 }
 #define __cleanup_ctrl_params __cleanup(cleanup_ctrl_params)
 
-/**
- * strchomp() - Strip trailing spaces
- * @str: String to strip
- * @max: Maximum length of string
- */
-static void strchomp(char *str, int max)
-{
-	int i;
-
-	for (i = max - 1; i >= 0 && str[i] == ' '; i--) {
-		str[i] = '\0';
-	}
-}
-
 const char *arg_str(const char * const *strings,
 		size_t array_size, size_t idx)
 {
@@ -676,7 +662,7 @@ __shr_public void libnvmf_context_free(struct libnvmf_context *fctx)
 __shr_public int libnvmf_context_set_discovery_hooks(
 		struct libnvmf_context *fctx,
 		void (*discovery_log)(struct libnvmf_context *fctx,
-			struct nvmf_discovery_log *log,
+			const struct nvmf_discovery_log *log,
 			uint64_t numrec, void *user_data))
 {
 	fctx->hooks.discovery_log = discovery_log;
@@ -2043,8 +2029,21 @@ static int nvme_discovery_log(libnvme_ctrl_t ctrl,
 static void sanitize_discovery_log_entry(struct libnvme_global_ctx *ctx,
 		struct nvmf_disc_log_entry *e)
 {
-	strchomp(e->trsvcid, sizeof(e->trsvcid));
-	strchomp(e->traddr, sizeof(e->traddr));
+	/*
+	 * Force a NUL terminator into the last byte. Every buffer here is
+	 * far larger than any value a compliant peer can send, so this only
+	 * ever truncates a non-compliant one. The purpose is to keep the
+	 * field from being read as an unbounded char *. This must run
+	 * before shr_rtrim() below, which relies on the field already being
+	 * terminated.
+	 */
+	e->trsvcid[sizeof(e->trsvcid) - 1] = '\0';
+	e->subnqn[sizeof(e->subnqn) - 1] = '\0';
+	e->traddr[sizeof(e->traddr) - 1] = '\0';
+
+	shr_rtrim(e->trsvcid);
+	shr_rtrim(e->traddr);
+	shr_rtrim(e->subnqn);
 
 	/*
 	 * Report traddr always in 'nn-0x:pn-0x' format, but some discovery logs
@@ -2999,6 +2998,12 @@ static void dc_walk_referral(struct libnvme_global_ctx *ctx,
  * this DC's own self entry (SUBTYPE 03h) -- the only place this DC's own
  * EPCSD is ever reported. Returns whether c should be disconnected once
  * its own Discovery Log Page has been fully walked.
+ *
+ * Sanitizing here, not right after the fetch, is deliberate: fctx->hooks
+ * .discovery_log fires before this runs, and it must see the log page
+ * exactly as the DC returned it (e.g. for --raw). Only Pass 2's connect
+ * logic, which runs after this pass completes, needs the guaranteed-
+ * terminated strings this pass produces.
  */
 static bool dc_survey_self_entry(struct libnvmf_context *fctx,
 		struct libnvme_ctrl *c, enum dc_ownership own, bool primary,

libnvme/src/nvme/fabrics.h

@@ -386,7 +386,7 @@ void libnvmf_context_free(struct libnvmf_context *fctx);
  */
 int libnvmf_context_set_discovery_hooks(struct libnvmf_context *fctx,
 		void (*discovery_log)(struct libnvmf_context *fctx,
-			struct nvmf_discovery_log *log,
+			const struct nvmf_discovery_log *log,
 			uint64_t numrec, void *user_data));
 
 /**

libnvme/src/nvme/private-fabrics.h

@@ -51,7 +51,7 @@ struct libnvmf_hooks {
 
 	/* discovery hooks */
 	void (*discovery_log)(struct libnvmf_context *fctx,
-			struct nvmf_discovery_log *log,
+			const struct nvmf_discovery_log *log,
 			uint64_t numrec, void *user_data);
 
 	/*

libnvme/tests/ioctl/discovery.c

@@ -23,7 +23,8 @@ static void arbitrary_ascii_string(size_t max_len, char *str, char *log_str)
 	size_t len;
 	size_t i;
 
-	len = arbitrary_range(max_len + 1);
+	/* Cap below max_len so at least one padding/terminator byte remains. */
+	len = arbitrary_range(max_len);
 	for (i = 0; i < len; i++) {
 		/*
 		 * ASCII strings shall contain only code values 20h through 7Eh.
@@ -54,6 +55,26 @@ static int fetch_discovery_log(libnvme_ctrl_t c,
 	return err;
 }
 
+/*
+ * Unlike trsvcid/traddr, subnqn is a null-terminated string (NVMe Base
+ * Spec 2.4, section 4.7): unused trailing bytes are padded with '\0' on
+ * the wire, not ' '. Generate it accordingly so it always carries a
+ * terminator, matching what a compliant discovery controller sends.
+ */
+static void arbitrary_nul_padded_ascii_string(size_t max_len, char *str,
+					       char *log_str)
+{
+	size_t len;
+	size_t i;
+
+	/* Cap below max_len so at least one terminator byte remains. */
+	len = arbitrary_range(max_len);
+	for (i = 0; i < len; i++)
+		str[i] = log_str[i] = arbitrary_range(0x7E - 0x20) + 0x20 + 1;
+	for (i = len; i < max_len; i++)
+		str[i] = log_str[i] = '\0';
+}
+
 static void arbitrary_entry(struct nvmf_disc_log_entry *entry,
                             struct nvmf_disc_log_entry *log_entry)
 {
@@ -63,6 +84,8 @@ static void arbitrary_entry(struct nvmf_disc_log_entry *entry,
 		sizeof(entry->trsvcid), entry->trsvcid, log_entry->trsvcid);
 	arbitrary_ascii_string(
 		sizeof(entry->traddr), entry->traddr, log_entry->traddr);
+	arbitrary_nul_padded_ascii_string(
+		sizeof(entry->subnqn), entry->subnqn, log_entry->subnqn);
 }
 
 static void arbitrary_entries(size_t len,
@@ -75,6 +98,38 @@ static void arbitrary_entries(size_t len,
 		arbitrary_entry(&entries[i], &log_entries[i]);
 }
 
+/*
+ * sanitize_discovery_log_entry() only guarantees trsvcid/traddr end up as
+ * valid, correctly terminated strings -- shr_rtrim() does not clear the
+ * buffer bytes after the new terminator the way the old strchomp() did.
+ * Compare those two fields as strings. subnqn is also rtrim'd, but the
+ * generator above never puts trailing whitespace in it, so it still
+ * matches byte-for-byte along with the rest of the entry.
+ */
+static void cmp_entries(const struct nvmf_disc_log_entry *actual,
+			 const struct nvmf_disc_log_entry *expected,
+			 size_t count, const char *msg)
+{
+	size_t i;
+
+	for (i = 0; i < count; i++) {
+		struct nvmf_disc_log_entry a = actual[i];
+		struct nvmf_disc_log_entry e = expected[i];
+
+		check(!strcmp(a.trsvcid, e.trsvcid),
+		      "%s: trsvcid mismatch", msg);
+		check(!strcmp(a.traddr, e.traddr),
+		      "%s: traddr mismatch", msg);
+
+		memset(a.trsvcid, 0, sizeof(a.trsvcid));
+		memset(e.trsvcid, 0, sizeof(e.trsvcid));
+		memset(a.traddr, 0, sizeof(a.traddr));
+		memset(e.traddr, 0, sizeof(e.traddr));
+
+		cmp(&a, &e, sizeof(a), msg);
+	}
+}
+
 static void test_no_entries(libnvme_ctrl_t c)
 {
 	struct nvmf_discovery_log header = {};
@@ -197,7 +252,7 @@ static void test_five_entries(libnvme_ctrl_t c)
 	check(fetch_discovery_log(c, &log, 1) == 0, "discovery failed");
 	end_mock_cmds();
 	cmp(log, &header, sizeof(header), "incorrect header");
-	cmp(log->entries, entries, sizeof(entries), "incorrect entries");
+	cmp_entries(log->entries, entries, num_entries, "incorrect entries");
 	free(log);
 }
 
@@ -265,7 +320,7 @@ static void test_genctr_change(libnvme_ctrl_t c)
 	check(fetch_discovery_log(c, &log, 2) == 0, "discovery failed");
 	end_mock_cmds();
 	cmp(log, &header2, sizeof(header2), "incorrect header");
-	cmp(log->entries, entries2, sizeof(entries2), "incorrect entries");
+	cmp_entries(log->entries, entries2, num_entries2, "incorrect entries");
 	free(log);
 }
 

libnvme/tests/test-fabrics.c

@@ -40,35 +40,36 @@ static int test_rc;
 	} while (0)
 
 /* -------------------------------------------------------------------------
- * strchomp — strip trailing spaces
+ * sanitize_discovery_log_entry — guarantee NUL-terminated string fields
  * -------------------------------------------------------------------------
  */
-static bool test_strchomp(void)
+static bool test_sanitize_discovery_log_entry(struct libnvme_global_ctx *ctx)
 {
 	bool pass = true;
-	char s[32];
-
-	printf("\ntest_strchomp:\n");
-
-	strncpy(s, "hello   ", sizeof(s));
-	strchomp(s, 8);
-	pass = !strcmp(s, "hello");
-	CHECK(pass, "trailing spaces removed: \"%s\"", s);
-
-	strncpy(s, "hello", sizeof(s));
-	strchomp(s, 5);
-	pass = !strcmp(s, "hello");
-	CHECK(pass, "no trailing spaces (unchanged): \"%s\"", s);
-
-	strncpy(s, "   ", sizeof(s));
-	strchomp(s, 3);
-	pass = (s[0] == '\0');
-	CHECK(pass, "all spaces → empty string");
-
-	strncpy(s, "x", sizeof(s));
-	strchomp(s, 0);
-	pass = (s[0] == 'x');
-	CHECK(pass, "max=0 → no change");
+	struct nvmf_disc_log_entry e;
+
+	printf("\ntest_sanitize_discovery_log_entry:\n");
+
+	/* Normal, space-padded wire data still ends up trimmed + terminated. */
+	memset(&e, 0, sizeof(e));
+	e.trtype = NVMF_TRTYPE_TCP;
+	memset(e.trsvcid, ' ', sizeof(e.trsvcid));
+	memcpy(e.trsvcid, "4420", 4);
+	memset(e.traddr, ' ', sizeof(e.traddr));
+	memcpy(e.traddr, "10.0.0.1", 8);
+	sanitize_discovery_log_entry(ctx, &e);
+	pass = !strcmp(e.trsvcid, "4420") && !strcmp(e.traddr, "10.0.0.1");
+	CHECK(pass, "space-padded fields trimmed and terminated: \"%s\" \"%s\"",
+	      e.trsvcid, e.traddr);
+
+	/* Fully packed, no space or NUL anywhere: the pathological case. */
+	memset(&e, 'a', sizeof(e));
+	e.trtype = NVMF_TRTYPE_TCP;
+	sanitize_discovery_log_entry(ctx, &e);
+	pass = (e.trsvcid[sizeof(e.trsvcid) - 1] == '\0') &&
+	       (e.subnqn[sizeof(e.subnqn) - 1] == '\0') &&
+	       (e.traddr[sizeof(e.traddr) - 1] == '\0');
+	CHECK(pass, "fully packed fields: last byte forced to NUL");
 
 	return pass;
 }
@@ -735,7 +736,7 @@ int main(int argc, char *argv[])
 	}
 	libnvme_set_logging_level(ctx, LIBNVME_LOG_ERR, false, false);
 
-	test_strchomp();
+	test_sanitize_discovery_log_entry(ctx);
 	test_hostid_from_hostnqn();
 	test_add_bool_argument();
 	test_add_hex_argument();

shared/string-util.h

@@ -64,23 +64,39 @@ static inline char *shr_xstrdup(const char *s)
 }
 
 /*
- * Trim leading and trailing whitespace from s in place and return a
- * pointer to the first non-whitespace character. s itself is modified:
- * the byte after the last non-whitespace character is overwritten with
- * '\0'.
+ * Trim trailing whitespace from s in place: the byte after the last
+ * non-whitespace character is overwritten with '\0'. Returns s.
  */
-static inline char *shr_trim(char *s)
+static inline char *shr_rtrim(char *s)
 {
-	char *end;
+	char *end = s + strlen(s);
 
-	s += strspn(s, " \t\n\r\v\f");
-	end = s + strlen(s);
 	while (end > s && isspace((unsigned char)end[-1]))
 		end--;
 	*end = '\0';
 	return s;
 }
 
+/*
+ * Return a pointer to the first non-whitespace character in s. s itself
+ * is not modified.
+ */
+static inline char *shr_ltrim(char *s)
+{
+	return s + strspn(s, " \t\n\r\v\f");
+}
+
+/*
+ * Trim leading and trailing whitespace from s in place and return a
+ * pointer to the first non-whitespace character. s itself is modified:
+ * the byte after the last non-whitespace character is overwritten with
+ * '\0'.
+ */
+static inline char *shr_trim(char *s)
+{
+	return shr_ltrim(shr_rtrim(s));
+}
+
 /* True if s is non-empty and every character is alphanumeric, '_', or '-'. */
 static inline bool shr_valid_name(const char *s)
 {

shared/tests/test-string-util.c

@@ -112,6 +112,46 @@ static bool test_startswith(void)
 	return pass;
 }
 
+static bool test_rtrim(void)
+{
+	char buf1[] = "hello world  ";
+	char buf2[] = "no-padding";
+	char buf3[] = "   ";
+	char buf4[] = "trailing only \t\n";
+	bool pass = true;
+
+	printf("test_rtrim:\n");
+
+	pass &= check_str("trailing whitespace stripped",
+			   shr_rtrim(buf1), "hello world");
+	pass &= check_str("no padding is a no-op",
+			   shr_rtrim(buf2), "no-padding");
+	pass &= check_str("all-whitespace trims to empty", shr_rtrim(buf3), "");
+	pass &= check_str("trailing only", shr_rtrim(buf4), "trailing only");
+
+	return pass;
+}
+
+static bool test_ltrim(void)
+{
+	char buf1[] = "  hello world";
+	char buf2[] = "no-padding";
+	char buf3[] = "   ";
+	char buf4[] = "\t\n leading only";
+	bool pass = true;
+
+	printf("test_ltrim:\n");
+
+	pass &= check_str("leading whitespace skipped",
+			   shr_ltrim(buf1), "hello world");
+	pass &= check_str("no padding is a no-op",
+			   shr_ltrim(buf2), "no-padding");
+	pass &= check_str("all-whitespace skips to empty", shr_ltrim(buf3), "");
+	pass &= check_str("leading only", shr_ltrim(buf4), "leading only");
+
+	return pass;
+}
+
 static bool test_trim(void)
 {
 	char buf1[] = "  hello world  ";
@@ -202,6 +242,8 @@ int main(void)
 	pass &= test_streqcase0();
 	pass &= test_xstrdup();
 	pass &= test_startswith();
+	pass &= test_rtrim();
+	pass &= test_ltrim();
 	pass &= test_trim();
 	pass &= test_valid_name();
 	pass &= test_kv_strip();

src/fabrics.c

@@ -144,7 +144,7 @@ void nvmf_args_to_params(struct libnvmf_params *params,
 				   fa->tls_key_identity);
 }
 
-static void save_discovery_log(char *raw, struct nvmf_discovery_log *log)
+static void save_discovery_log(char *raw, const struct nvmf_discovery_log *log)
 {
 	__cleanup_free char *path = NULL;
 	static unsigned int save_count;
@@ -257,7 +257,7 @@ static void hook_already_connected(struct libnvmf_context *fctx,
 }
 
 static void hook_discovery_log(struct libnvmf_context *fctx,
-		struct nvmf_discovery_log *log,
+		const struct nvmf_discovery_log *log,
 		uint64_t numrec, void *user_data)
 {
 	struct hook_fabrics_data *hfd = user_data;

src/nvme-print-binary.c

@@ -292,7 +292,8 @@ static void binary_lba_status(struct nvme_lba_status *list, unsigned long len)
 	d_raw((unsigned char *)list, len);
 }
 
-static void binary_discovery_log(struct nvmf_discovery_log *log, int numrec)
+static void binary_discovery_log(const struct nvmf_discovery_log *log,
+				  int numrec)
 {
 	d_raw((unsigned char *)log,
 	      sizeof(struct nvmf_discovery_log) +