GNU bug report logs - #35137
[df] incorrect parsing of /proc/self/mountinfo with \r in mount path

Previous Next

To reply to this bug, email your comments to 35137 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Zbigniew Jędrzejewski-Szmek <zbyszek <at> in.waw.pl>
To: bug-coreutils <at> gnu.org
Subject: [df] incorrect parsing of /proc/self/mountinfo with \r in mount path
Date: Thu, 4 Apr 2019 07:52:25 +0000

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.

Zbyszek

Message #8 received at 35137 <at> debbugs.gnu.org (full text, mbox):

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: Zbigniew Jędrzejewski-Szmek <zbyszek <at> in.waw.pl>,
 35137 <at> debbugs.gnu.org
Subject: Re: bug#35137: [df] incorrect parsing of /proc/self/mountinfo with \r
 in mount path
Date: Fri, 5 Apr 2019 09:01:00 +0200

[Message part 1 (text/plain, inline)]

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

At least util-linux' findmnt has already worked around that case, see
https://git.kernel.org/pub/scm/utils/util-linux/util-linux.git/commit/?id=18a52a5094f8

Finally, other users of gnulib/lib/mountlist.c are also affected.

Have a nice day,
Berny

[mountlist.diff (text/x-patch, attachment)]

Message #11 received at 35137 <at> debbugs.gnu.org (full text, mbox):

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: Zbigniew Jędrzejewski-Szmek <zbyszek <at> in.waw.pl>,
 35137 <at> debbugs.gnu.org, bug-gnulib <bug-gnulib <at> gnu.org>
Subject: Re: bug#35137: [df] incorrect parsing of /proc/self/mountinfo with \r
 in mount path
Date: Wed, 10 Apr 2019 00:31:35 +0200

[Message part 1 (text/plain, inline)]

[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

[0001-mountlist-make-parsing-proc-self-mountinfo-more-robu.patch (text/x-patch, attachment)]

Message #14 received at 35137 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Bernhard Voelker <mail <at> bernhard-voelker.de>,
 Zbigniew Jędrzejewski-Szmek <zbyszek <at> in.waw.pl>,
 35137 <at> debbugs.gnu.org, bug-gnulib <bug-gnulib <at> gnu.org>
Subject: Re: bug#35137: [df] incorrect parsing of /proc/self/mountinfo with \r
 in mount path
Date: Tue, 9 Apr 2019 19:15:04 -0700

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.

Message #17 received at 35137 <at> debbugs.gnu.org (full text, mbox):

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: Paul Eggert <eggert <at> cs.ucla.edu>,
 Zbigniew Jędrzejewski-Szmek <zbyszek <at> in.waw.pl>,
 35137 <at> debbugs.gnu.org, bug-gnulib <bug-gnulib <at> gnu.org>
Subject: Re: bug#35137: [df] incorrect parsing of /proc/self/mountinfo with \r
 in mount path
Date: Wed, 10 Apr 2019 09:23:59 +0200

[Message part 1 (text/plain, inline)]

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

[0001-gnulib-update-to-the-latest.patch (text/x-patch, attachment)]

Message #20 received at 35137 <at> debbugs.gnu.org (full text, mbox):

From: Zbigniew Jędrzejewski-Szmek <zbyszek <at> in.waw.pl>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: bug-gnulib <bug-gnulib <at> gnu.org>,
 Bernhard Voelker <mail <at> bernhard-voelker.de>, 35137 <at> debbugs.gnu.org
Subject: Re: bug#35137: [df] incorrect parsing of /proc/self/mountinfo with
 \r in mount path
Date: Wed, 10 Apr 2019 07:24:38 +0000

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

Message #23 received at 35137 <at> debbugs.gnu.org (full text, mbox):

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: Paul Eggert <eggert <at> cs.ucla.edu>,
 Zbigniew Jędrzejewski-Szmek <zbyszek <at> in.waw.pl>,
 35137 <at> debbugs.gnu.org
Subject: Re: bug#35137: [df] incorrect parsing of /proc/self/mountinfo with \r
 in mount path
Date: Thu, 18 Apr 2019 08:19:04 +0200

[Message part 1 (text/plain, inline)]

[removing bug-gnulib]

On 4/10/19 9:23 AM, Bernhard Voelker wrote:
> 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.

The tests are probably best done as a new root-test, and therefore need
extra caution.  Meanwhile, I intend to just pull in the gnulib change with
a NEWS entry - patch attached.

Have a nice day,
Berny

[0001-gnulib-update-to-the-latest.patch (text/x-patch, attachment)]

Message #26 received at 35137 <at> debbugs.gnu.org (full text, mbox):

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: Paul Eggert <eggert <at> cs.ucla.edu>,
 Zbigniew Jędrzejewski-Szmek <zbyszek <at> in.waw.pl>,
 35137 <at> debbugs.gnu.org
Subject: Re: bug#35137: [df] incorrect parsing of /proc/self/mountinfo with \r
 in mount path
Date: Fri, 19 Apr 2019 10:56:54 +0200

On 4/18/19 8:19 AM, Bernhard Voelker wrote:
> [...] Meanwhile, I intend to just pull in the gnulib change with
> a NEWS entry - patch attached.

I had to include another later gnulib commit:
  https://git.sv.gnu.org/cgit/gnulib.git/commit/?id=22b911f63ca1

Pushed for coreutils at:
  https://git.sv.gnu.org/cgit/coreutils.git/commit/?id=7d8adb20f6bb

Have a nice day,
Berny

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.