[PATCH v2] Add freopen special-case tests: thread cancellation
Adhemerval Zanella Netto
adhemerval.zanella@linaro.org
Tue Oct 1 12:59:40 GMT 2024
On 12/09/24 19:43, Joseph Myers wrote:
> On Thu, 12 Sep 2024, Florian Weimer wrote:
>
>> * 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.
>
> I don't expect this to work particularly well in applications (in terms of
> knowing if the stream is open).
>
>>> +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.
>
> I've moved the test. This makes it depend on my previous patch with other
> special-case tests (textually in the Makefile, not in any substantive
> way).
>
>>> + 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?
>
> It makes sure cancellation only occurs after the checked-for text is
> written to file3.
>
>>> +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?
>
> I'm not sure a check for sleeping is right, especially since we're
> concerned about the state of a thread not a process. But I've changed to
> use a fifo and avoided spinning that way.
>
>>> + 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.
>
> If cancellation occurred in the first xfclose, then the check for file3
> contents would fail. There's an implicit assumption that "c" works OK
> with fopen so the second xfclose isn't an issue (just changes in freopen
> are being tested here).
>
>
> Add freopen special-case tests: thread cancellation
>
> 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.
LGTM, thanks.
Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
>
> ---
>
> Changed in v2: moved tests to stdio-common; use a fifo in test_r_to_rc
> to avoid any need to spin reading or calling pthread_testcancel.
>
> diff --git a/stdio-common/Makefile b/stdio-common/Makefile
> index 62f8b99b06..79da56d055 100644
> --- a/stdio-common/Makefile
> +++ b/stdio-common/Makefile
> @@ -222,10 +222,12 @@ tests := \
> tst-freopen4 \
> tst-freopen5 \
> tst-freopen6 \
> + tst-freopen7 \
> tst-freopen64-2 \
> tst-freopen64-3 \
> tst-freopen64-4 \
> tst-freopen64-6 \
> + tst-freopen64-7 \
> tst-fseek \
> tst-fwrite \
> tst-fwrite-memstrm \
> @@ -620,3 +622,6 @@ $(objpfx)tst-setvbuf1-cmp.out: tst-setvbuf1.expect $(objpfx)tst-setvbuf1.out
>
> $(objpfx)tst-printf-round: $(libm)
> $(objpfx)tst-scanf-round: $(libm)
> +
> +$(objpfx)tst-freopen7: $(shared-thread-library)
> +$(objpfx)tst-freopen64-7: $(shared-thread-library)
> diff --git a/stdio-common/tst-freopen64-7.c b/stdio-common/tst-freopen64-7.c
> new file mode 100644
> index 0000000000..f34c280521
> --- /dev/null
> +++ b/stdio-common/tst-freopen64-7.c
> @@ -0,0 +1,2 @@
> +#define FREOPEN freopen64
> +#include <tst-freopen7-main.c>
> diff --git a/stdio-common/tst-freopen7-main.c b/stdio-common/tst-freopen7-main.c
> new file mode 100644
> index 0000000000..965e0b4adc
> --- /dev/null
> +++ b/stdio-common/tst-freopen7-main.c
> @@ -0,0 +1,155 @@
> +/* 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>
> +#include <support/xunistd.h>
> +
> +char *file1, *file2, *file3, *fifo;
> +
> +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 (fifo, "rc", fp);
> + TEST_VERIFY_EXIT (fp != NULL);
> + ret = sem_post (&sem);
> + TEST_VERIFY_EXIT (ret == 0);
> + /* No cancellation should occur for I/O on fifo. */
> + ret = fgetc (fp);
> + /* At this point, the other thread has called pthread_cancel and
> + then written a byte to the fifo, so this thread is cancelled at
> + the next cancellation point. */
> + TEST_VERIFY (ret == 'x');
> + xfclose (fp);
> + fp = xfopen (file3, "wc");
> + fputs ("r_to_rc got to fclose", fp);
> + xfclose (fp);
> + pthread_testcancel ();
> + FAIL_EXIT1 ("test_r_to_rc not cancelled\n");
> +}
> +
> +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);
> + fifo = xasprintf ("%s/fifo", temp_dir);
> + xmkfifo (fifo, 0666);
> + add_temp_file (fifo);
> + 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);
> + FILE *fp = xfopen (fifo, "w");
> + ret = sem_wait (&sem);
> + TEST_VERIFY_EXIT (ret == 0);
> + /* This call happens while, or before, the other thread is waiting
> + to read a character from the fifo. It thus verifies that
> + cancellation does not occur from the fgetc call in that thread
> + (it should instead occur only in pthread_testcancel call),
> + because the expected string is only written to file3 after that
> + thread closes the fifo. */
> + xpthread_cancel (thr);
> + fputc ('x', fp);
> + xfclose (fp);
> + 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/stdio-common/tst-freopen7.c b/stdio-common/tst-freopen7.c
> new file mode 100644
> index 0000000000..03d0de798e
> --- /dev/null
> +++ b/stdio-common/tst-freopen7.c
> @@ -0,0 +1,2 @@
> +#define FREOPEN freopen
> +#include <tst-freopen7-main.c>
>
>
More information about the Libc-alpha
mailing list