Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FS#2196 - block info confuses mount points #7351

Open
openwrt-bot opened this issue Mar 21, 2019 · 3 comments
Open

FS#2196 - block info confuses mount points #7351

openwrt-bot opened this issue Mar 21, 2019 · 3 comments
Labels

Comments

@openwrt-bot
Copy link

LongHairedHacker:

I've encountered a bug in /sbin/block.

If multiple block devices paths share the same prefix e.g. /dev/mmcblk0p1 and /dev/mmcblk0p10 the commands block info confuses them.
In my case /dev/mmcblk0p1 is a boot partition and /dev/mmcblk0p10 is a data partition.
There are a bunch of other partitions, which I'll omit for the sake clarity in the following example.

/etc/config/fstab:

config 'global'
option anon_swap '0'
option anon_mount '0'
option auto_swap '1'
option auto_mount '1'
option delay_root '5'
option check_fs '0'

config 'mount'
option target '/data'
option device '/dev/mmcblk0p10'
option fstype 'ext4'
option enabled '1'

config 'mount'
option device '/dev/mmcblk0p1'
option target '/uboot'
option fstype 'vfat'
option options 'rw,sync'
option enabled '1'
...

After boot the output of mount looks like this:

root@foobar:~# mount
/dev/root on /rom type squashfs (ro,relatime)
proc on /proc type proc (rw,nosuid,nodev,noexec,noatime)
sysfs on /sys type sysfs (rw,nosuid,nodev,noexec,noatime)
tmpfs on /tmp type tmpfs (rw,nosuid,nodev,noatime)
tmpfs on /dev type tmpfs (rw,nosuid,relatime,size=512k,mode=755)
devpts on /dev/pts type devpts (rw,nosuid,noexec,relatime,mode=600,ptmxmode=000)
debugfs on /sys/kernel/debug type debugfs (rw,noatime)
/dev/mmcblk0p10 on /data type ext4 (rw,relatime,data=ordered)
/dev/mmcblk0p1 on /uboot type vfat (rw,sync,relatime,fmask=0000,dmask=0000,allow_utime=0022,codepage=437,iocharset=iso8859-1,shortname=mixed,errors=remount-ro)
...

At least everything is mounted in the right place.
If I do a block info I get:

root@foobar:~# block info
/dev/mmcblk0p1: UUID="0037-0032" LABEL="" VERSION="FAT16" MOUNT="/data" TYPE="vfat"
/dev/mmcblk0p10: UUID="d8dff841-edc4-4094-ae68-f6f0596c2277" VERSION="1.0" MOUNT="/data" TYPE="ext4"
...

Which is a little bit odd, as /dev/mmcblk0p1 is clearly not mounted under /data.

I did some digging in the sources and found the culprit in [[https://git.openwrt.org/?p=project/fstools.git;a=blob;f=block.c;h=cfc87273ee9969a153a4beededc61a5c6ba40a16;hb=HEAD#l578|block.c:find_mount_point]].
In [[https://git.openwrt.org/?p=project/fstools.git;a=blob;f=block.c;h=cfc87273ee9969a153a4beededc61a5c6ba40a16;hb=HEAD#l646|line 646]] in block.c:

*pos = '\0';
devname = tmp;
if (!strncmp(block, devname, len)) { // <- problem
point = strdup(cpoint);
break;
}

block is the parameter passed to find_mount_point, len is the string length of block and devname is a block device name read from /proc/self/mountinfo.
If the string in block is a prefix of the string in devname, !strncmp will always be true as, therefore the mount point in this line will be returned to caller.
In my case the line for /dev/mmcblk0p10 is encountered first, therefore /data is returned as mountpoint for both /dev/mmcblk0p10 and /dev/mmcblk0p1.

Since the idea here is to look for an exactly equal string I would suggest to compare the string length as well as calling strncmp.
See the attached patch for my temporary solution.

As a last remark: I could not determine if this affects other functionalities of /sbin/block as well,
but since find_mount_point is also called during umounting this might be more than just a display bug.

@openwrt-bot
Copy link
Author

LongHairedHacker:

Since this issue has not be updated, I'll do it myself.

There seem to be similar string matching bugs in other parts of the codebase.
The following two commit contain the necessary changes:

https://git.openwrt.org/?p=project/fstools.git;a=commitdiff;h=3957dd39c6c2413c4341258d63a913948f99ac6f

https://git.openwrt.org/?p=project/fstools.git;a=commitdiff;h=9b36dc25dd31197681dfcbe4e83879a9b885f451

As personal side note: I don't think replacing all strncmps calls with strcmp is the best choice.

@openwrt-bot
Copy link
Author

jow-:

I didn't simply replace all strncmp() calls with strcmp() ones but analyzed the surrounding code context to see whether it makes sense or not.

For the changed places where strncmp() was used, it was not useful, inappropriate or offered no advantage over a simple strcmp().

  • ''strncmp(block, devname, len)'' (with ''len = strlen(block)'') wrongly matches substrings since it does not compare trailing ''\0''
  • ''strncmp(block, devname, len + 1)'' (with ''len = strlen(block)'') is identical to ''strcmp(block, devname)'' but more expensive due to the required ''strlen()'' call
  • ''strncmp(name, volname, strlen(volname) + 1)'' is equivalent to ''strcmp(name, volname)''

I would agree that changing ''strncmp()'' to ''strcmp()'' is a problem when the length argument is not derived from ''strlen()'' but for the cases that have been changed I do not see the problem.

What would have been the best choice in your opinion?

@crass
Copy link

crass commented Nov 6, 2022

Looks like jow- misunderstood LongHairedHacker, the later was saying that the changes from strncmp to strcmp were a good thing. Regardless, it appears as though this bug has been fixed, though I haven't tested it. I suggest this issue be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants