Bug 13288

*** Bug 13483 has been marked as a duplicate of this bug. ***

I like this solution and patch.

I don't know everything intended to achieved by pseudo following symlinks, or how much of that is relevant to /proc, but if there is a compulsion to follow the symlinks for proc, then maybe:

instead of never following a symlink on procfs, we could just avoid following the symlink whose lstat failed if it is on a procfs 

so:

lstat("/proc/6724/fd/63", {st_mode=S_IFLNK|0500, st_size=64, ...}) = 0
readlink("/proc/6724/fd/63", "pipe:[21346885]", 4096) = 15
lstat("/proc/6724/fd/pipe:[21346885]", 0x7ffe0f16e040) = -1 ENOENT (No such file or directory)

As the final lstat failed, use the pre-readlink value for open, the pre-readlink file was on a procfs

> we could just avoid following the symlink whose lstat failed

I had considered this, but there are cases where the lstat in procfs can improperly succeed and dereference the wrong path, such as when namespaces are in use or if a file with the suffix " (deleted)" exists (in the case of deleted files / directories).

Mount namespace example (two processes in different namespaces):
sh-5.0# readlink /proc/27570/cwd /proc/27574/cwd
/root/foo
/root/foo
sh-5.0# tail /proc/27570/cwd/asdf /proc/27574/cwd/asdf
==> /proc/27570/cwd/asdf <==
ns1

==> /proc/27574/cwd/asdf <==
ns2


Deleted file example:
sh-5.0# echo right file > bar
sh-5.0# echo wrong file > 'bar (deleted)'
sh-5.0# exec 200<>bar
sh-5.0# rm bar
sh-5.0# realpath /proc/self/fd/200
/root/bar (deleted)
sh-5.0# cat /proc/self/fd/200
right file
sh-5.0# cat "$(realpath /proc/self/fd/200)"
wrong file

Moved to M3 but the earlier we get pseudo patches in the better.

Sakib,
Peter thinks that the patch is okay.
He's a bit concerned about the cost of checking that for every single symlink path element, but he said that he can't think of a good way to avoid it.

Please prepare a patch and on an isolated system that only you use, time builds before and after to see if there is a measureable difference. If there isn't submit the patch and CC seebs. Thanks.

ping. 3.3-M4 or has this been merged?

ping. 3.3-M4 or has this been merged?

Medium+ needs a milestone.

As it's nearly 30 months since a patch has been supplied.

Instead of thinking about merging for years on end, please could you merge and then spend years thinking about reverting the merge for the rest of time?

If its so important for you, do it yourself. Cheers! :)

What kind of response is that?

I'm running the fix I posted in duplicate https://bugzilla.yoctoproject.org/show_bug.cgi?id=13483 so don't need the fix merging.

I'm thinking of all your other users for whom /dev/fd/2 etc hasn't been working for *years*

You've time for inane disrespect for people who take the time to report bugs *and* post fixes but no time to help users when you have the fix on a plate.

As far as I can tell this bug languishes because correct behaviour might take longer than the incorrect behaviour that causes total failure for simple code that ought to work.

You think it's not that important!

My apologies if my comment seemed inappropriate/disrespectful.

I have been trying to find time to work on upstream yocto bugs but have not been able to do so. I will get on them early October.

Thank you!

We added the patch and it seemed to mostly work okay but there is at least one use case, mentioned in the revert log, that failed so the patch was reverted:
   https://git.yoctoproject.org/pseudo/commit/?id=df1d1321fb093283485c387e3c933d2d264e509c

Early in the next release, we need to understand why this happened and decide if we want to add one or more special exceptions to the patch.

I suppose it failed because /proc/self/fd/3 is obviously reported by the kernel as a symlink to /home/pokybuild/yocto-worker/beaglebone/build/build/tmp/work/beaglebone_yocto-poky-linux-gnueabi/core-image-minimal/1.0-r0/sdk-ext/image/usr/local/oe-sdk-hardcoded-buildpath/layers/build/meta/

and that resolved path was needed by pseudo in order to perform more file system mappings, and therefore we cannot simply refuse to resolve /proc symlinks

However, the basis of the reverted patch is that we can't always safely resolve /proc symlinks. 

But maybe we can...

Certainly under /proc, opening a symlink is not the same as opening readlink(symlink) because for diagnostic reasons, the kernel emits bogus symlink target which the kernel itself does not follow when opening.

Apparently, only the kernel can properly resolve symlinks under proc -- it knows when the final stage symlink is bogus. (This problem is not restricted to pseudo; pre-use readlink resolution simply cannot be trusted in /proc by any process).

But can we tell after all? And in a simple way? 

Just as surely as we need to not fail when we fail to resolve /dev/*/fd/* to pipe:[...], 
we also need a way to resolve /proc symlinks that do direct back out of the /proc file system but which may have a deleted target

I had supposed in comment 2 [https://bugzilla.yoctoproject.org/show_bug.cgi?id=13288#c2] that the last stage readlink would always fail when the last stage was bogus:
/proc/6724/fd/63 -> pipe:[21346885]
/proc/6724/fd/64 -> /tmp/file (deleted)
But it is evident from the examples in comment 3 [https://bugzilla.yoctoproject.org/show_bug.cgi?id=13288#c3] that real clashing files can exist with those names. 

What makes the difference which I failed to take into account before, is whether or not the link target is absolute, meaning it begins with a /

Real clashing/deleted files can only exist for those names if the symlink target begins with a /

If the link target does NOT begin with a / then it will not leave the /proc file system and we can safely do as I proposed in comment 2 which is:
> instead of never following a symlink on procfs, we could just avoid 
> following the symlink whose lstat failed if it is on a procfs
adding the condition: provided that it was not an absolute symlink
This way /dev/fd/3 will resolve to /proc/self/fd/3 to /proc/6724/fd/3 and stop before resolving to pipe:[21346885] whose lstat will fail. The ultimate filename /proc/6724/fd/3 will work

But if the proc symlink target were an absolute path then we have additional conditions to follow, because the issue is how to safely navigate to that exiting link (/proc/*/fd/3 -> /home/pokybuild/...) in a way that won't choke on " (deleted)" or cross-namespace links.

The absolute symlink in /proc is not like a symlink in a pseudo chroot, or a normal filesystem. It seems to refer to real final paths with any symlinks already resolved, and no need for any chroot path fixups before further symlink resolution (unlike symlinks in the chroot directories on a normal non-proc file system).

So not only must the lstat of the symlink target not fail, but the st_dev and st_ino properties must match those of a stat of the symlink, and thus we know that the symlink target exists and is the original valid target and not a new one with the same name.

Therefore (if it is likely to matter in real life) we can tell whether " (deleted)" is part of the name or not, whether or not the target symlink is valid.

If they match, then the symlink target is valid however many times " (deleted)" may appear in it due to tricksie hobbitses, and thus we have the link target that we can continue resolving, having successfully navigated outside of the /proc filesystem

Note: there are absolute symlinks in /proc which refer back to /proc, e.g.: 
* /proc/1/task/1/fd/10 -> /proc/1/mountinfo
* /proc/482/fd/11 -> /proc/sys/kernel/hostname
but that won't be a problem with these methods, /proc links just continue to get processed as /proc as described above.

Does this seem reasonable?

Cross namespace links

Although these are mentioned in comment #3 I don't know how much of real a issue this is.

But because absolute links are resolved, you won't easily find anything like /proc/2345/fd/5 -> /proc/1234/fd/2
Even if you try to open the fd of another process with 5>/proc/1234/fd/2 ultimately /proc/2345/fd/5 will point to whatever /proc/1234/fd/2 is pointing to.

There are absolute symlinks in /proc which may back to another related process, e.g.:
/proc/4170249/root -> /proc/2527499/fdinfo\ (deleted)
/proc/4170249/task/4177332/fd/37 -> /proc/4170249/status
/proc/1337266/fd/8 -> /proc/1337266/mountinfo

but I'm not certain they can be an issue. 
Maybe we can refuse to traverse links from /proc/X/ to /proc/Y/ where X != Y
Maybe where they aren't part of the same process group? Or session? Or mount namespace? Or children of a common pseudo invocation?

Do we expect such symlinks to occur in /proc where the target also needs some path massaging to avoid an abort? Where the target despite being in a different namespace is in a pseudo chroot path that this instance of pseudo is controlling and can validly massage?

Surely if it's possible it's much less likely than the situation that reverted Matt's patch or the bug fixed by the Matt's patch, and it can be addressed when it happens.

(I wish I had kept my original patch now, and I see I didn't attach it to #13483)

Simply put quit processing without error for symlinks anywhere whose target in /proc fails lstat rather than symlink sources in /proc whose target fails lstat

In /proc if lstat target succeeds and (target is not in /proc) then we add the st_ino st_dev test to confirm


step by step:

* resolve the symlink to the fully qualified target
* if lstat(target) fails
  * and the target is in /proc then return the symlink not the target and let the kernel resolve it on use (it will be pipe:[...] or something like that
  * if st_no and st_dev of stat(symlink) don't match lstat of target then return the target knowing it will fail on use
* continue processing based on the new target as input

Sam

Sakib doesn't have time to continue to test this since it failed some YP autobuilder tests. -- YP bug review

Moved to M4.

Bulk move to 5.0 M1. -- Randy

Bulk move of bugs owned by "unassigned" from 5.0-M2 to 5.0-M3.

Bulk move of bugs from 5.0-M3 to 5.99.
The bugs in this list are all unassigned and are ones that I consider to be nice to do some time but not essential track from milestone to milestone in 5.0 or 5.1. We agreed on this bug handling approach in the YP bug review on Feb 22, 2024.

If you disagree, please move back to a 5.[012...]-M* milestone.

Is this resolved by commit 814e554564b2 ("pseudo_util.c: Skip realpath like expansion for /proc on Linux") in pseudo?

The issue was fixed as far as we could with the patch mentioned in my last comment, this doesn't exactly match the proposed fix described in earlier comments but it's the best we can reasonably do.