pseudo: do not expand symlinks in /proc

Submitted by Sakib Sajal on Sept. 25, 2020, 5:05 p.m. | Patch ID: 176801

Details

Message ID 20200925170532.19685-1-sakib.sajal@windriver.com
State New
Headers show

Commit Message

Sakib Sajal Sept. 25, 2020, 5:05 p.m.
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(+)

Patch hide | download patch | download mbox

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;

Comments

Randy MacLeod Sept. 25, 2020, 6:47 p.m.
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;
>
> 
>
Richard Purdie Oct. 7, 2020, 9:01 p.m.
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
-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#143102): https://lists.openembedded.org/g/openembedded-core/message/143102
Mute This Topic: https://lists.openembedded.org/mt/77113653/3617530
Group Owner: openembedded-core+owner@lists.openembedded.org
Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [oe-patchwork@oe-patch.openembedded.org]
-=-=-=-=-=-=-=-=-=-=-=-