From patchwork Thu Sep 12 22:43:50 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Joseph Myers X-Patchwork-Id: 97513 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 5FE203858C35 for ; Thu, 12 Sep 2024 22:45:09 +0000 (GMT) X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTP id 7784D3858432 for ; Thu, 12 Sep 2024 22:44:37 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 7784D3858432 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 7784D3858432 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.133.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1726181081; cv=none; b=UqI3H+ilaGkCNVWWdgIAoHHHP04whA1gmZPPNN2k4MkQJ+7H3eVG0NjnmY4/gAOoBrr04QaEFw8sYeAwkYA+BNmtNAF/CwT2ZmF2SqZ+CJRGQGYDsNmrSpetxpe5aRX1bz5ZFFUVNiAifpXTHbZ3JHTV9wew+I8i/g4JdvmAYAQ= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1726181081; c=relaxed/simple; bh=R4RKICkeZ1MHE4vaI0aKtn9v3mO9MbRx+rx1ChIEyrg=; h=DKIM-Signature:Date:From:To:Subject:Message-ID:MIME-Version; b=ply/o4RqNpMe5C2r7qCOkNAkWZW1MOFakvURqoMW4mfHJcOcdOWBQyyDP85heDpVUCK/VGn9m/gF8bTsfeXZXCNIk1voJ/wx64hLLWDvZAwqK3TKiVb94hoDtiL2LOLSVD6ftPj8xRiMOIyqAgXrXw7cc7MJlilzrfruHjgbS5Y= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1726181077; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=GiHFVv/mqaB/Kkbol3OVyQD4qjarAi/bxpJqwt6UqTI=; b=UXVs42EFVGYFkyokT/jmMVUxN90bd84I5hST9Arpv4BkVExWg8E70RjiCWjheADoMbSh6T qTanMyWTER/IJVNjzmN/xvWtKYZ3d+i6Sc2JR4ezP36jkcl0clGpjIpdvaP/iXr4bBMJRR QmbTQXeUuFsWPSOdNzVfZSDFHYF4huk= Received: from mail-wm1-f70.google.com (mail-wm1-f70.google.com [209.85.128.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-610-dhSG_D20Puq-wvUv-baUJg-1; Thu, 12 Sep 2024 18:44:35 -0400 X-MC-Unique: dhSG_D20Puq-wvUv-baUJg-1 Received: by mail-wm1-f70.google.com with SMTP id 5b1f17b1804b1-42cb22d396cso11184865e9.0 for ; Thu, 12 Sep 2024 15:44:35 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1726181074; x=1726785874; h=mime-version:references:message-id:in-reply-to:subject:cc:to:from :date:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=GiHFVv/mqaB/Kkbol3OVyQD4qjarAi/bxpJqwt6UqTI=; b=q5P5SKghRHe8KLYe2zTEN4btxyALZ9gY26Z3F8QeyHKceInlLJU2ldHhtqXqSdFQyU jv54gHNGVKgUCkqYyWS3w4sedBgV1ae63/ZmdI8TRMIL1zV4RHHXIlwcuvBDqVuSYR7c xfxMv8mHUPrYLtVS9OLDqBKEm1AKm5mCqBBlZrp6wtPTVKs/LvRmSowuEgXJKyn37Nyj gKUxTr8lUWSpkzmBwAE+WA4CHolYc1T/mNBWpshtgp+O/xL4snbyoby/RQMkZgR1GGeU zDDtCBmTs2qFuwe80k9+/vGHvHgG00xgrhm8sggKdACtN2MSi/zbM04TwuVisc3aLRmk nWWw== X-Gm-Message-State: AOJu0Yz1WdVKNYHNxtZQVHYtYqd8dzdyRBYCPe0VfSIULRXRXZSeo58y 2Wl/dpGn7yHFqOIr5Md1yHL60vIKP0+uEfQ+bhEfiXJlZpCpRwEsu0JIUFnN0zhaB0wV4OERSiX uh8HFkzU2e1NX0/Pwu0/3poG/nMS9BQ2dujvWd4+qGamTp8Gt0xbVR4CCfNLXjulY2A== X-Received: by 2002:adf:efcb:0:b0:374:c515:4441 with SMTP id ffacd0b85a97d-378c2d728d1mr2436768f8f.56.1726181073697; Thu, 12 Sep 2024 15:44:33 -0700 (PDT) X-Google-Smtp-Source: AGHT+IGoFp9LQ4x281J5OpdAwEGvButBoAEO2bPDbi0YEaDxFP12m5/x2mxca231mvefOIujiI25QQ== X-Received: by 2002:adf:efcb:0:b0:374:c515:4441 with SMTP id ffacd0b85a97d-378c2d728d1mr2436751f8f.56.1726181072747; Thu, 12 Sep 2024 15:44:32 -0700 (PDT) Received: from digraph.polyomino.org.uk (digraph.polyomino.org.uk. [2001:8b0:bf73:93f7::51bb:e332]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-378956de262sm15327089f8f.112.2024.09.12.15.44.32 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 12 Sep 2024 15:44:32 -0700 (PDT) Received: from jsm28 (helo=localhost) by digraph.polyomino.org.uk with local-esmtp (Exim 4.97) (envelope-from ) id 1sosXW-0000000C0Aj-3w8j; Thu, 12 Sep 2024 22:43:50 +0000 Date: Thu, 12 Sep 2024 22:43:50 +0000 (UTC) From: Joseph Myers To: Florian Weimer cc: libc-alpha@sourceware.org Subject: [PATCH v2] Add freopen special-case tests: thread cancellation In-Reply-To: <87wmjh2k56.fsf@oldenburg.str.redhat.com> Message-ID: References: <87wmjh2k56.fsf@oldenburg.str.redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-9.1 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libc-alpha-bounces~patchwork=sourceware.org@sourceware.org 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 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. --- 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 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 + . */ + +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include +#include + +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 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