[PATCH 0/3] Miscellaneous small fixes

[PATCH 0/3] Miscellaneous small fixes

[David Gibson]

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

Here's a handful of small fixes for passt I came across while working
on other things.

David Gibson (3):
  conf: Fix incorrect bounds checking for sock_path parameter
  gitignore README.plain.md
  Don't unnecessarily avoid CLOEXEC flags

 .gitignore |  1 +
 conf.c     | 12 ++++--------
 passt.c    |  6 ++----
 pasta.c    |  2 +-
 4 files changed, 8 insertions(+), 13 deletions(-)

-- 
2.37.2

[PATCH 1/3] conf: Fix incorrect bounds checking for sock_path parameter

[David Gibson]

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

Looks like a copy-paste error where we're checking against the size of the
pcap field, rather than the sock_path field.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 conf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/conf.c b/conf.c
index 83b2fe5..ac81c15 100644
--- a/conf.c
+++ b/conf.c
@@ -1269,7 +1269,7 @@ void conf(struct ctx *c, int argc, char **argv)
 
 			ret = snprintf(c->sock_path, UNIX_SOCK_MAX - 1, "%s",
 				       optarg);
-			if (ret <= 0 || ret >= (int)sizeof(c->pcap)) {
+			if (ret <= 0 || ret >= (int)sizeof(c->sock_path)) {
 				err("Invalid socket path: %s", optarg);
 				usage(argv[0]);
 			}
-- 
@@ -1269,7 +1269,7 @@ void conf(struct ctx *c, int argc, char **argv)
 
 			ret = snprintf(c->sock_path, UNIX_SOCK_MAX - 1, "%s",
 				       optarg);
-			if (ret <= 0 || ret >= (int)sizeof(c->pcap)) {
+			if (ret <= 0 || ret >= (int)sizeof(c->sock_path)) {
 				err("Invalid socket path: %s", optarg);
 				usage(argv[0]);
 			}
-- 
2.37.2

[PATCH 2/3] gitignore README.plain.md

[David Gibson]

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

Add the generated README.plain.md file to .gitignore.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 .gitignore | 1 +
 1 file changed, 1 insertion(+)

diff --git a/.gitignore b/.gitignore
index 4b86fff..4aab0f6 100644
--- a/.gitignore
+++ b/.gitignore
@@ -7,3 +7,4 @@
 /pasta.1
 /seccomp.h
 /passt.pid
+README.plain.md
-- 
@@ -7,3 +7,4 @@
 /pasta.1
 /seccomp.h
 /passt.pid
+README.plain.md
-- 
2.37.2

[PATCH 3/3] Don't unnecessarily avoid CLOEXEC flags

[David Gibson]

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

There are several places in the passt code where we have lint overrides
because we're not adding CLOEXEC flags to open or other operations.
Comments suggest this is because it's before we fork() into the background
but we'll need those file descriptors after we're in the background.

However, as the name suggests CLOEXEC closes on exec(), not on fork().  The
only place we exec() is either super early invoke the avx2 version of the
binary, or when we start a shell in pasta mode, which certainly *doesn't*
require the fds in question.

Add the CLOEXEC flag in those places, and remove the lint overrides.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 conf.c  | 10 +++-------
 passt.c |  6 ++----
 pasta.c |  2 +-
 3 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/conf.c b/conf.c
index ac81c15..d936157 100644
--- a/conf.c
+++ b/conf.c
@@ -562,18 +562,14 @@ static int conf_ns_opt(struct ctx *c,
 				continue;
 		}
 
-		/* Don't pass O_CLOEXEC here: ns_enter() needs those files */
 		if (!c->netns_only) {
 			if (*conf_userns)
-				/* NOLINTNEXTLINE(android-cloexec-open) */
-				ufd = open(conf_userns, O_RDONLY);
+				ufd = open(conf_userns, O_RDONLY | O_CLOEXEC);
 			else if (*userns)
-				/* NOLINTNEXTLINE(android-cloexec-open) */
-				ufd = open(userns, O_RDONLY);
+				ufd = open(userns, O_RDONLY | O_CLOEXEC);
 		}
 
-		/* NOLINTNEXTLINE(android-cloexec-open) */
-		nfd = open(netns, O_RDONLY);
+		nfd = open(netns, O_RDONLY | O_CLOEXEC);
 
 		if (nfd == -1 || (ufd == -1 && !c->netns_only)) {
 			if (nfd >= 0)
diff --git a/passt.c b/passt.c
index 0113002..bbf53d9 100644
--- a/passt.c
+++ b/passt.c
@@ -329,8 +329,7 @@ int main(int argc, char **argv)
 
 	__setlogmask(LOG_MASK(LOG_EMERG));
 
-	/* NOLINTNEXTLINE(android-cloexec-epoll-create1): forking in a moment */
-	c.epollfd = epoll_create1(0);
+	c.epollfd = epoll_create1(EPOLL_CLOEXEC);
 	if (c.epollfd == -1) {
 		perror("epoll_create1");
 		exit(EXIT_FAILURE);
@@ -381,8 +380,7 @@ int main(int argc, char **argv)
 	pcap_init(&c);
 
 	if (!c.foreground) {
-		/* NOLINTNEXTLINE(android-cloexec-open): see __daemon() */
-		if ((devnull_fd = open("/dev/null", O_RDWR)) < 0) {
+		if ((devnull_fd = open("/dev/null", O_RDWR | O_CLOEXEC)) < 0) {
 			perror("/dev/null open");
 			exit(EXIT_FAILURE);
 		}
diff --git a/pasta.c b/pasta.c
index 5a78065..830748f 100644
--- a/pasta.c
+++ b/pasta.c
@@ -223,7 +223,7 @@ void pasta_ns_conf(struct ctx *c)
  */
 int pasta_netns_quit_init(struct ctx *c)
 {
-	int flags = O_NONBLOCK | (c->foreground ? O_CLOEXEC : 0);
+	int flags = O_NONBLOCK | O_CLOEXEC;
 	struct epoll_event ev = { .events = EPOLLIN };
 	int inotify_fd;
 
-- 
@@ -223,7 +223,7 @@ void pasta_ns_conf(struct ctx *c)
  */
 int pasta_netns_quit_init(struct ctx *c)
 {
-	int flags = O_NONBLOCK | (c->foreground ? O_CLOEXEC : 0);
+	int flags = O_NONBLOCK | O_CLOEXEC;
 	struct epoll_event ev = { .events = EPOLLIN };
 	int inotify_fd;
 
-- 
2.37.2

Re: [PATCH 3/3] Don't unnecessarily avoid CLOEXEC flags

[Stefano Brivio]

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

On Tue, 23 Aug 2022 16:31:51 +1000
David Gibson <david(a)gibson.dropbear.id.au> wrote:

> There are several places in the passt code where we have lint overrides
> because we're not adding CLOEXEC flags to open or other operations.
> Comments suggest this is because it's before we fork() into the background
> but we'll need those file descriptors after we're in the background.
> 
> However, as the name suggests CLOEXEC closes on exec(), not on fork().

...after looking into it and trying to remember: it seems like the only
reason why I skipped O_CLOEXEC here was some bogus innate assumption I
had about file descriptors being closed on clone() too. Thanks for
spotting this.

-- 
Stefano

Re: [PATCH 0/3] Miscellaneous small fixes

[Stefano Brivio]

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

On Tue, 23 Aug 2022 16:31:48 +1000
David Gibson <david(a)gibson.dropbear.id.au> wrote:

> Here's a handful of small fixes for passt I came across while working
> on other things.
> 
> David Gibson (3):
>   conf: Fix incorrect bounds checking for sock_path parameter
>   gitignore README.plain.md
>   Don't unnecessarily avoid CLOEXEC flags

Applied!

-- 
Stefano