[PATCH 0/2] Fixes for nstool

[PATCH 0/2] Fixes for nstool

[David Gibson]

This fixes a couple of bugs I found in nstool.  I discovered these
during the Avocado work, but they are bugs independent of that.

David Gibson (2):
  test/nstool: Provide useful error if given a path that's too long
  test/nstool: Fix fd leak in accept() loop

 test/nstool.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

-- 
2.40.1

[PATCH 1/2] test/nstool: Provide useful error if given a path that's too long

[David Gibson]

Normal filesystem paths can be very long (PATH_MAX is around 8k), however
Unix domain sockets can only use relatively short paths (UNIX_PATH_MAX is
108 on Linux).  Currently nstool will simply truncate paths that are too
long, leading to difficult to understand failures.

Make such failures clearer, with an explicit error message if given a path
that's too long.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 test/nstool.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/test/nstool.c b/test/nstool.c
index e6d7d37..bca9569 100644
--- a/test/nstool.c
+++ b/test/nstool.c
@@ -93,14 +93,22 @@ static void usage(void)
 	    "    terminate.\n");
 }
 
+static void sockaddr_from_path(struct sockaddr_un *addr, const char *sockpath)
+{
+	if (strlen(sockpath) > UNIX_PATH_MAX)
+		die("\"%s\" is too long for Unix socket path (%zu > %d)",
+		    sockpath, strlen(sockpath), UNIX_PATH_MAX);
+
+	addr->sun_family = AF_UNIX;
+	strncpy(addr->sun_path, sockpath, UNIX_PATH_MAX);
+}
+
 static int connect_ctl(const char *sockpath, bool wait,
 		       struct holder_info *info,
 		       struct ucred *peercred)
 {
 	int fd = socket(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, PF_UNIX);
-	struct sockaddr_un addr = {
-		.sun_family = AF_UNIX,
-	};
+	struct sockaddr_un addr;
 	struct holder_info discard;
 	ssize_t len;
 	int rc;
@@ -108,7 +116,7 @@ static int connect_ctl(const char *sockpath, bool wait,
 	if (fd < 0)
 		die("socket(): %s\n", strerror(errno));
 
-	strncpy(addr.sun_path, sockpath, UNIX_PATH_MAX);
+	sockaddr_from_path(&addr, sockpath);
 
 	do {
 		rc = connect(fd, (struct sockaddr *)&addr, sizeof(addr));
@@ -149,9 +157,7 @@ static int connect_ctl(const char *sockpath, bool wait,
 static void cmd_hold(int argc, char *argv[])
 {
 	int fd = socket(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, PF_UNIX);
-	struct sockaddr_un addr = {
-		.sun_family = AF_UNIX,
-	};
+	struct sockaddr_un addr;
 	const char *sockpath = argv[1];
 	struct holder_info info;
 	int rc;
@@ -162,7 +168,7 @@ static void cmd_hold(int argc, char *argv[])
 	if (fd < 0)
 		die("socket(): %s\n", strerror(errno));
 
-	strncpy(addr.sun_path, sockpath, UNIX_PATH_MAX);
+	sockaddr_from_path(&addr, sockpath);
 
 	rc = bind(fd, (struct sockaddr *)&addr, sizeof(addr));
 	if (rc < 0)
-- 
@@ -93,14 +93,22 @@ static void usage(void)
 	    "    terminate.\n");
 }
 
+static void sockaddr_from_path(struct sockaddr_un *addr, const char *sockpath)
+{
+	if (strlen(sockpath) > UNIX_PATH_MAX)
+		die("\"%s\" is too long for Unix socket path (%zu > %d)",
+		    sockpath, strlen(sockpath), UNIX_PATH_MAX);
+
+	addr->sun_family = AF_UNIX;
+	strncpy(addr->sun_path, sockpath, UNIX_PATH_MAX);
+}
+
 static int connect_ctl(const char *sockpath, bool wait,
 		       struct holder_info *info,
 		       struct ucred *peercred)
 {
 	int fd = socket(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, PF_UNIX);
-	struct sockaddr_un addr = {
-		.sun_family = AF_UNIX,
-	};
+	struct sockaddr_un addr;
 	struct holder_info discard;
 	ssize_t len;
 	int rc;
@@ -108,7 +116,7 @@ static int connect_ctl(const char *sockpath, bool wait,
 	if (fd < 0)
 		die("socket(): %s\n", strerror(errno));
 
-	strncpy(addr.sun_path, sockpath, UNIX_PATH_MAX);
+	sockaddr_from_path(&addr, sockpath);
 
 	do {
 		rc = connect(fd, (struct sockaddr *)&addr, sizeof(addr));
@@ -149,9 +157,7 @@ static int connect_ctl(const char *sockpath, bool wait,
 static void cmd_hold(int argc, char *argv[])
 {
 	int fd = socket(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, PF_UNIX);
-	struct sockaddr_un addr = {
-		.sun_family = AF_UNIX,
-	};
+	struct sockaddr_un addr;
 	const char *sockpath = argv[1];
 	struct holder_info info;
 	int rc;
@@ -162,7 +168,7 @@ static void cmd_hold(int argc, char *argv[])
 	if (fd < 0)
 		die("socket(): %s\n", strerror(errno));
 
-	strncpy(addr.sun_path, sockpath, UNIX_PATH_MAX);
+	sockaddr_from_path(&addr, sockpath);
 
 	rc = bind(fd, (struct sockaddr *)&addr, sizeof(addr));
 	if (rc < 0)
-- 
2.40.1

[PATCH 2/2] test/nstool: Fix fd leak in accept() loop

[David Gibson]

nstool loops on accept(), but failed to close the accepted socket fds
before continuing on.  So, with repeated commands it would eventually die
with an EMFILE.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 test/nstool.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/test/nstool.c b/test/nstool.c
index bca9569..1bdf44e 100644
--- a/test/nstool.c
+++ b/test/nstool.c
@@ -201,6 +201,8 @@ static void cmd_hold(int argc, char *argv[])
 		rc = read(afd, &buf, sizeof(buf));
 		if (rc < 0)
 			die("read(): %s\n", strerror(errno));
+
+		close(afd);
 	} while (rc == 0);
 
 	unlink(sockpath);
-- 
@@ -201,6 +201,8 @@ static void cmd_hold(int argc, char *argv[])
 		rc = read(afd, &buf, sizeof(buf));
 		if (rc < 0)
 			die("read(): %s\n", strerror(errno));
+
+		close(afd);
 	} while (rc == 0);
 
 	unlink(sockpath);
-- 
2.40.1

Re: [PATCH 0/2] Fixes for nstool

[Stefano Brivio]

On Tue, 23 May 2023 12:25:41 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> This fixes a couple of bugs I found in nstool.  I discovered these
> during the Avocado work, but they are bugs independent of that.
> 
> David Gibson (2):
>   test/nstool: Provide useful error if given a path that's too long
>   test/nstool: Fix fd leak in accept() loop

Applied.

-- 
Stefano