Add freopen special-case tests: thread cancellation

Message ID d827bf0c-29cb-c8a1-cff7-edcd2cd60732@redhat.com
State Superseded
Headers
Series Add freopen special-case tests: thread cancellation |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent
redhat-pt-bot/TryBot-32bit success Build for i686
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 success Test passed
linaro-tcwg-bot/tcwg_glibc_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_glibc_check--master-arm success Test passed

Commit Message

Joseph Myers Sept. 11, 2024, 10:35 p.m. UTC
  Add tests of freopen adding or removing "c" (non-cancelling I/O) from
the mode string (so completing my planned tests of freopen with
different features used in the mode strings).  Note that it's in the
nature of the uncertain time at which cancellation might act (possibly
during freopen, possibly during subsequent reads) that these can leak
memory or file descriptors, so these do not include leak tests.

Tested for x86_64.
  

Comments

Florian Weimer Sept. 12, 2024, 11:20 a.m. UTC | #1
* Joseph Myers:

> Note that it's in the nature of the uncertain time at which
> cancellation might act (possibly during freopen, possibly during
> subsequent reads) that these can leak memory or file descriptors, so
> these do not include leak tests.

Hmm.  Is it even possible for applications to handle this correctly?
Given that freopen closes the original stream on error?  So the
cancellation handler can't know if the stream is still open or not.

> diff --git a/sysdeps/pthread/Makefile b/sysdeps/pthread/Makefile
> index 04ea56559e..319b6ac64f 100644
> --- a/sysdeps/pthread/Makefile
> +++ b/sysdeps/pthread/Makefile
> @@ -534,3 +534,13 @@ $(objpfx)tst-create1: $(shared-thread-library)
>  $(objpfx)tst-create1.out: $(objpfx)tst-create1mod.so
>  
>  endif
> +
> +ifeq ($(subdir),stdio-common)
> +tests += \
> +  tst-freopen-cancel \
> +  tst-freopen64-cancel \
> +  # tests
> +
> +$(objpfx)tst-freopen-cancel: $(shared-thread-library)
> +$(objpfx)tst-freopen64-cancel: $(shared-thread-library)
> +endif

The test could be in stdio-common.  It doesn't need serial execution.

> +void *
> +test_rc_to_r (void *p)
> +{
> +  int ret;
> +  FILE *fp, *fp2;
> +  ret = sem_post (&sem);
> +  TEST_VERIFY_EXIT (ret == 0);
> +  fp = xfopen (file1, "rc");
> +  for (int i = 0; i < 1000000; i++)
> +    {
> +      fgetc (fp);
> +      fseek (fp, 0, SEEK_SET);
> +    }
> +  fp2 = xfopen (file3, "wc");
> +  fputs ("rc_to_r got to freopen", fp2);
> +  xfclose (fp2);
> +  /* Cancellation should occur at some point from here onwards
> +     (possibly leaking memory and file descriptors associated with the
> +     FILE).  */
> +  fp = FREOPEN (file2, "r", fp);
> +  TEST_VERIFY_EXIT (fp != NULL);
> +  for (;;)
> +    {
> +      fgetc (fp);
> +      fseek (fp, 0, SEEK_SET);
> +    }
> +}

The test does not assert that cancellation happens in the expected
region.  You could set up fp in the main thread and pass it via the
test_rc_to_r pointer argument?

> +void *
> +test_r_to_rc (void *p)
> +{
> +  int ret;
> +  FILE *fp;
> +  fp = xfopen (file1, "r");
> +  fp = FREOPEN (file2, "rc", fp);
> +  TEST_VERIFY_EXIT (fp != NULL);
> +  ret = sem_post (&sem);
> +  TEST_VERIFY_EXIT (ret == 0);
> +  /* No cancellation should occur for I/O on file2.  */
> +  for (int i = 0; i < 1000000; i++)
> +    {
> +      fgetc (fp);
> +      fseek (fp, 0, SEEK_SET);
> +    }

Maybe you could use a FIFO and <support/process_state.h> to avoid
spinning here?  Call pthread_cancel only after
support_process_state_wait reports reaching
support_process_state_sleeping?

> +  xfclose (fp);
> +  fp = xfopen (file3, "wc");
> +  fputs ("r_to_rc got to fclose", fp);
> +  xfclose (fp);
> +  for (;;)
> +    pthread_testcancel ();
> +}

I assume you wrote it this way to assert that there actually is a
pending cancellation request.  I suggest to set a global variable after
the xfclose to make sure that the cancellation was not acted upon in
fclose.

Thanks,
Florian
  

Patch

diff --git a/sysdeps/pthread/Makefile b/sysdeps/pthread/Makefile
index 04ea56559e..319b6ac64f 100644
--- a/sysdeps/pthread/Makefile
+++ b/sysdeps/pthread/Makefile
@@ -534,3 +534,13 @@  $(objpfx)tst-create1: $(shared-thread-library)
 $(objpfx)tst-create1.out: $(objpfx)tst-create1mod.so
 
 endif
+
+ifeq ($(subdir),stdio-common)
+tests += \
+  tst-freopen-cancel \
+  tst-freopen64-cancel \
+  # tests
+
+$(objpfx)tst-freopen-cancel: $(shared-thread-library)
+$(objpfx)tst-freopen64-cancel: $(shared-thread-library)
+endif
diff --git a/sysdeps/pthread/tst-freopen-cancel-main.c b/sysdeps/pthread/tst-freopen-cancel-main.c
new file mode 100644
index 0000000000..60d28d513e
--- /dev/null
+++ b/sysdeps/pthread/tst-freopen-cancel-main.c
@@ -0,0 +1,142 @@ 
+/* Test freopen cancellation handling.
+   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 <fcntl.h>
+#include <mcheck.h>
+#include <pthread.h>
+#include <semaphore.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <wchar.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>
+#include <support/xthread.h>
+
+char *file1, *file2, *file3;
+
+sem_t sem;
+
+void *
+test_rc_to_r (void *p)
+{
+  int ret;
+  FILE *fp, *fp2;
+  ret = sem_post (&sem);
+  TEST_VERIFY_EXIT (ret == 0);
+  fp = xfopen (file1, "rc");
+  for (int i = 0; i < 1000000; i++)
+    {
+      fgetc (fp);
+      fseek (fp, 0, SEEK_SET);
+    }
+  fp2 = xfopen (file3, "wc");
+  fputs ("rc_to_r got to freopen", fp2);
+  xfclose (fp2);
+  /* Cancellation should occur at some point from here onwards
+     (possibly leaking memory and file descriptors associated with the
+     FILE).  */
+  fp = FREOPEN (file2, "r", fp);
+  TEST_VERIFY_EXIT (fp != NULL);
+  for (;;)
+    {
+      fgetc (fp);
+      fseek (fp, 0, SEEK_SET);
+    }
+}
+
+void *
+test_r_to_rc (void *p)
+{
+  int ret;
+  FILE *fp;
+  fp = xfopen (file1, "r");
+  fp = FREOPEN (file2, "rc", fp);
+  TEST_VERIFY_EXIT (fp != NULL);
+  ret = sem_post (&sem);
+  TEST_VERIFY_EXIT (ret == 0);
+  /* No cancellation should occur for I/O on file2.  */
+  for (int i = 0; i < 1000000; i++)
+    {
+      fgetc (fp);
+      fseek (fp, 0, SEEK_SET);
+    }
+  xfclose (fp);
+  fp = xfopen (file3, "wc");
+  fputs ("r_to_rc got to fclose", fp);
+  xfclose (fp);
+  for (;;)
+    pthread_testcancel ();
+}
+
+int
+do_test (void)
+{
+  char *temp_dir = support_create_temp_directory ("tst-freopen-cancel");
+  file1 = xasprintf ("%s/file1", temp_dir);
+  support_write_file_string (file1, "file1");
+  add_temp_file (file1);
+  file2 = xasprintf ("%s/file2", temp_dir);
+  support_write_file_string (file2, "file2");
+  add_temp_file (file2);
+  file3 = xasprintf ("%s/file3", temp_dir);
+  support_write_file_string (file3, "file3");
+  add_temp_file (file3);
+  int ret;
+  pthread_t thr;
+  void *retval;
+
+  /* Test changing to/from c (cancellation disabled).  */
+
+  verbose_printf ("Testing rc -> r\n");
+  ret = sem_init (&sem, 0, 0);
+  TEST_VERIFY_EXIT (ret == 0);
+  thr = xpthread_create (NULL, test_rc_to_r, NULL);
+  ret = sem_wait (&sem);
+  TEST_VERIFY_EXIT (ret == 0);
+  xpthread_cancel (thr);
+  ret = pthread_join (thr, &retval);
+  TEST_COMPARE (ret, 0);
+  TEST_VERIFY (retval == PTHREAD_CANCELED);
+  TEST_OPEN_AND_COMPARE_FILE_STRING (file3, "rc_to_r got to freopen");
+
+  verbose_printf ("Testing r -> rc\n");
+  ret = sem_init (&sem, 0, 0);
+  TEST_VERIFY_EXIT (ret == 0);
+  thr = xpthread_create (NULL, test_r_to_rc, NULL);
+  ret = sem_wait (&sem);
+  TEST_VERIFY_EXIT (ret == 0);
+  xpthread_cancel (thr);
+  ret = pthread_join (thr, &retval);
+  TEST_COMPARE (ret, 0);
+  TEST_VERIFY (retval == PTHREAD_CANCELED);
+  TEST_OPEN_AND_COMPARE_FILE_STRING (file3, "r_to_rc got to fclose");
+
+  free (temp_dir);
+  free (file1);
+  free (file2);
+  free (file3);
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/sysdeps/pthread/tst-freopen-cancel.c b/sysdeps/pthread/tst-freopen-cancel.c
new file mode 100644
index 0000000000..2996e9ad36
--- /dev/null
+++ b/sysdeps/pthread/tst-freopen-cancel.c
@@ -0,0 +1,2 @@ 
+#define FREOPEN freopen
+#include <tst-freopen-cancel-main.c>
diff --git a/sysdeps/pthread/tst-freopen64-cancel.c b/sysdeps/pthread/tst-freopen64-cancel.c
new file mode 100644
index 0000000000..53fbf099af
--- /dev/null
+++ b/sysdeps/pthread/tst-freopen64-cancel.c
@@ -0,0 +1,2 @@ 
+#define FREOPEN freopen64
+#include <tst-freopen-cancel-main.c>