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

2019-04-10 Thread 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.
> 



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

2019-04-10 Thread Bernhard Voelker
On 4/10/19 4:15 AM, 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.

Thanks for the review.
Pushed with all these changes at:
https://git.sv.gnu.org/cgit/gnulib.git/commit/?id=eb8278fefa

For coreutils, I'll push the (attached) gnulib update with a NEWS entry soon,
and then work on tests.

Have a nice day,
Berny

>From b938a1badf672f8168daf71fd751e947877c9fc7 Mon Sep 17 00:00:00 2001
From: Bernhard Voelker 
Date: Wed, 10 Apr 2019 09:13:27 +0200
Subject: [PATCH] gnulib: update to the latest

* gnulib: Update to latest, mainly for:
  > mountlist: make parsing /proc/self/mountinfo more robust
* NEWS: Mention the fix.

Fixes https://bugs.gnu.org/33468
---
 NEWS   | 7 +++
 gnulib | 2 +-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/NEWS b/NEWS
index 6844228be..132f2a0f3 100644
--- a/NEWS
+++ b/NEWS
@@ -4,6 +4,13 @@ GNU coreutils NEWS-*- outline -*-
 
 ** Bug fixes
 
+  df now correctly parses the /proc/self/mountinfo file for unusual entries
+  with '\r' in a field value ("mount -t tmpfs tmpfs /foo$'\r'bar"),
+  when the source field is empty ('mount -t tmpfs "" /mnt'), and when the
+  filesystem type contains characters like blank which need escaping.
+  [bugs introduced in coreutils-8.24 with the introduction of reading
+   the /proc/self/mountinfo file]
+
   factor again outputs immediately when stdout is a tty but stdin is not.
   [bug introduced in coreutils-8.24]
 
diff --git a/gnulib b/gnulib
index 188d87b05..eb8278fef 16
--- a/gnulib
+++ b/gnulib
@@ -1 +1 @@
-Subproject commit 188d87b05190690d6f8b0577ec65ef221a711d08
+Subproject commit eb8278fefa0bbf2a53b706bffb2c99ccfe5d7bd4
-- 
2.21.0