| Message ID | 1670974567-8005-2-git-send-email-bmarzins@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | multipath config fixes | expand |
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
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
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",
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(-)