* [dm-devel] [PATCH 0/3] multipath config fixes
@ 2022-12-13 23:36 Benjamin Marzinski
2022-12-13 23:36 ` [dm-devel] [PATCH 1/3] libmpathutil: simplify set_value Benjamin Marzinski
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Benjamin Marzinski @ 2022-12-13 23:36 UTC (permalink / raw)
To: Christophe Varoqui; +Cc: device-mapper development, Martin Wilck
The first two patches are a cleanup and a fix for a memory leak in the
config code. The third patch improves multipath's validation of the
strings it passes directly into the table, features, path_selector, and
hardware_handler. These three strings all have argument counts, and
getting them wrong causes the kernel to parse the table incorrectly.
When this happens the table load fails, but the error messages from the
kernel are often completely unhelpful. A bad argument count will cause
the rest of the table to be parsed incorrectly, and the kernel might not
hit an unworkable token till later in the parsing. Multipath now makes
sure that the count matches the actual number of arguments that it is
passing.
Benjamin Marzinski (3):
libmpathutil: simplify set_value
libmultipath: don't leak memory on invalid strings
libmutipath: validate the argument count of config strings
libmpathutil/parser.c | 64 ++++++++----------------
libmultipath/dict.c | 112 ++++++++++++++++++++++++++++++++++++++----
2 files changed, 124 insertions(+), 52 deletions(-)
--
2.17.2
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* [dm-devel] [PATCH 1/3] libmpathutil: simplify set_value
2022-12-13 23:36 [dm-devel] [PATCH 0/3] multipath config fixes Benjamin Marzinski
@ 2022-12-13 23:36 ` Benjamin Marzinski
2022-12-14 9:19 ` Martin Wilck
2022-12-13 23:36 ` [dm-devel] [PATCH 2/3] libmultipath: don't leak memory on invalid strings Benjamin Marzinski
2022-12-13 23:36 ` [dm-devel] [PATCH 3/3] libmutipath: validate the argument count of config strings Benjamin Marzinski
2 siblings, 1 reply; 9+ messages in thread
From: Benjamin Marzinski @ 2022-12-13 23:36 UTC (permalink / raw)
To: Christophe Varoqui; +Cc: device-mapper development, Martin Wilck
alloc_strvec() will never create a strvec with multiple tokens between
the quote tokens. Verify this in validate_config_strvec(), and simplify
set_value() by only reading one value after a quote token.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmpathutil/parser.c | 64 ++++++++++++++-----------------------------
1 file changed, 21 insertions(+), 43 deletions(-)
diff --git a/libmpathutil/parser.c b/libmpathutil/parser.c
index 8d3ac53a..ac4eb1fd 100644
--- a/libmpathutil/parser.c
+++ b/libmpathutil/parser.c
@@ -333,59 +333,33 @@ void *
set_value(vector strvec)
{
char *str = VECTOR_SLOT(strvec, 1);
- size_t size;
- int i = 0;
- int len = 0;
char *alloc = NULL;
- char *tmp;
if (!str) {
condlog(0, "option '%s' missing value",
(char *)VECTOR_SLOT(strvec, 0));
return NULL;
}
- if (!is_quote(str)) {
- size = strlen(str);
- if (size == 0) {
- condlog(0, "option '%s' has empty value",
- (char *)VECTOR_SLOT(strvec, 0));
- return NULL;
- }
- alloc = calloc(1, sizeof (char) * (size + 1));
- if (alloc)
- memcpy(alloc, str, size);
- else
- goto oom;
- return alloc;
- }
- /* Even empty quotes counts as a value (An empty string) */
- alloc = (char *)calloc(1, sizeof (char));
- if (!alloc)
- goto oom;
- for (i = 2; i < VECTOR_SIZE(strvec); i++) {
- str = VECTOR_SLOT(strvec, i);
- if (!str) {
- free(alloc);
- condlog(0, "parse error for option '%s'",
- (char *)VECTOR_SLOT(strvec, 0));
- return NULL;
+ if (is_quote(str)) {
+ if (VECTOR_SIZE(strvec) > 2) {
+ str = VECTOR_SLOT(strvec, 2);
+ if (!str) {
+ condlog(0, "parse error for option '%s'",
+ (char *)VECTOR_SLOT(strvec, 0));
+ return NULL;
+ }
}
- if (is_quote(str))
- break;
- tmp = alloc;
- /* The first +1 is for the NULL byte. The rest are for the
- * spaces between words */
- len += strlen(str) + 1;
- alloc = realloc(alloc, sizeof (char) * len);
- if (!alloc) {
- free(tmp);
- goto oom;
+ /* Even empty quotes counts as a value (An empty string) */
+ if (is_quote(str)) {
+ alloc = (char *)calloc(1, sizeof (char));
+ if (!alloc)
+ goto oom;
+ return alloc;
}
- if (*alloc != '\0')
- strncat(alloc, " ", len - strlen(alloc));
- strncat(alloc, str, len - strlen(alloc) - 1);
}
- return alloc;
+ alloc = strdup(str);
+ if (alloc)
+ return alloc;
oom:
condlog(0, "can't allocate memory for option '%s'",
(char *)VECTOR_SLOT(strvec, 0));
@@ -496,6 +470,10 @@ validate_config_strvec(vector strvec, const char *file)
if (VECTOR_SIZE(strvec) > i + 1)
condlog(0, "ignoring extra data starting with '%s' on line %d of %s", (char *)VECTOR_SLOT(strvec, (i + 1)), line_nr, file);
return 0;
+ } else if (i > 3) {
+ /* There should only ever be one token between quotes */
+ condlog(0, "parsing error starting with '%s' on line %d of %s", str, line_nr, file);
+ return -1;
}
}
condlog(0, "missing closing quotes on line %d of %s",
--
2.17.2
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [dm-devel] [PATCH 2/3] libmultipath: don't leak memory on invalid strings
2022-12-13 23:36 [dm-devel] [PATCH 0/3] multipath config fixes Benjamin Marzinski
2022-12-13 23:36 ` [dm-devel] [PATCH 1/3] libmpathutil: simplify set_value Benjamin Marzinski
@ 2022-12-13 23:36 ` Benjamin Marzinski
2022-12-14 9:21 ` Martin Wilck
2022-12-13 23:36 ` [dm-devel] [PATCH 3/3] libmutipath: validate the argument count of config strings Benjamin Marzinski
2 siblings, 1 reply; 9+ messages in thread
From: Benjamin Marzinski @ 2022-12-13 23:36 UTC (permalink / raw)
To: Christophe Varoqui; +Cc: device-mapper development, Martin Wilck
If set_path() or set_str_noslash() are called with a bad value, they
ignore it and continue to use the old value. But they weren't freeing
the bad value, causing a memory leak.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmultipath/dict.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/libmultipath/dict.c b/libmultipath/dict.c
index 97f43387..f4233882 100644
--- a/libmultipath/dict.c
+++ b/libmultipath/dict.c
@@ -130,6 +130,7 @@ set_path(vector strvec, void *ptr, const char *file, int line_nr)
if ((*str_ptr)[0] != '/'){
condlog(1, "%s line %d, %s is not an absolute path. Ignoring",
file, line_nr, *str_ptr);
+ free(*str_ptr);
*str_ptr = old_str;
} else
free(old_str);
@@ -150,6 +151,7 @@ set_str_noslash(vector strvec, void *ptr, const char *file, int line_nr)
if (strchr(*str_ptr, '/')) {
condlog(1, "%s line %d, %s cannot contain a slash. Ignoring",
file, line_nr, *str_ptr);
+ free(*str_ptr);
*str_ptr = old_str;
} else
free(old_str);
--
2.17.2
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [dm-devel] [PATCH 3/3] libmutipath: validate the argument count of config strings
2022-12-13 23:36 [dm-devel] [PATCH 0/3] multipath config fixes Benjamin Marzinski
2022-12-13 23:36 ` [dm-devel] [PATCH 1/3] libmpathutil: simplify set_value Benjamin Marzinski
2022-12-13 23:36 ` [dm-devel] [PATCH 2/3] libmultipath: don't leak memory on invalid strings Benjamin Marzinski
@ 2022-12-13 23:36 ` Benjamin Marzinski
2022-12-14 9:41 ` Martin Wilck
2 siblings, 1 reply; 9+ messages in thread
From: Benjamin Marzinski @ 2022-12-13 23:36 UTC (permalink / raw)
To: Christophe Varoqui; +Cc: device-mapper development, Martin Wilck
The features, path_selector, and hardware_handler config options pass
their strings directly into the kernel. If users omit the argument
counts from these strings, or use the wrong value, the kernel's table
parsing gets completely messed up, and the error messages it prints
don't reflect what actully went wrong. To avoid messing up the
kernel table parsing, verify that these strings correctly set the
argument count to the number of arguments they have.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmultipath/dict.c | 110 ++++++++++++++++++++++++++++++++++++++++----
1 file changed, 101 insertions(+), 9 deletions(-)
diff --git a/libmultipath/dict.c b/libmultipath/dict.c
index f4233882..6645de49 100644
--- a/libmultipath/dict.c
+++ b/libmultipath/dict.c
@@ -116,6 +116,58 @@ set_str(vector strvec, void *ptr, const char *file, int line_nr)
return 0;
}
+static int
+set_arg_str(vector strvec, void *ptr, int count_idx, const char *file,
+ int line_nr)
+{
+ char **str_ptr = (char **)ptr;
+ char *old_str = *str_ptr;
+ const char *spaces = " \f\n\r\t\v";
+ char *p, *end;
+ int idx = -1;
+ long int count = -1;
+
+ *str_ptr = set_value(strvec);
+ if (!*str_ptr) {
+ free(old_str);
+ return 1;
+ }
+ p = *str_ptr;
+ while (*p != '\0') {
+ p += strspn(p, spaces);
+ if (*p == '\0')
+ break;
+ idx += 1;
+ if (idx == count_idx) {
+ errno = 0;
+ count = strtol(p, &end, 10);
+ if (errno == ERANGE || end == p ||
+ !(isspace(*end) || *end == '\0')) {
+ count = -1;
+ break;
+ }
+ }
+ p += strcspn(p, spaces);
+ }
+ if (count < 0) {
+ condlog(1, "%s line %d, missing argument count for %s",
+ file, line_nr, (char*)VECTOR_SLOT(strvec, 0));
+ goto fail;
+ }
+ if (count != idx - count_idx) {
+ condlog(1, "%s line %d, invalid argument count for %s:, got '%ld' expected '%d'",
+ file, line_nr, (char*)VECTOR_SLOT(strvec, 0), count,
+ idx - count_idx);
+ goto fail;
+ }
+ free(old_str);
+ return 0;
+fail:
+ free(*str_ptr);
+ *str_ptr = old_str;
+ return 0;
+}
+
static int
set_path(vector strvec, void *ptr, const char *file, int line_nr)
{
@@ -288,6 +340,14 @@ def_ ## option ## _handler (struct config *conf, vector strvec, \
return set_int(strvec, &conf->option, minval, maxval, file, line_nr); \
}
+#define declare_def_arg_str_handler(option, count_idx) \
+static int \
+def_ ## option ## _handler (struct config *conf, vector strvec, \
+ const char *file, int line_nr) \
+{ \
+ return set_arg_str(strvec, &conf->option, count_idx, file, line_nr); \
+}
+
#define declare_def_snprint(option, function) \
static int \
snprint_def_ ## option (struct config *conf, struct strbuf *buff, \
@@ -340,6 +400,17 @@ hw_ ## option ## _handler (struct config *conf, vector strvec, \
return set_int(strvec, &hwe->option, minval, maxval, file, line_nr); \
}
+#define declare_hw_arg_str_handler(option, count_idx) \
+static int \
+hw_ ## option ## _handler (struct config *conf, vector strvec, \
+ const char *file, int line_nr) \
+{ \
+ struct hwentry * hwe = VECTOR_LAST_SLOT(conf->hwtable); \
+ if (!hwe) \
+ return 1; \
+ return set_arg_str(strvec, &hwe->option, count_idx, file, line_nr); \
+}
+
#define declare_hw_snprint(option, function) \
static int \
@@ -371,6 +442,16 @@ ovr_ ## option ## _handler (struct config *conf, vector strvec, \
file, line_nr); \
}
+#define declare_ovr_arg_str_handler(option, count_idx) \
+static int \
+ovr_ ## option ## _handler (struct config *conf, vector strvec, \
+ const char *file, int line_nr) \
+{ \
+ if (!conf->overrides) \
+ return 1; \
+ return set_arg_str(strvec, &conf->overrides->option, count_idx, file, line_nr); \
+}
+
#define declare_ovr_snprint(option, function) \
static int \
snprint_ovr_ ## option (struct config *conf, struct strbuf *buff, \
@@ -401,6 +482,17 @@ mp_ ## option ## _handler (struct config *conf, vector strvec, \
return set_int(strvec, &mpe->option, minval, maxval, file, line_nr); \
}
+#define declare_mp_arg_str_handler(option, count_idx) \
+static int \
+mp_ ## option ## _handler (struct config *conf, vector strvec, \
+ const char *file, int line_nr) \
+{ \
+ struct mpentry * mpe = VECTOR_LAST_SLOT(conf->mptable); \
+ if (!mpe) \
+ return 1; \
+ return set_arg_str(strvec, &mpe->option, count_idx, file, line_nr); \
+}
+
#define declare_mp_snprint(option, function) \
static int \
snprint_mp_ ## option (struct config *conf, struct strbuf *buff, \
@@ -584,13 +676,13 @@ snprint_def_marginal_pathgroups(struct config *conf, struct strbuf *buff,
}
-declare_def_handler(selector, set_str)
+declare_def_arg_str_handler(selector, 1)
declare_def_snprint_defstr(selector, print_str, DEFAULT_SELECTOR)
-declare_hw_handler(selector, set_str)
+declare_hw_arg_str_handler(selector, 1)
declare_hw_snprint(selector, print_str)
-declare_ovr_handler(selector, set_str)
+declare_ovr_arg_str_handler(selector, 1)
declare_ovr_snprint(selector, print_str)
-declare_mp_handler(selector, set_str)
+declare_mp_arg_str_handler(selector, 1)
declare_mp_snprint(selector, print_str)
static int snprint_uid_attrs(struct config *conf, struct strbuf *buff,
@@ -663,13 +755,13 @@ declare_hw_snprint(prio_args, print_str)
declare_mp_handler(prio_args, set_str)
declare_mp_snprint(prio_args, print_str)
-declare_def_handler(features, set_str)
+declare_def_arg_str_handler(features, 0)
declare_def_snprint_defstr(features, print_str, DEFAULT_FEATURES)
-declare_ovr_handler(features, set_str)
+declare_ovr_arg_str_handler(features, 0)
declare_ovr_snprint(features, print_str)
-declare_hw_handler(features, set_str)
+declare_hw_arg_str_handler(features, 0)
declare_hw_snprint(features, print_str)
-declare_mp_handler(features, set_str)
+declare_mp_arg_str_handler(features, 0)
declare_mp_snprint(features, print_str)
declare_def_handler(checker_name, set_str)
@@ -1821,7 +1913,7 @@ declare_hw_snprint(revision, print_str)
declare_hw_handler(bl_product, set_str)
declare_hw_snprint(bl_product, print_str)
-declare_hw_handler(hwhandler, set_str)
+declare_hw_arg_str_handler(hwhandler, 0)
declare_hw_snprint(hwhandler, print_str)
/*
--
2.17.2
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [dm-devel] [PATCH 1/3] libmpathutil: simplify set_value
2022-12-13 23:36 ` [dm-devel] [PATCH 1/3] libmpathutil: simplify set_value Benjamin Marzinski
@ 2022-12-14 9:19 ` Martin Wilck
2022-12-14 15:28 ` Benjamin Marzinski
0 siblings, 1 reply; 9+ messages in thread
From: Martin Wilck @ 2022-12-14 9:19 UTC (permalink / raw)
To: bmarzins, christophe.varoqui; +Cc: dm-devel
On Tue, 2022-12-13 at 17:36 -0600, Benjamin Marzinski wrote:
> alloc_strvec() will never create a strvec with multiple tokens
> between
> the quote tokens. Verify this in validate_config_strvec(), and
> simplify
> set_value() by only reading one value after a quote token.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
One suggestion below
> @@ -496,6 +470,10 @@ validate_config_strvec(vector strvec, const char
> *file)
> if (VECTOR_SIZE(strvec) > i + 1)
> condlog(0, "ignoring extra data
> starting with '%s' on line %d of %s", (char *)VECTOR_SLOT(strvec, (i
> + 1)), line_nr, file);
> return 0;
> + } else if (i > 3) {
> + /* There should only ever be one token
> between quotes */
> + condlog(0, "parsing error starting with '%s'
> on line %d of %s", str, line_nr, file);
> + return -1;
> }
> }
> condlog(0, "missing closing quotes on line %d of %s",
This could be further simplified. We know that strvec[1] is a quote. So
the only valid possibilities are
- strvec[2] is a quote (-> empty string)
- strvec[2] is not a quote and strvec[3] is a quote
The code would be better understandable if we just spell out these
possibilities rather than using a loop that start at 2 and is left at 3
already.
Martin
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dm-devel] [PATCH 2/3] libmultipath: don't leak memory on invalid strings
2022-12-13 23:36 ` [dm-devel] [PATCH 2/3] libmultipath: don't leak memory on invalid strings Benjamin Marzinski
@ 2022-12-14 9:21 ` Martin Wilck
0 siblings, 0 replies; 9+ messages in thread
From: Martin Wilck @ 2022-12-14 9:21 UTC (permalink / raw)
To: bmarzins, christophe.varoqui; +Cc: dm-devel
On Tue, 2022-12-13 at 17:36 -0600, Benjamin Marzinski wrote:
> If set_path() or set_str_noslash() are called with a bad value, they
> ignore it and continue to use the old value. But they weren't freeing
> the bad value, causing a memory leak.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
Reviewed-by: Martin Wilck <mwilck@suse.com>
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dm-devel] [PATCH 3/3] libmutipath: validate the argument count of config strings
2022-12-13 23:36 ` [dm-devel] [PATCH 3/3] libmutipath: validate the argument count of config strings Benjamin Marzinski
@ 2022-12-14 9:41 ` Martin Wilck
2022-12-14 16:12 ` Benjamin Marzinski
0 siblings, 1 reply; 9+ messages in thread
From: Martin Wilck @ 2022-12-14 9:41 UTC (permalink / raw)
To: bmarzins, christophe.varoqui; +Cc: dm-devel
On Tue, 2022-12-13 at 17:36 -0600, Benjamin Marzinski wrote:
> The features, path_selector, and hardware_handler config options pass
> their strings directly into the kernel. If users omit the argument
> counts from these strings, or use the wrong value, the kernel's table
> parsing gets completely messed up, and the error messages it prints
> don't reflect what actully went wrong. To avoid messing up the
> kernel table parsing, verify that these strings correctly set the
> argument count to the number of arguments they have.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
> libmultipath/dict.c | 110 ++++++++++++++++++++++++++++++++++++++++--
> --
> 1 file changed, 101 insertions(+), 9 deletions(-)
>
> diff --git a/libmultipath/dict.c b/libmultipath/dict.c
> index f4233882..6645de49 100644
> --- a/libmultipath/dict.c
> +++ b/libmultipath/dict.c
> @@ -116,6 +116,58 @@ set_str(vector strvec, void *ptr, const char
> *file, int line_nr)
> return 0;
> }
>
> +static int
> +set_arg_str(vector strvec, void *ptr, int count_idx, const char
> *file,
> + int line_nr)
> +{
> + char **str_ptr = (char **)ptr;
> + char *old_str = *str_ptr;
> + const char *spaces = " \f\n\r\t\v";
Nit: I believe '\n' can't occur in values passed from multipath.conf,
as we don't support multi-line values. Also, should this be "static
const char * const spaces", maybe?
Other than that, this looks good to me.
Regards,
Martin
> + char *p, *end;
> + int idx = -1;
> + long int count = -1;
> +
> + *str_ptr = set_value(strvec);
> + if (!*str_ptr) {
> + free(old_str);
> + return 1;
> + }
> + p = *str_ptr;
> + while (*p != '\0') {
> + p += strspn(p, spaces);
> + if (*p == '\0')
> + break;
> + idx += 1;
> + if (idx == count_idx) {
> + errno = 0;
> + count = strtol(p, &end, 10);
> + if (errno == ERANGE || end == p ||
> + !(isspace(*end) || *end == '\0')) {
> + count = -1;
> + break;
> + }
> + }
> + p += strcspn(p, spaces);
> + }
> + if (count < 0) {
> + condlog(1, "%s line %d, missing argument count for
> %s",
> + file, line_nr, (char*)VECTOR_SLOT(strvec,
> 0));
> + goto fail;
> + }
> + if (count != idx - count_idx) {
> + condlog(1, "%s line %d, invalid argument count for
> %s:, got '%ld' expected '%d'",
> + file, line_nr, (char*)VECTOR_SLOT(strvec, 0),
> count,
> + idx - count_idx);
> + goto fail;
> + }
> + free(old_str);
> + return 0;
> +fail:
> + free(*str_ptr);
> + *str_ptr = old_str;
> + return 0;
> +}
> +
> static int
> set_path(vector strvec, void *ptr, const char *file, int line_nr)
> {
> @@ -288,6 +340,14 @@ def_ ## option ## _handler (struct config *conf,
> vector strvec, \
> return set_int(strvec, &conf->option, minval, maxval, file,
> line_nr); \
> }
>
> +#define declare_def_arg_str_handler(option,
> count_idx) \
> +static
> int \
> +def_ ## option ## _handler (struct config *conf, vector
> strvec, \
> + const char *file, int
> line_nr) \
> +{
> \
> + return set_arg_str(strvec, &conf->option, count_idx, file,
> line_nr); \
> +}
> +
> #define declare_def_snprint(option,
> function) \
> static
> int \
> snprint_def_ ## option (struct config *conf, struct strbuf
> *buff, \
> @@ -340,6 +400,17 @@ hw_ ## option ## _handler (struct config *conf,
> vector strvec, \
> return set_int(strvec, &hwe->option, minval, maxval, file,
> line_nr); \
> }
>
> +#define declare_hw_arg_str_handler(option,
> count_idx) \
> +static
> int \
> +hw_ ## option ## _handler (struct config *conf, vector
> strvec, \
> + const char *file, int
> line_nr) \
> +{
> \
> + struct hwentry * hwe = VECTOR_LAST_SLOT(conf-
> >hwtable); \
> + if
> (!hwe) \
> + return
> 1; \
> + return set_arg_str(strvec, &hwe->option, count_idx, file,
> line_nr); \
> +}
> +
>
> #define declare_hw_snprint(option,
> function) \
> static
> int \
> @@ -371,6 +442,16 @@ ovr_ ## option ## _handler (struct config *conf,
> vector strvec, \
> file, line_nr); \
> }
>
> +#define declare_ovr_arg_str_handler(option,
> count_idx) \
> +static
> int \
> +ovr_ ## option ## _handler (struct config *conf, vector
> strvec, \
> + const char *file, int
> line_nr) \
> +{
> \
> + if (!conf-
> >overrides) \
> + return
> 1; \
> + return set_arg_str(strvec, &conf->overrides->option,
> count_idx, file, line_nr); \
> +}
> +
> #define declare_ovr_snprint(option,
> function) \
> static
> int \
> snprint_ovr_ ## option (struct config *conf, struct strbuf
> *buff, \
> @@ -401,6 +482,17 @@ mp_ ## option ## _handler (struct config *conf,
> vector strvec, \
> return set_int(strvec, &mpe->option, minval, maxval, file,
> line_nr); \
> }
>
> +#define declare_mp_arg_str_handler(option,
> count_idx) \
> +static
> int \
> +mp_ ## option ## _handler (struct config *conf, vector
> strvec, \
> + const char *file, int
> line_nr) \
> +{
> \
> + struct mpentry * mpe = VECTOR_LAST_SLOT(conf-
> >mptable); \
> + if
> (!mpe) \
> + return
> 1; \
> + return set_arg_str(strvec, &mpe->option, count_idx, file,
> line_nr); \
> +}
> +
> #define declare_mp_snprint(option,
> function) \
> static
> int \
> snprint_mp_ ## option (struct config *conf, struct strbuf
> *buff, \
> @@ -584,13 +676,13 @@ snprint_def_marginal_pathgroups(struct config
> *conf, struct strbuf *buff,
> }
>
>
> -declare_def_handler(selector, set_str)
> +declare_def_arg_str_handler(selector, 1)
> declare_def_snprint_defstr(selector, print_str, DEFAULT_SELECTOR)
> -declare_hw_handler(selector, set_str)
> +declare_hw_arg_str_handler(selector, 1)
> declare_hw_snprint(selector, print_str)
> -declare_ovr_handler(selector, set_str)
> +declare_ovr_arg_str_handler(selector, 1)
> declare_ovr_snprint(selector, print_str)
> -declare_mp_handler(selector, set_str)
> +declare_mp_arg_str_handler(selector, 1)
> declare_mp_snprint(selector, print_str)
>
> static int snprint_uid_attrs(struct config *conf, struct strbuf
> *buff,
> @@ -663,13 +755,13 @@ declare_hw_snprint(prio_args, print_str)
> declare_mp_handler(prio_args, set_str)
> declare_mp_snprint(prio_args, print_str)
>
> -declare_def_handler(features, set_str)
> +declare_def_arg_str_handler(features, 0)
> declare_def_snprint_defstr(features, print_str, DEFAULT_FEATURES)
> -declare_ovr_handler(features, set_str)
> +declare_ovr_arg_str_handler(features, 0)
> declare_ovr_snprint(features, print_str)
> -declare_hw_handler(features, set_str)
> +declare_hw_arg_str_handler(features, 0)
> declare_hw_snprint(features, print_str)
> -declare_mp_handler(features, set_str)
> +declare_mp_arg_str_handler(features, 0)
> declare_mp_snprint(features, print_str)
>
> declare_def_handler(checker_name, set_str)
> @@ -1821,7 +1913,7 @@ declare_hw_snprint(revision, print_str)
> declare_hw_handler(bl_product, set_str)
> declare_hw_snprint(bl_product, print_str)
>
> -declare_hw_handler(hwhandler, set_str)
> +declare_hw_arg_str_handler(hwhandler, 0)
> declare_hw_snprint(hwhandler, print_str)
>
> /*
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dm-devel] [PATCH 1/3] libmpathutil: simplify set_value
2022-12-14 9:19 ` Martin Wilck
@ 2022-12-14 15:28 ` Benjamin Marzinski
0 siblings, 0 replies; 9+ messages in thread
From: Benjamin Marzinski @ 2022-12-14 15:28 UTC (permalink / raw)
To: Martin Wilck; +Cc: dm-devel
On Wed, Dec 14, 2022 at 09:19:45AM +0000, Martin Wilck wrote:
> On Tue, 2022-12-13 at 17:36 -0600, Benjamin Marzinski wrote:
> > alloc_strvec() will never create a strvec with multiple tokens
> > between
> > the quote tokens. Verify this in validate_config_strvec(), and
> > simplify
> > set_value() by only reading one value after a quote token.
> >
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
>
> One suggestion below
>
>
> > @@ -496,6 +470,10 @@ validate_config_strvec(vector strvec, const char
> > *file)
> > if (VECTOR_SIZE(strvec) > i + 1)
> > condlog(0, "ignoring extra data
> > starting with '%s' on line %d of %s", (char *)VECTOR_SLOT(strvec, (i
> > + 1)), line_nr, file);
> > return 0;
> > + } else if (i > 3) {
> > + /* There should only ever be one token
> > between quotes */
> > + condlog(0, "parsing error starting with '%s'
> > on line %d of %s", str, line_nr, file);
> > + return -1;
> > }
> > }
> > condlog(0, "missing closing quotes on line %d of %s",
>
> This could be further simplified. We know that strvec[1] is a quote. So
> the only valid possibilities are
>
> - strvec[2] is a quote (-> empty string)
> - strvec[2] is not a quote and strvec[3] is a quote
>
> The code would be better understandable if we just spell out these
> possibilities rather than using a loop that start at 2 and is left at 3
> already.
Makes sense.
-Ben
>
> Martin
>
>
>
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [dm-devel] [PATCH 3/3] libmutipath: validate the argument count of config strings
2022-12-14 9:41 ` Martin Wilck
@ 2022-12-14 16:12 ` Benjamin Marzinski
0 siblings, 0 replies; 9+ messages in thread
From: Benjamin Marzinski @ 2022-12-14 16:12 UTC (permalink / raw)
To: Martin Wilck; +Cc: dm-devel
On Wed, Dec 14, 2022 at 09:41:50AM +0000, Martin Wilck wrote:
> On Tue, 2022-12-13 at 17:36 -0600, Benjamin Marzinski wrote:
> > The features, path_selector, and hardware_handler config options pass
> > their strings directly into the kernel. If users omit the argument
> > counts from these strings, or use the wrong value, the kernel's table
> > parsing gets completely messed up, and the error messages it prints
> > don't reflect what actully went wrong. To avoid messing up the
> > kernel table parsing, verify that these strings correctly set the
> > argument count to the number of arguments they have.
> >
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> > libmultipath/dict.c | 110 ++++++++++++++++++++++++++++++++++++++++--
> > --
> > 1 file changed, 101 insertions(+), 9 deletions(-)
> >
> > diff --git a/libmultipath/dict.c b/libmultipath/dict.c
> > index f4233882..6645de49 100644
> > --- a/libmultipath/dict.c
> > +++ b/libmultipath/dict.c
> > @@ -116,6 +116,58 @@ set_str(vector strvec, void *ptr, const char
> > *file, int line_nr)
> > return 0;
> > }
> >
> > +static int
> > +set_arg_str(vector strvec, void *ptr, int count_idx, const char
> > *file,
> > + int line_nr)
> > +{
> > + char **str_ptr = (char **)ptr;
> > + char *old_str = *str_ptr;
> > + const char *spaces = " \f\n\r\t\v";
>
> Nit: I believe '\n' can't occur in values passed from multipath.conf,
> as we don't support multi-line values.
Sure. The goal was to treat the strings the same way as the kernel
would, but I agree we can't get a '\n' from a value in multipath.conf.
Also, for what it's worth, the kernel also treats the character 0xa0 as
a whitespace character (nbsp) since it uses an
almost-but-not-quite-latin1 character set. I've just been ignoring this,
and plan to continue doing so unless someone complains.
> Also, should this be "static
> const char * const spaces", maybe?
Sure.
-Ben
> Other than that, this looks good to me.
>
> Regards,
> Martin
>
>
>
> > + char *p, *end;
> > + int idx = -1;
> > + long int count = -1;
> > +
> > + *str_ptr = set_value(strvec);
> > + if (!*str_ptr) {
> > + free(old_str);
> > + return 1;
> > + }
> > + p = *str_ptr;
> > + while (*p != '\0') {
> > + p += strspn(p, spaces);
> > + if (*p == '\0')
> > + break;
> > + idx += 1;
> > + if (idx == count_idx) {
> > + errno = 0;
> > + count = strtol(p, &end, 10);
> > + if (errno == ERANGE || end == p ||
> > + !(isspace(*end) || *end == '\0')) {
> > + count = -1;
> > + break;
> > + }
> > + }
> > + p += strcspn(p, spaces);
> > + }
> > + if (count < 0) {
> > + condlog(1, "%s line %d, missing argument count for
> > %s",
> > + file, line_nr, (char*)VECTOR_SLOT(strvec,
> > 0));
> > + goto fail;
> > + }
> > + if (count != idx - count_idx) {
> > + condlog(1, "%s line %d, invalid argument count for
> > %s:, got '%ld' expected '%d'",
> > + file, line_nr, (char*)VECTOR_SLOT(strvec, 0),
> > count,
> > + idx - count_idx);
> > + goto fail;
> > + }
> > + free(old_str);
> > + return 0;
> > +fail:
> > + free(*str_ptr);
> > + *str_ptr = old_str;
> > + return 0;
> > +}
> > +
> > static int
> > set_path(vector strvec, void *ptr, const char *file, int line_nr)
> > {
> > @@ -288,6 +340,14 @@ def_ ## option ## _handler (struct config *conf,
> > vector strvec, \
> > return set_int(strvec, &conf->option, minval, maxval, file,
> > line_nr); \
> > }
> >
> > +#define declare_def_arg_str_handler(option,
> > count_idx) \
> > +static
> > int \
> > +def_ ## option ## _handler (struct config *conf, vector
> > strvec, \
> > + const char *file, int
> > line_nr) \
> > +{
> > \
> > + return set_arg_str(strvec, &conf->option, count_idx, file,
> > line_nr); \
> > +}
> > +
> > #define declare_def_snprint(option,
> > function) \
> > static
> > int \
> > snprint_def_ ## option (struct config *conf, struct strbuf
> > *buff, \
> > @@ -340,6 +400,17 @@ hw_ ## option ## _handler (struct config *conf,
> > vector strvec, \
> > return set_int(strvec, &hwe->option, minval, maxval, file,
> > line_nr); \
> > }
> >
> > +#define declare_hw_arg_str_handler(option,
> > count_idx) \
> > +static
> > int \
> > +hw_ ## option ## _handler (struct config *conf, vector
> > strvec, \
> > + const char *file, int
> > line_nr) \
> > +{
> > \
> > + struct hwentry * hwe = VECTOR_LAST_SLOT(conf-
> > >hwtable); \
> > + if
> > (!hwe) \
> > + return
> > 1; \
> > + return set_arg_str(strvec, &hwe->option, count_idx, file,
> > line_nr); \
> > +}
> > +
> >
> > #define declare_hw_snprint(option,
> > function) \
> > static
> > int \
> > @@ -371,6 +442,16 @@ ovr_ ## option ## _handler (struct config *conf,
> > vector strvec, \
> > file, line_nr); \
> > }
> >
> > +#define declare_ovr_arg_str_handler(option,
> > count_idx) \
> > +static
> > int \
> > +ovr_ ## option ## _handler (struct config *conf, vector
> > strvec, \
> > + const char *file, int
> > line_nr) \
> > +{
> > \
> > + if (!conf-
> > >overrides) \
> > + return
> > 1; \
> > + return set_arg_str(strvec, &conf->overrides->option,
> > count_idx, file, line_nr); \
> > +}
> > +
> > #define declare_ovr_snprint(option,
> > function) \
> > static
> > int \
> > snprint_ovr_ ## option (struct config *conf, struct strbuf
> > *buff, \
> > @@ -401,6 +482,17 @@ mp_ ## option ## _handler (struct config *conf,
> > vector strvec, \
> > return set_int(strvec, &mpe->option, minval, maxval, file,
> > line_nr); \
> > }
> >
> > +#define declare_mp_arg_str_handler(option,
> > count_idx) \
> > +static
> > int \
> > +mp_ ## option ## _handler (struct config *conf, vector
> > strvec, \
> > + const char *file, int
> > line_nr) \
> > +{
> > \
> > + struct mpentry * mpe = VECTOR_LAST_SLOT(conf-
> > >mptable); \
> > + if
> > (!mpe) \
> > + return
> > 1; \
> > + return set_arg_str(strvec, &mpe->option, count_idx, file,
> > line_nr); \
> > +}
> > +
> > #define declare_mp_snprint(option,
> > function) \
> > static
> > int \
> > snprint_mp_ ## option (struct config *conf, struct strbuf
> > *buff, \
> > @@ -584,13 +676,13 @@ snprint_def_marginal_pathgroups(struct config
> > *conf, struct strbuf *buff,
> > }
> >
> >
> > -declare_def_handler(selector, set_str)
> > +declare_def_arg_str_handler(selector, 1)
> > declare_def_snprint_defstr(selector, print_str, DEFAULT_SELECTOR)
> > -declare_hw_handler(selector, set_str)
> > +declare_hw_arg_str_handler(selector, 1)
> > declare_hw_snprint(selector, print_str)
> > -declare_ovr_handler(selector, set_str)
> > +declare_ovr_arg_str_handler(selector, 1)
> > declare_ovr_snprint(selector, print_str)
> > -declare_mp_handler(selector, set_str)
> > +declare_mp_arg_str_handler(selector, 1)
> > declare_mp_snprint(selector, print_str)
> >
> > static int snprint_uid_attrs(struct config *conf, struct strbuf
> > *buff,
> > @@ -663,13 +755,13 @@ declare_hw_snprint(prio_args, print_str)
> > declare_mp_handler(prio_args, set_str)
> > declare_mp_snprint(prio_args, print_str)
> >
> > -declare_def_handler(features, set_str)
> > +declare_def_arg_str_handler(features, 0)
> > declare_def_snprint_defstr(features, print_str, DEFAULT_FEATURES)
> > -declare_ovr_handler(features, set_str)
> > +declare_ovr_arg_str_handler(features, 0)
> > declare_ovr_snprint(features, print_str)
> > -declare_hw_handler(features, set_str)
> > +declare_hw_arg_str_handler(features, 0)
> > declare_hw_snprint(features, print_str)
> > -declare_mp_handler(features, set_str)
> > +declare_mp_arg_str_handler(features, 0)
> > declare_mp_snprint(features, print_str)
> >
> > declare_def_handler(checker_name, set_str)
> > @@ -1821,7 +1913,7 @@ declare_hw_snprint(revision, print_str)
> > declare_hw_handler(bl_product, set_str)
> > declare_hw_snprint(bl_product, print_str)
> >
> > -declare_hw_handler(hwhandler, set_str)
> > +declare_hw_arg_str_handler(hwhandler, 0)
> > declare_hw_snprint(hwhandler, print_str)
> >
> > /*
>
>
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-12-14 16:13 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-13 23:36 [dm-devel] [PATCH 0/3] multipath config fixes Benjamin Marzinski
2022-12-13 23:36 ` [dm-devel] [PATCH 1/3] libmpathutil: simplify set_value Benjamin Marzinski
2022-12-14 9:19 ` Martin Wilck
2022-12-14 15:28 ` Benjamin Marzinski
2022-12-13 23:36 ` [dm-devel] [PATCH 2/3] libmultipath: don't leak memory on invalid strings Benjamin Marzinski
2022-12-14 9:21 ` Martin Wilck
2022-12-13 23:36 ` [dm-devel] [PATCH 3/3] libmutipath: validate the argument count of config strings Benjamin Marzinski
2022-12-14 9:41 ` Martin Wilck
2022-12-14 16:12 ` Benjamin Marzinski
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.