Cleanup option keyword/environment substitution
Right now the substitution for options seems quite fragile. Among the
issues...
- If bc had an error and returned no output, it caused a NULL reference
- Multiple variable substitutions (For example $ncpus * $pagesize)
caused an error as it tried to run bc after the first, with the second
still text
- Memory leak for every keyword substituted
- Multiplication caused shell wildcard expansion (*) of the current
directory when passing the input to bc
- Shell escape sequences would be parsed on the command line when bc is called
- Potential buffer overrun due to unchecked lengths on the input line
So I did a little cleanup to get rid of the issues. This patch also
moves the environment variable substitution to run before the keyword
substitution, so an environment variable can now indirectly perform a
keyword substitution.
Signed-off-by: Jens Axboe <axboe@kernel.dk>
diff --git a/options.c b/options.c
index bb46dc9..84bf5ac 100644
--- a/options.c
+++ b/options.c
@@ -2218,15 +2218,15 @@
static char *bc_calc(char *str)
{
- char *buf, *tmp, opt[80];
+ char buf[128], *tmp;
FILE *f;
int ret;
/*
* No math, just return string
*/
- if (!strchr(str, '+') && !strchr(str, '-') && !strchr(str, '*') &&
- !strchr(str, '/'))
+ if ((!strchr(str, '+') && !strchr(str, '-') && !strchr(str, '*') &&
+ !strchr(str, '/')) || strchr(str, '\''))
return str;
/*
@@ -2237,37 +2237,89 @@
return str;
tmp++;
- memset(opt, 0, sizeof(opt));
- strncpy(opt, str, tmp - str);
- buf = malloc(128);
+ /*
+ * Prevent buffer overflows; such a case isn't reasonable anyway
+ */
+ if (strlen(str) >= 128 || strlen(tmp) > 100)
+ return str;
sprintf(buf, "which %s > /dev/null", BC_APP);
if (system(buf)) {
log_err("fio: bc is needed for performing math\n");
- free(buf);
return NULL;
}
- sprintf(buf, "echo %s | %s", tmp, BC_APP);
+ sprintf(buf, "echo '%s' | %s", tmp, BC_APP);
f = popen(buf, "r");
if (!f) {
- free(buf);
return NULL;
}
- ret = fread(buf, 1, 128, f);
+ ret = fread(&buf[tmp - str], 1, 128 - (tmp - str), f);
if (ret <= 0) {
- free(buf);
return NULL;
}
- buf[ret - 1] = '\0';
- strcat(opt, buf);
- strcpy(buf, opt);
pclose(f);
+ buf[(tmp - str) + ret - 1] = '\0';
+ memcpy(buf, str, tmp - str);
free(str);
- return buf;
+ return strdup(buf);
+}
+
+/*
+ * Return a copy of the input string with substrings of the form ${VARNAME}
+ * substituted with the value of the environment variable VARNAME. The
+ * substitution always occurs, even if VARNAME is empty or the corresponding
+ * environment variable undefined.
+ */
+static char *option_dup_subs(const char *opt)
+{
+ char out[OPT_LEN_MAX+1];
+ char in[OPT_LEN_MAX+1];
+ char *outptr = out;
+ char *inptr = in;
+ char *ch1, *ch2, *env;
+ ssize_t nchr = OPT_LEN_MAX;
+ size_t envlen;
+
+ if (strlen(opt) + 1 > OPT_LEN_MAX) {
+ log_err("OPT_LEN_MAX (%d) is too small\n", OPT_LEN_MAX);
+ return NULL;
+ }
+
+ in[OPT_LEN_MAX] = '\0';
+ strncpy(in, opt, OPT_LEN_MAX);
+
+ while (*inptr && nchr > 0) {
+ if (inptr[0] == '$' && inptr[1] == '{') {
+ ch2 = strchr(inptr, '}');
+ if (ch2 && inptr+1 < ch2) {
+ ch1 = inptr+2;
+ inptr = ch2+1;
+ *ch2 = '\0';
+
+ env = getenv(ch1);
+ if (env) {
+ envlen = strlen(env);
+ if (envlen <= nchr) {
+ memcpy(outptr, env, envlen);
+ outptr += envlen;
+ nchr -= envlen;
+ }
+ }
+
+ continue;
+ }
+ }
+
+ *outptr++ = *inptr++;
+ --nchr;
+ }
+
+ *outptr = '\0';
+ return strdup(out);
}
/*
@@ -2277,6 +2329,7 @@
{
char *s;
int i;
+ int docalc = 0;
for (i = 0; fio_keywords[i].word != NULL; i++) {
struct fio_keyword *kw = &fio_keywords[i];
@@ -2306,29 +2359,51 @@
* replace opt and free the old opt
*/
opt = new;
- //free(o_org);
+ free(o_org);
- /*
- * Check for potential math and invoke bc, if possible
- */
- opt = bc_calc(opt);
+ docalc = 1;
}
}
+ /*
+ * Check for potential math and invoke bc, if possible
+ */
+ if (docalc)
+ opt = bc_calc(opt);
+
return opt;
}
+static char **dup_and_sub_options(char **opts, int num_opts)
+{
+ int i;
+ char **opts_copy = malloc(num_opts * sizeof(*opts));
+ for (i = 0; i < num_opts; i++) {
+ opts_copy[i] = option_dup_subs(opts[i]);
+ if (!opts_copy[i])
+ continue;
+ opts_copy[i] = fio_keyword_replace(opts_copy[i]);
+ }
+ return opts_copy;
+}
+
int fio_options_parse(struct thread_data *td, char **opts, int num_opts)
{
int i, ret;
+ char **opts_copy;
sort_options(opts, options, num_opts);
+ opts_copy = dup_and_sub_options(opts, num_opts);
for (ret = 0, i = 0; i < num_opts; i++) {
- opts[i] = fio_keyword_replace(opts[i]);
- ret |= parse_option(opts[i], options, td);
+ ret |= parse_option(opts_copy[i], opts[i], options, td);
+
+ if (opts_copy[i])
+ free(opts_copy[i]);
+ opts_copy[i] = NULL;
}
+ free(opts_copy);
return ret;
}
diff --git a/parse.c b/parse.c
index 18e8a40..7f7851c 100644
--- a/parse.c
+++ b/parse.c
@@ -199,9 +199,9 @@
if (len < 2)
return __get_mult_bytes(str, data, percent);
- /*
- * Go forward until we hit a non-digit, or +/- sign
- */
+ /*
+ * Go forward until we hit a non-digit, or +/- sign
+ */
while ((p - str) <= len) {
if (!isdigit((int) *p) &&
(((*p != '+') && (*p != '-')) || digit_seen))
@@ -798,83 +798,28 @@
return 1;
}
-/*
- * Return a copy of the input string with substrings of the form ${VARNAME}
- * substituted with the value of the environment variable VARNAME. The
- * substitution always occurs, even if VARNAME is empty or the corresponding
- * environment variable undefined.
- */
-static char *option_dup_subs(const char *opt)
-{
- char out[OPT_LEN_MAX+1];
- char in[OPT_LEN_MAX+1];
- char *outptr = out;
- char *inptr = in;
- char *ch1, *ch2, *env;
- ssize_t nchr = OPT_LEN_MAX;
- size_t envlen;
-
- if (strlen(opt) + 1 > OPT_LEN_MAX) {
- log_err("OPT_LEN_MAX (%d) is too small\n", OPT_LEN_MAX);
- return NULL;
- }
-
- in[OPT_LEN_MAX] = '\0';
- strncpy(in, opt, OPT_LEN_MAX);
-
- while (*inptr && nchr > 0) {
- if (inptr[0] == '$' && inptr[1] == '{') {
- ch2 = strchr(inptr, '}');
- if (ch2 && inptr+1 < ch2) {
- ch1 = inptr+2;
- inptr = ch2+1;
- *ch2 = '\0';
-
- env = getenv(ch1);
- if (env) {
- envlen = strlen(env);
- if (envlen <= nchr) {
- memcpy(outptr, env, envlen);
- outptr += envlen;
- nchr -= envlen;
- }
- }
-
- continue;
- }
- }
-
- *outptr++ = *inptr++;
- --nchr;
- }
-
- *outptr = '\0';
- return strdup(out);
-}
-
-int parse_option(const char *opt, struct fio_option *options, void *data)
+int parse_option(const char *opt, const char *input,
+ struct fio_option *options, void *data)
{
struct fio_option *o;
- char *post, *tmp;
+ char *post;
- tmp = option_dup_subs(opt);
- if (!tmp)
+ if (!opt) {
+ log_err("fio: failed parsing %s\n", input);
return 1;
+ }
- o = get_option(tmp, options, &post);
+ o = get_option(opt, options, &post);
if (!o) {
- log_err("Bad option <%s>\n", tmp);
- free(tmp);
+ log_err("Bad option <%s>\n", input);
return 1;
}
if (!handle_option(o, post, data)) {
- free(tmp);
return 0;
}
- log_err("fio: failed parsing %s\n", opt);
- free(tmp);
+ log_err("fio: failed parsing %s\n", input);
return 1;
}
diff --git a/parse.h b/parse.h
index cb0b48f..091923e 100644
--- a/parse.h
+++ b/parse.h
@@ -65,7 +65,7 @@
typedef int (str_cb_fn)(void *, char *);
-extern int parse_option(const char *, struct fio_option *, void *);
+extern int parse_option(const char *, const char *, struct fio_option *, void *);
extern void sort_options(char **, struct fio_option *, int);
extern int parse_cmd_option(const char *t, const char *l, struct fio_option *, void *);
extern int show_cmd_help(struct fio_option *, const char *);