nptl: Fix tst-cancel7 and tst-cancelx7 pidfile race

Message ID 20211016224154.2991161-1-shorne@gmail.com
State Committed
Commit 06acd6d1d6f485f2751dcfec881044938742bc8e
Delegated to: Adhemerval Zanella Netto
Headers
Series nptl: Fix tst-cancel7 and tst-cancelx7 pidfile race |

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent
dj/TryBot-32bit success Build for i686

Commit Message

Stafford Horne Oct. 16, 2021, 10:41 p.m. UTC
  The check for waiting for the pidfile to be created looks wrong.  At the
point when ACCESS is run the pid file will always be created and
accessible as it is created during DO_PREPARE.  This means that thread
cancellation may be performed before the pid is written to the pidfile.

This was found to be flaky when testing on my OpenRISC platform.

Fix this by using the semaphore to wait for pidfile pid write
completion.
---
 nptl/tst-cancel7.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)
  

Comments

Adhemerval Zanella Oct. 18, 2021, 4:30 p.m. UTC | #1
On 16/10/2021 19:41, Stafford Horne via Libc-alpha wrote:
> The check for waiting for the pidfile to be created looks wrong.  At the
> point when ACCESS is run the pid file will always be created and
> accessible as it is created during DO_PREPARE.  This means that thread
> cancellation may be performed before the pid is written to the pidfile.
> 
> This was found to be flaky when testing on my OpenRISC platform.
> 
> Fix this by using the semaphore to wait for pidfile pid write
> completion.

LGTM, thanks.

Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>

> ---
>  nptl/tst-cancel7.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/nptl/tst-cancel7.c b/nptl/tst-cancel7.c
> index 54ed2a54ff..e926f4455c 100644
> --- a/nptl/tst-cancel7.c
> +++ b/nptl/tst-cancel7.c
> @@ -115,16 +115,13 @@ do_test (void)
>  {
>    pthread_t th = xpthread_create (NULL, tf, NULL);
>  
> -  do
> -    nanosleep (&(struct timespec) { .tv_sec = 0, .tv_nsec = 100000000 }, NULL);
> -  while (access (pidfilename, R_OK) != 0);
> +  /* Wait to cancel until after the pid is written.  */
> +  if (sem_wait (sem) != 0)
> +    FAIL_EXIT1 ("sem_wait: %m");
>  
>    xpthread_cancel (th);
>    void *r = xpthread_join (th);
>  
> -  if (sem_wait (sem) != 0)
> -    FAIL_EXIT1 ("sem_wait: %m");
> -
>    FILE *f = xfopen (pidfilename, "r+");
>  
>    long long ll;
>
  

Patch

diff --git a/nptl/tst-cancel7.c b/nptl/tst-cancel7.c
index 54ed2a54ff..e926f4455c 100644
--- a/nptl/tst-cancel7.c
+++ b/nptl/tst-cancel7.c
@@ -115,16 +115,13 @@  do_test (void)
 {
   pthread_t th = xpthread_create (NULL, tf, NULL);
 
-  do
-    nanosleep (&(struct timespec) { .tv_sec = 0, .tv_nsec = 100000000 }, NULL);
-  while (access (pidfilename, R_OK) != 0);
+  /* Wait to cancel until after the pid is written.  */
+  if (sem_wait (sem) != 0)
+    FAIL_EXIT1 ("sem_wait: %m");
 
   xpthread_cancel (th);
   void *r = xpthread_join (th);
 
-  if (sem_wait (sem) != 0)
-    FAIL_EXIT1 ("sem_wait: %m");
-
   FILE *f = xfopen (pidfilename, "r+");
 
   long long ll;