[LTP] [PATCH v1] tst_tmpdir: Fix buffer overflow in tst_tmpdir.c

[LTP] [PATCH v1] tst_tmpdir: Fix buffer overflow in tst_tmpdir.c

[Wei Gao via 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

Re: [LTP] [PATCH v1] tst_tmpdir: Fix buffer overflow in tst_tmpdir.c

[Petr Vorel]

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

Re: [LTP] [PATCH v1] tst_tmpdir: Fix buffer overflow in tst_tmpdir.c

[Jan Stancek via 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

Re: [LTP] [PATCH v1] tst_tmpdir: Fix buffer overflow in tst_tmpdir.c

[Petr Vorel]

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

Re: [LTP] [PATCH v1] tst_tmpdir: Fix buffer overflow in tst_tmpdir.c

[Cyril Hrubis]

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

[LTP] [PATCH v2] tst_tmpdir: Fix buffer overflow in tst_tmpdir.c

[Wei Gao via 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

Re: [LTP] [PATCH v2] tst_tmpdir: Fix buffer overflow in tst_tmpdir.c

[Cyril Hrubis]

Hi!
> 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;
>  }

Again, there shouldn't be two function for the same job. There should be
only purge_dirat() and the tst_purge_dir() should call purge_dirat()
with AT_FDCWD as the dirfd.

> -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;
>  		}

I do not think that we can remove the part where we check the number of
links to the directory. I suppose that the new code should look like:

	int flags = AT_REMOVEDIR;

	if (lstatat(dir_fd, &statbuf)) {
		...
	}

	if (statbuf.st_nlink >= 3)
		flags = 0;

	if (unlinkat(dir_fd, obj, flags)) {
		...
	}


>  	} 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);
>  	}

We should pass AT_FDCWD to the two rmobjat() here since it's possible to
pass relative path in the TMPDIR environment variable. Otherwise the
code will not work with e.g. TMPDIR="." ./test_foo

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH v2] tst_tmpdir: Fix buffer overflow in tst_tmpdir.c

[Petr Vorel]

Hi all,

...
> > @@ -192,63 +253,36 @@ static int purge_dir(const char *path, char **errptr)
> >  	return ret_val;
> >  }

> Again, there shouldn't be two function for the same job. There should be
> only purge_dirat() and the tst_purge_dir() should call purge_dirat()
> with AT_FDCWD as the dirfd.

+1. That is what I meant by "it'd be nice if we could use only rmobjat()" in v1.

> >  	} 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);
> >  	}

> We should pass AT_FDCWD to the two rmobjat() here since it's possible to
> pass relative path in the TMPDIR environment variable. Otherwise the
> code will not work with e.g. TMPDIR="." ./test_foo

Very good catch. BTW we expect TMPDIR to be correct - without double quotes and
trailing '/' which are example for shell tests which lead to failures in LTP
NFS tests. That was fixed in:

273c497935 ("tst_test.sh: Remove possible double/trailing slashes from TMPDIR")

Wouldn't be better just to normalize relative TMPDIR to absolute path? Simple
realpath() would do the job, right?

e.g. this patch "swapon03: Remove grep dependency" [1] + fix attempt to swapoff
leftover from previous run (when one does ctrl+C in previous run) expect that
TMPDIR is absolute. I'll note it below the patch that either we change
lib/tst_tmpdir.c to convert relative to absolute, or swapon03.c test needs to do
it itself. I would prefer lib/tst_tmpdir.c do the job including normalizing the
path (more tests will benefit/need it).

Kind regards,
Petr

[1] https://lore.kernel.org/ltp/20251106163500.1063704-6-pvorel@suse.cz/


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

Re: [LTP] [PATCH v2] tst_tmpdir: Fix buffer overflow in tst_tmpdir.c

[Wei Gao via ltp]

On Fri, Nov 14, 2025 at 09:58:56AM +0100, Petr Vorel wrote:
> Hi all,
> 
> ...
> > > @@ -192,63 +253,36 @@ static int purge_dir(const char *path, char **errptr)
> > >  	return ret_val;
> > >  }
> 
> > Again, there shouldn't be two function for the same job. There should be
> > only purge_dirat() and the tst_purge_dir() should call purge_dirat()
> > with AT_FDCWD as the dirfd.
> 
> +1. That is what I meant by "it'd be nice if we could use only rmobjat()" in v1.
> 
> > >  	} 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);
> > >  	}
> 
> > We should pass AT_FDCWD to the two rmobjat() here since it's possible to
> > pass relative path in the TMPDIR environment variable. Otherwise the
> > code will not work with e.g. TMPDIR="." ./test_foo
> 
> Very good catch. BTW we expect TMPDIR to be correct - without double quotes and
> trailing '/' which are example for shell tests which lead to failures in LTP
> NFS tests. That was fixed in:
> 
> 273c497935 ("tst_test.sh: Remove possible double/trailing slashes from TMPDIR")
> 
> Wouldn't be better just to normalize relative TMPDIR to absolute path? Simple
> realpath() would do the job, right?
> 
> e.g. this patch "swapon03: Remove grep dependency" [1] + fix attempt to swapoff
> leftover from previous run (when one does ctrl+C in previous run) expect that
> TMPDIR is absolute. I'll note it below the patch that either we change
> lib/tst_tmpdir.c to convert relative to absolute, or swapon03.c test needs to do
> it itself. I would prefer lib/tst_tmpdir.c do the job including normalizing the
> path (more tests will benefit/need it).
Also i guess we need use another patch for this?
> 
> Kind regards,
> Petr
> 
> [1] https://lore.kernel.org/ltp/20251106163500.1063704-6-pvorel@suse.cz/
> 

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

Re: [LTP] [PATCH v2] tst_tmpdir: Fix buffer overflow in tst_tmpdir.c

[Wei Gao via ltp]

On Thu, Nov 13, 2025 at 03:38:08PM +0100, Cyril Hrubis wrote:
> Hi!
> > 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>
> 
> >  	} 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);
> >  	}
> 
> We should pass AT_FDCWD to the two rmobjat() here since it's possible to
> pass relative path in the TMPDIR environment variable. Otherwise the
> code will not work with e.g. TMPDIR="." ./test_foo
> 

My test show we never support TMPDIR=".", following error will show:
tst_tmpdir.c:135: TBROK: You must specify an absolute pathname for environment variable TMPDIR
I guess we need another patch enable this.

> -- 
> Cyril Hrubis
> chrubis@suse.cz

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

[LTP] [PATCH v3] tst_tmpdir: Fix buffer overflow in tst_tmpdir.c

[Wei Gao via 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 | 84 ++++++++++++++++++++++--------------------------
 1 file changed, 38 insertions(+), 46 deletions(-)

diff --git a/lib/tst_tmpdir.c b/lib/tst_tmpdir.c
index 6ed2367b9..89b216df9 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)
 {
@@ -144,34 +144,37 @@ const char *tst_get_startwd(void)
 	return test_start_work_dir;
 }
 
-static int purge_dir(const char *path, char **errptr)
+static int purge_dirat(int dir_fd, 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 subdir_fd;
 
-	/* Do NOT perform the request if the directory is "/" */
-	if (!strcmp(path, "/")) {
+	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) {
-			strcpy(err_msg, "Cannot purge system root directory");
+			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;
 	}
 
-	errno = 0;
-
-	/* Open the directory to get access to what is in it */
-	if (!(dir = opendir(path))) {
+	dir = fdopendir(subdir_fd);
+	if (!dir) {
 		if (errptr) {
-			sprintf(err_msg,
-				"Cannot open directory %s; errno=%d: %s",
-				path, errno, tst_strerrno(errno));
+			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;
 	}
 
@@ -183,8 +186,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,25 +194,27 @@ 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;
 
+		int flags = AT_REMOVEDIR;
+
 		/* Get the link count, now that all the entries have been removed */
-		if (lstat(obj, &statbuf) < 0) {
+		if (fstatat(dir_fd, obj, &statbuf, AT_SYMLINK_NOFOLLOW) < 0) {
 			if (errmsg != NULL) {
 				sprintf(err_msg,
 					"lstat(%s) failed; errno=%d: %s", obj,
@@ -220,35 +224,23 @@ static int rmobj(const char *obj, char **errmsg)
 			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,
+		if (statbuf.st_nlink >= 3)
+			flags = 0;
+
+		if (unlinkat(dir_fd, obj, flags) < 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;
+				*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 +297,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(AT_FDCWD, TESTDIR, &errmsg) == -1) {
 			tst_resm(TWARN, "%s: rmobj(%s) failed: %s",
 				 __func__, TESTDIR, errmsg);
 		}
@@ -343,7 +335,7 @@ void tst_rmdir(void)
 	/*
 	 * Attempt to remove the "TESTDIR" directory, using rmobj().
 	 */
-	if (rmobj(TESTDIR, &errmsg) == -1) {
+	if (rmobjat(AT_FDCWD, TESTDIR, &errmsg) == -1) {
 		tst_resm(TWARN, "%s: rmobj(%s) failed: %s",
 			 __func__, TESTDIR, errmsg);
 	}
@@ -353,7 +345,7 @@ void tst_purge_dir(const char *path)
 {
 	char *err;
 
-	if (purge_dir(path, &err))
+	if (purge_dirat(AT_FDCWD, path, &err))
 		tst_brkm(TBROK, NULL, "%s: %s", __func__, err);
 }
 
-- 
2.51.0


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

Re: [LTP] [PATCH v3] tst_tmpdir: Fix buffer overflow in tst_tmpdir.c

[Andrea Cervesato via ltp]

Hi!

>  				sprintf(err_msg,
>  					"lstat(%s) failed; errno=%d: %s", obj,

Here we are still using sprintf().

-- 
Andrea Cervesato
SUSE QE Automation Engineer Linux
andrea.cervesato@suse.com


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

[LTP] [PATCH v4] tst_tmpdir: Fix buffer overflow in tst_tmpdir.c

[Wei Gao via 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
Signed-off-by: Wei Gao <wegao@suse.com>
---
 lib/tst_tmpdir.c | 88 ++++++++++++++++++++++--------------------------
 1 file changed, 40 insertions(+), 48 deletions(-)

diff --git a/lib/tst_tmpdir.c b/lib/tst_tmpdir.c
index b9924d85d..9b024a74e 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)
 {
@@ -144,34 +144,37 @@ const char *tst_get_startwd(void)
 	return test_start_work_dir;
 }
 
-static int purge_dir(const char *path, char **errptr)
+static int purge_dirat(int dir_fd, 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 subdir_fd;
 
-	/* Do NOT perform the request if the directory is "/" */
-	if (!strcmp(path, "/")) {
+	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) {
-			strcpy(err_msg, "Cannot purge system root directory");
+			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;
 	}
 
-	errno = 0;
-
-	/* Open the directory to get access to what is in it */
-	if (!(dir = opendir(path))) {
+	dir = fdopendir(subdir_fd);
+	if (!dir) {
 		if (errptr) {
-			sprintf(err_msg,
-				"Cannot open directory %s; errno=%d: %s",
-				path, errno, tst_strerrno(errno));
+			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;
 	}
 
@@ -183,8 +186,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 +194,53 @@ 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;
 
+		int flags = AT_REMOVEDIR;
+
 		/* Get the link count, now that all the entries have been removed */
-		if (lstat(obj, &statbuf) < 0) {
+		if (fstatat(dir_fd, obj, &statbuf, AT_SYMLINK_NOFOLLOW) < 0) {
 			if (errmsg != NULL) {
-				sprintf(err_msg,
-					"lstat(%s) failed; errno=%d: %s", obj,
+				snprintf(err_msg, sizeof(err_msg),
+					"fstatat(%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,
+		if (statbuf.st_nlink >= 3)
+			flags = 0;
+
+		if (unlinkat(dir_fd, obj, flags) < 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;
+				*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 +297,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(AT_FDCWD, TESTDIR, &errmsg) == -1) {
 			tst_resm(TWARN, "%s: rmobj(%s) failed: %s",
 				 __func__, TESTDIR, errmsg);
 		}
@@ -343,7 +335,7 @@ void tst_rmdir(void)
 	/*
 	 * Attempt to remove the "TESTDIR" directory, using rmobj().
 	 */
-	if (rmobj(TESTDIR, &errmsg) == -1) {
+	if (rmobjat(AT_FDCWD, TESTDIR, &errmsg) == -1) {
 		tst_resm(TWARN, "%s: rmobj(%s) failed: %s",
 			 __func__, TESTDIR, errmsg);
 	}
@@ -353,7 +345,7 @@ void tst_purge_dir(const char *path)
 {
 	char *err;
 
-	if (purge_dir(path, &err))
+	if (purge_dirat(AT_FDCWD, path, &err))
 		tst_brkm(TBROK, NULL, "%s: %s", __func__, err);
 }
 
-- 
2.52.0


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

Re: [LTP] [PATCH v4] tst_tmpdir: Fix buffer overflow in tst_tmpdir.c

[Andrea Cervesato via ltp]

Hi!

> +		if (unlinkat(dir_fd, obj, flags) < 0) {
> +			if (errmsg != NULL) {
> +				snprintf(err_msg, sizeof(err_msg),
>  						"remove(%s) failed; errno=%d: %s",

There's still one more issue in here:

We are checking the error for `unlinkat`, so error message should not
mention `remove(%s)`. But this can be fixed before merge.

Otherwise LGTM

-- 
Andrea Cervesato
SUSE QE Automation Engineer Linux
andrea.cervesato@suse.com


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

Re: [LTP] [PATCH v4] tst_tmpdir: Fix buffer overflow in tst_tmpdir.c

[Andrea Cervesato via ltp]

Reviewed-by: Andrea Cervesato <andrea.cervesato@suse.com>


-- 
Andrea Cervesato
SUSE QE Automation Engineer Linux
andrea.cervesato@suse.com

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

Re: [LTP] [PATCH v4] tst_tmpdir: Fix buffer overflow in tst_tmpdir.c

[Cyril Hrubis]

Hi!
> @@ -183,8 +186,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;
>  	}

Shouldn't we close(subdir_fd) here as well?

> @@ -192,63 +194,53 @@ static int purge_dir(const char *path, char **errptr)
>  	return ret_val;
>  }
>  

The rest seems to be fine.

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH v4] tst_tmpdir: Fix buffer overflow in tst_tmpdir.c

[Wei Gao via ltp]

On Wed, Feb 25, 2026 at 11:58:53AM +0100, Cyril Hrubis wrote:
> Hi!
> > @@ -183,8 +186,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;
> >  	}
> 
> Shouldn't we close(subdir_fd) here as well?
I guess you mean following change? Then i think adding close(subdir_fd) inside loop will actually cause a double-close, 
since after for loop there is another closedir(dir) will be called. closedir will also try closes underlying file descriptor.
Correct me if any mistake, thanks.

diff --git a/lib/tst_tmpdir.c b/lib/tst_tmpdir.c
index 9b024a74e..0c06a306c 100644
--- a/lib/tst_tmpdir.c
+++ b/lib/tst_tmpdir.c
@@ -186,8 +186,10 @@ static int purge_dirat(int dir_fd, const char *path, char **errptr)
                        continue;
 
                /* Recursively remove the current entry */
-               if (rmobjat(subdir_fd, dir_ent->d_name, errptr) != 0)
+               if (rmobjat(subdir_fd, dir_ent->d_name, errptr) != 0) {
+                       close(subdir_fd);  <<<<<<<<<< first close
                        ret_val = -1;
+               }
        }
        
        closedir(dir);  <<<<<<<<<<<<< second close
        return ret_val;
> 
> > @@ -192,63 +194,53 @@ static int purge_dir(const char *path, char **errptr)
> >  	return ret_val;
> >  }
> >  
> 
> The rest seems to be fine.
> 
> -- 
> Cyril Hrubis
> chrubis@suse.cz

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

Re: [LTP] [PATCH v4] tst_tmpdir: Fix buffer overflow in tst_tmpdir.c

[Cyril Hrubis]

Hi!
> diff --git a/lib/tst_tmpdir.c b/lib/tst_tmpdir.c
> index 9b024a74e..0c06a306c 100644
> --- a/lib/tst_tmpdir.c
> +++ b/lib/tst_tmpdir.c
> @@ -186,8 +186,10 @@ static int purge_dirat(int dir_fd, const char *path, char **errptr)
>                         continue;
>  
>                 /* Recursively remove the current entry */
> -               if (rmobjat(subdir_fd, dir_ent->d_name, errptr) != 0)
> +               if (rmobjat(subdir_fd, dir_ent->d_name, errptr) != 0) {
> +                       close(subdir_fd);  <<<<<<<<<< first close
>                         ret_val = -1;
> +               }
>         }
>         
>         closedir(dir);  <<<<<<<<<<<<< second close

Ah, right, we hand the fd to the fdopendir() and it's closed in the
closedir() call. I've missed that since the closedir() is not shown in
the diff since that part of the code wasn't modified and haven't shown
up in the diff.

The patch looks good to me then.

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH v4] tst_tmpdir: Fix buffer overflow in tst_tmpdir.c

[Andrea Cervesato via ltp]

Hi!

On Thu Feb 26, 2026 at 9:59 AM CET, Cyril Hrubis wrote:
> Hi!
> > diff --git a/lib/tst_tmpdir.c b/lib/tst_tmpdir.c
> > index 9b024a74e..0c06a306c 100644
> > --- a/lib/tst_tmpdir.c
> > +++ b/lib/tst_tmpdir.c
> > @@ -186,8 +186,10 @@ static int purge_dirat(int dir_fd, const char *path, char **errptr)
> >                         continue;
> >  
> >                 /* Recursively remove the current entry */
> > -               if (rmobjat(subdir_fd, dir_ent->d_name, errptr) != 0)
> > +               if (rmobjat(subdir_fd, dir_ent->d_name, errptr) != 0) {
> > +                       close(subdir_fd);  <<<<<<<<<< first close
> >                         ret_val = -1;
> > +               }
> >         }
> >         
> >         closedir(dir);  <<<<<<<<<<<<< second close
>
> Ah, right, we hand the fd to the fdopendir() and it's closed in the
> closedir() call. I've missed that since the closedir() is not shown in
> the diff since that part of the code wasn't modified and haven't shown
> up in the diff.
>
> The patch looks good to me then.

Feel free to ack and merge :-)

-- 
Andrea Cervesato
SUSE QE Automation Engineer Linux
andrea.cervesato@suse.com


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

Re: [LTP] [PATCH v4] tst_tmpdir: Fix buffer overflow in tst_tmpdir.c

[Cyril Hrubis]

Hi!
> > > diff --git a/lib/tst_tmpdir.c b/lib/tst_tmpdir.c
> > > index 9b024a74e..0c06a306c 100644
> > > --- a/lib/tst_tmpdir.c
> > > +++ b/lib/tst_tmpdir.c
> > > @@ -186,8 +186,10 @@ static int purge_dirat(int dir_fd, const char *path, char **errptr)
> > >                         continue;
> > >  
> > >                 /* Recursively remove the current entry */
> > > -               if (rmobjat(subdir_fd, dir_ent->d_name, errptr) != 0)
> > > +               if (rmobjat(subdir_fd, dir_ent->d_name, errptr) != 0) {
> > > +                       close(subdir_fd);  <<<<<<<<<< first close
> > >                         ret_val = -1;
> > > +               }
> > >         }
> > >         
> > >         closedir(dir);  <<<<<<<<<<<<< second close
> >
> > Ah, right, we hand the fd to the fdopendir() and it's closed in the
> > closedir() call. I've missed that since the closedir() is not shown in
> > the diff since that part of the code wasn't modified and haven't shown
> > up in the diff.
> >
> > The patch looks good to me then.
> 
> Feel free to ack and merge :-)

Done, thanks Andrea and Wei.

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH v1] tst_tmpdir: Fix buffer overflow in tst_tmpdir.c

[Cyril Hrubis]

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