All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] pseudo: do not expand symlinks in /proc
@ 2020-10-07 16:20 Richard Purdie
  2020-10-07 16:20 ` [PATCH 2/2] pseudo: Fix statx function usage Richard Purdie
  0 siblings, 1 reply; 6+ messages in thread
From: Richard Purdie @ 2020-10-07 16:20 UTC (permalink / raw)
  To: openembedded-core; +Cc: seebs

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.

[YOCTO #13288]

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
---
 meta/recipes-devtools/pseudo/files/proc.patch | 86 +++++++++++++++++++
 meta/recipes-devtools/pseudo/pseudo_git.bb    |  1 +
 2 files changed, 87 insertions(+)
 create mode 100644 meta/recipes-devtools/pseudo/files/proc.patch

diff --git a/meta/recipes-devtools/pseudo/files/proc.patch b/meta/recipes-devtools/pseudo/files/proc.patch
new file mode 100644
index 00000000000..57ae9bf9504
--- /dev/null
+++ b/meta/recipes-devtools/pseudo/files/proc.patch
@@ -0,0 +1,86 @@
+From: Matt Cowell <matt.cowell@nokia.com>
+From: "Sakib Sajal" <sakib.sajal@windriver.com>
+
+pseudo: do not expand symlinks in /proc
+
+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.
+
+[YOCTO #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;
diff --git a/meta/recipes-devtools/pseudo/pseudo_git.bb b/meta/recipes-devtools/pseudo/pseudo_git.bb
index bc20a2f134e..90f5cb0bcf2 100644
--- a/meta/recipes-devtools/pseudo/pseudo_git.bb
+++ b/meta/recipes-devtools/pseudo/pseudo_git.bb
@@ -9,6 +9,7 @@ SRC_URI = "git://git.yoctoproject.org/pseudo;branch=oe-core \
            file://xattr_fix.patch \
            file://mayunlink.patch \
            file://pathfix.patch \
+           file://proc.patch \
            file://fallback-passwd \
            file://fallback-group \
            "
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 2/2] pseudo: Fix statx function usage
  2020-10-07 16:20 [PATCH 1/2] pseudo: do not expand symlinks in /proc Richard Purdie
@ 2020-10-07 16:20 ` Richard Purdie
  2020-10-12 19:08   ` [OE-core] " Seebs
  0 siblings, 1 reply; 6+ messages in thread
From: Richard Purdie @ 2020-10-07 16:20 UTC (permalink / raw)
  To: openembedded-core; +Cc: seebs

There is magic in the posts where specific variable names have specific
magic. For that magic to work, "path" needs to be used not "pathname" as
is currently there. Fix this, which fixes path issues on systems using
statx (Ubuntu 20.04 in particular).

Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
---
 .../pseudo/files/statxfix.patch               | 56 +++++++++++++++++++
 meta/recipes-devtools/pseudo/pseudo_git.bb    |  1 +
 2 files changed, 57 insertions(+)
 create mode 100644 meta/recipes-devtools/pseudo/files/statxfix.patch

diff --git a/meta/recipes-devtools/pseudo/files/statxfix.patch b/meta/recipes-devtools/pseudo/files/statxfix.patch
new file mode 100644
index 00000000000..c47ff27f9f4
--- /dev/null
+++ b/meta/recipes-devtools/pseudo/files/statxfix.patch
@@ -0,0 +1,56 @@
+There is magic in the posts where specific variable names have specific
+magic. For that magic to work, "path" needs to be used not "pathname" as
+is currently there. Fix this, which fixes path issues on systems using 
+statx (Ubuntu 20.04 in particular).
+
+Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
+Upstream-Status: Pending
+
+Index: git/ports/linux/statx/guts/statx.c
+===================================================================
+--- git.orig/ports/linux/statx/guts/statx.c
++++ git/ports/linux/statx/guts/statx.c
+@@ -5,14 +5,14 @@
+  * SPDX-License-Identifier: LGPL-2.1-only
+  *
+  * int
+- * statx(int dirfd, const char *pathname, int flags, unsigned int mask, struct statx *statxbuf) {
++ * statx(int dirfd, const char *path, int flags, unsigned int mask, struct statx *statxbuf) {
+  *	int rc = -1;
+  */
+ 	pseudo_msg_t *msg;
+ 	PSEUDO_STATBUF buf;
+ 	int save_errno;
+ 
+-	rc = real_statx(dirfd, pathname, flags, mask, statxbuf);
++	rc = real_statx(dirfd, path, flags, mask, statxbuf);
+ 	save_errno = errno;
+ 	if (rc == -1) {
+ 		return rc;
+@@ -25,16 +25,16 @@
+ 	buf.st_mode = statxbuf->stx_mode;
+ 	buf.st_rdev = makedev(statxbuf->stx_rdev_major, statxbuf->stx_rdev_minor);
+ 	buf.st_nlink = statxbuf->stx_nlink;
+-	msg = pseudo_client_op(OP_STAT, 0, -1, dirfd, pathname, &buf);
++	msg = pseudo_client_op(OP_STAT, 0, -1, dirfd, path, &buf);
+ 	if (msg && msg->result == RESULT_SUCCEED) {
+-		pseudo_debug(PDBGF_FILE, "statx(path %s), flags %o, stat rc %d, stat uid %o\n", pathname, flags, rc, statxbuf->stx_uid);
++		pseudo_debug(PDBGF_FILE, "statx(path %s), flags %o, stat rc %d, stat uid %o\n", path, flags, rc, statxbuf->stx_uid);
+ 		statxbuf->stx_uid = msg->uid;
+ 		statxbuf->stx_gid = msg->gid;
+ 		statxbuf->stx_mode = msg->mode;
+ 		statxbuf->stx_rdev_major = major(msg->rdev);
+ 		statxbuf->stx_rdev_minor = minor(msg->rdev);
+ 	} else {
+-		pseudo_debug(PDBGF_FILE, "statx(path %s) failed, flags %o, stat rc %d, stat uid %o\n", pathname, flags, rc, statxbuf->stx_uid);
++		pseudo_debug(PDBGF_FILE, "statx(path %s) failed, flags %o, stat rc %d, stat uid %o\n", path, flags, rc, statxbuf->stx_uid);
+ 	}
+ 	errno = save_errno;
+ /*	return rc;
+Index: git/ports/linux/statx/wrapfuncs.in
+===================================================================
+--- git.orig/ports/linux/statx/wrapfuncs.in
++++ git/ports/linux/statx/wrapfuncs.in
+@@ -1 +1 @@
+-int statx(int dirfd, const char *pathname, int flags, unsigned int mask, struct statx *statxbuf);
++int statx(int dirfd, const char *path, int flags, unsigned int mask, struct statx *statxbuf);
diff --git a/meta/recipes-devtools/pseudo/pseudo_git.bb b/meta/recipes-devtools/pseudo/pseudo_git.bb
index 90f5cb0bcf2..626e658f65d 100644
--- a/meta/recipes-devtools/pseudo/pseudo_git.bb
+++ b/meta/recipes-devtools/pseudo/pseudo_git.bb
@@ -10,6 +10,7 @@ SRC_URI = "git://git.yoctoproject.org/pseudo;branch=oe-core \
            file://mayunlink.patch \
            file://pathfix.patch \
            file://proc.patch \
+           file://statxfix.patch \
            file://fallback-passwd \
            file://fallback-group \
            "
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [OE-core] [PATCH 2/2] pseudo: Fix statx function usage
  2020-10-07 16:20 ` [PATCH 2/2] pseudo: Fix statx function usage Richard Purdie
@ 2020-10-12 19:08   ` Seebs
  2020-10-12 19:14     ` Mark Hatle
  2020-10-12 20:25     ` Richard Purdie
  0 siblings, 2 replies; 6+ messages in thread
From: Seebs @ 2020-10-12 19:08 UTC (permalink / raw)
  To: Richard Purdie; +Cc: openembedded-core

On Wed,  7 Oct 2020 17:20:18 +0100
"Richard Purdie" <richard.purdie@linuxfoundation.org> wrote:

> +There is magic in the posts where specific variable names have
> specific +magic. For that magic to work, "path" needs to be used not
> "pathname" as +is currently there. Fix this, which fixes path issues
> on systems using +statx (Ubuntu 20.04 in particular).

So, usually if "pathname" is used, it's intentional to suppress the
special magic behavior. Usually, but not always. In this case, the man
page does use "pathname" and sometimes that means it's just a cut and
paste error.

My man page also says there's no glibc wrapper for statx, making me
slightly surprised that a wrapper for it is needed/relevant.

-s

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [OE-core] [PATCH 2/2] pseudo: Fix statx function usage
  2020-10-12 19:08   ` [OE-core] " Seebs
@ 2020-10-12 19:14     ` Mark Hatle
  2020-10-12 19:21       ` Seebs
  2020-10-12 20:25     ` Richard Purdie
  1 sibling, 1 reply; 6+ messages in thread
From: Mark Hatle @ 2020-10-12 19:14 UTC (permalink / raw)
  To: Seebs, Richard Purdie; +Cc: openembedded-core



On 10/12/20 2:08 PM, Seebs wrote:
> On Wed,  7 Oct 2020 17:20:18 +0100
> "Richard Purdie" <richard.purdie@linuxfoundation.org> wrote:
> 
>> +There is magic in the posts where specific variable names have
>> specific +magic. For that magic to work, "path" needs to be used not
>> "pathname" as +is currently there. Fix this, which fixes path issues
>> on systems using +statx (Ubuntu 20.04 in particular).
> 
> So, usually if "pathname" is used, it's intentional to suppress the
> special magic behavior. Usually, but not always. In this case, the man
> page does use "pathname" and sometimes that means it's just a cut and
> paste error.
> 
> My man page also says there's no glibc wrapper for statx, making me
> slightly surprised that a wrapper for it is needed/relevant.

I looked into this.  It appears (after pseudo was written, but) about 7 or so
years ago, all of the man pages were re-written and the language was changed.

So 'pathname' no longer has the same syntactic meaning that it did when pseudo
was first written.  (It's not just statx, I looked at things like fopen, open, etc.)

--Mark

> -s
> 
> 
> 
> 
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [OE-core] [PATCH 2/2] pseudo: Fix statx function usage
  2020-10-12 19:14     ` Mark Hatle
@ 2020-10-12 19:21       ` Seebs
  0 siblings, 0 replies; 6+ messages in thread
From: Seebs @ 2020-10-12 19:21 UTC (permalink / raw)
  To: Mark Hatle; +Cc: Richard Purdie, openembedded-core

On Mon, 12 Oct 2020 14:14:36 -0500
"Mark Hatle" <mark.hatle@kernel.crashing.org> wrote:

> I looked into this.  It appears (after pseudo was written, but) about
> 7 or so years ago, all of the man pages were re-written and the
> language was changed.

I don't think I originally intentionally synchronized it with man
pages, I just had the convention of special-casing "path", and then
started using "pathname" as a convention for "a name that holds a
path but we don't want to do the magic on it".

-s

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [OE-core] [PATCH 2/2] pseudo: Fix statx function usage
  2020-10-12 19:08   ` [OE-core] " Seebs
  2020-10-12 19:14     ` Mark Hatle
@ 2020-10-12 20:25     ` Richard Purdie
  1 sibling, 0 replies; 6+ messages in thread
From: Richard Purdie @ 2020-10-12 20:25 UTC (permalink / raw)
  To: Seebs; +Cc: openembedded-core

On Mon, 2020-10-12 at 14:08 -0500, Seebs wrote:
> On Wed,  7 Oct 2020 17:20:18 +0100
> "Richard Purdie" <richard.purdie@linuxfoundation.org> wrote:
> 
> > +There is magic in the posts where specific variable names have
> > specific +magic. For that magic to work, "path" needs to be used
> > not
> > "pathname" as +is currently there. Fix this, which fixes path
> > issues
> > on systems using +statx (Ubuntu 20.04 in particular).
> 
> So, usually if "pathname" is used, it's intentional to suppress the
> special magic behavior.

In this case I added the statx wrapper and didn't realise there was
magic related to the name "path", so its a bug I introduced. This patch
fixes my mistake. I think these are only on the oe-core branch, not
master.

>  Usually, but not always. In this case, the man
> page does use "pathname" and sometimes that means it's just a cut and
> paste error.

Its my own copy and paste bug :)

> My man page also says there's no glibc wrapper for statx, making me
> slightly surprised that a wrapper for it is needed/relevant.

Its definitely being used by Ubuntu 20.04 systems as we're findining
bugs related to it!

Thanks for the review!

Cheers,

Richard


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-10-12 20:25 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-07 16:20 [PATCH 1/2] pseudo: do not expand symlinks in /proc Richard Purdie
2020-10-07 16:20 ` [PATCH 2/2] pseudo: Fix statx function usage Richard Purdie
2020-10-12 19:08   ` [OE-core] " Seebs
2020-10-12 19:14     ` Mark Hatle
2020-10-12 19:21       ` Seebs
2020-10-12 20:25     ` Richard Purdie

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.