On Sat, 28 Oct 2017 10:07:39 +0200
Martin Pieuchot <m...@openbsd.org> wrote:
> On 17/10/17(Tue) 14:26, Helg Bredow wrote:
> > [...]
> > I've split the patch. This one improves argument and option parsing so that
> > almost all sshfs arguments and options will now parse. It won't recognise
> > -ocache=no since fuse_opt_match() is incorrect (fuse will treat it the same
> > as -ocache=yes). I'll submit a patch for that some other time.
>
> Nice.
>
> > Running check_sym tells me:
> >
> > No dynamic export changes
> > External reference changes:
> > added:
> > atoi
> > strchr
> > strsep
> > strstr
> > strtoul
> >
> > I take that to mean that there is no need to bump the major or minor
> > version for this patch. Is that correct?
> >
>
> That's correct.
>
> Regarding fuse options, I'd suggest writing more tests and fuzzing this
> code. Complex argument parsing is hard to get right.
>
I've tried fuzzing this logic using AFL, indirectly by calling
fuse_parse_cmdline() but I'm not sure it's that useful. It'll just end up
supplying nonsensical arguments that never match and won't execute much of the
other code paths. I've tested this pretty exhaustively and am confident that
it's as stable as I can make it. I'll post the regression tests that I've been
using in a separate email. All the changes that I'm making to fuse are a result
of testing and not auditing the code.
I agree that this is hard to get right and it took me a few attempts to
simplify. Your comments did prompt me to try a few more tests and I've found
and fixed another edge case. It now prints a useful message if you don't supply
a value to an argument that expects a value. e.g. "sshfs -p".
> Comments inline.
Responses and updated patch below.
>
> > Index: fuse_opt.c
[...]
> > - keyval = 0;
> > +#define DISCARD 0
> > +#define KEEP 1
> > +#define NEED_ANOTHER_ARG 2
>
> I'd prefix these defines with _FOPT or w/o underscore if they are part
> of the API and move them on the top of the file since you use them in
> multiple functions.
>
Done.
> >
> > - /* check if it is a key=value entry */
> > - idx = strcspn(val, "=");
> > - if (idx != strlen(val)) {
> > - idx++;
> > - keyval = 1;
> > - }
> > + if (o == NULL)
> > + return KEEP;
>
> I know it's note your code, but one-letter variable should be avoided.
> It's hard to understand what they represent. But let's not clutter this
> diff :)
>
Agreed. I've been careful not to tidy the code at the same time.
> > +
> > + keyval = 0;
> >
> > for(; o->templ; o++) {
> > - if ((keyval && strncmp(val, o->templ, idx) == 0) ||
> > - (!keyval && strcmp(val, o->templ) == 0)) {
> > + /* check key=value or -p n */
> > + idx = strcspn(o->templ, "= ");
> > +
> > + if (strncmp(val, o->templ, idx) == 0) {
> > + if (o->templ[idx] == '=') {
> > + keyval = 1;
> > + val = &val[idx + 1];
>
> How can you be sure that val[idx + 1] is valid? For example:
>
> o->temp = { 'k', 'e', 'y', '=', 'v', 'a', 'l', '\0' }
> val = { 'k', '\0' }
This example is not possible. The previous strncmp() test guarantees that both
0->templ and val are identical up to either a '=' or ' '. val[idx + 1] will at
worst equal '\0'.
o->temp = { 'k', 'e', 'y', '=', 'v', 'a', 'l', '\0' }
val = { 'k', 'e', 'y', '=', '\0' }
>
> This looks like an overflow to me. Well not yet, but when you
> dereference `val' below.
>
> > + } else if (o->templ[idx] == ' ') {
> > + keyval = 1;
> > + if (idx == strlen(val)) {
> > + /* ask for next arg to be included */
> > + return NEED_ANOTHER_ARG;
> > + } else if (strchr(o->templ, '%') != NULL) {
> > + val = &val[idx];
>
> Same here, how can you be sure that val[idx] is valid?
Worst case it will be '\0'. Even by supplying a matching 'opt=' or '-p' I can't
get it to crash.
>
> > + }
> > + }
> > +
> > if (o->val == FUSE_OPT_KEY_DISCARD)
> > - return (1);
> > + return (DISCARD);
> > +
> > + if (o->val == FUSE_OPT_KEY_KEEP)
> > + return (KEEP);
> >
> > if (FUSE_OPT_IS_OPT_KEY(o)) {
> > - if (keyval)
> > - ret = f(data, &val[idx], o->val, arg);
> > - else
> > - ret = f(data, val, o->val, arg);
> > - }
> > + if (f == NULL)
> > + return KEEP;
> >
> > - if (o->off != ULONG_MAX && data && o->val >= 0) {
> > - ret = f(data, val, o->val, arg);
> > - int *addr = (int *)(data + o->off);
> > - *addr = o->val;
> > - ret = 0;
> > + return f(data, val, o->val, arg);
> > + } else if (data == NULL) {
> > + return -1;
> > + } else if (strchr(o->templ, '%') == NULL) {
> > + *((int *)(data + o->off)) = o->val;
>
> Are you sure you can simply deference "data + o->off" w/o sanity check?
I don't know what sanity checks we can apply here. We are relying on the fuse
file system to have supplied the correct offset into the struct. What's the
worst that can happen? A badly written file system crashes? I'm happy to
receive advice on how to make this more robust.
>
> > + } else if (strstr(o->templ, "%u") != NULL) {
> > + *((unsigned int*)(data + o->off)) = atoi(val);
> > + } else if (strstr(o->templ, "%lu") != NULL) {
> > + *((unsigned long*)(data + o->off)) =
> > + strtoul(val, NULL, 0);
> > + } else if (strstr(o->templ, "%s") != NULL) {
> > + *((char **)(data + o->off)) = strdup(val);
>
> Same here.
[...]
Index: fuse_opt.c
===================================================================
RCS file: /cvs/src/lib/libfuse/fuse_opt.c,v
retrieving revision 1.18
diff -u -p -u -p -r1.18 fuse_opt.c
--- fuse_opt.c 4 Jan 2017 12:01:22 -0000 1.18
+++ fuse_opt.c 29 Oct 2017 09:13:04 -0000
@@ -25,6 +25,10 @@
#include "fuse_opt.h"
#include "fuse_private.h"
+#define IFUSE_OPT_DISCARD 0
+#define IFUSE_OPT_KEEP 1
+#define IFUSE_OPT_NEED_ANOTHER_ARG 2
+
static void
free_argv(char **argv, int argc)
{
@@ -222,57 +226,66 @@ static int
parse_opt(const struct fuse_opt *o, const char *val, void *data,
fuse_opt_proc_t f, struct fuse_args *arg)
{
- int found, ret, keyval;
+ int keyval;
size_t idx;
- ret = 0;
- found = 0;
- keyval = 0;
+ if (o == NULL)
+ return IFUSE_OPT_KEEP;
- /* check if it is a key=value entry */
- idx = strcspn(val, "=");
- if (idx != strlen(val)) {
- idx++;
- keyval = 1;
- }
+ keyval = 0;
for(; o->templ; o++) {
- if ((keyval && strncmp(val, o->templ, idx) == 0) ||
- (!keyval && strcmp(val, o->templ) == 0)) {
+ /* check key=value or -p n */
+ idx = strcspn(o->templ, "= ");
+
+ if (strncmp(val, o->templ, idx) == 0) {
+ if (o->templ[idx] == '=') {
+ keyval = 1;
+ val = &val[idx + 1];
+ } else if (o->templ[idx] == ' ') {
+ keyval = 1;
+ if (idx == strlen(val)) {
+ /* ask for next arg to be included */
+ return IFUSE_OPT_NEED_ANOTHER_ARG;
+ } else if (strchr(o->templ, '%') != NULL) {
+ val = &val[idx];
+ }
+ }
+
if (o->val == FUSE_OPT_KEY_DISCARD)
- return (1);
+ return (IFUSE_OPT_DISCARD);
+
+ if (o->val == FUSE_OPT_KEY_KEEP)
+ return (IFUSE_OPT_KEEP);
if (FUSE_OPT_IS_OPT_KEY(o)) {
- if (keyval)
- ret = f(data, &val[idx], o->val, arg);
- else
- ret = f(data, val, o->val, arg);
- }
+ if (f == NULL)
+ return IFUSE_OPT_KEEP;
- if (o->off != ULONG_MAX && data && o->val >= 0) {
- ret = f(data, val, o->val, arg);
- int *addr = (int *)(data + o->off);
- *addr = o->val;
- ret = 0;
+ return f(data, val, o->val, arg);
+ } else if (data == NULL) {
+ return -1;
+ } else if (strchr(o->templ, '%') == NULL) {
+ *((int *)(data + o->off)) = o->val;
+ } else if (strstr(o->templ, "%u") != NULL) {
+ *((unsigned int*)(data + o->off)) = atoi(val);
+ } else if (strstr(o->templ, "%lu") != NULL) {
+ *((unsigned long*)(data + o->off)) =
+ strtoul(val, NULL, 0);
+ } else if (strstr(o->templ, "%s") != NULL) {
+ *((char **)(data + o->off)) = strdup(val);
+ } else {
+ /* TODO other parameterised templates */
}
- if (ret == -1)
- return (ret);
- if (ret == 1)
- fuse_opt_add_arg(arg, val);
- found++;
- break;
+ return IFUSE_OPT_DISCARD;
}
}
- if (!found) {
- ret = f(data, val, FUSE_OPT_KEY_OPT, arg);
- if (ret == 1)
- fuse_opt_add_arg(arg, val);
- return (ret);
- }
+ if (f != NULL)
+ return f(data, val, FUSE_OPT_KEY_OPT, arg);
- return (ret);
+ return (IFUSE_OPT_KEEP);
}
/*
@@ -287,11 +300,12 @@ fuse_opt_parse(struct fuse_args *args, v
const struct fuse_opt *opt, fuse_opt_proc_t f)
{
struct fuse_args outargs;
- const char *arg;
- int ret = 0;
+ const char *arg, *ap;
+ char *optlist, *tofree;
+ int ret;
int i;
- if (!f || !args || !args->argc || !args->argv)
+ if (!args || !args->argc || !args->argv)
return (0);
bzero(&outargs, sizeof(outargs));
@@ -299,28 +313,68 @@ fuse_opt_parse(struct fuse_args *args, v
for (i = 1; i < args->argc; i++) {
arg = args->argv[i];
+ ret = 0;
/* not - and not -- */
if (arg[0] != '-') {
- ret = f(data, arg, FUSE_OPT_KEY_NONOPT, &outargs);
+ if (f == NULL)
+ ret = IFUSE_OPT_KEEP;
+ else
+ ret = f(data, arg, FUSE_OPT_KEY_NONOPT,
&outargs);
- if (ret == 1)
+ if (ret == IFUSE_OPT_KEEP)
fuse_opt_add_arg(&outargs, arg);
if (ret == -1)
goto err;
} else if (arg[1] == 'o') {
if (arg[2])
arg += 2; /* -ofoo,bar */
- else
- arg = args->argv[++i];
+ else {
+ if (++i >= args->argc)
+ goto err;
- ret = parse_opt(opt, arg, data, f, &outargs);
+ arg = args->argv[i];
+ }
+
+ tofree = optlist = strdup(arg);
+ if (optlist == NULL)
+ goto err;
+
+ while ((ap = strsep(&optlist, ",")) != NULL &&
+ ret != -1) {
+ ret = parse_opt(opt, ap, data, f, &outargs);
+ if (ret == IFUSE_OPT_KEEP) {
+ fuse_opt_add_arg(&outargs, "-o");
+ fuse_opt_add_arg(&outargs, ap);
+ }
+ }
+
+ free(tofree);
if (ret == -1)
goto err;
} else {
ret = parse_opt(opt, arg, data, f, &outargs);
+ if (ret == IFUSE_OPT_KEEP)
+ fuse_opt_add_arg(&outargs, arg);
+ else if (ret == IFUSE_OPT_NEED_ANOTHER_ARG) {
+ /* arg needs a value */
+ if (++i >= args->argc) {
+ fprintf(stderr, "fuse: missing argument
after %s\n", arg);
+ goto err;
+ }
+
+ if (asprintf(&tofree, "%s%s", arg,
+ args->argv[i]) == -1)
+ goto err;
+
+ ret = parse_opt(opt, tofree, data, f, &outargs);
+ if (ret == IFUSE_OPT_KEEP)
+ fuse_opt_add_arg(&outargs, tofree);
+ free(tofree);
+ }
+
if (ret == -1)
goto err;
}
@@ -333,6 +387,8 @@ err:
args->allocated = outargs.allocated;
args->argc = outargs.argc;
args->argv = outargs.argv;
+ if (ret != 0)
+ ret = -1;
return (ret);
}
