[LTP] [PATCH v2 1/2] open: fix cleanup condition and use snprintf

[LTP] [PATCH v2 1/2] open: fix cleanup condition and use snprintf

[Jinseok Kim]

Replace remaining sprintf() with snprintf() in setup()

Thanks for the review.

Signed-off-by: Jinseok Kim <always.starving0@gmail.com>
---
 testcases/kernel/syscalls/open/open04.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/testcases/kernel/syscalls/open/open04.c b/testcases/kernel/syscalls/open/open04.c
index 3dc3486d3..5d39c1569 100644
--- a/testcases/kernel/syscalls/open/open04.c
+++ b/testcases/kernel/syscalls/open/open04.c
@@ -30,7 +30,7 @@ static void setup(void)
 	fds[0] = first;

 	for (i = first + 1; i < fds_limit; i++) {
-		sprintf(fname, FNAME ".%d", i);
+		snprintf(fname, sizeof(fname), FNAME ".%d", i);
 		fd = open(fname, O_RDWR | O_CREAT, 0777);
 		if (fd == -1) {
 			if (errno != EMFILE)
@@ -44,13 +44,13 @@ static void setup(void)

 static void run(void)
 {
-	sprintf(fname, FNAME ".%d", fds_limit);
+	snprintf(fname, sizeof(fname), FNAME ".%d", fds_limit);
 	TST_EXP_FAIL2(open(fname, O_RDWR | O_CREAT, 0777), EMFILE);
 }

 static void cleanup(void)
 {
-	if (!first || !fds)
+	if (first < 0 || !fds)
 		return;

 	for (i = first; i < fds_limit; i++)
--
2.43.0

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

[LTP] [PATCH v2 2/2] open: replace getdtablesize with getrlimit

[Jinseok Kim]

Signed-off-by: Jinseok Kim <always.starving0@gmail.com>
---
 testcases/kernel/syscalls/open/open04.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/testcases/kernel/syscalls/open/open04.c b/testcases/kernel/syscalls/open/open04.c
index 5d39c1569..166787518 100644
--- a/testcases/kernel/syscalls/open/open04.c
+++ b/testcases/kernel/syscalls/open/open04.c
@@ -11,6 +11,7 @@

 #include <stdio.h>
 #include <stdlib.h>
+#include <sys/resource.h>
 #include "tst_test.h"

 #define FNAME "open04"
@@ -22,8 +23,10 @@ static char fname[20];
 static void setup(void)
 {
 	int fd;
+	struct rlimit rlim;

-	fds_limit = getdtablesize();
+	SAFE_GETRLIMIT(RLIMIT_NOFILE, &rlim);
+	fds_limit = rlim.rlim_cur;
 	first = SAFE_OPEN(FNAME, O_RDWR | O_CREAT, 0777);

 	fds = SAFE_MALLOC(sizeof(int) * (fds_limit - first));
--
2.43.0

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

Re: [LTP] [PATCH v2 1/2] open: fix cleanup condition and use snprintf

[Andrea Cervesato via ltp]

Hi!

On Wed Feb 18, 2026 at 3:47 PM CET, Jinseok Kim wrote:
> Replace remaining sprintf() with snprintf() in setup()
>
> Thanks for the review.

Again, there's something wrong with your automated system. Is it LLM? :-)
The message is misleading, also because not only snprintf() touched, but
also cleanup() logic.

This has to be fixed, otherwise patch can't be accepted.

>
> Signed-off-by: Jinseok Kim <always.starving0@gmail.com>
> ---
>  testcases/kernel/syscalls/open/open04.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/testcases/kernel/syscalls/open/open04.c b/testcases/kernel/syscalls/open/open04.c
> index 3dc3486d3..5d39c1569 100644
> --- a/testcases/kernel/syscalls/open/open04.c
> +++ b/testcases/kernel/syscalls/open/open04.c
> @@ -30,7 +30,7 @@ static void setup(void)
>  	fds[0] = first;
>
>  	for (i = first + 1; i < fds_limit; i++) {
> -		sprintf(fname, FNAME ".%d", i);
> +		snprintf(fname, sizeof(fname), FNAME ".%d", i);
>  		fd = open(fname, O_RDWR | O_CREAT, 0777);
>  		if (fd == -1) {
>  			if (errno != EMFILE)
> @@ -44,13 +44,13 @@ static void setup(void)
>
>  static void run(void)
>  {
> -	sprintf(fname, FNAME ".%d", fds_limit);
> +	snprintf(fname, sizeof(fname), FNAME ".%d", fds_limit);
>  	TST_EXP_FAIL2(open(fname, O_RDWR | O_CREAT, 0777), EMFILE);
>  }
>
>  static void cleanup(void)
>  {
> -	if (!first || !fds)
> +	if (first < 0 || !fds)

And this cleanup has an issue. See below..

>  		return;
>
>  	for (i = first; i < fds_limit; i++)
> --
> 2.43.0

This test has to work also for `-i 0` option. The way we can make it
works is the following:

- "first" should be initialized to -1, otherwise "first < 0" won't be
  catched
- the fds should be initialized with -1 after SAFE_MALLOC(), otherwise
  you will fail the cleanup if malloc will break the test


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


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

[LTP] [PATCH v3 1/2] open: fix cleanup condition and use snprintf

[Jinseok Kim]

The test uses sprintf() to build temporary file names, which may
overflow the fixed-size buffer. Replace it with snprintf() to avoid
potential buffer overflows.

The cleanup logic also checked '!first' to decide whether to close
file descriptors. Since file descriptor 0 is valid, this condition
can incorrectly skip cleanup and leak file descriptors.

To fix this:
- Initialize first = -1 to correctly detect uninitialized state
- Initialize fds array with -1 after malloc to avoid closing invalid fds

Signed-off-by: Jinseok Kim <always.starving0@gmail.com>
---
 testcases/kernel/syscalls/open/open04.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/testcases/kernel/syscalls/open/open04.c b/testcases/kernel/syscalls/open/open04.c
index 3dc3486d3..152bec2d4 100644
--- a/testcases/kernel/syscalls/open/open04.c
+++ b/testcases/kernel/syscalls/open/open04.c
@@ -15,7 +15,8 @@

 #define FNAME "open04"

-static int fds_limit, first, i;
+static int fds_limit, i;
+static int first = -1;
 static int *fds;
 static char fname[20];

@@ -27,10 +28,11 @@ static void setup(void)
 	first = SAFE_OPEN(FNAME, O_RDWR | O_CREAT, 0777);

 	fds = SAFE_MALLOC(sizeof(int) * (fds_limit - first));
+	memset(fds, -1, sizeof(int) * (fds_limit - first));
 	fds[0] = first;

 	for (i = first + 1; i < fds_limit; i++) {
-		sprintf(fname, FNAME ".%d", i);
+		snprintf(fname, sizeof(fname), FNAME ".%d", i);
 		fd = open(fname, O_RDWR | O_CREAT, 0777);
 		if (fd == -1) {
 			if (errno != EMFILE)
@@ -44,13 +46,13 @@ static void setup(void)

 static void run(void)
 {
-	sprintf(fname, FNAME ".%d", fds_limit);
+	snprintf(fname, sizeof(fname), FNAME ".%d", fds_limit);
 	TST_EXP_FAIL2(open(fname, O_RDWR | O_CREAT, 0777), EMFILE);
 }

 static void cleanup(void)
 {
-	if (!first || !fds)
+	if (first < 0 || !fds)
 		return;

 	for (i = first; i < fds_limit; i++)
--
2.43.0

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

[LTP] [PATCH v3 2/2] open: replace getdtablesize with getrlimit

[Jinseok Kim]

The test currently uses getdtablesize() to determine the maximum
number of file descriptors for the process. This interface is
considered legacy and is not specified by POSIX.

Use getrlimit() instead, which provides a well-defined
and portable way to obtain the per-process file descriptor limit.

Signed-off-by: Jinseok Kim <always.starving0@gmail.com>
---
 testcases/kernel/syscalls/open/open04.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/testcases/kernel/syscalls/open/open04.c b/testcases/kernel/syscalls/open/open04.c
index 152bec2d4..9c5634ba9 100644
--- a/testcases/kernel/syscalls/open/open04.c
+++ b/testcases/kernel/syscalls/open/open04.c
@@ -11,6 +11,7 @@

 #include <stdio.h>
 #include <stdlib.h>
+#include <sys/resource.h>
 #include "tst_test.h"

 #define FNAME "open04"
@@ -23,8 +24,10 @@ static char fname[20];
 static void setup(void)
 {
 	int fd;
+	struct rlimit rlim;

-	fds_limit = getdtablesize();
+	SAFE_GETRLIMIT(RLIMIT_NOFILE, &rlim);
+	fds_limit = rlim.rlim_cur;
 	first = SAFE_OPEN(FNAME, O_RDWR | O_CREAT, 0777);

 	fds = SAFE_MALLOC(sizeof(int) * (fds_limit - first));
--
2.43.0

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

Re: [LTP] [PATCH v3 2/2] open: replace getdtablesize with getrlimit

[Petr Vorel]

Hi Jinseok,

> The test currently uses getdtablesize() to determine the maximum
> number of file descriptors for the process. This interface is
> considered legacy and is not specified by POSIX.

> Use getrlimit() instead, which provides a well-defined
> and portable way to obtain the per-process file descriptor limit.

Hm, it's still supported by all libc on Linux including musl [1] and bionic [2],
and we even test getdomainname [3], but ok, let's move beyond POSIX 2004.

[1] https://git.musl-libc.org/cgit/musl/tree/src/legacy/getdtablesize.c
[2] https://android.googlesource.com/platform/bionic.git/+/refs/heads/main/libc/bionic/ndk_cruft.cpp#288
[3] testcases/kernel/syscalls/getdomainname/getdomainname01.c

...
> +++ b/testcases/kernel/syscalls/open/open04.c
> @@ -11,6 +11,7 @@

>  #include <stdio.h>
>  #include <stdlib.h>
> +#include <sys/resource.h>
nit: I removed this before merge (header included by other headers).

Merged this patch only (not the first one).

Kind regards,
Petr

>  #include "tst_test.h"

>  #define FNAME "open04"
> @@ -23,8 +24,10 @@ static char fname[20];
>  static void setup(void)
>  {
>  	int fd;
> +	struct rlimit rlim;

> -	fds_limit = getdtablesize();
> +	SAFE_GETRLIMIT(RLIMIT_NOFILE, &rlim);
> +	fds_limit = rlim.rlim_cur;
>  	first = SAFE_OPEN(FNAME, O_RDWR | O_CREAT, 0777);

>  	fds = SAFE_MALLOC(sizeof(int) * (fds_limit - first));

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

Re: [LTP] [PATCH v3 1/2] open: fix cleanup condition and use snprintf

[Andrea Cervesato via ltp]

Hi!

>  	fds = SAFE_MALLOC(sizeof(int) * (fds_limit - first));
> +	memset(fds, -1, sizeof(int) * (fds_limit - first));
>  	fds[0] = first;

If you set all fds to -1, then we should also check that it's not -1 in the
cleanup() loop, otherwise SAFE_CLOSE() will TBROK.

Since the patch 2/2 has been merged, please rebase on the current master
sending only this corrected patch.

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

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

[LTP] [PATCH v4] open: fix cleanup condition and use snprintf

[Jinseok Kim]

Replace sprintf() with snprintf() for safer string handling.

The cleanup logic checked '!first' to decide whether to close file
descriptors. Since file descriptor 0 is valid, this may incorrectly
skip cleanup and leak descriptors.

Also initialize "first" to -1 and set the fds array to -1 after
allocation. Skip such entries in cleanup() to avoid calling SAFE_CLOSE()
on invalid file descriptors.

Signed-off-by: Jinseok Kim <always.starving0@gmail.com>
---
 testcases/kernel/syscalls/open/open04.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/testcases/kernel/syscalls/open/open04.c b/testcases/kernel/syscalls/open/open04.c
index 7573af1c8..78254d397 100644
--- a/testcases/kernel/syscalls/open/open04.c
+++ b/testcases/kernel/syscalls/open/open04.c
@@ -15,7 +15,8 @@

 #define FNAME "open04"

-static int fds_limit, first, i;
+static int fds_limit, i;
+static int first = -1;
 static int *fds;
 static char fname[20];

@@ -29,10 +30,11 @@ static void setup(void)
 	first = SAFE_OPEN(FNAME, O_RDWR | O_CREAT, 0777);

 	fds = SAFE_MALLOC(sizeof(int) * (fds_limit - first));
+	memset(fds, -1, sizeof(int) * (fds_limit - first));
 	fds[0] = first;

 	for (i = first + 1; i < fds_limit; i++) {
-		sprintf(fname, FNAME ".%d", i);
+		snprintf(fname, sizeof(fname), FNAME ".%d", i);
 		fd = open(fname, O_RDWR | O_CREAT, 0777);
 		if (fd == -1) {
 			if (errno != EMFILE)
@@ -46,17 +48,19 @@ static void setup(void)

 static void run(void)
 {
-	sprintf(fname, FNAME ".%d", fds_limit);
+	snprintf(fname, sizeof(fname), FNAME ".%d", fds_limit);
 	TST_EXP_FAIL2(open(fname, O_RDWR | O_CREAT, 0777), EMFILE);
 }

 static void cleanup(void)
 {
-	if (!first || !fds)
+	if (first < 0 || !fds)
 		return;

-	for (i = first; i < fds_limit; i++)
-		SAFE_CLOSE(fds[i - first]);
+	for (i = first; i < fds_limit; i++) {
+		if (fds[i - first] != -1)
+			SAFE_CLOSE(fds[i - first]);
+	}

 	if (fds)
 		free(fds);
--
2.43.0

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

Re: [LTP] [PATCH v4] open: fix cleanup condition and use snprintf

[Andrea Cervesato via ltp]

Hi!

Since we are cleaning up the test, we can just do this instead:

static void cleanup(void)
{
	if (first >= 0) {
		int limit = fds_limit - first;

		for (int i = 0; i < limit; i++) {
			if (fds[i] != -1)
				SAFE_CLOSE(fds[i]);
		}
	}

	free(fds);
}

We don't need to calculate the index twice every iteration. Also, free(NULL)
is noop and we can safely call it without checking if it's NULL.

And please remove the `i` definition at static level. We don't really need that.
We can safely use `for (int i = 0`.

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

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

[LTP] [PATCH v5] open: fix cleanup condition and use snprintf

[Jinseok Kim]

Replace sprintf() with snprintf() for safer string handling.

The cleanup logic checked '!first' to decide whether to close file
descriptors. Since file descriptor 0 is valid, this may incorrectly
skip cleanup and leak descriptors.

Also initialize "first" to -1 and set the fds array to -1 after
allocation. Skip such entries in cleanup() to avoid calling SAFE_CLOSE()
on invalid file descriptors.

Signed-off-by: Jinseok Kim <always.starving0@gmail.com>
---
 testcases/kernel/syscalls/open/open04.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/testcases/kernel/syscalls/open/open04.c b/testcases/kernel/syscalls/open/open04.c
index 7573af1c8..3457024fc 100644
--- a/testcases/kernel/syscalls/open/open04.c
+++ b/testcases/kernel/syscalls/open/open04.c
@@ -15,7 +15,8 @@

 #define FNAME "open04"

-static int fds_limit, first, i;
+static int fds_limit;
+static int first = -1;
 static int *fds;
 static char fname[20];

@@ -29,10 +30,11 @@ static void setup(void)
 	first = SAFE_OPEN(FNAME, O_RDWR | O_CREAT, 0777);

 	fds = SAFE_MALLOC(sizeof(int) * (fds_limit - first));
+	memset(fds, -1, sizeof(int) * (fds_limit - first));
 	fds[0] = first;

-	for (i = first + 1; i < fds_limit; i++) {
-		sprintf(fname, FNAME ".%d", i);
+	for (int i = first + 1; i < fds_limit; i++) {
+		snprintf(fname, sizeof(fname), FNAME ".%d", i);
 		fd = open(fname, O_RDWR | O_CREAT, 0777);
 		if (fd == -1) {
 			if (errno != EMFILE)
@@ -46,20 +48,22 @@ static void setup(void)

 static void run(void)
 {
-	sprintf(fname, FNAME ".%d", fds_limit);
+	snprintf(fname, sizeof(fname), FNAME ".%d", fds_limit);
 	TST_EXP_FAIL2(open(fname, O_RDWR | O_CREAT, 0777), EMFILE);
 }

 static void cleanup(void)
 {
-	if (!first || !fds)
-		return;
+	if (first >= 0) {
+		int limit = fds_limit - first;

-	for (i = first; i < fds_limit; i++)
-		SAFE_CLOSE(fds[i - first]);
+		for (int i = 0; i < limit; i++) {
+			if (fds[i] != -1)
+				SAFE_CLOSE(fds[i]);
+		}
+	}

-	if (fds)
-		free(fds);
+	free(fds);
 }

 static struct tst_test test = {
--
2.43.0

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

Re: [LTP] [PATCH v5] open: fix cleanup condition and use snprintf

[Andrea Cervesato via ltp]

Hi Jinseok,

> static void cleanup(void)
> {
>-	if (!first || !fds)
>-		return;
>+	if (first >= 0) {
>+		int limit = fds_limit - first;
>
>+		for (int i = 0; i < limit; i++) {
>+			if (fds[i] != -1)
>+				SAFE_CLOSE(fds[i]);
>+		}
>+	}
>
>-	if (fds)
>-		free(fds);
>+	free(fds);

If SAFE_MALLOC() fails, the fds == NULL and we have a wrong memory
access. Please change the condition to:

	if (first >= 0 && fds) {

> static int *fds;
> static char fname[20];

This was pre-existing and I didn't noticed. We should use PATH_MAX
in here, instead of a pre-defined length.

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

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

[LTP] [PATCH v6] open: fix cleanup condition and use snprintf

[Jinseok Kim]

Replace sprintf() with snprintf() for safer string handling.

The cleanup logic checked '!first' to decide whether to close file
descriptors. Since file descriptor 0 is valid, this may incorrectly
skip cleanup and leak descriptors.

Also initialize "first" to -1 and set the fds array to -1 after
allocation. Skip such entries in cleanup() to avoid calling SAFE_CLOSE()
on invalid file descriptors.

Signed-off-by: Jinseok Kim <always.starving0@gmail.com>
---
 testcases/kernel/syscalls/open/open04.c | 26 ++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/testcases/kernel/syscalls/open/open04.c b/testcases/kernel/syscalls/open/open04.c
index 7573af1c8..6ec18cbfa 100644
--- a/testcases/kernel/syscalls/open/open04.c
+++ b/testcases/kernel/syscalls/open/open04.c
@@ -15,9 +15,10 @@

 #define FNAME "open04"

-static int fds_limit, first, i;
+static int fds_limit;
+static int first = -1;
 static int *fds;
-static char fname[20];
+static char fname[PATH_MAX];

 static void setup(void)
 {
@@ -29,10 +30,11 @@ static void setup(void)
 	first = SAFE_OPEN(FNAME, O_RDWR | O_CREAT, 0777);

 	fds = SAFE_MALLOC(sizeof(int) * (fds_limit - first));
+	memset(fds, -1, sizeof(int) * (fds_limit - first));
 	fds[0] = first;

-	for (i = first + 1; i < fds_limit; i++) {
-		sprintf(fname, FNAME ".%d", i);
+	for (int i = first + 1; i < fds_limit; i++) {
+		snprintf(fname, sizeof(fname), FNAME ".%d", i);
 		fd = open(fname, O_RDWR | O_CREAT, 0777);
 		if (fd == -1) {
 			if (errno != EMFILE)
@@ -46,20 +48,22 @@ static void setup(void)

 static void run(void)
 {
-	sprintf(fname, FNAME ".%d", fds_limit);
+	snprintf(fname, sizeof(fname), FNAME ".%d", fds_limit);
 	TST_EXP_FAIL2(open(fname, O_RDWR | O_CREAT, 0777), EMFILE);
 }

 static void cleanup(void)
 {
-	if (!first || !fds)
-		return;
+	if (first >= 0 && fds) {
+		int limit = fds_limit - first;

-	for (i = first; i < fds_limit; i++)
-		SAFE_CLOSE(fds[i - first]);
+		for (int i = 0; i < limit; i++) {
+			if (fds[i] != -1)
+				SAFE_CLOSE(fds[i]);
+		}
+	}

-	if (fds)
-		free(fds);
+	free(fds);
 }

 static struct tst_test test = {
--
2.43.0

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

Re: [LTP] [PATCH v6] open: fix cleanup condition and use snprintf

[Andrea Cervesato via ltp]

LGTM

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