ltp.lists.linux.it archive mirror
 help / color / mirror / Atom feed
* [LTP] [PATCH v1] tst_tmpdir: Fix buffer overflow in tst_tmpdir.c
@ 2025-05-21 14:16 Wei Gao via ltp
  2025-05-22 19:39 ` Petr Vorel
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Wei Gao via ltp @ 2025-05-21 14:16 UTC (permalink / raw)
  To: ltp

Using sprintf without length checking in tst_tmpdir may lead to buffer overflow.
So in this patch use openat() instead of open().

Fixs:1241
Signed-off-by: Wei Gao <wegao@suse.com>
---
 lib/tst_tmpdir.c | 119 +++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 111 insertions(+), 8 deletions(-)

diff --git a/lib/tst_tmpdir.c b/lib/tst_tmpdir.c
index 6ed2367b9..7bd55022d 100644
--- a/lib/tst_tmpdir.c
+++ b/lib/tst_tmpdir.c
@@ -102,6 +102,7 @@ static char test_start_work_dir[PATH_MAX];
 extern futex_t *tst_futexes;
 
 static int rmobj(const char *obj, char **errmsg);
+static int rmobjat(int dir_fd, const char *obj, char **errmsg);
 
 int tst_tmpdir_created(void)
 {
@@ -149,8 +150,8 @@ static int purge_dir(const char *path, char **errptr)
 	int ret_val = 0;
 	DIR *dir;
 	struct dirent *dir_ent;
-	char dirobj[PATH_MAX];
 	static char err_msg[PATH_MAX + 1280];
+	int dir_fd = -1;
 
 	/* Do NOT perform the request if the directory is "/" */
 	if (!strcmp(path, "/")) {
@@ -167,7 +168,7 @@ static int purge_dir(const char *path, char **errptr)
 	/* Open the directory to get access to what is in it */
 	if (!(dir = opendir(path))) {
 		if (errptr) {
-			sprintf(err_msg,
+			snprintf(err_msg, sizeof(err_msg),
 				"Cannot open directory %s; errno=%d: %s",
 				path, errno, tst_strerrno(errno));
 			*errptr = err_msg;
@@ -175,6 +176,18 @@ static int purge_dir(const char *path, char **errptr)
 		return -1;
 	}
 
+	dir_fd = dirfd(dir);
+	if (dir_fd == -1) {
+		closedir(dir);
+		if (errptr) {
+			snprintf(err_msg, sizeof(err_msg),
+				"Cannot get file descriptor for directory %s; errno=%d: %s",
+				path, errno, tst_strerrno(errno));
+			*errptr = err_msg;
+		}
+		return -1;
+	}
+
 	/* Loop through the entries in the directory, removing each one */
 	for (dir_ent = readdir(dir); dir_ent; dir_ent = readdir(dir)) {
 		/* Don't remove "." or ".." */
@@ -183,8 +196,57 @@ static int purge_dir(const char *path, char **errptr)
 			continue;
 
 		/* Recursively remove the current entry */
-		sprintf(dirobj, "%s/%s", path, dir_ent->d_name);
-		if (rmobj(dirobj, errptr) != 0)
+		if (rmobjat(dir_fd, dir_ent->d_name, errptr) != 0)
+			ret_val = -1;
+	}
+
+	closedir(dir);
+	return ret_val;
+}
+
+static int purge_dirat(int dir_fd, const char *path, char **errptr)
+{
+	int ret_val = 0;
+	DIR *dir;
+	struct dirent *dir_ent;
+	static char err_msg[PATH_MAX + 1280];
+	int subdir_fd;
+
+	errno = 0;
+
+	/* Open the subdirectory using openat */
+	subdir_fd = openat(dir_fd, path, O_RDONLY | O_DIRECTORY | O_NOFOLLOW);
+	if (subdir_fd < 0) {
+		if (errptr) {
+			snprintf(err_msg, sizeof(err_msg),
+				 "Cannot open subdirectory %s (via fd %d); errno=%d: %s",
+				 path, dir_fd, errno, tst_strerrno(errno));
+			*errptr = err_msg;
+		}
+		return -1;
+	}
+
+	dir = fdopendir(subdir_fd);
+	if (!dir) {
+		if (errptr) {
+			snprintf(err_msg, sizeof(err_msg),
+				 "Cannot open directory stream for %s (via fd %d); errno=%d: %s",
+				 path, dir_fd, errno, tst_strerrno(errno));
+			*errptr = err_msg;
+		}
+		close(subdir_fd);
+		return -1;
+	}
+
+	/* Loop through the entries in the directory, removing each one */
+	for (dir_ent = readdir(dir); dir_ent; dir_ent = readdir(dir)) {
+		/* Don't remove "." or ".." */
+		if (!strcmp(dir_ent->d_name, ".")
+		    || !strcmp(dir_ent->d_name, ".."))
+			continue;
+
+		/* Recursively remove the current entry */
+		if (rmobjat(subdir_fd, dir_ent->d_name, errptr) != 0)
 			ret_val = -1;
 	}
 
@@ -212,7 +274,7 @@ static int rmobj(const char *obj, char **errmsg)
 		/* Get the link count, now that all the entries have been removed */
 		if (lstat(obj, &statbuf) < 0) {
 			if (errmsg != NULL) {
-				sprintf(err_msg,
+				snprintf(err_msg, sizeof(err_msg),
 					"lstat(%s) failed; errno=%d: %s", obj,
 					errno, tst_strerrno(errno));
 				*errmsg = err_msg;
@@ -225,7 +287,7 @@ static int rmobj(const char *obj, char **errmsg)
 			/* The directory is linked; unlink() must be used */
 			if (unlink(obj) < 0) {
 				if (errmsg != NULL) {
-					sprintf(err_msg,
+					snprintf(err_msg, sizeof(err_msg),
 						"unlink(%s) failed; errno=%d: %s",
 						obj, errno, tst_strerrno(errno));
 					*errmsg = err_msg;
@@ -236,7 +298,7 @@ static int rmobj(const char *obj, char **errmsg)
 			/* The directory is not linked; remove() can be used */
 			if (remove(obj) < 0) {
 				if (errmsg != NULL) {
-					sprintf(err_msg,
+					snprintf(err_msg, sizeof(err_msg),
 						"remove(%s) failed; errno=%d: %s",
 						obj, errno, tst_strerrno(errno));
 					*errmsg = err_msg;
@@ -247,7 +309,7 @@ static int rmobj(const char *obj, char **errmsg)
 	} else {
 		if (unlink(obj) < 0) {
 			if (errmsg != NULL) {
-				sprintf(err_msg,
+				snprintf(err_msg, sizeof(err_msg),
 					"unlink(%s) failed; errno=%d: %s", obj,
 					errno, tst_strerrno(errno));
 				*errmsg = err_msg;
@@ -259,6 +321,47 @@ static int rmobj(const char *obj, char **errmsg)
 	return 0;
 }
 
+static int rmobjat(int dir_fd, const char *obj, char **errmsg)
+{
+	int ret_val = 0;
+	struct stat statbuf;
+	static char err_msg[PATH_MAX + 1280];
+	int fd;
+
+	fd = openat(dir_fd, obj, O_DIRECTORY | O_NOFOLLOW);
+	if (fd >= 0) {
+		close(fd);
+		ret_val = purge_dirat(dir_fd, obj, errmsg);
+
+		/* If there were problems removing an entry, don't attempt to
+		   remove the directory itself */
+		if (ret_val == -1)
+			return -1;
+
+		if (unlinkat(dir_fd, obj, AT_REMOVEDIR) < 0) {
+			if (errmsg != NULL) {
+				snprintf(err_msg, sizeof(err_msg),
+						"remove(%s) failed; errno=%d: %s",
+						obj, errno, tst_strerrno(errno));
+				*errmsg = err_msg;
+			}
+			return -1;
+		}
+	} else {
+		if (unlinkat(dir_fd, obj, 0) < 0) {
+			if (errmsg != NULL) {
+				snprintf(err_msg, sizeof(err_msg),
+					"unlinkat(%s) failed; errno=%d: %s", obj,
+					errno, tst_strerrno(errno));
+				*errmsg = err_msg;
+			}
+			return -1;
+		}
+	}
+
+	return 0;
+}
+
 void tst_tmpdir(void)
 {
 	char template[PATH_MAX];
-- 
2.49.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

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

* Re: [LTP] [PATCH v1] tst_tmpdir: Fix buffer overflow in tst_tmpdir.c
  2025-05-21 14:16 [LTP] [PATCH v1] tst_tmpdir: Fix buffer overflow in tst_tmpdir.c Wei Gao via ltp
@ 2025-05-22 19:39 ` Petr Vorel
  2025-05-23  6:01   ` Jan Stancek via ltp
  2025-05-23 21:09 ` [LTP] [PATCH v2] " Wei Gao via ltp
  2025-09-01 15:08 ` [LTP] [PATCH v1] " Cyril Hrubis
  2 siblings, 1 reply; 7+ messages in thread
From: Petr Vorel @ 2025-05-22 19:39 UTC (permalink / raw)
  To: Wei Gao; +Cc: ltp

Hi Wei, all,

@all: is this a candidate for a release? It'd be nice to get it fixed.

> Using sprintf without length checking in tst_tmpdir may lead to buffer overflow.
> So in this patch use openat() instead of open().

LGTM, but it'd be nice if we could use only rmobjat().

Could you please remove the unused variable?

tst_tmpdir.c: In function ‘rmobjat’:
tst_tmpdir.c:327:21: warning: unused variable ‘statbuf’ [-Wunused-variable]
  327 |         struct stat statbuf;
      |                     ^~~~~~~


Suggested-by: Cyril Hrubis <chrubis@suse.cz>

> Fixs:1241

This is better as it shows link in GitHub web:
Fixes: #1241

Or, IMHO better
Fixes: https://github.com/linux-test-project/ltp/issues/1241

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

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

* Re: [LTP] [PATCH v1] tst_tmpdir: Fix buffer overflow in tst_tmpdir.c
  2025-05-22 19:39 ` Petr Vorel
@ 2025-05-23  6:01   ` Jan Stancek via ltp
  2025-05-23  6:29     ` Petr Vorel
  2025-05-23  6:29     ` Cyril Hrubis
  0 siblings, 2 replies; 7+ messages in thread
From: Jan Stancek via ltp @ 2025-05-23  6:01 UTC (permalink / raw)
  To: Petr Vorel; +Cc: ltp

On Thu, May 22, 2025 at 9:39 PM Petr Vorel <pvorel@suse.cz> wrote:
>
> Hi Wei, all,
>
> @all: is this a candidate for a release? It'd be nice to get it fixed.

I'd wait after release, it's not a trivial change and other than static analysis
there are no reports of it actually happening.  And it also allows more time
for review.

>
> > Using sprintf without length checking in tst_tmpdir may lead to buffer overflow.
> > So in this patch use openat() instead of open().
>
> LGTM, but it'd be nice if we could use only rmobjat().
>
> Could you please remove the unused variable?
>
> tst_tmpdir.c: In function ‘rmobjat’:
> tst_tmpdir.c:327:21: warning: unused variable ‘statbuf’ [-Wunused-variable]
>   327 |         struct stat statbuf;
>       |                     ^~~~~~~
>
>
> Suggested-by: Cyril Hrubis <chrubis@suse.cz>
>
> > Fixs:1241
>
> This is better as it shows link in GitHub web:
> Fixes: #1241
>
> Or, IMHO better
> Fixes: https://github.com/linux-test-project/ltp/issues/1241
>
> Kind regards,
> Petr
>


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

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

* Re: [LTP] [PATCH v1] tst_tmpdir: Fix buffer overflow in tst_tmpdir.c
  2025-05-23  6:01   ` Jan Stancek via ltp
@ 2025-05-23  6:29     ` Petr Vorel
  2025-05-23  6:29     ` Cyril Hrubis
  1 sibling, 0 replies; 7+ messages in thread
From: Petr Vorel @ 2025-05-23  6:29 UTC (permalink / raw)
  To: Jan Stancek; +Cc: ltp

Hi Jan, all,

> On Thu, May 22, 2025 at 9:39 PM Petr Vorel <pvorel@suse.cz> wrote:

> > Hi Wei, all,

> > @all: is this a candidate for a release? It'd be nice to get it fixed.

> I'd wait after release, it's not a trivial change and other than static analysis
> there are no reports of it actually happening.  And it also allows more time
> for review.

Makes sense.

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

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

* Re: [LTP] [PATCH v1] tst_tmpdir: Fix buffer overflow in tst_tmpdir.c
  2025-05-23  6:01   ` Jan Stancek via ltp
  2025-05-23  6:29     ` Petr Vorel
@ 2025-05-23  6:29     ` Cyril Hrubis
  1 sibling, 0 replies; 7+ messages in thread
From: Cyril Hrubis @ 2025-05-23  6:29 UTC (permalink / raw)
  To: Jan Stancek; +Cc: ltp

Hi!
> > @all: is this a candidate for a release? It'd be nice to get it fixed.
> 
> I'd wait after release, it's not a trivial change and other than static analysis
> there are no reports of it actually happening.  And it also allows more time
> for review.

Agree here.

I guess that it could be triggered if we pass long enough TMPDIR= to the
tests, but for changes like this we need a proper testing phase and we
are too close to the release for that to happen.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

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

* [LTP] [PATCH v2] tst_tmpdir: Fix buffer overflow in tst_tmpdir.c
  2025-05-21 14:16 [LTP] [PATCH v1] tst_tmpdir: Fix buffer overflow in tst_tmpdir.c Wei Gao via ltp
  2025-05-22 19:39 ` Petr Vorel
@ 2025-05-23 21:09 ` Wei Gao via ltp
  2025-09-01 15:08 ` [LTP] [PATCH v1] " Cyril Hrubis
  2 siblings, 0 replies; 7+ messages in thread
From: Wei Gao via ltp @ 2025-05-23 21:09 UTC (permalink / raw)
  To: ltp

Using sprintf without length checking in tst_tmpdir may lead to buffer overflow.
So in this patch use openat() instead of open().

Fixes: https://github.com/linux-test-project/ltp/issues/1241
Suggested-by: Cyril Hrubis <chrubis@suse.cz>
Signed-off-by: Wei Gao <wegao@suse.com>
---
 lib/tst_tmpdir.c | 122 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 78 insertions(+), 44 deletions(-)

diff --git a/lib/tst_tmpdir.c b/lib/tst_tmpdir.c
index 6ed2367b9..bcb9534c5 100644
--- a/lib/tst_tmpdir.c
+++ b/lib/tst_tmpdir.c
@@ -101,7 +101,7 @@ static char test_start_work_dir[PATH_MAX];
 /* lib/tst_checkpoint.c */
 extern futex_t *tst_futexes;
 
-static int rmobj(const char *obj, char **errmsg);
+static int rmobjat(int dir_fd, const char *obj, char **errmsg);
 
 int tst_tmpdir_created(void)
 {
@@ -149,8 +149,8 @@ static int purge_dir(const char *path, char **errptr)
 	int ret_val = 0;
 	DIR *dir;
 	struct dirent *dir_ent;
-	char dirobj[PATH_MAX];
 	static char err_msg[PATH_MAX + 1280];
+	int dir_fd = -1;
 
 	/* Do NOT perform the request if the directory is "/" */
 	if (!strcmp(path, "/")) {
@@ -167,7 +167,7 @@ static int purge_dir(const char *path, char **errptr)
 	/* Open the directory to get access to what is in it */
 	if (!(dir = opendir(path))) {
 		if (errptr) {
-			sprintf(err_msg,
+			snprintf(err_msg, sizeof(err_msg),
 				"Cannot open directory %s; errno=%d: %s",
 				path, errno, tst_strerrno(errno));
 			*errptr = err_msg;
@@ -175,6 +175,68 @@ static int purge_dir(const char *path, char **errptr)
 		return -1;
 	}
 
+	dir_fd = dirfd(dir);
+	if (dir_fd == -1) {
+		closedir(dir);
+		if (errptr) {
+			snprintf(err_msg, sizeof(err_msg),
+				"Cannot get file descriptor for directory %s; errno=%d: %s",
+				path, errno, tst_strerrno(errno));
+			*errptr = err_msg;
+		}
+		return -1;
+	}
+
+	/* Loop through the entries in the directory, removing each one */
+	for (dir_ent = readdir(dir); dir_ent; dir_ent = readdir(dir)) {
+		/* Don't remove "." or ".." */
+		if (!strcmp(dir_ent->d_name, ".")
+		    || !strcmp(dir_ent->d_name, ".."))
+			continue;
+
+		/* Recursively remove the current entry */
+		if (rmobjat(dir_fd, dir_ent->d_name, errptr) != 0)
+			ret_val = -1;
+	}
+
+	closedir(dir);
+	return ret_val;
+}
+
+static int purge_dirat(int dir_fd, const char *path, char **errptr)
+{
+	int ret_val = 0;
+	DIR *dir;
+	struct dirent *dir_ent;
+	static char err_msg[PATH_MAX + 1280];
+	int subdir_fd;
+
+	errno = 0;
+
+	/* Open the subdirectory using openat */
+	subdir_fd = openat(dir_fd, path, O_RDONLY | O_DIRECTORY | O_NOFOLLOW);
+	if (subdir_fd < 0) {
+		if (errptr) {
+			snprintf(err_msg, sizeof(err_msg),
+				 "Cannot open subdirectory %s (via fd %d); errno=%d: %s",
+				 path, dir_fd, errno, tst_strerrno(errno));
+			*errptr = err_msg;
+		}
+		return -1;
+	}
+
+	dir = fdopendir(subdir_fd);
+	if (!dir) {
+		if (errptr) {
+			snprintf(err_msg, sizeof(err_msg),
+				 "Cannot open directory stream for %s (via fd %d); errno=%d: %s",
+				 path, dir_fd, errno, tst_strerrno(errno));
+			*errptr = err_msg;
+		}
+		close(subdir_fd);
+		return -1;
+	}
+
 	/* Loop through the entries in the directory, removing each one */
 	for (dir_ent = readdir(dir); dir_ent; dir_ent = readdir(dir)) {
 		/* Don't remove "." or ".." */
@@ -183,8 +245,7 @@ static int purge_dir(const char *path, char **errptr)
 			continue;
 
 		/* Recursively remove the current entry */
-		sprintf(dirobj, "%s/%s", path, dir_ent->d_name);
-		if (rmobj(dirobj, errptr) != 0)
+		if (rmobjat(subdir_fd, dir_ent->d_name, errptr) != 0)
 			ret_val = -1;
 	}
 
@@ -192,63 +253,36 @@ static int purge_dir(const char *path, char **errptr)
 	return ret_val;
 }
 
-static int rmobj(const char *obj, char **errmsg)
+static int rmobjat(int dir_fd, const char *obj, char **errmsg)
 {
 	int ret_val = 0;
-	struct stat statbuf;
 	static char err_msg[PATH_MAX + 1280];
 	int fd;
 
-	fd = open(obj, O_DIRECTORY | O_NOFOLLOW);
+	fd = openat(dir_fd, obj, O_DIRECTORY | O_NOFOLLOW);
 	if (fd >= 0) {
 		close(fd);
-		ret_val = purge_dir(obj, errmsg);
+		ret_val = purge_dirat(dir_fd, obj, errmsg);
 
 		/* If there were problems removing an entry, don't attempt to
 		   remove the directory itself */
 		if (ret_val == -1)
 			return -1;
 
-		/* Get the link count, now that all the entries have been removed */
-		if (lstat(obj, &statbuf) < 0) {
+		if (unlinkat(dir_fd, obj, AT_REMOVEDIR) < 0) {
 			if (errmsg != NULL) {
-				sprintf(err_msg,
-					"lstat(%s) failed; errno=%d: %s", obj,
-					errno, tst_strerrno(errno));
-				*errmsg = err_msg;
-			}
-			return -1;
-		}
-
-		/* Remove the directory itself */
-		if (statbuf.st_nlink >= 3) {
-			/* The directory is linked; unlink() must be used */
-			if (unlink(obj) < 0) {
-				if (errmsg != NULL) {
-					sprintf(err_msg,
-						"unlink(%s) failed; errno=%d: %s",
-						obj, errno, tst_strerrno(errno));
-					*errmsg = err_msg;
-				}
-				return -1;
-			}
-		} else {
-			/* The directory is not linked; remove() can be used */
-			if (remove(obj) < 0) {
-				if (errmsg != NULL) {
-					sprintf(err_msg,
+				snprintf(err_msg, sizeof(err_msg),
 						"remove(%s) failed; errno=%d: %s",
 						obj, errno, tst_strerrno(errno));
-					*errmsg = err_msg;
-				}
-				return -1;
+				*errmsg = err_msg;
 			}
+			return -1;
 		}
 	} else {
-		if (unlink(obj) < 0) {
+		if (unlinkat(dir_fd, obj, 0) < 0) {
 			if (errmsg != NULL) {
-				sprintf(err_msg,
-					"unlink(%s) failed; errno=%d: %s", obj,
+				snprintf(err_msg, sizeof(err_msg),
+					"unlinkat(%s) failed; errno=%d: %s", obj,
 					errno, tst_strerrno(errno));
 				*errmsg = err_msg;
 			}
@@ -305,7 +339,7 @@ void tst_tmpdir(void)
 		tst_resm(TERRNO, "%s: chdir(%s) failed", __func__, TESTDIR);
 
 		/* Try to remove the directory */
-		if (rmobj(TESTDIR, &errmsg) == -1) {
+		if (rmobjat(0, TESTDIR, &errmsg) == -1) {
 			tst_resm(TWARN, "%s: rmobj(%s) failed: %s",
 				 __func__, TESTDIR, errmsg);
 		}
@@ -343,7 +377,7 @@ void tst_rmdir(void)
 	/*
 	 * Attempt to remove the "TESTDIR" directory, using rmobj().
 	 */
-	if (rmobj(TESTDIR, &errmsg) == -1) {
+	if (rmobjat(0, TESTDIR, &errmsg) == -1) {
 		tst_resm(TWARN, "%s: rmobj(%s) failed: %s",
 			 __func__, TESTDIR, errmsg);
 	}
-- 
2.49.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

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

* Re: [LTP] [PATCH v1] tst_tmpdir: Fix buffer overflow in tst_tmpdir.c
  2025-05-21 14:16 [LTP] [PATCH v1] tst_tmpdir: Fix buffer overflow in tst_tmpdir.c Wei Gao via ltp
  2025-05-22 19:39 ` Petr Vorel
  2025-05-23 21:09 ` [LTP] [PATCH v2] " Wei Gao via ltp
@ 2025-09-01 15:08 ` Cyril Hrubis
  2 siblings, 0 replies; 7+ messages in thread
From: Cyril Hrubis @ 2025-09-01 15:08 UTC (permalink / raw)
  To: Wei Gao; +Cc: ltp

Hi!
> Using sprintf without length checking in tst_tmpdir may lead to buffer overflow.
> So in this patch use openat() instead of open().
> 
> Fixs:1241
> Signed-off-by: Wei Gao <wegao@suse.com>
> ---
>  lib/tst_tmpdir.c | 119 +++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 111 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/tst_tmpdir.c b/lib/tst_tmpdir.c
> index 6ed2367b9..7bd55022d 100644
> --- a/lib/tst_tmpdir.c
> +++ b/lib/tst_tmpdir.c
> @@ -102,6 +102,7 @@ static char test_start_work_dir[PATH_MAX];
>  extern futex_t *tst_futexes;
>  
>  static int rmobj(const char *obj, char **errmsg);

We shouldn't keep the old rmobj() purge_dir() calls, these should be
replaced by rmobjat() and purge_dirat(). Hint: use AT_FDCWD when there
is no dir_fd to pass.

> +static int rmobjat(int dir_fd, const char *obj, char **errmsg);
>  
>  int tst_tmpdir_created(void)
>  {
> @@ -149,8 +150,8 @@ static int purge_dir(const char *path, char **errptr)
>  	int ret_val = 0;
>  	DIR *dir;
>  	struct dirent *dir_ent;
> -	char dirobj[PATH_MAX];
>  	static char err_msg[PATH_MAX + 1280];
> +	int dir_fd = -1;
>  
>  	/* Do NOT perform the request if the directory is "/" */
>  	if (!strcmp(path, "/")) {
> @@ -167,7 +168,7 @@ static int purge_dir(const char *path, char **errptr)
>  	/* Open the directory to get access to what is in it */
>  	if (!(dir = opendir(path))) {
>  		if (errptr) {
> -			sprintf(err_msg,
> +			snprintf(err_msg, sizeof(err_msg),
>  				"Cannot open directory %s; errno=%d: %s",
>  				path, errno, tst_strerrno(errno));
>  			*errptr = err_msg;
> @@ -175,6 +176,18 @@ static int purge_dir(const char *path, char **errptr)
>  		return -1;
>  	}
>  
> +	dir_fd = dirfd(dir);
> +	if (dir_fd == -1) {
> +		closedir(dir);
> +		if (errptr) {
> +			snprintf(err_msg, sizeof(err_msg),
> +				"Cannot get file descriptor for directory %s; errno=%d: %s",
> +				path, errno, tst_strerrno(errno));
> +			*errptr = err_msg;
> +		}
> +		return -1;
> +	}
> +
>  	/* Loop through the entries in the directory, removing each one */
>  	for (dir_ent = readdir(dir); dir_ent; dir_ent = readdir(dir)) {
>  		/* Don't remove "." or ".." */
> @@ -183,8 +196,57 @@ static int purge_dir(const char *path, char **errptr)
>  			continue;
>  
>  		/* Recursively remove the current entry */
> -		sprintf(dirobj, "%s/%s", path, dir_ent->d_name);
> -		if (rmobj(dirobj, errptr) != 0)
> +		if (rmobjat(dir_fd, dir_ent->d_name, errptr) != 0)
> +			ret_val = -1;
> +	}

close(dir_fd) here?

> +	closedir(dir);
> +	return ret_val;
> +}
> +
> +static int purge_dirat(int dir_fd, const char *path, char **errptr)
> +{
> +	int ret_val = 0;
> +	DIR *dir;
> +	struct dirent *dir_ent;
> +	static char err_msg[PATH_MAX + 1280];
> +	int subdir_fd;
> +
> +	errno = 0;
> +
> +	/* Open the subdirectory using openat */
> +	subdir_fd = openat(dir_fd, path, O_RDONLY | O_DIRECTORY | O_NOFOLLOW);
> +	if (subdir_fd < 0) {
> +		if (errptr) {
> +			snprintf(err_msg, sizeof(err_msg),
> +				 "Cannot open subdirectory %s (via fd %d); errno=%d: %s",
> +				 path, dir_fd, errno, tst_strerrno(errno));
> +			*errptr = err_msg;
> +		}
> +		return -1;
> +	}
> +
> +	dir = fdopendir(subdir_fd);
> +	if (!dir) {
> +		if (errptr) {
> +			snprintf(err_msg, sizeof(err_msg),
> +				 "Cannot open directory stream for %s (via fd %d); errno=%d: %s",
> +				 path, dir_fd, errno, tst_strerrno(errno));
> +			*errptr = err_msg;
> +		}
> +		close(subdir_fd);
> +		return -1;
> +	}
> +
> +	/* Loop through the entries in the directory, removing each one */
> +	for (dir_ent = readdir(dir); dir_ent; dir_ent = readdir(dir)) {
> +		/* Don't remove "." or ".." */
> +		if (!strcmp(dir_ent->d_name, ".")
> +		    || !strcmp(dir_ent->d_name, ".."))
> +			continue;
> +
> +		/* Recursively remove the current entry */
> +		if (rmobjat(subdir_fd, dir_ent->d_name, errptr) != 0)
>  			ret_val = -1;
>  	}
>  
> @@ -212,7 +274,7 @@ static int rmobj(const char *obj, char **errmsg)
>  		/* Get the link count, now that all the entries have been removed */
>  		if (lstat(obj, &statbuf) < 0) {
>  			if (errmsg != NULL) {
> -				sprintf(err_msg,
> +				snprintf(err_msg, sizeof(err_msg),
>  					"lstat(%s) failed; errno=%d: %s", obj,
>  					errno, tst_strerrno(errno));
>  				*errmsg = err_msg;
> @@ -225,7 +287,7 @@ static int rmobj(const char *obj, char **errmsg)
>  			/* The directory is linked; unlink() must be used */
>  			if (unlink(obj) < 0) {
>  				if (errmsg != NULL) {
> -					sprintf(err_msg,
> +					snprintf(err_msg, sizeof(err_msg),
>  						"unlink(%s) failed; errno=%d: %s",
>  						obj, errno, tst_strerrno(errno));
>  					*errmsg = err_msg;
> @@ -236,7 +298,7 @@ static int rmobj(const char *obj, char **errmsg)
>  			/* The directory is not linked; remove() can be used */
>  			if (remove(obj) < 0) {
>  				if (errmsg != NULL) {
> -					sprintf(err_msg,
> +					snprintf(err_msg, sizeof(err_msg),
>  						"remove(%s) failed; errno=%d: %s",
>  						obj, errno, tst_strerrno(errno));
>  					*errmsg = err_msg;
> @@ -247,7 +309,7 @@ static int rmobj(const char *obj, char **errmsg)
>  	} else {
>  		if (unlink(obj) < 0) {
>  			if (errmsg != NULL) {
> -				sprintf(err_msg,
> +				snprintf(err_msg, sizeof(err_msg),
>  					"unlink(%s) failed; errno=%d: %s", obj,
>  					errno, tst_strerrno(errno));
>  				*errmsg = err_msg;
> @@ -259,6 +321,47 @@ static int rmobj(const char *obj, char **errmsg)
>  	return 0;
>  }
>  
> +static int rmobjat(int dir_fd, const char *obj, char **errmsg)
> +{
> +	int ret_val = 0;
> +	struct stat statbuf;
> +	static char err_msg[PATH_MAX + 1280];
> +	int fd;
> +
> +	fd = openat(dir_fd, obj, O_DIRECTORY | O_NOFOLLOW);
> +	if (fd >= 0) {
> +		close(fd);

We close the fd here just to open it in purge_dirat(), shouldn't we pass
it to the purge_dirat() function instead?

> +		ret_val = purge_dirat(dir_fd, obj, errmsg);
> +
> +		/* If there were problems removing an entry, don't attempt to
> +		   remove the directory itself */
> +		if (ret_val == -1)
> +			return -1;
> +
> +		if (unlinkat(dir_fd, obj, AT_REMOVEDIR) < 0) {
> +			if (errmsg != NULL) {
> +				snprintf(err_msg, sizeof(err_msg),
> +						"remove(%s) failed; errno=%d: %s",
> +						obj, errno, tst_strerrno(errno));
> +				*errmsg = err_msg;
> +			}
> +			return -1;
> +		}
> +	} else {
> +		if (unlinkat(dir_fd, obj, 0) < 0) {
> +			if (errmsg != NULL) {
> +				snprintf(err_msg, sizeof(err_msg),
> +					"unlinkat(%s) failed; errno=%d: %s", obj,
> +					errno, tst_strerrno(errno));
> +				*errmsg = err_msg;
> +			}
> +			return -1;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  void tst_tmpdir(void)
>  {
>  	char template[PATH_MAX];
> -- 
> 2.49.0
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

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

end of thread, other threads:[~2025-09-01 15:07 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-05-21 14:16 [LTP] [PATCH v1] tst_tmpdir: Fix buffer overflow in tst_tmpdir.c Wei Gao via ltp
2025-05-22 19:39 ` Petr Vorel
2025-05-23  6:01   ` Jan Stancek via ltp
2025-05-23  6:29     ` Petr Vorel
2025-05-23  6:29     ` Cyril Hrubis
2025-05-23 21:09 ` [LTP] [PATCH v2] " Wei Gao via ltp
2025-09-01 15:08 ` [LTP] [PATCH v1] " Cyril Hrubis

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).