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.
Comments inline.
> 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 17 Oct 2017 13:57:35 -0000
> @@ -222,57 +222,70 @@ 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;
> +#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.
>
> - /* 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 :)
> +
> + 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 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?
> + }
> + }
> +
> 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?
> + } 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.
> + } else {
> + /* TODO other parameterised templates */
> }
>
> - if (ret == -1)
> - return (ret);
> - if (ret == 1)
> - fuse_opt_add_arg(arg, val);
> - found++;
> - break;
> + return 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 (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,66 @@ 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 = KEEP;
> + else
> + ret = f(data, arg, FUSE_OPT_KEY_NONOPT,
> &outargs);
>
> - if (ret == 1)
> + if (ret == 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 == 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 == KEEP)
> + fuse_opt_add_arg(&outargs, arg);
> + else if (ret == NEED_ANOTHER_ARG) {
> + /* arg needs a value */
> + if (++i >= args->argc)
> + goto err;
> +
> + if (asprintf(&tofree, "%s%s", arg,
> + args->argv[i]) == -1)
> + goto err;
> +
> + ret = parse_opt(opt, tofree, data, f, &outargs);
> + if (ret == KEEP)
> + fuse_opt_add_arg(&outargs, tofree);
> + free(tofree);
> + }
> +
> if (ret == -1)
> goto err;
> }
> @@ -333,6 +385,8 @@ err:
> args->allocated = outargs.allocated;
> args->argc = outargs.argc;
> args->argv = outargs.argv;
> + if (ret != 0)
> + ret = -1;
>
> return (ret);
> }
>
