BUG: chiainit is_drive_mounted() grep pattern matches incorrect targets

[j4s0n]

Problem

This line in chiainit.py causes is_drive_mounted() to incorrectly report drives as being mounted when they are not.

Proof of concept:

I am trying to initialize /dev/sdb, which is not mounted, but /dev/sdba1 is mounted. The grep expressing matches /dev/sdba1, and thus reports that /dev/sdb is mounted when it is not.

$ drive="sdb"
$ if grep -qs "/dev/${drive}[0-9]*" /proc/mounts ; then echo 'FOUND'; fi
FOUND
$ grep -s "/dev/${drive}[0-9]*" /proc/mounts
/dev/sdba1 /mnt/jbod1/jbod1-00014KYV ext4 rw,noatime 0 0

This was on a Debian Linux system, so YMMV if you are using a different grep variant or have configured bash globbing differently.

Explanation

Typically an asterisk is used in bash with paths for globbing. Ie, if we consider ls /dev/sda[0-9]*, it would only list all of the partitions of /dev/sda since the expansion is occuring via bash globbing.

However, since it's the path is part of a grep PATTERN, instead of globbing, the asterisk is being used as part of the REGEX as a repetition specifier. The default REGEX engine used is BRE, which treats the asterisk as: The preceding item will be matched zero or more times. Thus the [0-9]* pattern makes the digit optional (ie, 'match 0 or more times'). It's effectively just looking for any entry in /proc/mounts that contains /dev/$drive.

Solution

To solve this, simply match one or more instead of zero or more:

• via BRE: grep -qs "/dev/${drive}[0-9]\+" /proc/mounts
• via ERE: grep -qsE "/dev/${drive}a[[:digit:]]+"
• via PCRE: grep -qsP "/dev/${drive}\d+" /proc/mounts

However, it's technically possible in Linux to mount a full-disk filesystem without a partition table (mkfs.ext3 /dev/sdb ; mount /dev/sdb /mnt/something). We probably want to also detect this, just in case! So something like this might be more appropriate:

grep -qs "/dev/${drive}[0-9]* " /proc/mounts

Note the addition of the space at the end of the PATTERN. That would only match the expected $drive OR any of it's partitions, but not any other drive whose name is a superset of our drive. And 1-character bug fixes are cool :)

Thanks!

[efnats]

Hey, thank you for your well explained input. I really appreciate.

I had disabled the checking for mounted disks in the later iterations of chiainit. I suspect there are lots of people who are going to try this out on jbods with a gnome or kde preinstalled distribution, where disks are automounted. The checking for mounted disks would then add those disks in the "excluded list" automatically - essentially causing chiainit to do nothing and not giving any error codes. So for that reason the function is currently never called.

After removing checking for mounted disks, wipefs, parted and mkfs will fail anyways on mounted disks - and in this case giving decent error codes. Labeling will not fail, but then its not a destructive action either.

I am not sure if this is good practice, but I intend to leave the function disabled for now.

You do have a very valid point though, that writing filesystems without partition tables is possible. In hindsight I wish I had never included partitionning of the drives in chiainit because I think its somewhat unneccesary. The issue is that gardenmount is picking up on those parititons and if I change the formatting behaviour of chiainit I would have to find a smooth transition in gardenmount for "old" users.
I guess your suggested one character bugfix may be a solution for that problem, too?

I've included your suggested bugfix for the (disabled) function in the latest push. We never know, when we'll use it. Thank you!!

[j4s0n]

Woops, I didn't realize I wasn't on the latest version and you mitigated the bug already! I was on 775516d and just started using the tool yesterday, so bad timing on my part

I'd keep the partitions -- it's more standard and makes the drives more portable. I was most justifying a different appraoch to correcting the (non)issue. The partition entry also double as a nice place to store some metadata (label). Chalk that up as another case of 'if it aint broke, dont fix it' :)

Thanks for all the hard work!