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

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

[Bernhard Voelker]

[-- Attachment #1: Type: text/plain, Size: 1583 bytes --]

[adding gnulib, coming from https://bugs.gnu.org/35137]

On 4/5/19 9:01 AM, Bernhard Voelker wrote:
> On 4/4/19 9:52 AM, Zbigniew Jędrzejewski-Szmek wrote:
>> See https://github.com/systemd/systemd/issues/12018 and
>> https://github.com/karelzak/util-linux/issues/780 for additional context.
>>
>> $ mkdir "$(echo -e foo\\rbar)"
>> $ sudo mount -t tmpfs tmpfs foo^Mbar/
>> $ cat -v /proc/self/mountinfo|grep foo
>> 865 39 0:59 / /tmp/foo^Mbar rw,relatime shared:462 - tmpfs tmpfs rw,seclabel
>> $ df -h | grep foo
>> $ df -h /tmp/foo$'\r'bar
>> Filesystem      Size  Used Avail Use% Mounted on
>> -               3.9G     0  3.9G   0% /tmp/foo?bar
>>
>> When asked to show all filesystems, the mount point is not shown at all.
>> When asked to show just that one, df parses the mount point correctly,
>> but it gets the filesystem type wrong.
> 
> Thanks for the report.
> 
> I see the issue is not yet solved in util-linux as well.
> For coreutils, the fix is in gnulib.  Parsing the line is starting
> to get ugly ... dirty patch attached.
> 
> This also caters for the issue that df(1) totally skips a file system
> if the source is an empty string which is allowed for e.g. tmpfs:
> 
>   $ mount -t tmpfs '' /mnt
>   $ df -h | grep mnt

The attached is a patch for gnulib to fix the reported and two other issues
when parsing /proc/self/mountinfo.
Any objections?

Meanwhile, I'm working a test for coreutils' df.

FWIW: Karel fixed this issue in util-linux yesterday:
https://git.kernel.org/pub/scm/utils/util-linux/util-linux.git/commit/?id=86673b3a46

Have a nice day,
Berny

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-mountlist-make-parsing-proc-self-mountinfo-more-robu.patch --]
[-- Type: text/x-patch; name="0001-mountlist-make-parsing-proc-self-mountinfo-more-robu.patch", Size: 7413 bytes --]

From 15be8e6bcb6203fdb91d4a01fb65c2d4ef76995e Mon Sep 17 00:00:00 2001
From: Bernhard Voelker <mail@bernhard-voelker.de>
Date: Tue, 9 Apr 2019 22:30:16 +0200
Subject: [PATCH] mountlist: make parsing /proc/self/mountinfo more robust
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Cater for the following issues with mountinfo parsing (the first
one was reported by Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
in <https://bugs.gnu.org/35137>).

1. The fields source, target, mntroot and fstype may contain characters
like '\r'; sscanf(3) fails to read such values with the %s format
specifier because it would stop at such characters.
Example: "mount -t tmpfs tmpfs /foo^Mbar".
The only true separator in that file is the ' ' character.

2. The source field may be an empty string, which happens e.g. with
"mount -t tmpfs '' /target".

3. The fstype field may contain mangled characters as well which need
unescaping.

* lib/mountlist.c (terminate_at_blank): Add utility function.
(read_file_system_list): In the block trying to read the mountinfo file,
avoid using sscanf(3) with %s format; instead, parse the above fields
separated by white spaces one by one.
Handle the case when the source field is an empty string.
Unescape the fstype field.
---
 ChangeLog       | 22 +++++++++++++
 lib/mountlist.c | 83 +++++++++++++++++++++++++++++++------------------
 2 files changed, 74 insertions(+), 31 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 87d95ed99..e049e81a0 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,25 @@
+2019-04-09  Bernhard Voelker  <mail@bernhard-voelker.de>
+
+	mountlist: make parsing /proc/self/mountinfo more robust
+	Cater for the following issues with mountinfo parsing (the first
+	one was reported by Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
+	in <https://bugs.gnu.org/35137>).
+	1. The fields source, target, mntroot and fstype may contain characters
+	like '\r'; sscanf(3) fails to read such values with the %s format
+	specifier because it would stop at such characters.
+	Example: "mount -t tmpfs tmpfs /foo^Mbar".
+	The only true separator in that file is the ' ' character.
+	2. The source field may be an empty string, which happens e.g. with
+	"mount -t tmpfs '' /target".
+	3. The fstype field may contain mangled characters as well which need
+	unescaping.
+	* lib/mountlist.c (terminate_at_blank): Add utility function.
+	(read_file_system_list): In the block trying to read the mountinfo file,
+	avoid using sscanf(3) with %s format; instead, parse the above fields
+	separated by white spaces one by one.
+	Handle the case when the source field is an empty string.
+	Unescape the fstype field.
+
 2019-04-09  Bruno Haible  <bruno@clisp.org>
 
 	openmp: Add workaround for 32-bit programs on AIX.
diff --git a/lib/mountlist.c b/lib/mountlist.c
index 9b54a2cf7..9e01d13f4 100644
--- a/lib/mountlist.c
+++ b/lib/mountlist.c
@@ -418,6 +418,18 @@ unescape_tab (char *str)
         str[j++] = str[i];
     }
 }
+
+/* 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;
+}
 #endif
 
 /* Return a list of the currently mounted file systems, or NULL on error.
@@ -453,56 +465,65 @@ read_file_system_list (bool need_fs_type)
         while (getline (&line, &buf_size, fp) != -1)
           {
             unsigned int devmaj, devmin;
-            int target_s, target_e, type_s, type_e;
-            int source_s, source_e, mntroot_s, mntroot_e;
-            char test;
-            char *dash;
+            int mntroot_s;
+            char *mntroot, *blank, *target, *dash, *fstype, *source;
             int rc;
 
             rc = sscanf(line, "%*u "        /* id - discarded  */
                               "%*u "        /* parent - discarded */
                               "%u:%u "      /* dev major:minor  */
-                              "%n%*s%n "    /* mountroot */
-                              "%n%*s%n"     /* target, start and end  */
-                              "%c",         /* more data...  */
+                              "%n",         /* mountroot */
                               &devmaj, &devmin,
-                              &mntroot_s, &mntroot_e,
-                              &target_s, &target_e,
-                              &test);
+                              &mntroot_s);
+
+            if (rc != 2 && rc != 3)  /* 3 if %n included in count.  */
+              continue;
 
-            if (rc != 3 && rc != 7)  /* 7 if %n included in count.  */
+            /* find end of MNTROOT.  */
+            mntroot = line + mntroot_s;
+            if (! (blank = terminate_at_blank (mntroot)))
+              continue;
+
+            /* find end of TARGET.  */
+            target = blank + 1;
+            if (! (blank = terminate_at_blank (target)))
               continue;
 
             /* skip optional fields, terminated by " - "  */
-            dash = strstr (line + target_e, " - ");
+            dash = strstr (blank + 1, " - ");
             if (! dash)
               continue;
 
-            rc = sscanf(dash, " - "
-                              "%n%*s%n "    /* FS type, start and end  */
-                              "%n%*s%n "    /* source, start and end  */
-                              "%c",         /* more data...  */
-                              &type_s, &type_e,
-                              &source_s, &source_e,
-                              &test);
-            if (rc != 1 && rc != 5)  /* 5 if %n included in count.  */
+            /* advance past the " - " separator.  */
+            fstype = dash + 3;
+            if (! (blank = terminate_at_blank (fstype)))
               continue;
 
+            source = blank + 1;
+            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;
+              }
+
             /* manipulate the sub-strings in place.  */
-            line[mntroot_e] = '\0';
-            line[target_e] = '\0';
-            dash[type_e] = '\0';
-            dash[source_e] = '\0';
-            unescape_tab (dash + source_s);
-            unescape_tab (line + target_s);
-            unescape_tab (line + mntroot_s);
+            unescape_tab (source);
+            unescape_tab (target);
+            unescape_tab (mntroot);
+            unescape_tab (fstype);
 
             me = xmalloc (sizeof *me);
 
-            me->me_devname = xstrdup (dash + source_s);
-            me->me_mountdir = xstrdup (line + target_s);
-            me->me_mntroot = xstrdup (line + mntroot_s);
-            me->me_type = xstrdup (dash + type_s);
+            me->me_devname = xstrdup (source);
+            me->me_mountdir = xstrdup (target);
+            me->me_mntroot = xstrdup (mntroot);
+            me->me_type = xstrdup (fstype);
             me->me_type_malloced = 1;
             me->me_dev = makedev (devmaj, devmin);
             /* we pass "false" for the "Bind" option as that's only
-- 
2.21.0

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

[Paul Eggert]

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

[Bernhard Voelker]

[-- Attachment #1: Type: text/plain, Size: 2231 bytes --]

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


[-- Attachment #2: 0001-gnulib-update-to-the-latest.patch --]
[-- Type: text/x-patch, Size: 1389 bytes --]

From b938a1badf672f8168daf71fd751e947877c9fc7 Mon Sep 17 00:00:00 2001
From: Bernhard Voelker <mail@bernhard-voelker.de>
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 160000
--- a/gnulib
+++ b/gnulib
@@ -1 +1 @@
-Subproject commit 188d87b05190690d6f8b0577ec65ef221a711d08
+Subproject commit eb8278fefa0bbf2a53b706bffb2c99ccfe5d7bd4
-- 
2.21.0

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.
>