[PATCH] generic/736: fix a buffer overflow in readdir-while-renames.c

[PATCH] generic/736: fix a buffer overflow in readdir-while-renames.c

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

The test is using a 32 characters buffer to print the full path for each
file name, which in some setups it's not enough because $TEST_DIR can
point to a path name longer than that, or even smaller but then the buffer
is still not large enough after appending a file name. When that's the
case it results in a core dump like this:

  generic/736       QA output created by 736
  *** buffer overflow detected ***: terminated
  /opt/xfstests/tests/generic/736: line 32:  9217 Aborted                 (core dumped) $here/src/readdir-while-renames $target_dir
  Silence is golden
  - output mismatch (see /opt/xfstests/results//generic/736.out.bad)
      --- tests/generic/736.out	2024-01-14 12:01:35.000000000 -0500
      +++ /opt/xfstests/results//generic/736.out.bad	2024-01-23 18:58:37.990000000 -0500
      @@ -1,2 +1,4 @@
       QA output created by 736
      +*** buffer overflow detected ***: terminated
      +/opt/xfstests/tests/generic/736: line 32:  9217 Aborted                 (core dumped) $here/src/readdir-while-renames $target_dir
       Silence is golden
      ...
      (Run diff -u /opt/xfstests/tests/generic/736.out /opt/xfstests/results//generic/736.out.bad  to see the entire diff)
  Ran: generic/736
  Failures: generic/736
  Failed 1 of 1 tests

We don't actually need to print the full path into the buffer, because we
have previously set the current directory (chdir) to the path pointed by
"dir_path". So fix this by printing only the relative path name which
uses at most 5 characters (NUM_FILES is 5000 plus the nul terminator).

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 src/readdir-while-renames.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/src/readdir-while-renames.c b/src/readdir-while-renames.c
index afeefb04..b99c0490 100644
--- a/src/readdir-while-renames.c
+++ b/src/readdir-while-renames.c
@@ -55,10 +55,16 @@ int main(int argc, char *argv[])
 
 	/* Now create all files inside the directory. */
 	for (i = 1; i <= NUM_FILES; i++) {
-		char file_name[32];
+		/* 8 characters is enough for NUM_FILES name plus '\0'. */
+		char file_name[8];
 		FILE *f;
 
-		sprintf(file_name, "%s/%d", dir_path, i);
+		ret = snprintf(file_name, sizeof(file_name), "%d", i);
+		if (ret < 0 || ret >= sizeof(file_name)) {
+			fprintf(stderr, "Buffer to small for filename %i\n", i);
+			ret = EOVERFLOW;
+			goto out;
+		}
 		f = fopen(file_name, "w");
 		if (f == NULL) {
 			fprintf(stderr, "Failed to create file number %d: %d\n",
-- 
2.40.1

Re: [PATCH] generic/736: fix a buffer overflow in readdir-while-renames.c

[David Disseldorp]

Reviewed-by: David Disseldorp <ddiss@suse.de>

Re: [PATCH] generic/736: fix a buffer overflow in readdir-while-renames.c

[Qu Wenruo]

On 2024/2/11 21:58, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
>
> The test is using a 32 characters buffer to print the full path for each
> file name, which in some setups it's not enough because $TEST_DIR can
> point to a path name longer than that, or even smaller but then the buffer
> is still not large enough after appending a file name. When that's the
> case it results in a core dump like this:
>
>    generic/736       QA output created by 736
>    *** buffer overflow detected ***: terminated
>    /opt/xfstests/tests/generic/736: line 32:  9217 Aborted                 (core dumped) $here/src/readdir-while-renames $target_dir
>    Silence is golden
>    - output mismatch (see /opt/xfstests/results//generic/736.out.bad)
>        --- tests/generic/736.out	2024-01-14 12:01:35.000000000 -0500
>        +++ /opt/xfstests/results//generic/736.out.bad	2024-01-23 18:58:37.990000000 -0500
>        @@ -1,2 +1,4 @@
>         QA output created by 736
>        +*** buffer overflow detected ***: terminated
>        +/opt/xfstests/tests/generic/736: line 32:  9217 Aborted                 (core dumped) $here/src/readdir-while-renames $target_dir
>         Silence is golden
>        ...
>        (Run diff -u /opt/xfstests/tests/generic/736.out /opt/xfstests/results//generic/736.out.bad  to see the entire diff)
>    Ran: generic/736
>    Failures: generic/736
>    Failed 1 of 1 tests
>
> We don't actually need to print the full path into the buffer, because we
> have previously set the current directory (chdir) to the path pointed by
> "dir_path". So fix this by printing only the relative path name which
> uses at most 5 characters (NUM_FILES is 5000 plus the nul terminator).
>
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu
> ---
>   src/readdir-while-renames.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/src/readdir-while-renames.c b/src/readdir-while-renames.c
> index afeefb04..b99c0490 100644
> --- a/src/readdir-while-renames.c
> +++ b/src/readdir-while-renames.c
> @@ -55,10 +55,16 @@ int main(int argc, char *argv[])
>
>   	/* Now create all files inside the directory. */
>   	for (i = 1; i <= NUM_FILES; i++) {
> -		char file_name[32];
> +		/* 8 characters is enough for NUM_FILES name plus '\0'. */
> +		char file_name[8];
>   		FILE *f;
>
> -		sprintf(file_name, "%s/%d", dir_path, i);
> +		ret = snprintf(file_name, sizeof(file_name), "%d", i);
> +		if (ret < 0 || ret >= sizeof(file_name)) {
> +			fprintf(stderr, "Buffer to small for filename %i\n", i);
> +			ret = EOVERFLOW;
> +			goto out;
> +		}
>   		f = fopen(file_name, "w");
>   		if (f == NULL) {
>   			fprintf(stderr, "Failed to create file number %d: %d\n",

Re: [PATCH] generic/736: fix a buffer overflow in readdir-while-renames.c

[David Sterba]

On Sun, Feb 11, 2024 at 11:28:26AM +0000, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> The test is using a 32 characters buffer to print the full path for each
> file name, which in some setups it's not enough because $TEST_DIR can
> point to a path name longer than that, or even smaller but then the buffer
> is still not large enough after appending a file name. When that's the
> case it results in a core dump like this:
> 
>   generic/736       QA output created by 736
>   *** buffer overflow detected ***: terminated
>   /opt/xfstests/tests/generic/736: line 32:  9217 Aborted                 (core dumped) $here/src/readdir-while-renames $target_dir
>   Silence is golden
>   - output mismatch (see /opt/xfstests/results//generic/736.out.bad)
>       --- tests/generic/736.out	2024-01-14 12:01:35.000000000 -0500
>       +++ /opt/xfstests/results//generic/736.out.bad	2024-01-23 18:58:37.990000000 -0500
>       @@ -1,2 +1,4 @@
>        QA output created by 736
>       +*** buffer overflow detected ***: terminated
>       +/opt/xfstests/tests/generic/736: line 32:  9217 Aborted                 (core dumped) $here/src/readdir-while-renames $target_dir
>        Silence is golden
>       ...
>       (Run diff -u /opt/xfstests/tests/generic/736.out /opt/xfstests/results//generic/736.out.bad  to see the entire diff)
>   Ran: generic/736
>   Failures: generic/736
>   Failed 1 of 1 tests
> 
> We don't actually need to print the full path into the buffer, because we
> have previously set the current directory (chdir) to the path pointed by
> "dir_path". So fix this by printing only the relative path name which
> uses at most 5 characters (NUM_FILES is 5000 plus the nul terminator).
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Reviewed-by: David Sterba <dsterba@suse.com>