re: cifs: use standard token parser for mount options

re: cifs: use standard token parser for mount options

[Dan Carpenter]

Hello Sachin Prabhu,

This is a semi-automatic email about new static checker warnings.

The patch 8830d7e07a5e: "cifs: use standard token parser for mount 
options" from Mar 23, 2012, leads to the following Smatch complaint:

fs/cifs/connect.c:1607 cifs_parse_mount_options()
	 error: we previously assumed 'value' could be null (see line 1568)

fs/cifs/connect.c
  1567				value = strchr(data, '=');
  1568				if (value != NULL)
                                    ^^^^^^^^^^^^^
Check.

  1569					*value++ = '\0';
  1570	
  1571				/* Set tmp_end to end of the string */
  1572				tmp_end = (char *) value + strlen(value);
                                                                  ^^^^^
We can't run strlen() on NULL pointers.

  1573	
  1574				/* Check if following character is the deliminator

regards,
dan carpenter

re: cifs: use standard token parser for mount options

[Sachin Prabhu]

Hello Dan, 

You are right.

On Wed, 2012-03-28 at 13:45 +0300, Dan Carpenter wrote:
> Hello Sachin Prabhu,
> 
> This is a semi-automatic email about new static checker warnings.
> 
> The patch 8830d7e07a5e: "cifs: use standard token parser for mount 
> options" from Mar 23, 2012, leads to the following Smatch complaint:
> 
> fs/cifs/connect.c:1607 cifs_parse_mount_options()
> 	 error: we previously assumed 'value' could be null (see line 1568)
> 
> fs/cifs/connect.c
>   1567				value = strchr(data, '=');
>   1568				if (value != NULL)
>                                     ^^^^^^^^^^^^^
> Check.

static const match_table_t cifs_mount_option_tokens = {
..
        { Opt_pass, "pass=%s" },
        { Opt_pass, "password=%s" },
..
^^^ The parser will only match Opt_pass if the token passed contains a
'=' character so value cannot be NULL. That check for NULL is
unnecessary and shouldn't be there. This is otherwise harmless.

This is an artifact of the old code which was backported. Opt_pass
needed special handling to allow it to process password with ','
characters in them. This bit of code was at top of the while loop and
was used to parse the values passed with the mount option. This was set
to point to the value in case the mount option was passed with a value
or set to NULL in case it was parsing mount options which didn't pass
any values. 
In this case, the Opt_pass will be called only when it contains a value.
The case where a blank password was set is handled separately with
Opt_blank_pass.

> 
>   1569					*value++ = '\0';
>   1570	
>   1571				/* Set tmp_end to end of the string */
>   1572				tmp_end = (char *) value + strlen(value);
>                                                                   ^^^^^
> We can't run strlen() on NULL pointers.
> 
>   1573	
>   1574				/* Check if following character is the deliminator
> 
> regards,
> dan carpenter

Sachin Prabhu

Re: cifs: use standard token parser for mount options

[Dan Carpenter]

On Wed, Mar 28, 2012 at 12:13:01PM +0100, Sachin Prabhu wrote:
> Hello Dan, 
> 
> You are right.
> 
> On Wed, 2012-03-28 at 13:45 +0300, Dan Carpenter wrote:
> > Hello Sachin Prabhu,
> > 
> > This is a semi-automatic email about new static checker warnings.
> > 
> > The patch 8830d7e07a5e: "cifs: use standard token parser for mount 
> > options" from Mar 23, 2012, leads to the following Smatch complaint:
> > 
> > fs/cifs/connect.c:1607 cifs_parse_mount_options()
> > 	 error: we previously assumed 'value' could be null (see line 1568)
> > 
> > fs/cifs/connect.c
> >   1567				value = strchr(data, '=');
> >   1568				if (value != NULL)
> >                                     ^^^^^^^^^^^^^
> > Check.
> 
> static const match_table_t cifs_mount_option_tokens = {
> ..
>         { Opt_pass, "pass=%s" },
>         { Opt_pass, "password=%s" },
> ..
> ^^^ The parser will only match Opt_pass if the token passed contains a
> '=' character so value cannot be NULL. That check for NULL is
> unnecessary and shouldn't be there. This is otherwise harmless.
> 

Great.  Could you send a patch to remove the unneeded check?  Please
give me the Reported-by: tag, I am trying to catch up to Randy.

regards,
dan carpenter

[PATCH] Remove unnecessary check for NULL in password parser

[Sachin Prabhu]

The password parser has an unnecessary check for a NULL value which
triggers warnings in source checking tools. The code contains artifacts
from the old parsing code which are no longer required.

Signed-off-by: Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/connect.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 302a15c..0511fdb 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1565,8 +1565,7 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
 
 			/* Obtain the value string */
 			value = strchr(data, '=');
-			if (value != NULL)
-				*value++ = '\0';
+			value++;
 
 			/* Set tmp_end to end of the string */
 			tmp_end = (char *) value + strlen(value);
-- 
1.7.7.6

Re: [PATCH] Remove unnecessary check for NULL in password parser

[Jeff Layton]

On Wed, 28 Mar 2012 18:07:08 +0100
Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:

> The password parser has an unnecessary check for a NULL value which
> triggers warnings in source checking tools. The code contains artifacts
> from the old parsing code which are no longer required.
> 
> Signed-off-by: Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/connect.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 302a15c..0511fdb 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -1565,8 +1565,7 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
>  
>  			/* Obtain the value string */
>  			value = strchr(data, '=');
> -			if (value != NULL)
> -				*value++ = '\0';
> +			value++;
>  
>  			/* Set tmp_end to end of the string */
>  			tmp_end = (char *) value + strlen(value);

Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>