os.path.ismount doesn't work for mounts the user doesn't have permission to see

[rossburton]

BPO	2466
Nosy	@loewis, @pitrou, @rossburton, @bitdancer, @berkerpeksag, @serhiy-storchaka, @zhangyangyu
Files	ismount-permission.patch test_fix_ismount_directory_not_readable.patch: patch for issue 2466 including testcase minimal_fix_ismount_directory_not_readable.patch issue2466_ismount_py2.7_port.patch bot_failure_fix.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2016-08-24.13:01:53.236>
created_at = <Date 2008-03-23.17:30:27.631>
labels = ['type-bug', 'library']
title = "os.path.ismount doesn't work for mounts the user doesn't have permission to see"
updated_at = <Date 2016-08-24.13:01:53.235>
user = 'https://github.com/rossburton'

bugs.python.org fields:

activity = <Date 2016-08-24.13:01:53.235>
actor = 'r.david.murray'
assignee = 'none'
closed = True
closed_date = <Date 2016-08-24.13:01:53.236>
closer = 'r.david.murray'
components = ['Library (Lib)']
creation = <Date 2008-03-23.17:30:27.631>
creator = 'rossburton'
dependencies = []
files = ['40641', '41041', '43140', '44145', '44207']
hgrepos = []
issue_num = 2466
keywords = ['patch']
message_count = 26.0
messages = ['64370', '64374', '64382', '71128', '71294', '137401', '222528', '252006', '253732', '253904', '254662', '256646', '263989', '267054', '267085', '267089', '273070', '273071', '273072', '273081', '273481', '273483', '273508', '273531', '273554', '273556']
nosy_count = 11.0
nosy_names = ['loewis', 'pitrou', 'rossburton', 'r.david.murray', 'python-dev', 'drawks', 'berker.peksag', 'serhiy.storchaka', 'pablo.sole', 'xiang.zhang', 'Robin Roth']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue2466'
versions = ['Python 2.7', 'Python 3.5', 'Python 3.6']

[rossburton]

I'm not sure why this is, but ismount doesn't always work for me. It
appears to fail on NTFS mounts.

$ mount
...
/dev/sda1 on /media/windows type ntfs (ro,noexec,nosuid,nodev,user=ross)
redbeard.local:/home on /media/home type nfs
(rw,user=ross,noatime,rsize=65536,wsize=65536,retry=1,nfsvers=3,posix,intr,addr=192.168.1.67)

$ python
Python 2.4.5 (#2, Mar 12 2008, 00:15:51) 
[GCC 4.2.3 (Debian 4.2.3-2)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> ismount("/media/windows")
False
>>> ismount("/media/home")
True

[loewis]

I cannot reproduce that; it works fine for me, with the same Python
version, on Linux 2.6.22.

Can you please debug through ismount, and report which of the calls fail?

The code of ismount reads

def ismount(path):
    """Test whether a path is a mount point"""
    try:
        s1 = os.stat(path)
        s2 = os.stat(join(path, '..'))
    except os.error:
        return False # It doesn't exist -- so not a mount point :-)
    dev1 = s1.st_dev
    dev2 = s2.st_dev
    if dev1 != dev2:
        return True     # path/.. on a different device as path
    ino1 = s1.st_ino
    ino2 = s2.st_ino
    if ino1 == ino2:
        return True     # path/.. is the same i-node as path
    return False

[rossburton]

Aha. The contents of the mount point are only accessible by root:

$ stat /media/windows/..
stat: cannot stat `/media/windows/..': Permission denied

This falls into the except block, so false is returned.

If ismount() used os.path.dirname() instead of appending "..", then this
wouldn't happen.

[pitrou]

If ismount() used os.path.dirname() instead of appending "..", then this
wouldn't happen.

But it may change the function result if the argument is a symlink to
something (directory or mount point) on another filesystem. It should be
verified before making a decision.

[pitrou]

Another problem of using os.path.dirname is that for /A/B, it will
return /A, and if /A is itself a symlink to a mount point itself, /A and
/A/B will have different st_dev values... which will be wrongly
interpreted as meaning that /A/B is a mount point.

So here is a possible course of action for ismount:
1- first test if the arg is a symlink and if so, return False (as it
already does today)
2- then use os.path.realname(os.path.dirname(arg)) instead of joining
with ".."

[drawks]

Confirm this problem exists in 2.7 as well.

[pablosole]

I found another case where the result of ismount() is misleading. I'm using a FUSE-based filesystem controlled by a python supervisor daemon.

When the fuse daemon dies and you try to access the filesystem with os.stat() it returns:
OSError: [Errno 107] Transport endpoint is not connected: '/tmp/fuse-test'. Although, the filesystem is actually mounted and you can verify this:
# cat /proc/self/mountinfo | grep fuse
26 25 0:20 / /tmp/fuse-test rw,nosuid,nodev,relatime - fuse /dev/fuse rw,user_id=1000,group_id=1000,default_permissions,allow_other

If the idea of ismount() is to show what paths are mountpoints, in this case it should return True, even if it's non-accessible (the fuse daemon died in this case, it might also happen for a stale NFS mount *not checked* ).

[RobinRoth]

This bug is still present and annoying.
For me it appeared when running ismount on a nfs-mounted directory, when the user who runs python has no read permission inside the directory. Therefore the lstat on /mntdir/.. fails.
Attached a patch that fixes this by running abspath on the path.

Symlinks will also work because they are checked/excluded before the part of the code where the patch is in.

msg222528 is not related to the original bug and should be opened separately.

[RobinRoth]

Is there any more info needed to the issue or the patch?
It would be really helpful to have this fixed and I don't see any critical changes due to the patch.

There is an issue in ansible depending on this fix: ansible/ansible-modules-core#2186

Also the current development version is affected (I manually checked 2.7, 3.0, 3.5, dev).

[berkerpeksag]

Thanks for the patch, Robin. The patch needs a test case. See Lib/test/test_posixpath.py. The test coverage of ismount() is not high, so we need more tests. Also, I don't have much time to work on this right now, but Antoine's suggestion in msg71294 sounds good to me. Did you try it?

[RobinRoth]

Antoine's suggestion does not work, because "dirname" does not cover enough cases (for example trailing slash, possibly more).

As suggested by him I now use realpath (instead of abspath). I can't come up with a symlink-situation that is broken with the old code, but realpath is what "ismount" actually means.

I also added a testcase that resembles the issue, i.e. it fails with the old code and passes with the fix.

I mock the "Permission denied" by raising a generic OSError. Mocking can not resemble every real-life situation but by simulating all issues reporting and then fixing them, one should get a solid test coverage.

I also took the liberty of minor cleanup in/around the functions changed, i.e. remove unused imports and remove single-use variables to make the code easier to read.

Attached the updated patch.

[RobinRoth]

any comments/updates on merging this?