+++ This bug was initially created as a clone of Bug #1624591 +++

Description of problem:

If a Filesystem resource's device name or mountpoint directory path contains whitespace, the filesystem fails to mount.

Whitespace in the device name is most likely to occur for CIFS filesystems, since it's more common in Windows directory names than *nix ones.


Cause:

The mount commands in the resource agent don't double-quote their variables. The shell splits variables containing whitespace into multiple strings/tokens instead of treating the variable as a single string.

 528 Filesystem_start()
 529 {
...
 590         # Mount the filesystem.
 591         case "$FSTYPE" in
 592                 none) $MOUNT $options $DEVICE $MOUNTPOINT &&
 593                         bind_mount
 594                         ;;
 595                 "") $MOUNT $options $DEVICE $MOUNTPOINT ;;
 596                 *) $MOUNT -t $FSTYPE $options $DEVICE $MOUNTPOINT ;;
 597         esac

Reference:
  - 5.1. Quoting Variables (http://tldp.org/LDP/abs/html/quotingvar.html)


It would be a good idea to go ahead and proactively double quote the $options variable too, as well as any other occurrences in the agent that we wouldn't want to split on whitespace.


--------------------

Version-Release number of selected component (if applicable):

resource-agents-3.9.5-124.el7.x86_64


--------------------

How reproducible:

Always


--------------------

Steps to Reproduce:

SETUP:

1. Create one directory path with spaces and one path without spaces, which will be shared through Samba. This can be done on your cluster node for testing. Grant ownership to a Samba user, or give the path open permissions.

    # mkdir -p '/srv/samba/dir1/dir2 with spaces/dir3'
    # mkdir -p /srv/samba/dir1/dir2_without_spaces/dir3
    # chmod 2777 -R /srv/samba


2. Create a Samba share in /etc/samba/smb.conf that exports this path near its base.

    # testparm -s
    ...
    # Global parameters
    [global]
    	log file = /var/log/samba/%m.log
    	netbios name = SERVER
    	security = USER
    	workgroup = EXAMPLE-WG
	idmap config * : backend = tdb


    [example]
    	path = /srv/samba
    	read only = No


3. Restart the smb service.

    # systemctl restart smb


4. Create a mountpoint directory containing a space in its name.

    # mkdir "/test mnt"

-----

TEST WHITESPACE IN DEVICE:

1. Create a Filesystem resource with fstype=cifs, using the shared path with spaces as the device attribute.

    # pcs resource create test_fs1 Filesystem device="//127.0.0.1/example/dir1/dir2 with spaces/dir3" directory=/mnt fstype=cifs options='username=testuser,password=<password>' trace_ra=1 meta target-role=Stopped


2. Attempt to debug-start the resource and observe failure due to `mount` syntax error (too many arguments).

    # pcs resource debug-start test_fs1
    Operation start for test_fs1 (ocf:heartbeat:Filesystem) returned: 'unknown error' (1)
    ...
     >  stderr: + 03:21:57: Filesystem_start:596: mount -t cifs -o username=testuser,password=redhat //127.0.0.1/example/dir1/dir2 with spaces/dir3 /mnt
     >  stderr: 
     >  stderr: Usage:
    ...
     >  stderr: + 03:21:57: 1209: exit 1

-----

TEST WHITESPACE IN DIRECTORY:

1. Create a Filesystem resource with fstype=cifs, using the shared path without spaces as the device attribute. This way we can get past the device failure and observe the mountpoint failure. Use the mountpoint directory that contains whitespace in its name.

    # pcs resource create test_fs2 Filesystem device="//127.0.0.1/example/dir1/dir2_without_spaces/dir3" directory="/test mnt" fstype=cifs options='username=testuser,password=<password>' trace_ra=1 meta target-role=Stopped


2. Attempt to debug-start the resource and observe failure due to `mount` syntax error (too many arguments).

    # pcs resource debug-start test_fs2
    Operation start for test_fs2 (ocf:heartbeat:Filesystem) returned: 'unknown error' (1)
    ...
     >  stderr: + 03:33:38: Filesystem_start:596: mount -t cifs -o username=testuser,password=redhat //127.0.0.1/example/dir1/dir2_without_spaces/dir3 /test mnt
     >  stderr: 
     >  stderr: Usage:
    ...
     >  stderr: + 03:33:38: 1209: exit 1


--------------------

Actual results:

    # pcs resource debug-start test_fs1
    Operation start for test_fs1 (ocf:heartbeat:Filesystem) returned: 'unknown error' (1)
    ...
     >  stderr: + 03:21:57: Filesystem_start:596: mount -t cifs -o username=testuser,password=redhat //127.0.0.1/example/dir1/dir2 with spaces/dir3 /mnt
     >  stderr: 
     >  stderr: Usage:
    ...
     >  stderr: + 03:21:57: 1209: exit 1

    # pcs resource debug-start test_fs2
    Operation start for test_fs2 (ocf:heartbeat:Filesystem) returned: 'unknown error' (1)
    ...
     >  stderr: + 03:33:38: Filesystem_start:596: mount -t cifs -o username=testuser,password=redhat //127.0.0.1/example/dir1/dir2_without_spaces/dir3 /test mnt
     >  stderr: 
     >  stderr: Usage:
    ...
     >  stderr: + 03:33:38: 1209: exit 1


--------------------

Expected results (produced by double-quoting the $DEVICE and $MOUNTPOINT variables in the agent):

    # pcs resource debug-start test_fs1
    Operation start for test_fs1 (ocf:heartbeat:Filesystem) returned: 'ok' (0)
    ...
     >  stderr: + 03:35:07: Filesystem_start:596: mount -t cifs -o username=testuser,password=redhat '//127.0.0.1/example/dir1/dir2 with spaces/dir3' /mnt
     >  stderr: + 03:35:07: Filesystem_start:599: '[' 0 -ne 0 ']'
     >  stderr: + 03:35:07: Filesystem_start:606: return 0
     >  stderr: + 03:35:07: 1209: exit 0


    # pcs resource debug-start test_fs2
    Operation start for test_fs2 (ocf:heartbeat:Filesystem) returned: 'ok' (0)
    ...
     >  stderr: + 03:35:36: Filesystem_start:596: mount -t cifs -o username=testuser,password=redhat //127.0.0.1/example/dir1/dir2_without_spaces/dir3 '/test mnt'
     >  stderr: + 03:35:36: Filesystem_start:599: '[' 0 -ne 0 ']'
     >  stderr: + 03:35:36: Filesystem_start:606: return 0
     >  stderr: + 03:35:36: 1209: exit 0

--- Additional comment from Red Hat Bugzilla Rules Engine on 2018-09-02 01:37:50 UTC ---

Since this bug report was entered in Red Hat Bugzilla, the release flag has been set to ? to ensure that it is properly evaluated for this release.

--- Additional comment from Reid Wahl on 2018-09-02 04:22:44 UTC ---

I put the "device" and "directory" issues into the same bug because the causes and fixes are pretty similar. Upon further testing, it looks like the "directory" issue will be a bit trickier to fix.

Let's say we're mounting a filesystem onto "/test mnt". /proc/mounts converts the space character to the octal \040, while the "mount" command does not. The output of `list_mounts` could come in either format.

    # grep /test /proc/mounts
    //127.0.0.1/example/dir1/dir2_without_spaces/dir3 /test\040mnt cifs rw,relatime,vers=1.0,cache=strict,username=testuser,domain=,uid=0,noforceuid,gid=0,noforcegid,addr=127.0.0.1,soft,unix,posixpaths,serverino,mapposix,acl,rsize=1048576,wsize=65536,echo_interval=60,actimeo=1 0 0

    # mount | grep /test
    //127.0.0.1/example/dir1/dir2_without_spaces/dir3 on /test mnt type cifs (rw,relatime,vers=1.0,cache=strict,username=testuser,domain=,uid=0,noforceuid,gid=0,noforcegid,addr=127.0.0.1,soft,unix,posixpaths,serverino,mapposix,acl,rsize=1048576,wsize=65536,echo_interval=60,actimeo=1)


Handling this will require complicating the `list_mounts` parsing, and in the long run will need to be coordinated with Bug 1624203.


-----

We've only received a customer reported issue for a failure due to whitespace in "device". A failure due to whitespace in "directory" is only hypothetical at this point.

--- Additional comment from Reid Wahl on 2018-09-02 07:53:20 UTC ---

In fact... when considering the need to make the `mount` output the same format as the `cat /proc/mounts` output in `list_mounts`, whitespace in EITHER the `device` attribute OR the `directory` attribute skews the field positions.

                $MOUNT | cut -d' ' -f1,3,5

Since `mount` doesn't convert spaces to \040 like `cat /proc/mounts` does, any whitespace would split a device or a mountpoint over multiple fields. Fields 1,3,5 would no longer have the same meaning.

Using a regex to match groups around the " on " and " type " strings (instead of cutting out static field numbers as in the current implementation) would still vulnerable to failure in the edge case that one of those strings is present in the device name or directory path.

So comments 2 and 3 raise two needs:
  1. Ensure we're getting the same fields from the `mount` output as we're getting from /proc/mounts.
  2. Either
     a. Ensure that the `grep " $MOUNTPOINT "`calls in Filesystem_status(), Filesystem_start(), and determine_blockdevice() are updated to match against \040 for spaces (for /proc/mounts), or
     b. Sed the '\040' to ' ' before piping to grep.


This would be much simpler if we were only handling /proc/mounts and /etc/mtab. The `mount` output is not **consistently** parsable in the case of additional whitespace.


I'm going to hold off pending input from @e-ddie. I thought this would be a simple fix :)


Feel free to ping me (@narwahl) if you see me online in EMEA's morning.


-----

Side note, from description:
> It would be a good idea to go ahead and proactively double quote the $options variable too...

This would actually serve no clear purpose that I can see, so nevermind.

--- Additional comment from Reid Wahl on 2018-09-03 06:42:13 UTC ---

Implementing `findmnt -r` instead of `mount` would make output parsing more consistent, provided it is available on all our supported platforms. It may even obsolete the need for using the current "/proc/mounts, else /etc/mtab, else /bin/mount" approach altogether.

I hesitate to make any change to the implementation of list_mounts() but this may be a viable approach for RHEL 7+.

    # findmnt -r | grep /test
    /test\x20mnt //127.0.0.1/example/dir1/dir2_without_spaces/dir3[/dir1/dir2_without_spaces/dir3] cifs rw,relatime,vers=1.0,cache=strict,username=testuser,domain=,uid=0,noforceuid,gid=0,noforcegid,addr=127.0.0.1,soft,unix,posixpaths,serverino,mapposix,acl,rsize=1048576,wsize=65536,echo_interval=60,actimeo=1

--- Additional comment from Reid Wahl on 2018-09-05 06:06:38 UTC ---

Here's the overall direction I'm leaning toward. It's very much subject to change.

The mount(8) man page says:
       The listing and help.
              The listing mode is maintained for backward compatibility only.

              For more robust and definable output use findmnt(8), especially in your scripts. Note that control characters in the mountpoint name are replaced with '?'.


Considering that:
  - `mount` has significant limitations regarding whitespace -- specifically, inability to guarantee which fields contain which data.
  - `list_mounts` relies on the `mount` command if `/proc/mounts` and `/etc/mtab` are nonexistent.
  - `/proc/mounts` and `findmnt` represent space characters differently.
  - `findmnt` has built-in options for a lot of what we're using grep and cut for.
  - The `mount` documentation recommends `findmnt` for scripts.

...using `findmnt` in place of `list_mounts` seems a reasonable approach.

Another option would be to incorporate `findmnt` into the existing `list_mounts` function with generic options and add different methods of whitespace parsing for each approach. It seems that `findmnt` can obsolete all three of the existing approaches... incorporating it into `list_mounts` would just be for the purpose of minimizing total changes.


-----

The initial issue was lack of double-quoting, causing $DEVICE and $MOUNTPOINT to be split on whitespace. This caused the start operation to fail.

The next issue, after fixing start, was monitoring. /proc/mounts and `mount` display spaces differently, and `mount` can't guarantee the fields will be consistent, rendering it unreliable.

The last major issue (that I can remember :) ) was the submount checks.
  - `findmnt` has a built-in utility to find submounts (`findmnt -R -r /mountpoint | sort -r`, where -R prints the submount tree and -r removes the tree formatting and prints in raw mode). It would be more concise than getting a list of all mounts and would render unnecessary a couple of the processing steps.
    - This works fine when /dev/cvg/clv1 is mounted on /mnt and /dev/cvg/clv2 is mounted on /mnt/dir2, and we are trying to unmount /mnt.
    - It does NOT catch if clv2 is mounted on /mnt/dir2 and we are trying to MOUNT clv1 on /mnt (filesystem already mounted under the mountpoint).
    - So I opted for a more generic list_submounts implementation.

  - The `for` loop that iterates over submounts and then the mountpoint itself had to be reimplemented as a while loop. `for` splits on whitespace. To parse the `list_submounts` output when a mountpoint directory contains whitespace, we need to split on newlines only.



It's fine if we settle on a different approach. These are problems encountered ideas on ways to solve them.

--- Additional comment from Reid Wahl on 2018-09-05 06:20:58 UTC ---



--- Additional comment from Reid Wahl on 2020-05-06 04:19:53 UTC ---

While findmnt still seems like the logical approach, I stumbled across something while looking through the legacy rgmanager fs-lib.sh file.

~~~
        # XXX fork/clone warning XXX
        # Mountpoint from /proc/mounts containing spaces will
        # have spaces represented in octal.  printf takes care
        # of this for us.
        tmp_mp="$(printf "$tmp_mp")"
~~~

--- Additional comment from Reid Wahl on 2020-07-11 00:30:57 UTC ---

https://github.com/ClusterLabs/resource-agents/pull/1532

--- Additional comment from Reid Wahl on 2020-07-20 09:54:15 UTC ---

Fixed upstream by commit a8051cf.