classification
Title: os.path.ismount doesn't work for mounts the user doesn't have permission to see
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.6, Python 3.5, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Robin Roth, berker.peksag, drawks, loewis, pablo.sole, pitrou, python-dev, r.david.murray, rossburton, serhiy.storchaka, xiang.zhang
Priority: normal Keywords: patch

Created on 2008-03-23 17:30 by rossburton, last changed 2016-08-24 13:01 by r.david.murray. This issue is now closed.

Files
File name Uploaded Description Edit
ismount-permission.patch Robin Roth, 2015-10-01 08:00 review
test_fix_ismount_directory_not_readable.patch Robin Roth, 2015-11-14 14:26 patch for issue 2466 including testcase review
minimal_fix_ismount_directory_not_readable.patch Robin Roth, 2016-06-03 06:19 review
issue2466_ismount_py2.7_port.patch xiang.zhang, 2016-08-19 05:21 review
bot_failure_fix.patch xiang.zhang, 2016-08-24 04:16 review
Messages (26)
msg64370 - (view) Author: Ross Burton (rossburton) * Date: 2008-03-23 17:30
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
msg64374 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2008-03-23 19:51
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
msg64382 - (view) Author: Ross Burton (rossburton) * Date: 2008-03-23 22:46
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.
msg71128 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-08-14 14:00
> 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.
msg71294 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-08-17 22:19
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 ".."
msg137401 - (view) Author: Dave Rawks (drawks) Date: 2011-05-31 23:21
Confirm this problem exists in 2.7 as well.
msg222528 - (view) Author: Pablo Sole (pablo.sole) Date: 2014-07-07 22:34
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* ).
msg252006 - (view) Author: Robin Roth (Robin Roth) * Date: 2015-10-01 08:00
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.
msg253732 - (view) Author: Robin Roth (Robin Roth) * Date: 2015-10-30 13:11
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: https://github.com/ansible/ansible-modules-core/issues/2186

Also the current development version is affected (I manually checked 2.7, 3.0, 3.5, dev).
msg253904 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2015-11-02 11:09
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?
msg254662 - (view) Author: Robin Roth (Robin Roth) * Date: 2015-11-14 14:26
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.
msg256646 - (view) Author: Robin Roth (Robin Roth) * Date: 2015-12-18 07:33
any comments/updates on merging this?
msg263989 - (view) Author: Robin Roth (Robin Roth) * Date: 2016-04-22 09:16
Any progress on merging this?
The fix is simple, there is a test; anything else I can do to help?

Ansible integrated the patch posted here as a workaround of an issue this caused. So there was some external review of the fix. See
https://github.com/ansible/ansible-modules-core/pull/2737
msg267054 - (view) Author: Robin Roth (Robin Roth) * Date: 2016-06-03 06:19
Based on the review by SilentGhost I removed all refactoring-changes and only left the one line needed for the fix.
msg267085 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-06-03 10:06
What if use os.stat(dirname(path)) instead of os.lstat(parent)?

-    if isinstance(path, bytes):
-        parent = join(path, b'..')
-    else:
-        parent = join(path, '..')
     try:
-        s2 = os.lstat(parent)
+        s2 = os.stat(dirname(path))
     except OSError:
         return False
msg267089 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-06-03 10:41
Ah, forget, this doesn't work with path like "./.". Agreed, using realpath() is the simplest solution.
msg273070 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-08-19 01:35
New changeset cbc78ca21977 by R David Murray in branch '3.5':
#2466: ismount now recognizes mount points user can't access.
https://hg.python.org/cpython/rev/cbc78ca21977

New changeset db9126139969 by R David Murray in branch 'default':
Merge: #2466: ismount now recognizes mount points user can't access.
https://hg.python.org/cpython/rev/db9126139969
msg273071 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2016-08-19 01:36
Thanks, Robin.

The patch doesn't apply on 2.7.  I'll leave this open to see if anyone wants to do the 2.7 port.
msg273072 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-08-19 01:45
New changeset dd0f1fa809c5 by R David Murray in branch 'default':
Rewrap long lines in Misc/NEWS.
https://hg.python.org/cpython/rev/dd0f1fa809c5
msg273081 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-08-19 05:21
David, issue2466_ismount_py2.7_port.patch does the py2.7 port. I also back port other ismount test cases not existing in py2.7.
msg273481 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-08-23 16:30
New changeset 75111791110b by R David Murray in branch '2.7':
# 2466: ismount now recognizes mount points user can't access.
https://hg.python.org/cpython/rev/75111791110b
msg273483 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2016-08-23 16:32
Thanks, Xiang.
msg273508 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2016-08-23 19:00
This is causing a test failure on the windows 8 bot.  Can you fix it Xiang?  Otherwise I'll have to revert.

http://buildbot.python.org/all/builders/AMD64%20Windows8%202.7/builds/870

======================================================================
ERROR: test_islink (test.test_posixpath.PosixPathTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\buildarea\2.7.bolen-windows8\build\lib\test\test_posixpath.py", line 110, in test_islink
    os.symlink(test_support.TESTFN + "1", test_support.TESTFN + "2")
AttributeError: 'module' object has no attribute 'symlink'

======================================================================
ERROR: test_ismount_symlinks (test.test_posixpath.PosixPathTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\buildarea\2.7.bolen-windows8\build\lib\test\test_posixpath.py", line 221, in test_ismount_symlinks
    os.unlink(ABSTFN)
WindowsError: [Error 2] The system cannot find the file specified: 'D:\\buildarea\\2.7.bolen-windows8\\build\\build\\test_python_2024/@test_2024_tmp'

----------------------------------------------------------------------
msg273531 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-08-24 04:16
Oh, sorry. Upload bot_failure_fix.patch to fix this.
msg273554 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-08-24 13:00
New changeset 63799c310b69 by R David Murray in branch '2.7':
#2466: fix test failure on windows.
https://hg.python.org/cpython/rev/63799c310b69
msg273556 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2016-08-24 13:01
Thanks, Xiang.  Also thanks to SilentGhost, who noticed the buildbot failure I missed.
History
Date User Action Args
2016-08-24 13:01:53r.david.murraysetstatus: open -> closed

messages: + msg273556
stage: needs patch -> resolved
2016-08-24 13:00:14python-devsetmessages: + msg273554
2016-08-24 04:16:15xiang.zhangsetfiles: + bot_failure_fix.patch

messages: + msg273531
2016-08-23 19:00:11r.david.murraysetstatus: closed -> open

messages: + msg273508
stage: resolved -> needs patch
2016-08-23 16:32:24r.david.murraysetstatus: open -> closed
versions: + Python 3.5, Python 3.6
messages: + msg273483

resolution: fixed
stage: needs patch -> resolved
2016-08-23 16:30:49python-devsetmessages: + msg273481
2016-08-19 05:21:55xiang.zhangsetfiles: + issue2466_ismount_py2.7_port.patch
nosy: + xiang.zhang
messages: + msg273081

2016-08-19 01:45:33python-devsetmessages: + msg273072
2016-08-19 01:36:55r.david.murraysetversions: - Python 3.5, Python 3.6
nosy: + r.david.murray

messages: + msg273071

stage: commit review -> needs patch
2016-08-19 01:35:31python-devsetnosy: + python-dev
messages: + msg273070
2016-06-03 10:41:23serhiy.storchakasetmessages: + msg267089
2016-06-03 10:06:09serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg267085
2016-06-03 06:19:31Robin Rothsetfiles: + minimal_fix_ismount_directory_not_readable.patch

messages: + msg267054
2016-06-03 06:05:37SilentGhostsetstage: patch review -> commit review
versions: - Python 3.2, Python 3.3, Python 3.4
2016-04-22 09:16:15Robin Rothsetmessages: + msg263989
versions: + Python 3.2, Python 3.3
2015-12-18 07:33:42Robin Rothsetmessages: + msg256646
2015-11-14 14:26:11Robin Rothsetfiles: + test_fix_ismount_directory_not_readable.patch

messages: + msg254662
2015-11-02 11:09:25berker.peksagsetversions: - Python 2.6, Python 3.0, Python 3.2, Python 3.3
nosy: + berker.peksag

messages: + msg253904

stage: patch review
2015-10-30 13:11:49Robin Rothsetmessages: + msg253732
versions: + Python 3.2, Python 3.3, Python 3.4, Python 3.5, Python 3.6
2015-10-01 08:00:30Robin Rothsetfiles: + ismount-permission.patch

nosy: + Robin Roth
messages: + msg252006

keywords: + patch
2014-07-07 22:34:43pablo.solesetnosy: + pablo.sole
messages: + msg222528
2011-05-31 23:21:36drawkssetnosy: + drawks

messages: + msg137401
versions: + Python 2.7
2008-08-17 22:19:38pitrousetmessages: + msg71294
2008-08-14 14:00:02pitrousetpriority: normal
nosy: + pitrou
messages: + msg71128
versions: + Python 2.6, Python 3.0, - Python 2.5
2008-08-14 13:15:24rossburtonsettitle: os.path.ismount doesn't work for NTFS mounts -> os.path.ismount doesn't work for mounts the user doesn't have permission to see
versions: + Python 2.5, - Python 2.4
2008-03-23 22:46:05rossburtonsetmessages: + msg64382
2008-03-23 19:51:19loewissetnosy: + loewis
messages: + msg64374
2008-03-23 17:30:27rossburtoncreate