Message ID | eb8f1b5d-e6ea-70b-d970-2d9a826a8f1@redhat.com |
---|---|
State | New |
Headers | show |
Series | [v2] Add freopen special-case tests: chroot, EFBIG, stdin/stdout/stderr | expand |
Ping. This patch <https://sourceware.org/pipermail/libc-alpha/2024-September/159856.html> is pending review.
On 9/10/24 6:26 PM, Joseph Myers wrote: > Add tests of special cases for freopen that were omitted from the more > general tests of different modes and similar issues. The special > cases in the three tests here are logically unconnected, it was simply > convenient to put these tests in one patch. > > * Test freopen with a NULL path to the new file, in a chroot. Rather > than asserting that this fails (logically, failure in this case is > an implementation detail; it's not required for freopen to rely on > /proc), verify that either it fails (without memory leaks) or that > it succeeds and behaves as expected on success. There is no check > for file descriptor leaks because the machinery for that also > depends on /proc, so can't be used in a chroot. > > * Test that freopen and freopen64 are genuinely different in > configurations with 32-bit off_t by checking for an EFBIG trying to > write past 2GB in a file opened with freopen in such a configuration > but no error with 64-bit off_t or when opening with freopen64. > > * Test freopen of stdin, stdout and stderr. LGTM. Adds Florian's suggestions from v1. Includes additional tests for stdin, stdout and stderr. Reviewed-by: Carlos O'Donell <carlos@redhat.com> > Tested for x86_64 and x86. > > --- > > Changed in v2: test of freopen of stdin / stdout / stderr added; > unused OTHER_FREOPEN definitions removed in tst-freopen4 / > tst-freopen64-4. > > diff --git a/stdio-common/Makefile b/stdio-common/Makefile > index ce7f7cdd3b..62f8b99b06 100644 > --- a/stdio-common/Makefile > +++ b/stdio-common/Makefile > @@ -219,8 +219,13 @@ tests := \ > tst-fphex-wide \ > tst-freopen2 \ > tst-freopen3 \ > + tst-freopen4 \ > + tst-freopen5 \ > + tst-freopen6 \ OK. 3 new tests. > tst-freopen64-2 \ > tst-freopen64-3 \ > + tst-freopen64-4 \ > + tst-freopen64-6 \ OK. 2 new tests. > tst-fseek \ > tst-fwrite \ > tst-fwrite-memstrm \ > @@ -324,8 +329,13 @@ ifneq ($(PERL),no) > tests-special += \ > $(objpfx)tst-freopen2-mem.out \ > $(objpfx)tst-freopen3-mem.out \ > + $(objpfx)tst-freopen4-mem.out \ > + $(objpfx)tst-freopen5-mem.out \ > + $(objpfx)tst-freopen6-mem.out \ OK. 3 new freopen tests have leak checking. > $(objpfx)tst-freopen64-2-mem.out \ > $(objpfx)tst-freopen64-3-mem.out \ > + $(objpfx)tst-freopen64-4-mem.out \ > + $(objpfx)tst-freopen64-6-mem.out \ OK. 2 new freopen64 tests have leak checking. > $(objpfx)tst-getline-enomem-mem.out \ > $(objpfx)tst-getline-mem.out \ > $(objpfx)tst-printf-bz18872-mem.out \ > @@ -341,10 +351,20 @@ generated += \ > tst-freopen2.mtrace \ > tst-freopen3-mem.out \ > tst-freopen3.mtrace \ > + tst-freopen4-mem.out \ > + tst-freopen4.mtrace \ > + tst-freopen5-mem.out \ > + tst-freopen5.mtrace \ > + tst-freopen6-mem.out \ > + tst-freopen6.mtrace \ OK. out and trace. > tst-freopen64-2-mem.out \ > tst-freopen64-2.mtrace \ > tst-freopen64-3-mem.out \ > tst-freopen64-3.mtrace \ > + tst-freopen64-4-mem.out \ > + tst-freopen64-4.mtrace \ > + tst-freopen64-6-mem.out \ > + tst-freopen64-6.mtrace \ OK. out and trace. > tst-getline-enomem-mem.out \ > tst-getline-enomem.mtrace \ > tst-getline-mem.out \ > @@ -476,6 +496,21 @@ tst-freopen3-ENV = \ > tst-freopen64-3-ENV = \ > MALLOC_TRACE=$(objpfx)tst-freopen64-3.mtrace \ > LD_PRELOAD=$(common-objpfx)malloc/libc_malloc_debug.so > +tst-freopen4-ENV = \ > + MALLOC_TRACE=$(objpfx)tst-freopen4.mtrace \ > + LD_PRELOAD=$(common-objpfx)malloc/libc_malloc_debug.so > +tst-freopen64-4-ENV = \ > + MALLOC_TRACE=$(objpfx)tst-freopen64-4.mtrace \ > + LD_PRELOAD=$(common-objpfx)malloc/libc_malloc_debug.so > +tst-freopen5-ENV = \ > + MALLOC_TRACE=$(objpfx)tst-freopen5.mtrace \ > + LD_PRELOAD=$(common-objpfx)malloc/libc_malloc_debug.so > +tst-freopen6-ENV = \ > + MALLOC_TRACE=$(objpfx)tst-freopen6.mtrace \ > + LD_PRELOAD=$(common-objpfx)malloc/libc_malloc_debug.so > +tst-freopen64-6-ENV = \ > + MALLOC_TRACE=$(objpfx)tst-freopen64-6.mtrace \ > + LD_PRELOAD=$(common-objpfx)malloc/libc_malloc_debug.so OK. Sorted :-) > > $(objpfx)tst-unbputc.out: tst-unbputc.sh $(objpfx)tst-unbputc > $(SHELL) $< $(common-objpfx) '$(test-program-prefix)'; \ > diff --git a/stdio-common/tst-freopen4-main.c b/stdio-common/tst-freopen4-main.c > new file mode 100644 > index 0000000000..e169442cf4 > --- /dev/null > +++ b/stdio-common/tst-freopen4-main.c > @@ -0,0 +1,100 @@ > +/* Test freopen in chroot. > + Copyright (C) 2024 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + <https://www.gnu.org/licenses/>. */ > + > +#include <mcheck.h> > +#include <stdio.h> > +#include <stdlib.h> > + > +#include <support/check.h> > +#include <support/file_contents.h> > +#include <support/namespace.h> > +#include <support/support.h> > +#include <support/temp_file.h> > +#include <support/test-driver.h> > +#include <support/xstdio.h> > +#include <support/xunistd.h> > + > +int > +do_test (void) > +{ > + mtrace (); OK. Start tracing early. > + char *temp_dir = support_create_temp_directory ("tst-freopen4"); > + FILE *fp; > + int ret; > + > + /* These chroot tests verify that either reopening a renamed or > + deleted file works even in the absence of /proc, or that it fails > + (without memory leaks); thus, for example, such reopening does > + not crash in the absence of /proc. */ > + > + support_become_root (); > + if (!support_can_chroot ()) > + return EXIT_UNSUPPORTED; > + xchroot (temp_dir); > + > + /* Test freopen with NULL, renamed file. This verifies that > + reopening succeeds (and resets the file position indicator to > + start of file) even when the original path could no longer be > + opened, or fails without a memory leak. (It is not possible to > + use <support/descriptors.h> to test for file descriptor leaks > + here, because that also depends on /proc.) */ > + > + verbose_printf ("testing freopen with NULL, renamed file\n"); > + fp = xfopen ("/file1", "w+"); > + ret = fputs ("file has been renamed", fp); > + TEST_VERIFY (ret >= 0); > + ret = rename ("/file1", "/file1a"); > + TEST_COMPARE (ret, 0); > + fp = FREOPEN (NULL, "r+", fp); > + if (fp != NULL) > + { > + puts ("freopen of renamed file succeeded"); > + TEST_COMPARE_FILE_STRING (fp, "file has been renamed"); > + xfclose (fp); > + } > + else > + puts ("freopen of renamed file failed (OK)"); > + ret = rename ("/file1a", "/file1"); > + TEST_COMPARE (ret, 0); > + > + /* Test freopen with NULL, deleted file. This verifies that > + reopening succeeds (and resets the file position indicator to > + start of file) even when the original path could no longer be > + opened, or fails without a memory leak. */ > + > + verbose_printf ("testing freopen with NULL, deleted file\n"); > + fp = xfopen ("/file1", "r+"); > + ret = fputs ("file has now been deleted", fp); > + TEST_VERIFY (ret >= 0); > + ret = remove ("/file1"); > + TEST_COMPARE (ret, 0); > + fp = FREOPEN (NULL, "r+", fp); > + if (fp != NULL) > + { > + puts ("freopen of deleted file succeeded"); > + TEST_COMPARE_FILE_STRING (fp, "file has now been deleted"); > + xfclose (fp); > + } > + else > + puts ("freopen of deleted file failed (OK)"); > + > + free (temp_dir); > + return 0; > +} > + > +#include <support/test-driver.c> > diff --git a/stdio-common/tst-freopen4.c b/stdio-common/tst-freopen4.c > new file mode 100644 > index 0000000000..f39ec0d217 > --- /dev/null > +++ b/stdio-common/tst-freopen4.c > @@ -0,0 +1,2 @@ > +#define FREOPEN freopen > +#include <tst-freopen4-main.c> > diff --git a/stdio-common/tst-freopen5.c b/stdio-common/tst-freopen5.c > new file mode 100644 > index 0000000000..f32626bccf > --- /dev/null > +++ b/stdio-common/tst-freopen5.c > @@ -0,0 +1,144 @@ > +/* Test freopen and freopen64 with large offsets. > + Copyright (C) 2024 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + <https://www.gnu.org/licenses/>. */ > + > +#include <errno.h> > +#include <mcheck.h> > +#include <stdio.h> > +#include <stdlib.h> > + > +#include <support/check.h> > +#include <support/descriptors.h> > +#include <support/support.h> > +#include <support/temp_file.h> > +#include <support/test-driver.h> > +#include <support/xstdio.h> > + > +#define START_TEST(DESC) \ > + do \ > + { \ > + fds = support_descriptors_list (); \ > + verbose_printf (DESC); \ > + } \ > + while (0) > + > +#define END_TEST \ > + do \ > + { \ > + support_descriptors_check (fds); \ > + support_descriptors_free (fds); \ > + } \ > + while (0) > + > +int > +do_test (void) > +{ > + mtrace (); > + struct support_descriptors *fds; > + FILE *fp; > + int ret; > + > + char *temp_dir = support_create_temp_directory ("tst-freopen5"); > + /* This file is removed at the end of each test rather than left > + around between tests to avoid problems with subsequent tests > + reopening it as a large (2GB + 1 byte) file. */ > + char *file1 = xasprintf ("%s/file1", temp_dir); > + > + /* fopen with freopen64: large offsets OK. */ > + START_TEST ("testing fopen with freopen64\n"); > + fp = fopen ("/dev/null", "r"); > + TEST_VERIFY_EXIT (fp != NULL); > + fp = freopen64 (file1, "w", fp); > + TEST_VERIFY_EXIT (fp != NULL); > + setbuf (fp, NULL); > + ret = fseeko64 (fp, 1LL << 32, SEEK_SET); > + TEST_COMPARE (ret, 0); > + ret = fputc ('x', fp); > + TEST_COMPARE (ret, 'x'); > + xfclose (fp); > + ret = remove (file1); > + TEST_COMPARE (ret, 0); > + END_TEST; > + > + /* fopen64 with freopen64: large offsets OK. */ > + START_TEST ("testing fopen64 with freopen64\n"); > + fp = fopen64 ("/dev/null", "r"); > + TEST_VERIFY_EXIT (fp != NULL); > + fp = freopen64 (file1, "w", fp); > + TEST_VERIFY_EXIT (fp != NULL); > + setbuf (fp, NULL); > + ret = fseeko64 (fp, 1LL << 32, SEEK_SET); > + TEST_COMPARE (ret, 0); > + ret = fputc ('x', fp); > + TEST_COMPARE (ret, 'x'); > + xfclose (fp); > + ret = remove (file1); > + TEST_COMPARE (ret, 0); > + END_TEST; > + > + /* fopen with freopen: large offsets not OK on 32-bit systems. */ > + START_TEST ("testing fopen with freopen\n"); > + fp = fopen ("/dev/null", "r"); > + TEST_VERIFY_EXIT (fp != NULL); > + fp = freopen (file1, "w", fp); > + TEST_VERIFY_EXIT (fp != NULL); > + setbuf (fp, NULL); > + ret = fseeko64 (fp, 1LL << 32, SEEK_SET); > + TEST_COMPARE (ret, 0); > + errno = 0; > + ret = fputc ('x', fp); > + if (sizeof (off_t) == 4) > + { > + TEST_COMPARE (ret, EOF); > + TEST_COMPARE (errno, EFBIG); > + } > + else > + TEST_COMPARE (ret, 'x'); > + fclose (fp); > + ret = remove (file1); > + TEST_COMPARE (ret, 0); > + END_TEST; > + > + /* fopen64 with freopen: large offsets not OK on 32-bit systems. */ > + START_TEST ("testing fopen64 with freopen\n"); > + fp = fopen64 ("/dev/null", "r"); > + TEST_VERIFY_EXIT (fp != NULL); > + fp = freopen (file1, "w", fp); > + TEST_VERIFY_EXIT (fp != NULL); > + setbuf (fp, NULL); > + ret = fseeko64 (fp, 1LL << 32, SEEK_SET); > + TEST_COMPARE (ret, 0); > + errno = 0; > + ret = fputc ('x', fp); > + if (sizeof (off_t) == 4) > + { > + TEST_COMPARE (ret, EOF); > + TEST_COMPARE (errno, EFBIG); > + } > + else > + TEST_COMPARE (ret, 'x'); > + fclose (fp); > + ret = remove (file1); > + TEST_COMPARE (ret, 0); > + END_TEST; > + > + free (temp_dir); > + free (file1); > + return 0; > +} > + > +#include <support/test-driver.c> > diff --git a/stdio-common/tst-freopen6-main.c b/stdio-common/tst-freopen6-main.c > new file mode 100644 > index 0000000000..f493f42fd7 > --- /dev/null > +++ b/stdio-common/tst-freopen6-main.c > @@ -0,0 +1,98 @@ > +/* Test freopen of stdin / stdout / stderr. > + Copyright (C) 2024 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + <https://www.gnu.org/licenses/>. */ > + > +#include <errno.h> > +#include <mcheck.h> > +#include <stdio.h> > +#include <stdlib.h> > +#include <unistd.h> > + > +#include <support/check.h> > +#include <support/file_contents.h> > +#include <support/support.h> > +#include <support/temp_file.h> > +#include <support/test-driver.h> > +#include <support/xstdio.h> > + > +int > +do_test (void) > +{ > + mtrace (); > + char *temp_dir = support_create_temp_directory ("tst-freopen6"); > + char *file1 = xasprintf ("%s/file1", temp_dir); > + support_write_file_string (file1, "file1"); > + add_temp_file (file1); > + FILE *fp; > + int ret; > + > + verbose_printf ("Testing reopening stdin\n"); > + fp = FREOPEN (file1, "r", stdin); > + TEST_VERIFY_EXIT (fp == stdin); > + ret = getchar (); > + TEST_COMPARE (ret, 'f'); > + ret = getchar (); > + TEST_COMPARE (ret, 'i'); > + ret = getchar (); > + TEST_COMPARE (ret, 'l'); > + ret = getchar (); > + TEST_COMPARE (ret, 'e'); > + ret = getchar (); > + TEST_COMPARE (ret, '1'); > + ret = getchar (); > + TEST_COMPARE (ret, EOF); > + xfclose (fp); > + > + verbose_printf ("Testing reopening stderr\n"); > + fp = FREOPEN (file1, "w+", stderr); > + TEST_VERIFY_EXIT (fp == stderr); > + errno = EINVAL; > + perror ("test"); > + ret = fseek (fp, 0, SEEK_SET); > + TEST_COMPARE (ret, 0); > + TEST_COMPARE_FILE_STRING (fp, "test: Invalid argument\n"); > + xfclose (fp); > + > + verbose_printf ("Testing reopening stdout\n"); > + /* Defer checks until the old stdout has been restored to make it > + more likely any errors are written to the old stdout (rather than > + the temporary file used for the redirected stdout). */ > + int old_stdout = dup (STDOUT_FILENO); > + TEST_VERIFY_EXIT (old_stdout != -1); > + int ret_fseek = 0; > + int ret_compare = 0; > + fp = FREOPEN (file1, "w+", stdout); > + int fp_eq_stdout = fp == stdout; > + if (fp != NULL) > + { > + printf ("reopened\n"); > + ret_fseek = fseek (fp, 0, SEEK_SET); > + ret_compare = support_compare_file_string (fp, "reopened\n"); > + } > + xfclose (fp); > + stdout = fdopen (old_stdout, "w"); > + TEST_VERIFY (fp_eq_stdout); > + TEST_COMPARE (ret_fseek, 0); > + TEST_COMPARE (ret_compare, 0); > + xfclose (stdout); > + > + free (temp_dir); > + free (file1); > + return 0; > +} > + > +#include <support/test-driver.c> > diff --git a/stdio-common/tst-freopen6.c b/stdio-common/tst-freopen6.c > new file mode 100644 > index 0000000000..8fd6957b54 > --- /dev/null > +++ b/stdio-common/tst-freopen6.c > @@ -0,0 +1,2 @@ > +#define FREOPEN freopen > +#include <tst-freopen6-main.c> > diff --git a/stdio-common/tst-freopen64-4.c b/stdio-common/tst-freopen64-4.c > new file mode 100644 > index 0000000000..1411be2bfa > --- /dev/null > +++ b/stdio-common/tst-freopen64-4.c > @@ -0,0 +1,2 @@ > +#define FREOPEN freopen64 > +#include <tst-freopen4-main.c> > diff --git a/stdio-common/tst-freopen64-6.c b/stdio-common/tst-freopen64-6.c > new file mode 100644 > index 0000000000..3ec509a36c > --- /dev/null > +++ b/stdio-common/tst-freopen64-6.c > @@ -0,0 +1,2 @@ > +#define FREOPEN freopen64 > +#include <tst-freopen6-main.c> >
Hi Joseph, On Tue, Sep 10, 2024 at 10:26:12PM +0000, Joseph Myers wrote: > Add tests of special cases for freopen that were omitted from the more > general tests of different modes and similar issues. The special > cases in the three tests here are logically unconnected, it was simply > convenient to put these tests in one patch. > > * Test freopen with a NULL path to the new file, in a chroot. Rather > than asserting that this fails (logically, failure in this case is > an implementation detail; it's not required for freopen to rely on > /proc), verify that either it fails (without memory leaks) or that > it succeeds and behaves as expected on success. There is no check > for file descriptor leaks because the machinery for that also > depends on /proc, so can't be used in a chroot. > > * Test that freopen and freopen64 are genuinely different in > configurations with 32-bit off_t by checking for an EFBIG trying to > write past 2GB in a file opened with freopen in such a configuration > but no error with 64-bit off_t or when opening with freopen64. > > * Test freopen of stdin, stdout and stderr. > > Tested for x86_64 and x86. I see two of the new tests consistently FAIL on the glibc-fedora-x86_64 buildbot: https://builder.sourceware.org/buildbot/#/builders/glibc-fedora-x86_64 (This is a container builder) While the non-mem variants get marked as UNSUPPORTED the mem variants FAIL with a leak report. UNSUPPORTED: stdio-common/tst-freopen4 FAIL: stdio-common/tst-freopen4-mem UNSUPPORTED: stdio-common/tst-freopen64-4 FAIL: stdio-common/tst-freopen64-4-mem The bunsen logs show the non-mem variants are skipped because the root chroot is not permitted. warning: could not become root outside namespace (Operation not permitted) warning: this process does not support chroot: Operation not permitted https://builder.sourceware.org/testrun/2e1455bb40d9ba87d244a65127bdb0d8725a9059?filename=stdio-common%2Ftst-freopen4.out While the -mem variants show a memory leak: Memory not freed: ----------------- Address Size Caller 0x000055557af032a0 0x18 at 0x80948 https://builder.sourceware.org/testrun/2e1455bb40d9ba87d244a65127bdb0d8725a9059?filename=stdio-common%2Ftst-freopen4-mem.out I don't really understand the build/test logic to know whether the -mem variants should also be skipped. I note that on non-container builds, like fedora-arm64 or ubuntu-riscv all these tests just PASS. Cheers, Mark
* Mark Wielaard: > While the -mem variants show a memory leak: > > Memory not freed: > ----------------- > Address Size Caller > 0x000055557af032a0 0x18 at 0x80948 > > https://builder.sourceware.org/testrun/2e1455bb40d9ba87d244a65127bdb0d8725a9059?filename=stdio-common%2Ftst-freopen4-mem.out > > I don't really understand the build/test logic to know whether the > -mem variants should also be skipped. We can fix the leak instead, I think: [PATCH] stdio-common: Fix memory leak in tst-freopen4* tests on UNSUPPORTED <https://inbox.sourceware.org/libc-alpha/871q16alim.fsf@oldenburg.str.redhat.com/> I don't see the failure, but since there was just one allocation reported, and the patch fixes a leak, I assume it makes the -mem test pass. Thanks, Florian
Hi Florian, On Thu, Sep 26, 2024 at 10:06:32AM +0200, Florian Weimer wrote: > * Mark Wielaard: > > I don't really understand the build/test logic to know whether the > > -mem variants should also be skipped. > > We can fix the leak instead, I think: > > [PATCH] stdio-common: Fix memory leak in tst-freopen4* tests on UNSUPPORTED > <https://inbox.sourceware.org/libc-alpha/871q16alim.fsf@oldenburg.str.redhat.com/> Aha, that makes sense. There is an actual memory leak if support_become_root () fails. > I don't see the failure, but since there was just one allocation > reported, and the patch fixes a leak, I assume it makes the -mem test > pass. Assuming you followed https://sourceware.org/cgit/builder/tree/README_containers it depends on the engine you use whether it allows support_become_root (). The podman one does, the docker/moby one doesn't. Unfortunately podman isn't fully compatible with buildbot docker LatentWorkers. With docker/moby make subdirs=stdio-common check shows: UNSUPPORTED: stdio-common/tst-freopen4 FAIL: stdio-common/tst-freopen4-mem UNSUPPORTED: stdio-common/tst-freopen64-4 FAIL: stdio-common/tst-freopen64-4-mem UNSUPPORTED: stdio-common/tst-popen3 === Summary of results === 2 FAIL 171 PASS 3 UNSUPPORTED With your patch that turns into UNSUPPORTED: stdio-common/tst-freopen4 UNSUPPORTED: stdio-common/tst-freopen64-4 UNSUPPORTED: stdio-common/tst-popen3 === Summary of results === 173 PASS 3 UNSUPPORTED Thanks, Mark
* Mark Wielaard: > Hi Florian, > > On Thu, Sep 26, 2024 at 10:06:32AM +0200, Florian Weimer wrote: >> * Mark Wielaard: >> > I don't really understand the build/test logic to know whether the >> > -mem variants should also be skipped. >> >> We can fix the leak instead, I think: >> >> [PATCH] stdio-common: Fix memory leak in tst-freopen4* tests on UNSUPPORTED >> <https://inbox.sourceware.org/libc-alpha/871q16alim.fsf@oldenburg.str.redhat.com/> > > Aha, that makes sense. There is an actual memory leak if > support_become_root () fails. > >> I don't see the failure, but since there was just one allocation >> reported, and the patch fixes a leak, I assume it makes the -mem test >> pass. > > Assuming you followed > https://sourceware.org/cgit/builder/tree/README_containers it depends > on the engine you use whether it allows support_become_root (). The > podman one does, the docker/moby one doesn't. Unfortunately podman > isn't fully compatible with buildbot docker LatentWorkers. > > With docker/moby make subdirs=stdio-common check shows: > > UNSUPPORTED: stdio-common/tst-freopen4 > FAIL: stdio-common/tst-freopen4-mem > UNSUPPORTED: stdio-common/tst-freopen64-4 > FAIL: stdio-common/tst-freopen64-4-mem > UNSUPPORTED: stdio-common/tst-popen3 > === Summary of results === > 2 FAIL > 171 PASS > 3 UNSUPPORTED > > With your patch that turns into > > UNSUPPORTED: stdio-common/tst-freopen4 > UNSUPPORTED: stdio-common/tst-freopen64-4 > UNSUPPORTED: stdio-common/tst-popen3 > === Summary of results === > 173 PASS > 3 UNSUPPORTED Thanks, I'm going to push the patch then. Florian
diff --git a/stdio-common/Makefile b/stdio-common/Makefile index ce7f7cdd3b..62f8b99b06 100644 --- a/stdio-common/Makefile +++ b/stdio-common/Makefile @@ -219,8 +219,13 @@ tests := \ tst-fphex-wide \ tst-freopen2 \ tst-freopen3 \ + tst-freopen4 \ + tst-freopen5 \ + tst-freopen6 \ tst-freopen64-2 \ tst-freopen64-3 \ + tst-freopen64-4 \ + tst-freopen64-6 \ tst-fseek \ tst-fwrite \ tst-fwrite-memstrm \ @@ -324,8 +329,13 @@ ifneq ($(PERL),no) tests-special += \ $(objpfx)tst-freopen2-mem.out \ $(objpfx)tst-freopen3-mem.out \ + $(objpfx)tst-freopen4-mem.out \ + $(objpfx)tst-freopen5-mem.out \ + $(objpfx)tst-freopen6-mem.out \ $(objpfx)tst-freopen64-2-mem.out \ $(objpfx)tst-freopen64-3-mem.out \ + $(objpfx)tst-freopen64-4-mem.out \ + $(objpfx)tst-freopen64-6-mem.out \ $(objpfx)tst-getline-enomem-mem.out \ $(objpfx)tst-getline-mem.out \ $(objpfx)tst-printf-bz18872-mem.out \ @@ -341,10 +351,20 @@ generated += \ tst-freopen2.mtrace \ tst-freopen3-mem.out \ tst-freopen3.mtrace \ + tst-freopen4-mem.out \ + tst-freopen4.mtrace \ + tst-freopen5-mem.out \ + tst-freopen5.mtrace \ + tst-freopen6-mem.out \ + tst-freopen6.mtrace \ tst-freopen64-2-mem.out \ tst-freopen64-2.mtrace \ tst-freopen64-3-mem.out \ tst-freopen64-3.mtrace \ + tst-freopen64-4-mem.out \ + tst-freopen64-4.mtrace \ + tst-freopen64-6-mem.out \ + tst-freopen64-6.mtrace \ tst-getline-enomem-mem.out \ tst-getline-enomem.mtrace \ tst-getline-mem.out \ @@ -476,6 +496,21 @@ tst-freopen3-ENV = \ tst-freopen64-3-ENV = \ MALLOC_TRACE=$(objpfx)tst-freopen64-3.mtrace \ LD_PRELOAD=$(common-objpfx)malloc/libc_malloc_debug.so +tst-freopen4-ENV = \ + MALLOC_TRACE=$(objpfx)tst-freopen4.mtrace \ + LD_PRELOAD=$(common-objpfx)malloc/libc_malloc_debug.so +tst-freopen64-4-ENV = \ + MALLOC_TRACE=$(objpfx)tst-freopen64-4.mtrace \ + LD_PRELOAD=$(common-objpfx)malloc/libc_malloc_debug.so +tst-freopen5-ENV = \ + MALLOC_TRACE=$(objpfx)tst-freopen5.mtrace \ + LD_PRELOAD=$(common-objpfx)malloc/libc_malloc_debug.so +tst-freopen6-ENV = \ + MALLOC_TRACE=$(objpfx)tst-freopen6.mtrace \ + LD_PRELOAD=$(common-objpfx)malloc/libc_malloc_debug.so +tst-freopen64-6-ENV = \ + MALLOC_TRACE=$(objpfx)tst-freopen64-6.mtrace \ + LD_PRELOAD=$(common-objpfx)malloc/libc_malloc_debug.so $(objpfx)tst-unbputc.out: tst-unbputc.sh $(objpfx)tst-unbputc $(SHELL) $< $(common-objpfx) '$(test-program-prefix)'; \ diff --git a/stdio-common/tst-freopen4-main.c b/stdio-common/tst-freopen4-main.c new file mode 100644 index 0000000000..e169442cf4 --- /dev/null +++ b/stdio-common/tst-freopen4-main.c @@ -0,0 +1,100 @@ +/* Test freopen in chroot. + Copyright (C) 2024 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <https://www.gnu.org/licenses/>. */ + +#include <mcheck.h> +#include <stdio.h> +#include <stdlib.h> + +#include <support/check.h> +#include <support/file_contents.h> +#include <support/namespace.h> +#include <support/support.h> +#include <support/temp_file.h> +#include <support/test-driver.h> +#include <support/xstdio.h> +#include <support/xunistd.h> + +int +do_test (void) +{ + mtrace (); + char *temp_dir = support_create_temp_directory ("tst-freopen4"); + FILE *fp; + int ret; + + /* These chroot tests verify that either reopening a renamed or + deleted file works even in the absence of /proc, or that it fails + (without memory leaks); thus, for example, such reopening does + not crash in the absence of /proc. */ + + support_become_root (); + if (!support_can_chroot ()) + return EXIT_UNSUPPORTED; + xchroot (temp_dir); + + /* Test freopen with NULL, renamed file. This verifies that + reopening succeeds (and resets the file position indicator to + start of file) even when the original path could no longer be + opened, or fails without a memory leak. (It is not possible to + use <support/descriptors.h> to test for file descriptor leaks + here, because that also depends on /proc.) */ + + verbose_printf ("testing freopen with NULL, renamed file\n"); + fp = xfopen ("/file1", "w+"); + ret = fputs ("file has been renamed", fp); + TEST_VERIFY (ret >= 0); + ret = rename ("/file1", "/file1a"); + TEST_COMPARE (ret, 0); + fp = FREOPEN (NULL, "r+", fp); + if (fp != NULL) + { + puts ("freopen of renamed file succeeded"); + TEST_COMPARE_FILE_STRING (fp, "file has been renamed"); + xfclose (fp); + } + else + puts ("freopen of renamed file failed (OK)"); + ret = rename ("/file1a", "/file1"); + TEST_COMPARE (ret, 0); + + /* Test freopen with NULL, deleted file. This verifies that + reopening succeeds (and resets the file position indicator to + start of file) even when the original path could no longer be + opened, or fails without a memory leak. */ + + verbose_printf ("testing freopen with NULL, deleted file\n"); + fp = xfopen ("/file1", "r+"); + ret = fputs ("file has now been deleted", fp); + TEST_VERIFY (ret >= 0); + ret = remove ("/file1"); + TEST_COMPARE (ret, 0); + fp = FREOPEN (NULL, "r+", fp); + if (fp != NULL) + { + puts ("freopen of deleted file succeeded"); + TEST_COMPARE_FILE_STRING (fp, "file has now been deleted"); + xfclose (fp); + } + else + puts ("freopen of deleted file failed (OK)"); + + free (temp_dir); + return 0; +} + +#include <support/test-driver.c> diff --git a/stdio-common/tst-freopen4.c b/stdio-common/tst-freopen4.c new file mode 100644 index 0000000000..f39ec0d217 --- /dev/null +++ b/stdio-common/tst-freopen4.c @@ -0,0 +1,2 @@ +#define FREOPEN freopen +#include <tst-freopen4-main.c> diff --git a/stdio-common/tst-freopen5.c b/stdio-common/tst-freopen5.c new file mode 100644 index 0000000000..f32626bccf --- /dev/null +++ b/stdio-common/tst-freopen5.c @@ -0,0 +1,144 @@ +/* Test freopen and freopen64 with large offsets. + Copyright (C) 2024 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <https://www.gnu.org/licenses/>. */ + +#include <errno.h> +#include <mcheck.h> +#include <stdio.h> +#include <stdlib.h> + +#include <support/check.h> +#include <support/descriptors.h> +#include <support/support.h> +#include <support/temp_file.h> +#include <support/test-driver.h> +#include <support/xstdio.h> + +#define START_TEST(DESC) \ + do \ + { \ + fds = support_descriptors_list (); \ + verbose_printf (DESC); \ + } \ + while (0) + +#define END_TEST \ + do \ + { \ + support_descriptors_check (fds); \ + support_descriptors_free (fds); \ + } \ + while (0) + +int +do_test (void) +{ + mtrace (); + struct support_descriptors *fds; + FILE *fp; + int ret; + + char *temp_dir = support_create_temp_directory ("tst-freopen5"); + /* This file is removed at the end of each test rather than left + around between tests to avoid problems with subsequent tests + reopening it as a large (2GB + 1 byte) file. */ + char *file1 = xasprintf ("%s/file1", temp_dir); + + /* fopen with freopen64: large offsets OK. */ + START_TEST ("testing fopen with freopen64\n"); + fp = fopen ("/dev/null", "r"); + TEST_VERIFY_EXIT (fp != NULL); + fp = freopen64 (file1, "w", fp); + TEST_VERIFY_EXIT (fp != NULL); + setbuf (fp, NULL); + ret = fseeko64 (fp, 1LL << 32, SEEK_SET); + TEST_COMPARE (ret, 0); + ret = fputc ('x', fp); + TEST_COMPARE (ret, 'x'); + xfclose (fp); + ret = remove (file1); + TEST_COMPARE (ret, 0); + END_TEST; + + /* fopen64 with freopen64: large offsets OK. */ + START_TEST ("testing fopen64 with freopen64\n"); + fp = fopen64 ("/dev/null", "r"); + TEST_VERIFY_EXIT (fp != NULL); + fp = freopen64 (file1, "w", fp); + TEST_VERIFY_EXIT (fp != NULL); + setbuf (fp, NULL); + ret = fseeko64 (fp, 1LL << 32, SEEK_SET); + TEST_COMPARE (ret, 0); + ret = fputc ('x', fp); + TEST_COMPARE (ret, 'x'); + xfclose (fp); + ret = remove (file1); + TEST_COMPARE (ret, 0); + END_TEST; + + /* fopen with freopen: large offsets not OK on 32-bit systems. */ + START_TEST ("testing fopen with freopen\n"); + fp = fopen ("/dev/null", "r"); + TEST_VERIFY_EXIT (fp != NULL); + fp = freopen (file1, "w", fp); + TEST_VERIFY_EXIT (fp != NULL); + setbuf (fp, NULL); + ret = fseeko64 (fp, 1LL << 32, SEEK_SET); + TEST_COMPARE (ret, 0); + errno = 0; + ret = fputc ('x', fp); + if (sizeof (off_t) == 4) + { + TEST_COMPARE (ret, EOF); + TEST_COMPARE (errno, EFBIG); + } + else + TEST_COMPARE (ret, 'x'); + fclose (fp); + ret = remove (file1); + TEST_COMPARE (ret, 0); + END_TEST; + + /* fopen64 with freopen: large offsets not OK on 32-bit systems. */ + START_TEST ("testing fopen64 with freopen\n"); + fp = fopen64 ("/dev/null", "r"); + TEST_VERIFY_EXIT (fp != NULL); + fp = freopen (file1, "w", fp); + TEST_VERIFY_EXIT (fp != NULL); + setbuf (fp, NULL); + ret = fseeko64 (fp, 1LL << 32, SEEK_SET); + TEST_COMPARE (ret, 0); + errno = 0; + ret = fputc ('x', fp); + if (sizeof (off_t) == 4) + { + TEST_COMPARE (ret, EOF); + TEST_COMPARE (errno, EFBIG); + } + else + TEST_COMPARE (ret, 'x'); + fclose (fp); + ret = remove (file1); + TEST_COMPARE (ret, 0); + END_TEST; + + free (temp_dir); + free (file1); + return 0; +} + +#include <support/test-driver.c> diff --git a/stdio-common/tst-freopen6-main.c b/stdio-common/tst-freopen6-main.c new file mode 100644 index 0000000000..f493f42fd7 --- /dev/null +++ b/stdio-common/tst-freopen6-main.c @@ -0,0 +1,98 @@ +/* Test freopen of stdin / stdout / stderr. + Copyright (C) 2024 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <https://www.gnu.org/licenses/>. */ + +#include <errno.h> +#include <mcheck.h> +#include <stdio.h> +#include <stdlib.h> +#include <unistd.h> + +#include <support/check.h> +#include <support/file_contents.h> +#include <support/support.h> +#include <support/temp_file.h> +#include <support/test-driver.h> +#include <support/xstdio.h> + +int +do_test (void) +{ + mtrace (); + char *temp_dir = support_create_temp_directory ("tst-freopen6"); + char *file1 = xasprintf ("%s/file1", temp_dir); + support_write_file_string (file1, "file1"); + add_temp_file (file1); + FILE *fp; + int ret; + + verbose_printf ("Testing reopening stdin\n"); + fp = FREOPEN (file1, "r", stdin); + TEST_VERIFY_EXIT (fp == stdin); + ret = getchar (); + TEST_COMPARE (ret, 'f'); + ret = getchar (); + TEST_COMPARE (ret, 'i'); + ret = getchar (); + TEST_COMPARE (ret, 'l'); + ret = getchar (); + TEST_COMPARE (ret, 'e'); + ret = getchar (); + TEST_COMPARE (ret, '1'); + ret = getchar (); + TEST_COMPARE (ret, EOF); + xfclose (fp); + + verbose_printf ("Testing reopening stderr\n"); + fp = FREOPEN (file1, "w+", stderr); + TEST_VERIFY_EXIT (fp == stderr); + errno = EINVAL; + perror ("test"); + ret = fseek (fp, 0, SEEK_SET); + TEST_COMPARE (ret, 0); + TEST_COMPARE_FILE_STRING (fp, "test: Invalid argument\n"); + xfclose (fp); + + verbose_printf ("Testing reopening stdout\n"); + /* Defer checks until the old stdout has been restored to make it + more likely any errors are written to the old stdout (rather than + the temporary file used for the redirected stdout). */ + int old_stdout = dup (STDOUT_FILENO); + TEST_VERIFY_EXIT (old_stdout != -1); + int ret_fseek = 0; + int ret_compare = 0; + fp = FREOPEN (file1, "w+", stdout); + int fp_eq_stdout = fp == stdout; + if (fp != NULL) + { + printf ("reopened\n"); + ret_fseek = fseek (fp, 0, SEEK_SET); + ret_compare = support_compare_file_string (fp, "reopened\n"); + } + xfclose (fp); + stdout = fdopen (old_stdout, "w"); + TEST_VERIFY (fp_eq_stdout); + TEST_COMPARE (ret_fseek, 0); + TEST_COMPARE (ret_compare, 0); + xfclose (stdout); + + free (temp_dir); + free (file1); + return 0; +} + +#include <support/test-driver.c> diff --git a/stdio-common/tst-freopen6.c b/stdio-common/tst-freopen6.c new file mode 100644 index 0000000000..8fd6957b54 --- /dev/null +++ b/stdio-common/tst-freopen6.c @@ -0,0 +1,2 @@ +#define FREOPEN freopen +#include <tst-freopen6-main.c> diff --git a/stdio-common/tst-freopen64-4.c b/stdio-common/tst-freopen64-4.c new file mode 100644 index 0000000000..1411be2bfa --- /dev/null +++ b/stdio-common/tst-freopen64-4.c @@ -0,0 +1,2 @@ +#define FREOPEN freopen64 +#include <tst-freopen4-main.c> diff --git a/stdio-common/tst-freopen64-6.c b/stdio-common/tst-freopen64-6.c new file mode 100644 index 0000000000..3ec509a36c --- /dev/null +++ b/stdio-common/tst-freopen64-6.c @@ -0,0 +1,2 @@ +#define FREOPEN freopen64 +#include <tst-freopen6-main.c>