diff --git a/libnvme/src/nvme/fabrics.c b/libnvme/src/nvme/fabrics.c index d1aa9d95e3..e73ebb2e24 100644 --- a/libnvme/src/nvme/fabrics.c +++ b/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, diff --git a/libnvme/src/nvme/fabrics.h b/libnvme/src/nvme/fabrics.h index 01345fa3cd..fc8687f40d 100644 --- a/libnvme/src/nvme/fabrics.h +++ b/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)); /** diff --git a/libnvme/src/nvme/private-fabrics.h b/libnvme/src/nvme/private-fabrics.h index 5029dae25a..f3f45d3bd5 100644 --- a/libnvme/src/nvme/private-fabrics.h +++ b/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); /* diff --git a/libnvme/tests/ioctl/discovery.c b/libnvme/tests/ioctl/discovery.c index 6b0c557a7d..7d9ccfbfc3 100644 --- a/libnvme/tests/ioctl/discovery.c +++ b/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); } diff --git a/libnvme/tests/test-fabrics.c b/libnvme/tests/test-fabrics.c index d7e22345e6..62ecd9fc39 100644 --- a/libnvme/tests/test-fabrics.c +++ b/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(); diff --git a/shared/string-util.h b/shared/string-util.h index 64908f50a4..2f765df0ce 100644 --- a/shared/string-util.h +++ b/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) { diff --git a/shared/tests/test-string-util.c b/shared/tests/test-string-util.c index 92e204ee39..642fe46602 100644 --- a/shared/tests/test-string-util.c +++ b/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(); diff --git a/src/fabrics.c b/src/fabrics.c index 5d4505e2a4..e1d51c1c61 100644 --- a/src/fabrics.c +++ b/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; diff --git a/src/nvme-print-binary.c b/src/nvme-print-binary.c index d3974d7e3d..b801dc4df3 100644 --- a/src/nvme-print-binary.c +++ b/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) + diff --git a/src/nvme-print-json.c b/src/nvme-print-json.c index a6fc87746f..9f5549379e 100644 --- a/src/nvme-print-json.c +++ b/src/nvme-print-json.c @@ -19,6 +19,7 @@ #include #include +#include #include "nvme-print.h" #include "nvme-json.h" @@ -5247,7 +5248,23 @@ static void json_directive_show(__u8 type, __u8 oper, __u16 spec, __u32 nsid, __ } #ifdef CONFIG_FABRICS -static void json_discovery_log(struct nvmf_discovery_log *log, int numrec) +/* + * Copy a fixed-size, not-necessarily-NUL-terminated wire field @s of + * size @sz into a newly allocated, NUL-terminated, right-trimmed C + * string. "%.*s" bounds the read to @sz regardless of whether s + * contains a NUL. Returns NULL on allocation failure. + */ +static char *buf2str(const char s[], size_t sz) +{ + char *p; + + if (asprintf(&p, "%.*s", (int)sz, s) < 0) + return NULL; + + return shr_rtrim(p); +} + +static void json_discovery_log(const struct nvmf_discovery_log *log, int numrec) { struct json_object *r = json_r; struct json_object *entries = json_create_array(); @@ -5257,17 +5274,31 @@ static void json_discovery_log(struct nvmf_discovery_log *log, int numrec) obj_add_array(r, "records", entries); for (i = 0; i < numrec; i++) { - struct nvmf_disc_log_entry *e = &log->entries[i]; + const struct nvmf_disc_log_entry *e = &log->entries[i]; struct json_object *entry = json_create_object(); + /* + * e->trsvcid/subnqn/traddr are fixed-width fields off the wire, + * not guaranteed NUL-terminated. json_object_new_string() + * needs a NUL-terminated C string, so make NUL-terminated + * copies and trim trailing spaces (if any). Members of e + * cannot be modified directly (we do not own that data). + */ + __cleanup_free char *trsvcid = NULL; + __cleanup_free char *traddr = NULL; + __cleanup_free char *subnqn = NULL; + + trsvcid = buf2str(e->trsvcid, sizeof(e->trsvcid)); + traddr = buf2str(e->traddr, sizeof(e->traddr)); + subnqn = buf2str(e->subnqn, sizeof(e->subnqn)); obj_add_str(entry, "trtype", libnvmf_trtype_str(e->trtype)); obj_add_str(entry, "adrfam", libnvmf_adrfam_str(e->adrfam)); obj_add_str(entry, "subtype", libnvmf_subtype_str(e->subtype)); obj_add_str(entry, "treq", libnvmf_treq_str(e->treq)); obj_add_uint(entry, "portid", le16_to_cpu(e->portid)); - obj_add_str(entry, "trsvcid", e->trsvcid); - obj_add_str(entry, "subnqn", e->subnqn); - obj_add_str(entry, "traddr", e->traddr); + obj_add_str(entry, "trsvcid", trsvcid); + obj_add_str(entry, "subnqn", subnqn); + obj_add_str(entry, "traddr", traddr); obj_add_str(entry, "eflags", libnvmf_eflags_str(le16_to_cpu(e->eflags))); switch (e->trtype) { @@ -5287,7 +5318,10 @@ static void json_discovery_log(struct nvmf_discovery_log *log, int numrec) } } #else -static void json_discovery_log(struct nvmf_discovery_log *log, int numrec) {} +static void json_discovery_log(const struct nvmf_discovery_log *log, + int numrec) +{ +} #endif #ifdef CONFIG_FABRICS diff --git a/src/nvme-print-stdout.c b/src/nvme-print-stdout.c index 00f7d75f00..e18631896d 100644 --- a/src/nvme-print-stdout.c +++ b/src/nvme-print-stdout.c @@ -6639,7 +6639,8 @@ static void stdout_key_value(const char *key, const char *val, va_list ap) } #ifdef CONFIG_FABRICS -static void stdout_discovery_log(struct nvmf_discovery_log *log, int numrec) +static void stdout_discovery_log(const struct nvmf_discovery_log *log, + int numrec) { int i; @@ -6647,19 +6648,24 @@ static void stdout_discovery_log(struct nvmf_discovery_log *log, int numrec) numrec, le64_to_cpu(log->genctr)); for (i = 0; i < numrec; i++) { - struct nvmf_disc_log_entry *e = &log->entries[i]; + const struct nvmf_disc_log_entry *e = &log->entries[i]; + /* + * e->trsvcid/subnqn/traddr are fixed-width fields off the + * wire, not guaranteed NUL-terminated by a non-compliant DC. + * %-.*s bounds the read to the field's own size regardless. + */ printf("=====Discovery Log Entry %d======\n", i); printf("trtype: %s\n", libnvmf_trtype_str(e->trtype)); printf("adrfam: %s\n", - strlen(e->traddr) ? + e->traddr[0] ? libnvmf_adrfam_str(e->adrfam) : ""); printf("subtype: %s\n", libnvmf_subtype_str(e->subtype)); printf("treq: %s\n", libnvmf_treq_str(e->treq)); printf("portid: %d\n", le16_to_cpu(e->portid)); - printf("trsvcid: %s\n", e->trsvcid); - printf("subnqn: %s\n", e->subnqn); - printf("traddr: %s\n", e->traddr); + printf("trsvcid: %-.*s\n", (int)sizeof(e->trsvcid), e->trsvcid); + printf("subnqn: %-.*s\n", (int)sizeof(e->subnqn), e->subnqn); + printf("traddr: %-.*s\n", (int)sizeof(e->traddr), e->traddr); printf("eflags: %s\n", libnvmf_eflags_str(le16_to_cpu(e->eflags))); @@ -6682,7 +6688,10 @@ static void stdout_discovery_log(struct nvmf_discovery_log *log, int numrec) } } #else -static void stdout_discovery_log(struct nvmf_discovery_log *log, int numrec) {} +static void stdout_discovery_log(const struct nvmf_discovery_log *log, + int numrec) +{ +} #endif #ifdef CONFIG_FABRICS diff --git a/src/nvme-print.c b/src/nvme-print.c index 16d9251902..339c1fe821 100644 --- a/src/nvme-print.c +++ b/src/nvme-print.c @@ -1827,8 +1827,8 @@ void nvme_show_verbose_key_value(const char *key, const char *val, ...) va_end(ap); } -void nvme_show_discovery_log(struct nvmf_discovery_log *log, uint64_t numrec, - nvme_print_flags_t flags) +void nvme_show_discovery_log(const struct nvmf_discovery_log *log, + uint64_t numrec, nvme_print_flags_t flags) { nvme_print(discovery_log, flags, log, numrec); } diff --git a/src/nvme-print.h b/src/nvme-print.h index dc44738b10..f7c7c5ab5c 100644 --- a/src/nvme-print.h +++ b/src/nvme-print.h @@ -101,7 +101,7 @@ struct print_ops { void (*ctrl_registers)(void *bar, bool fabrics); void (*ctrl_register)(int offset, uint64_t value); void (*directive)(__u8 type, __u8 oper, __u16 spec, __u32 nsid, __u64 result, void *buf, __u32 len); - void (*discovery_log)(struct nvmf_discovery_log *log, int numrec); + void (*discovery_log)(const struct nvmf_discovery_log *log, int numrec); void (*effects_log_list)(struct list_head *list); void (*endurance_group_event_agg_log)(struct nvme_aggregate_endurance_group_event *endurance_log, __u64 log_entries, __u32 size, const char *devname); void (*endurance_group_list)(struct nvme_id_endurance_group_list *endgrp_list); @@ -388,8 +388,8 @@ void nvme_show_fdp_usage(struct nvme_fdp_ruhu_log *log, size_t len, void nvme_show_fdp_ruh_status(struct nvme_fdp_ruh_status *status, size_t len, nvme_print_flags_t flags); -void nvme_show_discovery_log(struct nvmf_discovery_log *log, uint64_t numrec, - nvme_print_flags_t flags); +void nvme_show_discovery_log(const struct nvmf_discovery_log *log, + uint64_t numrec, nvme_print_flags_t flags); void nvme_show_connect_msg(libnvme_ctrl_t c, nvme_print_flags_t flags); void nvme_show_config_conn_list(struct libnvmf_config *config, nvme_print_flags_t flags);