diff mbox series

[v2] Add freopen special-case tests: chroot, EFBIG, stdin/stdout/stderr

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

Commit Message

Joseph Myers Sept. 10, 2024, 10:26 p.m. UTC
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.

---

Changed in v2: test of freopen of stdin / stdout / stderr added;
unused OTHER_FREOPEN definitions removed in tst-freopen4 /
tst-freopen64-4.

Comments

Joseph Myers Sept. 20, 2024, 9:26 p.m. UTC | #1
Ping.  This patch 
<https://sourceware.org/pipermail/libc-alpha/2024-September/159856.html> 
is pending review.
Carlos O'Donell Sept. 20, 2024, 9:45 p.m. UTC | #2
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>
>
Mark Wielaard Sept. 25, 2024, 11:42 p.m. UTC | #3
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
Florian Weimer Sept. 26, 2024, 8:06 a.m. UTC | #4
* 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
Mark Wielaard Sept. 28, 2024, 4:52 p.m. UTC | #5
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
Florian Weimer Sept. 28, 2024, 7:06 p.m. UTC | #6
* 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 mbox series

Patch

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>