[PATCH] pseudo: do not expand symlinks in /proc

[PATCH] pseudo: do not expand symlinks in /proc

[Sakib Sajal]

From: Matt Cowell <matt.cowell@nokia.com>

Some symlinks in /proc, such as those under /proc/[pid]/fd,
/proc/[pid]/cwd, and /proc/[pid]/exe that are not real and should not
have readlink called on them.  These look like symlinks, but behave like
hardlinks.  Readlink does not return actual paths.  Previously
pseudo_fix_path would expand files such as /dev/stdin to paths such as
/proc/6680/fd/pipe:[1270830076] which do not exist.

This issue affects:
- deleted files
- deleted directories
- fifos
- sockets
- anon_inodes (epoll, eventfd, inotify, signalfd, timerfd, etc)

Testing:
timed builds before and after applying patch, without significant
measurable difference.
$ bitbake -c compile <image>; time bitbake <image>

installed pseudo on an image and was unable to reproduce the test
on bug report after applying the patch.

FIXES: Bug 13288

Signed-off-by: Sakib Sajal <sakib.sajal@windriver.com>
---
 pseudo_util.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/pseudo_util.c b/pseudo_util.c
index c867ed6..bce4d1e 100644
--- a/pseudo_util.c
+++ b/pseudo_util.c
@@ -21,6 +21,8 @@
 #include <sys/time.h>
 #include <unistd.h>
 #include <limits.h>
+#include <sys/vfs.h>
+#include <linux/magic.h>
 
 /* see the comments below about (*real_regcomp)() */
 #include <dlfcn.h>
@@ -29,6 +31,11 @@
 #include "pseudo_ipc.h"
 #include "pseudo_db.h"
 
+/* O_PATH is defined in glibc 2.16 and later only */
+#ifndef O_PATH
+#define O_PATH          010000000
+#endif
+
 struct pseudo_variables {
 	char *key;
 	size_t key_len;
@@ -677,6 +684,26 @@ pseudo_append_element(char *newpath, char *root, size_t allocated, char **pcurre
 	 */
 	if (!leave_this && is_dir) {
 		int is_link = S_ISLNK(buf->st_mode);
+
+		/* do not expand symlinks in the proc filesystem, since they may not be real */
+		if (is_link) {
+			struct statfs sfs;
+			int fd;
+
+			/* statfs follows symlinks, so use fstatfs */
+			fd = open(newpath, O_CLOEXEC | O_PATH | O_NOFOLLOW);
+			if (-1 != fd) {
+				if (0 == fstatfs(fd, &sfs) && sfs.f_type == PROC_SUPER_MAGIC) {
+					pseudo_debug(PDBGF_PATH | PDBGF_VERBOSE,
+						"pae: '%s' is procfs symlink, not expanding\n",
+						newpath);
+					is_link = 0;
+				}
+
+				close(fd);
+			}
+		}
+
 		if (link_recursion >= PSEUDO_MAX_LINK_RECURSION && is_link) {
 			pseudo_diag("link recursion too deep, not expanding path '%s'.\n", newpath);
 			is_link = 0;
-- 
2.27.0

Re: [OE-core] [PATCH] pseudo: do not expand symlinks in /proc

[Randy MacLeod]

[-- Attachment #1: Type: text/plain, Size: 3370 bytes --]

pseduo patches are usually sent to the yocto list so
I've added that list and only BCCed oe-core here so
people know where to look for follow-up.


On 2020-09-25 1:05 p.m., Sakib Sajal wrote:
> From: Matt Cowell <matt.cowell@nokia.com>
>
> Some symlinks in /proc, such as those under /proc/[pid]/fd,
> /proc/[pid]/cwd, and /proc/[pid]/exe that are not real and should not
> have readlink called on them.  These look like symlinks, but behave like
> hardlinks.  Readlink does not return actual paths.  Previously
> pseudo_fix_path would expand files such as /dev/stdin to paths such as
> /proc/6680/fd/pipe:[1270830076] which do not exist.
>
> This issue affects:
> - deleted files
> - deleted directories
> - fifos
> - sockets
> - anon_inodes (epoll, eventfd, inotify, signalfd, timerfd, etc)
>
> Testing:
> timed builds before and after applying patch, without significant
> measurable difference.
> $ bitbake -c compile <image>; time bitbake <image>
>
> installed pseudo on an image and was unable to reproduce the test
> on bug report after applying the patch.

There might be a better test so please let Sakib know if so.

Sakib,
   If there's a v2 or in future work, please give the actual results
such as:

   5 trials with an average time of 18m:14s and a range of 22 seconds
   on a build host that no one else was using.

This will help people to understand what you mean by
    "without significant measurable difference.

Thanks for testing and sending this change.

../Randy

>
> FIXES: Bug 13288
>
> Signed-off-by: Sakib Sajal <sakib.sajal@windriver.com>
> ---
>   pseudo_util.c | 27 +++++++++++++++++++++++++++
>   1 file changed, 27 insertions(+)
>
> diff --git a/pseudo_util.c b/pseudo_util.c
> index c867ed6..bce4d1e 100644
> --- a/pseudo_util.c
> +++ b/pseudo_util.c
> @@ -21,6 +21,8 @@
>   #include <sys/time.h>
>   #include <unistd.h>
>   #include <limits.h>
> +#include <sys/vfs.h>
> +#include <linux/magic.h>
>   
>   /* see the comments below about (*real_regcomp)() */
>   #include <dlfcn.h>
> @@ -29,6 +31,11 @@
>   #include "pseudo_ipc.h"
>   #include "pseudo_db.h"
>   
> +/* O_PATH is defined in glibc 2.16 and later only */
> +#ifndef O_PATH
> +#define O_PATH          010000000
> +#endif
> +
>   struct pseudo_variables {
>   	char *key;
>   	size_t key_len;
> @@ -677,6 +684,26 @@ pseudo_append_element(char *newpath, char *root, size_t allocated, char **pcurre
>   	 */
>   	if (!leave_this && is_dir) {
>   		int is_link = S_ISLNK(buf->st_mode);
> +
> +		/* do not expand symlinks in the proc filesystem, since they may not be real */
> +		if (is_link) {
> +			struct statfs sfs;
> +			int fd;
> +
> +			/* statfs follows symlinks, so use fstatfs */
> +			fd = open(newpath, O_CLOEXEC | O_PATH | O_NOFOLLOW);
> +			if (-1 != fd) {
> +				if (0 == fstatfs(fd, &sfs) && sfs.f_type == PROC_SUPER_MAGIC) {
> +					pseudo_debug(PDBGF_PATH | PDBGF_VERBOSE,
> +						"pae: '%s' is procfs symlink, not expanding\n",
> +						newpath);
> +					is_link = 0;
> +				}
> +
> +				close(fd);
> +			}
> +		}
> +
>   		if (link_recursion >= PSEUDO_MAX_LINK_RECURSION && is_link) {
>   			pseudo_diag("link recursion too deep, not expanding path '%s'.\n", newpath);
>   			is_link = 0;
>
> 
>

-- 
# Randy MacLeod
# Wind River Linux


[-- Attachment #2: Type: text/html, Size: 4418 bytes --]

Re: [OE-core] [PATCH] pseudo: do not expand symlinks in /proc

[Richard Purdie]

On Fri, 2020-09-25 at 14:47 -0400, Randy MacLeod wrote:
> pseduo patches are usually sent to the yocto list so
> I've added that list and only BCCed oe-core here so
> people know where to look for follow-up.

In Sakib's defence, did you read the README in pseudo? :)

"Discussions and patches should be directed at the openembedded-core
mailing list at openembedded-core at lists.openembedded.org."

Whether that is right or not, less sure but it is what it says!

Cheers,

Richard

Re: [OE-core] [PATCH] pseudo: do not expand symlinks in /proc

[Richard Purdie]

On Fri, 2020-09-25 at 13:05 -0400, Sakib Sajal wrote:
> From: Matt Cowell <matt.cowell@nokia.com>
> 
> Some symlinks in /proc, such as those under /proc/[pid]/fd,
> /proc/[pid]/cwd, and /proc/[pid]/exe that are not real and should not
> have readlink called on them.  These look like symlinks, but behave
> like
> hardlinks.  Readlink does not return actual paths.  Previously
> pseudo_fix_path would expand files such as /dev/stdin to paths such
> as
> /proc/6680/fd/pipe:[1270830076] which do not exist.
> 
> This issue affects:
> - deleted files
> - deleted directories
> - fifos
> - sockets
> - anon_inodes (epoll, eventfd, inotify, signalfd, timerfd, etc)
> 
> Testing:
> timed builds before and after applying patch, without significant
> measurable difference.
> $ bitbake -c compile <image>; time bitbake <image>
> 
> installed pseudo on an image and was unable to reproduce the test
> on bug report after applying the patch.
> 
> FIXES: Bug 13288
> 
> Signed-off-by: Sakib Sajal <sakib.sajal@windriver.com>

I added this on top of the other recent pseudo patches and
unfortunately, it threw a lot of errors. One example:

path mismatch [2 links]: ino 44446916 db '/home/pokybuild/yocto-worker/edgerouter-alt/build/build/tmp/work/all-poky-linux/volatile-binds/1.0-r0/package/sbin' req '/proc/self/fd/3/./sbin'.

which shows it ended up passing an invalid path into the database.

I'm going to have to back this change out until we can figure out how
to reconcile this with path mismatch errors now being fatal.

Cheers,

Richard