Re: bug#35137: [df] incorrect parsing of /proc/self/mountinfo with \r in mount path

[Zbigniew Jędrzejewski-Szmek]

Hi,

I don't know this codebase, so can't comment on the patch, but the
same bug in util-linux was solved by ditching scanf.

https://github.com/karelzak/util-linux/commit/e902609400a861dbdb47d5c3eb98b951530bf01d
https://github.com/karelzak/util-linux/commit/e3782bf6776dcef329b09f4324e1be680f690f3c

Zbyszek


On Tue, Apr 09, 2019 at 07:15:04PM -0700, Paul Eggert wrote:
> Bernhard Voelker wrote:
> 
> +/* Find the next white space in STR, terminate the string there in place,
> +   and return that position.  Otherwise return NULL.  */
> +
> +static char *
> +terminate_at_blank (char const *str)
> +{
> +  char *s = NULL;
> +  if ((s = strchr (str, ' ')) != NULL)
> +    *s = '\0';
> +  return s;
> +}
> 
> Since the function modifies its argument, the argument type should
> be char *, not char const *. Also, the code has an assignment in an
> 'if' conditional and the comment is not quite right. Better is:
> 
>   /* Find the next space in STR, terminate the string there in place,
>      and return that position.  Otherwise return NULL.  */
> 
>   static char *
>   terminate_at_blank (char *str)
>   {
>     char *s = strchr (str, ' ');
>     if (s)
>       *s = '\0';
>     return s;
>   }
> 
> >+            if (! (blank = terminate_at_blank (mntroot)))
> >+              continue;
> 
> Avoid assignments in 'if' conditionals. Better is:
> 
>     blank = terminate_at_blank (target);
>     if (! blank)
>       continue;
> 
> +            if (*source == ' ')
> +              {
> +                /* The source is an empty string, which is e.g. allowed for
> +                   tmpfs: "mount -t tmpfs '' /mnt".  */
> +                *source = '\0';
> +              }
> +            else
> +              {
> +                if (! (blank = terminate_at_blank (source)))
> +                  continue;
> +              }
> 
> Since 'blank' is not used later, surely these 11 lines of code can
> be simplified to 2 lines:
> 
>     if (! terminate_at_blank (source))
>       continue;
> 
> >+            int mntroot_s;
> >+            char *mntroot, *blank, *target, *dash, *fstype, *source;
> 
> I suggest using C99-style declaration-after-statement style rather
> than this old-fashioned C89-style declarations-at-start-of-block
> style, just for the changed part of the code anyway.
>