linux-kernel - Re: [PATCH v2 2/2] lib.c: skip --param parameters

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANeU7QnLad1f4KkLpTGSWB8eD8_La-tACim6rR7DLQ=hggwhOQ@mail.gmail.com>
Date:	Sat, 28 Jun 2014 09:59:58 -0700
From:	Christopher Li <sparse@...isli.org>
To:	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:	Josh Triplett <josh@...htriplett.org>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	Linux-Sparse <linux-sparse@...r.kernel.org>
Subject: Re: [PATCH v2 2/2] lib.c: skip --param parameters

Oops, I just click send before I type up the reply. Here we go again.

On Tue, Jun 17, 2014 at 2:11 AM, Andy Shevchenko
<andriy.shevchenko@...ux.intel.com> wrote:
> Very dumb patch to just skip --param allow-store-data-races=0 introduced in
> newer GCC versions.
>
> +static char **handle_param(char *arg, char **next)
> +{
> +       const char *value = NULL;
> +
> +       /* For now just skip any '--param=*' or '--param *' */
> +       value = split_value_from_arg(arg, value);
> +       if (!value)
> +               ++next;
> +
> +       return ++next;
> +}

I think this is problematic.There are three possible input
from args:
1) "--parm", you need to ++next skip to next arg, which is the value for parm.
2) "--parm=x",  you don't need to skip to next arg.
3) "--parm-with-crap", invalid argument. You don't need to skip next arg.

I think the patch is wrong on case 2) and case 3).
In case 2), the patch skip two arguments and make next point
points to out of bound memory.

The split_value_from_arg function is not a good abstraction for this job.
Its return value can only indicate 2 possible out come.
Also, returning the default value force the test against the input
default value. That make the logic a bit complicate.

>  struct switches {
>         const char *name;
>         char **(*fn)(char *, char **);
> @@ -686,13 +698,14 @@ struct switches {
>  static char **handle_long_options(char *arg, char **next)
>  {
>         static struct switches cmd[] = {
> +               { "param", handle_param },
>                 { "version", handle_version },
>                 { NULL, NULL }
>         };
>         struct switches *s = cmd;
>
>         while (s->name) {
> -               if (!strcmp(s->name, arg))
> +               if (!strncmp(arg, s->name, strlen(s->name)))

This will allow "--version-with-crap" as valid arguments.

I think we can have one extra member in "struct switch"
to indicate this option is a prefix rather than a whole word.
For "parm", it need to set that prefix member to non zero.

Please let me know if there is a V3 coming.

Chris
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/