From patchwork Sat Feb 10 21:22:05 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Zack Weinberg X-Patchwork-Id: 25903 Received: (qmail 72429 invoked by alias); 10 Feb 2018 21:22:10 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 72416 invoked by uid 89); 10 Feb 2018 21:22:09 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.8 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, SPF_PASS, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy=accumulated, ifd X-HELO: mailbackend.panix.com From: Zack Weinberg To: libc-alpha@sourceware.org Subject: [PATCH] [BZ #22830] malloc_stats: restore cancellation for stderr correctly. Date: Sat, 10 Feb 2018 16:22:05 -0500 Message-Id: <20180210212205.1357-1-zackw@panix.com> MIME-Version: 1.0 malloc_stats means to disable cancellation for writes to stderr while it runs, but it restores stderr->_flags2 with |= instead of =, so what it actually does is disable cancellation on stderr permanently. I'm going to go ahead and commit this as an obvious bugfix, but I would welcome feedback on the test case, which is rather complicated and could probably be made more robust (for instance, setting stderr to line-buffered breaks the test and I don't understand why). Perhaps _IO_lock_acquire_clear_flags2 should be extended to handle _IO_FLAGS2_NOTCANCEL as well as what it already does, but that would _not_ be an obvious bugfix. :-) [BZ #22830] * malloc/malloc.c (__malloc_stats): Restore stderr->_flags2 correctly. * malloc/tst-malloc-stats-cancellation.c: New test case. * malloc/Makefile: Add new test case. --- malloc/Makefile | 2 + malloc/malloc.c | 2 +- malloc/tst-malloc-stats-cancellation.c | 216 +++++++++++++++++++++++++++++++++ 3 files changed, 219 insertions(+), 1 deletion(-) create mode 100644 malloc/tst-malloc-stats-cancellation.c diff --git a/malloc/Makefile b/malloc/Makefile index 17873e67c4b..7d54bad866f 100644 --- a/malloc/Makefile +++ b/malloc/Makefile @@ -37,6 +37,7 @@ tests := mallocbug tst-malloc tst-valloc tst-calloc tst-obstack \ tst-malloc-tcache-leak \ tst-malloc_info \ tst-malloc-too-large \ + tst-malloc-stats-cancellation \ tests-static := \ tst-interpose-static-nothread \ @@ -96,6 +97,7 @@ $(objpfx)tst-malloc-backtrace: $(shared-thread-library) $(objpfx)tst-malloc-thread-exit: $(shared-thread-library) $(objpfx)tst-malloc-thread-fail: $(shared-thread-library) $(objpfx)tst-malloc-fork-deadlock: $(shared-thread-library) +$(objpfx)tst-malloc-stats-cancellation: $(shared-thread-library) # Export the __malloc_initialize_hook variable to libc.so. LDFLAGS-tst-mallocstate = -rdynamic diff --git a/malloc/malloc.c b/malloc/malloc.c index f8e7250f70f..3224b3a899f 100644 --- a/malloc/malloc.c +++ b/malloc/malloc.c @@ -5009,7 +5009,7 @@ __malloc_stats (void) fprintf (stderr, "max mmap regions = %10u\n", (unsigned int) mp_.max_n_mmaps); fprintf (stderr, "max mmap bytes = %10lu\n", (unsigned long) mp_.max_mmapped_mem); - ((_IO_FILE *) stderr)->_flags2 |= old_flags2; + ((_IO_FILE *) stderr)->_flags2 = old_flags2; _IO_funlockfile (stderr); } diff --git a/malloc/tst-malloc-stats-cancellation.c b/malloc/tst-malloc-stats-cancellation.c new file mode 100644 index 00000000000..9a8f475830b --- /dev/null +++ b/malloc/tst-malloc-stats-cancellation.c @@ -0,0 +1,216 @@ +/* Bug 22830: malloc_stats fails to re-enable cancellation on exit. + Copyright (C) 2018 Free Software Foundation. + Copying and distribution of this file, with or without modification, + are permitted in any medium without royalty provided the copyright + notice and this notice are preserved. This file is offered as-is, + without any warranty. */ + +#include +#include +#include + +#include +#include +#include +#include +#include + +#include + +static void * +test_threadproc (void *gatep) +{ + /* When we are released from the barrier, there is a cancellation + request pending for this thread. N.B. pthread_barrier_wait is + not itself a cancellation point (oddly enough). */ + pthread_barrier_wait ((pthread_barrier_t *)gatep); + malloc_stats (); + fputs ("this call should trigger cancellation\n", stderr); + return 0; +} + +/* We cannot replace stderr with a memstream because writes to memstreams + do not trigger cancellation. Instead, main swaps out fd 2 to point to + a pipe, and this thread reads from the pipe and writes to a memstream + until EOF, then returns the data accumulated in the memstream. main + can't do that itself because, when the test thread gets cancelled, + it doesn't close the pipe. */ + +struct buffer_tp_args +{ + int ifd; + FILE *real_stderr; +}; + +static void * +buffer_threadproc (void *argp) +{ + struct buffer_tp_args *args = argp; + int ifd = args->ifd; + char block[BUFSIZ], *p; + ssize_t nread; + size_t nwritten; + + char *obuf = 0; + size_t obufsz = 0; + FILE *ofp = open_memstream (&obuf, &obufsz); + if (!ofp) + { + fprintf (args->real_stderr, + "buffer_threadproc: open_memstream: %s\n", strerror (errno)); + return 0; + } + + while ((nread = read (ifd, block, BUFSIZ)) > 0) + { + p = block; + do + { + nwritten = fwrite_unlocked (p, 1, nread, ofp); + if (nwritten == 0) + { + fprintf (args->real_stderr, + "buffer_threadproc: fwrite_unlocked: %s\n", + strerror (errno)); + return 0; + } + nread -= nwritten; + p += nwritten; + } + while (nread > 0); + } + if (nread == -1) + { + fprintf (args->real_stderr, "buffer_threadproc: read: %s\n", + strerror (errno)); + return 0; + } + close (ifd); + fclose (ofp); + return obuf; +} + + +int +main (void) +{ + int result = 0, err, real_stderr_fd, bufpipe[2]; + pthread_t t_thr, b_thr; + pthread_barrier_t gate; + void *rv; + FILE *real_stderr; + char *obuf; + void *obuf_v; + struct buffer_tp_args b_args; + + real_stderr_fd = dup (2); + if (real_stderr_fd == -1) + { + perror ("dup"); + return 2; + } + real_stderr = fdopen(real_stderr_fd, "w"); + if (!real_stderr) + { + perror ("fdopen"); + return 2; + } + if (setvbuf (real_stderr, 0, _IOLBF, 0)) + { + perror ("setvbuf(real_stderr)"); + return 2; + } + + if (pipe (bufpipe)) + { + perror ("pipe"); + return 2; + } + + /* Below this point, nobody other than the test_threadproc should use + the normal stderr. */ + if (dup2 (bufpipe[1], 2) == -1) + { + fprintf (real_stderr, "dup2: %s\n", strerror (errno)); + return 2; + } + close (bufpipe[1]); + + b_args.ifd = bufpipe[0]; + b_args.real_stderr = real_stderr; + err = pthread_create (&b_thr, 0, buffer_threadproc, &b_args); + if (err) + { + fprintf (real_stderr, "pthread_create(buffer_thr): %s\n", + strerror (err)); + return 2; + } + + err = pthread_barrier_init (&gate, 0, 2); + if (err) + { + fprintf (real_stderr, "pthread_barrier_init: %s\n", strerror (err)); + return 2; + } + + err = pthread_create (&t_thr, 0, test_threadproc, &gate); + if (err) + { + fprintf (real_stderr, "pthread_create(test_thr): %s\n", strerror (err)); + return 2; + } + + err = pthread_cancel (t_thr); + if (err) + { + fprintf (real_stderr, "pthread_cancel: %s\n", strerror (err)); + return 2; + } + + pthread_barrier_wait (&gate); /* cannot fail */ + + err = pthread_join (t_thr, &rv); + if (err) + { + fprintf (real_stderr, "pthread_join(test_thr): %s\n", strerror (err)); + return 2; + } + + /* Closing the normal stderr releases the buffer_threadproc from its + loop. */ + fclose (stderr); + err = pthread_join (b_thr, &obuf_v); + if (err) + { + fprintf (real_stderr, "pthread_join(buffer_thr): %s\n", strerror (err)); + return 2; + } + obuf = obuf_v; + if (obuf == 0) + return 2; /* error within buffer_threadproc, already reported */ + + if (rv != PTHREAD_CANCELED) + { + fputs ("FAIL: thread was not cancelled\n", real_stderr); + result = 1; + } + /* obuf should have received all of the text printed by malloc_stats, + but not the text printed by the final call to fputs. */ + if (!strstr (obuf, "max mmap bytes")) + { + fputs ("FAIL: malloc_stats output incomplete\n", real_stderr); + result = 1; + } + if (strstr (obuf, "this call should trigger cancellation")) + { + fputs ("FAIL: fputs produced output\n", real_stderr); + result = 1; + } + + if (result == 1) + { + fputs ("--- output from thread below ---\n", real_stderr); + fputs (obuf, real_stderr); + } + return result; +}