-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
os.path.ismounts: further situations it doesn't detect correctly #96328
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
Comments
A further bug in
assuming that The function reports |
If direct access is denied, then The point about btfrs subvolumes is a duplicate of #81520. Returning true for btrfs subvolumes might be useful. The fact that According to POSIX, a mount point is "[e]ither the system root directory or a directory for which the Regarding |
Aren't these results then also bogus?! If one does e.g. Same für btrfs and #81520 ... having it documented is better than nothing, but it would still not match the semantics of what one could reasonably expect from functions named I'm not so sure whether strictly following the POSIX definition is appropriate here. That is probably decades old and back then there weren't concepts like subvolumes or bind mounts, or at least POSIX didn't really care too much about them. Imagine you write a tool, that e.g. deletes a file tree and wants to check before whether anything is a mounpoiunt (cause if so, it couldn't delete that)… this would already fail with the current I don't think there's a real portable way (which is because POSIX doesn't really standardise these things too deeply)... the inode number doesn't work, not even under Linux (again btrfs, e.g.). Use case… well I don't know… perhaps something like: if some dir is a mountpoint, one want's to set some fs properties or maybe remount,ro it… so it should work when / is truly the mountpoint but not e.g. in a chroot, unless the chroot is in its own filesystem. Maybe one could add non-portable ways of determining (e.g. using |
Some more things, which are however not possible bugs: First: Lines 194 to 196 in e860e52
Edit: this part is bogus see the 2nd comment after this oneAFAIU, this is in principle not necessary, and even without it, the code should never consider a symlink to be a mountpoint. `lstat()` is used everywhere, so the symlink’s device ID should always be different from it's parent directory's.If that piece of code is however just for speed up (i.e. to save the |
Second: Line 202 in e860e52
I already tried to mail the two authors of the commit 750018b (issue #46718) which introduced that. The call to
without:
That's an extra of 556130
the numbers match (off a few, because stuff gets written/deleted in Even after a lot of thinking, I don't quite understand why it would be needed. The bug #46718 mentions describes it as fixing it for the case when there's some directory where the user doesn't have permissions, but wouldn't a better solution for that be to simply not catch the exception in: Lines 205 to 206 in e860e52
?
Consider e.g.
I copy&pasted
just as it should. Could it be, that something changed and the realpath() is no longer Looking at the
=> both on dev 28 (that's the device of test/) Whereas:
=> the symlink to
=> both, the mountpoint, dev 78
=> again, the plain dir outside the mountpoint, dev 28 So Do I miss anything here, what the call of |
The point above in #96328 (comment) is bogus.... well partially. The check for not-a-symlink is needed, though I still think one could perhaps improve it to check for the pathname being a true directory (as no other file types can be mountpoints, I'd guess). An example where the function wouldn't work without the not-a-symlink check is:
then an So together with #96328 (comment) I'd say that it seems Does that make sense? |
I might have also found a (questionable?) reason for the Consider e.g. a directory With
The seems IMO, at least the 3 latter, because Python cannot know what
which is clearly wrong, but as root:
Likewise, the
Which I think is correct, unlike what Python does. To simply collapsing But now back to Now arguably, the first case is not so uncommon, but still the solution seems extremely expensive, especially because of the many |
Maybe one could catch the |
I further thought about how one could get rid of the expensive Assuming we remove the
After the no-symlink-check, construct the parent path as removing the last pathname component, not using Now for the case of But do we still get the true parent?
(Whether we have a trailing Now @pitrou wrote in #46718 (comment):
But that's anyway already ruled out because of the non-symlink-check. And in #46718 (comment) he said:
I understand this as follows in my terms: So if TBH, even with the `path/..` way for getting the parent **and** using `lstat()`, I don't understand @pitrou’s above case (at least not the way as he wrote). What would IMO happen is rather: If `/A` is used as parent pathname and points to a mountpoint, then the `lstat()` would still get the device ID of the symlink. This may or may not be the same device ID as where `/A` points to and which is the true parent. So regardless of whether `/A/B` is a mountpont or not, the result might be right or wrong. But back to my proposal: Are the other cases ok (i.e. do we stat the right thing?), if we do
Not sure if I missed anything or messed up my case, but AFAIU, we should be able to drop the
Any thoughts? |
btw: I followed that a bit up, and while you're right with So I'd say that the POSIX definition really means the true system root dir, and not any chroot’s root dir. And thus strictly (and practically) speaking, the check comparing the inode numbers is wrong. |
More reason why using It says in 4.13 Pathname Resolution:
"may" And in 3.144 Empty Directory:
"at most", IMO indicates that it could also lack them.
Not sure if on can interpret the "shall" in the sense of "it must be there" or just "if it's there, then it shall do.." Of course, in practise this probably never happens unless there's some filesystem bug (which I've however actually already seen, i.e. directories where |
Also, For Thought, AFAICS, this strange property of |
@eryksun Any expert ideas on my follow-up comments? |
Another situation that python3 -m pip install ratarmount
mkdir folder
ratarmount folder mounted
kill -9 $( pgrep ratarmount | tail -1 )
ls -lad mounted # ls: cannot access 'mounted': Transport endpoint is not connected
mount | tail -1 # FuseMount on /tmp/mounted type fuse (rw,nosuid,nodev,relatime)
python3 -c 'import os; print(os.path.ismount("mounted"))' # False
mountpoint -q mounted; echo $? # 1 It seems like the |
Bug report
I had a look at
os.path.ismount()
:cpython/Lib/posixpath.py
Lines 186 to 216 in 43a6dea
And it may have some issues:
ismount()
is IMO still simply wrong in these cases, cause it is a mountpoint.st_dev
equals that of its parent. For example, btrfs subvolumes. This is however not documented.Looking a the code, it effectively does:
lstat()
, check whether it’s a symlink,lstat()
the parent, check whether the device numbers are different (then it's a mountpoint), check whether the inodes are the same.Now I thought about the latter, and what it is meant for.
The only case I could think of at first is
/
, where/..
is the same inode. [Is there any other?]"/"
orb"/"
. Would perhaps require somerealpath()
though, to also catch things like"/../.."
and so on./
is not the only case: Within a chroot, the chroot's/
may (or may not) be a mountpoint - the problem is just, the current algorithm doesn't detect this either. Instead, it wrongly claims that a chroot’s "/" would be a mountpoint, even when it isn't.Not sure what one could do about all that…
/etc/mtab
is dead.../proc/mounts
may not be available on all POSIX systems and even on Linux it's nowadays rather superseded by/proc/self/mountinfo
.Your environment
Cheers,
Chris.
The text was updated successfully, but these errors were encountered: