From patchwork Mon Sep 7 17:17:33 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Phil Blundell X-Patchwork-Id: 8592 Received: (qmail 65389 invoked by alias); 7 Sep 2015 17:17:43 -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 65308 invoked by uid 89); 7 Sep 2015 17:17:42 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.9 required=5.0 tests=BAYES_00, KAM_LAZY_DOMAIN_SECURITY, T_RP_MATCHES_RCVD autolearn=no version=3.3.2 X-HELO: hetzner.pbcl.net Message-ID: <1441646253.22688.58.camel@pbcl.net> Subject: Re: [PATCH 08/08] nptl: arm: Fix Race conditions in pthread cancellation (BZ#12683) From: Phil Blundell To: Adhemerval Zanella Cc: GNU C Library Date: Mon, 07 Sep 2015 18:17:33 +0100 In-Reply-To: <55E4C300.9080800@linaro.org> References: <55E4C300.9080800@linaro.org> Mime-Version: 1.0 On Mon, 2015-08-31 at 18:11 -0300, Adhemerval Zanella wrote: > This patch adds the ARM modifications required for the BZ#12683 fix. > It basically removes the enable_asynccancel/disable_asynccancel function > usage on code, provide a arch-specific symbol that contains global > markers to be used in SIGCANCEL handler. I looked at this in a bit more detail. Here are some further comments: > .fnstart; /* matched by the .fnend in UNDOARGS below. */ \ > - DOCARGS_##args; /* save syscall args etc. around CENABLE. */ \ > - CENABLE; \ > - mov ip, r0; /* put mask in safe place. */ \ > - UNDOCARGS_##args; /* restore syscall args. */ \ > - ldr r7, =SYS_ify (syscall_name); \ > - swi 0x0; /* do the call. */ \ > - mov r7, r0; /* save syscall return value. */ \ > - mov r0, ip; /* get mask back. */ \ > - CDISABLE; \ > - mov r0, r7; /* retrieve return value. */ \ > - RESTORE_LR_##args; \ After this change, DOCARGS(), UNDOCARGS() and RESTORE_LR() are all dead code. Please delete them. > - UNDOARGS_##args; \ > + push {r4, r5, lr}; \ > + .save {r4, r5, lr}; \ > + PSEUDO_CANCEL_BEFORE; \ > + movw r0, SYS_ify (syscall_name); \ This fails when compiling for non-thumb2. Please use "ldr r0, =..." instead. > + PSEUDO_CANCEL_AFTER; \ > + pop {r4, r5, pc}; \ Popping {pc} here causes an immediate return, which means that the errno handling code which follows is never executed. Somewhat embarrassingly it seems that there is no existing glibc test which catches this failure. I've attached a proof of concept which demonstrates it, but I rather wonder whether we should extend the test harness so that some/most of the existing glibc tests are run both single-threaded (as now) and with an additional dummy thread created at startup in order to force this code down the multi-threaded path. Also note that the two testcases in the attached patch give slightly different results and I think they would continue to do so (in a different way) if the bug above was fixed. It's not entirely clear to me that this part of __syscall_cancel() from your other patch is correct: + /* If cancellation is not enabled, call the syscall directly. */ + if (pd->cancelhandling & CANCELSTATE_BITMASK) + { + INTERNAL_SYSCALL_DECL (err); + result = INTERNAL_SYSCALL_NCS (nr, err, 6, a1, a2, a3, a4, a5, a6); + return INTERNAL_SYSCALL_ERROR_P (result, err) ? -result : result; + } p. From f1bf4248ba69931442f045a9b6b71c0c3dfcced4 Mon Sep 17 00:00:00 2001 From: Phil Blundell Date: Mon, 7 Sep 2015 17:01:27 +0000 Subject: [PATCH] nptl/tst-cancel27.c, nptl/tst-cancel28.c: New tests --- nptl/Makefile | 2 +- nptl/tst-cancel27.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++ nptl/tst-cancel28.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 141 insertions(+), 1 deletion(-) create mode 100644 nptl/tst-cancel27.c create mode 100644 nptl/tst-cancel28.c diff --git a/nptl/Makefile b/nptl/Makefile index 4bc6608..05a4872 100644 --- a/nptl/Makefile +++ b/nptl/Makefile @@ -255,7 +255,7 @@ tests = tst-typesizes \ tst-cancel11 tst-cancel12 tst-cancel13 tst-cancel14 tst-cancel15 \ tst-cancel16 tst-cancel17 tst-cancel18 tst-cancel19 tst-cancel20 \ tst-cancel21 tst-cancel22 tst-cancel23 tst-cancel24 tst-cancel25 \ - tst-cancel26 \ + tst-cancel26 tst-cancel27 tst-cancel28 \ tst-cancel-self tst-cancel-self-cancelstate \ tst-cancel-self-canceltype tst-cancel-self-testcancel \ tst-cleanup0 tst-cleanup1 tst-cleanup2 tst-cleanup3 tst-cleanup4 \ diff --git a/nptl/tst-cancel27.c b/nptl/tst-cancel27.c new file mode 100644 index 0000000..2d71a7f --- /dev/null +++ b/nptl/tst-cancel27.c @@ -0,0 +1,69 @@ +/* Copyright (C) 2015 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 + +static void * +tf (void *arg) +{ + int e, r; + + errno = 0; + + /* This is a cancellation point, but we should not be cancelled. */ + r = write (-1, 0, 0); + e = errno; + + /* Check that the cancelling syscall wrapper has handled the error correctly. */ + if (r != -1 || errno != EBADF) + { + printf ("write returned %d, errno %d\n", r, e); + exit (1); + } + + return NULL; +} + +static int +do_test (void) +{ + pthread_t th; + + if (pthread_create (&th, NULL, tf, NULL) != 0) + { + puts ("create failed"); + exit (1); + } + + void *r; + if (pthread_join (th, &r) != 0) + { + puts ("join failed"); + exit (1); + } + + return 0; +} + + +#define TEST_FUNCTION do_test () +#include "../test-skeleton.c" diff --git a/nptl/tst-cancel28.c b/nptl/tst-cancel28.c new file mode 100644 index 0000000..4dccae2 --- /dev/null +++ b/nptl/tst-cancel28.c @@ -0,0 +1,71 @@ +/* Copyright (C) 2015 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 + +static void * +tf (void *arg) +{ + int e, r; + + errno = 0; + + pthread_setcancelstate (PTHREAD_CANCEL_DISABLE, NULL); + + /* This is a cancellation point, but we should not be cancelled. */ + r = write (-1, 0, 0); + e = errno; + + /* Check that the cancelling syscall wrapper has handled the error correctly. */ + if (r != -1 || errno != EBADF) + { + printf ("write returned %d, errno %d\n", r, e); + exit (1); + } + + return NULL; +} + +static int +do_test (void) +{ + pthread_t th; + + if (pthread_create (&th, NULL, tf, NULL) != 0) + { + puts ("create failed"); + exit (1); + } + + void *r; + if (pthread_join (th, &r) != 0) + { + puts ("join failed"); + exit (1); + } + + return 0; +} + + +#define TEST_FUNCTION do_test () +#include "../test-skeleton.c" -- 2.1.4