[PATCH COMITTED] stdio-common/Makefile: Fix FAIL: lint-makefiles
H.J. Lu
hjl.tools@gmail.com
Tue Oct 8 00:54:25 GMT 2024
On Tue, Oct 1, 2024 at 9:00 PM Adhemerval Zanella Netto
<adhemerval.zanella@linaro.org> wrote:
>
>
>
> 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>
> >
> >
>
I am checking in this.
--
H.J.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-stdio-common-Makefile-Fix-FAIL-lint-makefiles.patch
Type: application/x-patch
Size: 1086 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/libc-alpha/attachments/20241008/be545927/attachment-0001.bin>
More information about the Libc-alpha
mailing list