diff mbox

[v3] Add cancellation regression test for getpwuid_r.

Message ID 4c7c6c97-3bf5-2c92-5e2a-70443039b3e3@redhat.com
State New
Headers show

Commit Message

Carlos O'Donell Dec. 9, 2016, 12:13 a.m. UTC
On 12/08/2016 02:02 PM, Mike Frysinger wrote:
> On 08 Dec 2016 12:15, Carlos O'Donell wrote:
>> +static void *
>> +worker( void *arg )
> 
> seems like this test case is mingling code from diff sources since it has
> diff styles.  there's a number of non-GNU style errors in it -- this is
> just the first one i saw.

I started with an uncontributed internal Red Hat reproducer, but all that
was left by the time I was done was a few lines, which you're right, have
all the wrong style.

>> +{
>> +  int ret;
>> +  unsigned int iter = 0;
>> +  struct passwd pwbuf, *pw;
>> +  uid_t uid;
>> +  uid = geteuid();
>> +  /* Use a reasonable sized buffer.  Note that _SC_GETPW_R_SIZE_MAX is
>> +     just a hint and not any kind of maximum value.  */
>> +  wbufsz = sysconf(_SC_GETPW_R_SIZE_MAX);
>> +  if (wbufsz == -1)
>> +    wbufsz = 1024;
>> +  wbuf = xmalloc (wbufsz);
>> +  pthread_cleanup_push (worker_free, wbuf);
>> +  sem_post (&started);
>> +  while (1)
> 
> this func could do with a few newlines sprinkled around.  it's pretty
> dense to read as-is.  actually the whole testcase is written this way :).
> 
>> +  printf ("INFO: Previoulsy we would never get here\n");
> 
> "Previously"
> 
>> +  printf ("PASS: Cancelled getpwuid_r successfully"
> 
> "Canceled"
> -mike
> 

v3
- Fix two spelling mistakes.
- Fix GNU Coding Style violations.
- Add more whitespace to make the test more readable.

index 1f016d9..9132e17 100644
---

Comments

Florian Weimer Dec. 9, 2016, 10:09 a.m. UTC | #1
On 12/09/2016 01:13 AM, Carlos O'Donell wrote:
> +	{
> +	  pthread_setcancelstate (PTHREAD_CANCEL_DISABLE, NULL);
> +	  /* Increase the buffer size.  */
> +	  free (wbuf);
> +	  wbufsz = wbufsz * 2;
> +	  wbuf = xmalloc (wbufsz);
> +	  pthread_setcancelstate (PTHREAD_CANCEL_ENABLE, NULL);
> +	}

Is it really necessary to disable cancellation around free and (x)malloc?

Our implementations really shouldn't be cancellation points, and for 
free, this is rather critical.

Thanks,
Florian
Carlos O'Donell Dec. 9, 2016, 7:39 p.m. UTC | #2
On 12/08/2016 07:13 PM, Carlos O'Donell wrote:
> On 12/08/2016 02:02 PM, Mike Frysinger wrote:
>> On 08 Dec 2016 12:15, Carlos O'Donell wrote:
>>> +static void *
>>> +worker( void *arg )
>>
>> seems like this test case is mingling code from diff sources since it has
>> diff styles.  there's a number of non-GNU style errors in it -- this is
>> just the first one i saw.
> 
> I started with an uncontributed internal Red Hat reproducer, but all that
> was left by the time I was done was a few lines, which you're right, have
> all the wrong style.
> 
>>> +{
>>> +  int ret;
>>> +  unsigned int iter = 0;
>>> +  struct passwd pwbuf, *pw;
>>> +  uid_t uid;
>>> +  uid = geteuid();
>>> +  /* Use a reasonable sized buffer.  Note that _SC_GETPW_R_SIZE_MAX is
>>> +     just a hint and not any kind of maximum value.  */
>>> +  wbufsz = sysconf(_SC_GETPW_R_SIZE_MAX);
>>> +  if (wbufsz == -1)
>>> +    wbufsz = 1024;
>>> +  wbuf = xmalloc (wbufsz);
>>> +  pthread_cleanup_push (worker_free, wbuf);
>>> +  sem_post (&started);
>>> +  while (1)
>>
>> this func could do with a few newlines sprinkled around.  it's pretty
>> dense to read as-is.  actually the whole testcase is written this way :).
>>
>>> +  printf ("INFO: Previoulsy we would never get here\n");
>>
>> "Previously"
>>
>>> +  printf ("PASS: Cancelled getpwuid_r successfully"
>>
>> "Canceled"
>> -mike
>>
> 
> v3
> - Fix two spelling mistakes.
> - Fix GNU Coding Style violations.
> - Add more whitespace to make the test more readable.

I believe v3 addresses Florian and Mike's review.

I'll commit this next week if nobody objects.
diff mbox

Patch

--- a/nss/Makefile
+++ b/nss/Makefile
@@ -54,6 +54,12 @@  tests			= test-netdb tst-nss-test1 test-digits-dots \
 			  $(tests-static)
 xtests			= bug-erange
 
+# If we have a thread library then we can test cancellation against
+# some routines like getpwuid_r.
+ifeq (yes,$(have-thread-library))
+tests += tst-cancel-getpwuid_r
+endif
+
 # Specify rules for the nss_* modules.  We have some services.
 services		:= files db
 
@@ -125,3 +131,7 @@  $(objpfx)/libnss_test1.so$(libnss_test1.so-version): $(objpfx)/libnss_test1.so
 	$(make-link)
 endif
 $(objpfx)tst-nss-test1.out: $(objpfx)/libnss_test1.so$(libnss_test1.so-version)
+
+ifeq (yes,$(have-thread-library))
+$(objpfx)tst-cancel-getpwuid_r: $(shared-thread-library)
+endif
diff --git a/nss/tst-cancel-getpwuid_r.c b/nss/tst-cancel-getpwuid_r.c
new file mode 100644
index 0000000..cc807a9
--- /dev/null
+++ b/nss/tst-cancel-getpwuid_r.c
@@ -0,0 +1,184 @@ 
+/* Test cancellation of getpwuid_r.
+   Copyright (C) 2016 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
+   <http://www.gnu.org/licenses/>.  */
+
+/* Test if cancellation of getpwuid_r incorrectly leaves internal
+   function state locked resulting in hang of subsequent calls to
+   getpwuid_r.  The main thread creates a second thread which will do
+   the calls to getpwuid_r.  A semaphore is used by the second thread to
+   signal to the main thread that it is as close as it can be to the
+   call site of getpwuid_r.  The goal of the semaphore is to avoid any
+   cancellable function calls between the sem_post and the call to
+   getpwuid_r.  The main thread then attempts to cancel the second
+   thread.  Without the fixes the cancellation happens at any number of
+   calls to cancellable functions in getpuid_r, but with the fix the
+   cancellation either does not happen or happens only at expected
+   points where the internal state is consistent.  We use an explicit
+   pthread_testcancel call to terminate the loop in a timely fashion
+   if the implementation does not have a cancellation point.  */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <pthread.h>
+#include <pwd.h>
+#include <sys/types.h>
+#include <unistd.h>
+#include <semaphore.h>
+#include <errno.h>
+
+static int do_test (void);
+#define TEST_FUNCTION do_test ()
+
+#include "../test-skeleton.c"
+
+sem_t started;
+char *wbuf;
+long wbufsz;
+
+void
+worker_free (void *arg)
+{
+  free (arg);
+}
+
+static void *
+worker (void *arg)
+{
+  int ret;
+  unsigned int iter = 0;
+  struct passwd pwbuf, *pw;
+  uid_t uid;
+
+  uid = geteuid ();
+
+  /* Use a reasonable sized buffer.  Note that _SC_GETPW_R_SIZE_MAX is
+     just a hint and not any kind of maximum value.  */
+  wbufsz = sysconf (_SC_GETPW_R_SIZE_MAX);
+  if (wbufsz == -1)
+    wbufsz = 1024;
+  wbuf = xmalloc (wbufsz);
+
+  pthread_cleanup_push (worker_free, wbuf);
+  sem_post (&started);
+  while (1)
+    {
+      iter++;
+
+      ret = getpwuid_r (uid, &pwbuf, wbuf, wbufsz, &pw);
+
+      /* The call to getpwuid_r may not cancel so we need to test
+	 for cancellation after some number of iterations of the
+	 function.  Choose an arbitrary 100,000 iterations of running
+	 getpwuid_r in a tight cancellation loop before testing for
+	 cancellation.  */
+      if (iter > 100000)
+	pthread_testcancel ();
+
+      if (ret == ERANGE)
+	{
+	  pthread_setcancelstate (PTHREAD_CANCEL_DISABLE, NULL);
+	  /* Increase the buffer size.  */
+	  free (wbuf);
+	  wbufsz = wbufsz * 2;
+	  wbuf = xmalloc (wbufsz);
+	  pthread_setcancelstate (PTHREAD_CANCEL_ENABLE, NULL);
+	}
+
+    }
+  pthread_cleanup_pop (1);
+
+  return NULL;
+}
+
+static int
+do_test (void)
+{
+  int ret;
+  char *buf;
+  long bufsz;
+  void *retval;
+  struct passwd pwbuf, *pw;
+  pthread_t thread;
+
+  /* Configure the test to only use files. We control the files plugin
+     as part of glibc so we assert that it should be deferred
+     cancellation safe.  */
+  __nss_configure_lookup ("passwd", "files");
+
+  /* Use a reasonable sized buffer.  Note that  _SC_GETPW_R_SIZE_MAX is
+     just a hint and not any kind of maximum value.  */
+  bufsz = sysconf(_SC_GETPW_R_SIZE_MAX);
+  if (bufsz == -1)
+    bufsz = 1024;
+  buf = xmalloc (bufsz);
+
+  sem_init (&started, 0, 0);
+
+  pthread_create(&thread, NULL, worker, NULL);
+
+  do
+  {
+    ret = sem_wait (&started);
+    if (ret == -1 && errno != EINTR)
+      {
+        printf ("FAIL: Failed to wait for second thread to start.\n");
+	exit (EXIT_FAILURE);
+      }
+  }
+  while (ret != 0);
+
+  printf( "INFO: Cancelling thread\n" );
+  if ((ret = pthread_cancel(thread)) != 0)
+    {
+      printf ("FAIL: Failed to cancel thread. Returned %d\n", ret);
+      exit (EXIT_FAILURE);
+    }
+
+  printf( "INFO: Joining...\n");
+  pthread_join(thread, &retval);
+  if (retval != PTHREAD_CANCELED)
+    {
+      printf ("FAIL: Thread was not cancelled.\n");
+      exit (EXIT_FAILURE);
+    }
+  printf( "INFO: Joined, trying getpwuid_r call\n" );
+
+  /* Before the fix in 312be3f9f5eab1643d7dcc7728c76d413d4f2640 for this
+     issue the cancellation point could happen in any number of internal
+     calls, and therefore locks would be left held and the following
+     call to getpwuid_r would block and the test would time out.  */
+  do
+    {
+      ret = getpwuid_r (geteuid(), &pwbuf, buf, bufsz, &pw);
+      if (ret == ERANGE)
+	{
+	  /* Increase the buffer size.  */
+	  free (buf);
+	  bufsz = bufsz * 2;
+	  buf = xmalloc (bufsz);
+	}
+    }
+  while (ret == ERANGE);
+
+  free (buf);
+
+  /* Before the fix we would never get here.  */
+  printf ("PASS: Canceled getpwuid_r successfully"
+	  " and called it again without blocking.\n");
+
+  return 0;
+}