| Message ID | eff8549698ca7a61089e17727b3e1d45a4839e6f.1707650891.git.fdmanana@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | generic/736: fix a buffer overflow in readdir-while-renames.c | expand |
Reviewed-by: David Disseldorp <ddiss@suse.de>
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",
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>
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",