[roland/sem_post] BZ#18434: Fix sem_post EOVERFLOW check for [!__HAVE_64B_ATOMICS].

Message ID 20150519221106.7114B2C3A73@topped-with-meat.com
State Committed
Headers

Commit Message

Roland McGrath May 19, 2015, 10:11 p.m. UTC
  This bug is a regression introduced by the 2.21 semaphore rewrite.
It affects all machines without __HAVE_64B_ATOMICS.

Tested i686-linux-gnu and arm-nacl, where the new test case fails before
the fix and passes after it.  I didn't test the sparc32 code, but the bug
and the fix are the same as in the generic code.

This seems appropriate for a 2.21 backport, and I've marked the bug that
way (I've just added the glibc_2.21 keyword since we didn't have one yet).


Thanks,
Roland


2015-05-19  Roland McGrath  <roland@hack.frob.com>

	[BZ #18434]
	* nptl/tst-sem15.c: New file.
	* nptl/Makefile (tests): Add it.
	* nptl/sem_post.c (__new_sem_post) [!__HAVE_64B_ATOMICS]:
	s/<</>>/ to fix typo in EOVERFLOW check.
	* sysdeps/sparc/sparc32/sem_post.c (__new_sem_post): Likewise.
  

Comments

Torvald Riegel May 19, 2015, 10:24 p.m. UTC | #1
On Tue, 2015-05-19 at 15:11 -0700, Roland McGrath wrote:
> This bug is a regression introduced by the 2.21 semaphore rewrite.
> It affects all machines without __HAVE_64B_ATOMICS.
> 
> Tested i686-linux-gnu and arm-nacl, where the new test case fails before
> the fix and passes after it.  I didn't test the sparc32 code, but the bug
> and the fix are the same as in the generic code.
> 
> This seems appropriate for a 2.21 backport, and I've marked the bug that
> way (I've just added the glibc_2.21 keyword since we didn't have one yet).

Ouch :(  Thanks for fixing this!
  
Roland McGrath May 19, 2015, 10:31 p.m. UTC | #2
> Ouch :(  Thanks for fixing this!

I'll take that as approval and commit it to trunk now.
I'll leave it to Carlos to cherry-pick for 2.21.


Thanks,
Roland
  
Joseph Myers May 19, 2015, 11:06 p.m. UTC | #3
On Tue, 19 May 2015, Roland McGrath wrote:

> > Ouch :(  Thanks for fixing this!
> 
> I'll take that as approval and commit it to trunk now.

NEWS update to the list of fixed bugs needed.
  
Roland McGrath May 19, 2015, 11:52 p.m. UTC | #4
> NEWS update to the list of fixed bugs needed.

Oops.  I also forgot to close the bug.  Both done now (no ChangeLog entry
for NEWS, which I think is protocol).

Thanks,
Roland
  

Patch

diff --git a/nptl/Makefile b/nptl/Makefile
index d784c8d..fe1ba05 100644
--- a/nptl/Makefile
+++ b/nptl/Makefile
@@ -228,6 +228,7 @@  tests = tst-typesizes \
 	tst-key1 tst-key2 tst-key3 tst-key4 \
 	tst-sem1 tst-sem2 tst-sem3 tst-sem4 tst-sem5 tst-sem6 tst-sem7 \
 	tst-sem8 tst-sem9 tst-sem10 tst-sem11 tst-sem12 tst-sem13 tst-sem14 \
+	tst-sem15 \
 	tst-barrier1 tst-barrier2 tst-barrier3 tst-barrier4 \
 	tst-align tst-align3 \
 	tst-basic1 tst-basic2 tst-basic3 tst-basic4 tst-basic5 tst-basic6 \
diff --git a/nptl/sem_post.c b/nptl/sem_post.c
index 6e495ed..b6d30b5 100644
--- a/nptl/sem_post.c
+++ b/nptl/sem_post.c
@@ -84,14 +84,14 @@  __new_sem_post (sem_t *sem)
   unsigned int v = atomic_load_relaxed (&isem->value);
   do
     {
-      if ((v << SEM_VALUE_SHIFT) == SEM_VALUE_MAX)
+      if ((v >> SEM_VALUE_SHIFT) == SEM_VALUE_MAX)
 	{
 	  __set_errno (EOVERFLOW);
 	  return -1;
 	}
     }
-  while (!atomic_compare_exchange_weak_release (&isem->value,
-      &v, v + (1 << SEM_VALUE_SHIFT)));
+  while (!atomic_compare_exchange_weak_release
+	 (&isem->value, &v, v + (1 << SEM_VALUE_SHIFT)));
 
   /* If there is any potentially blocked waiter, wake one of them.  */
   if ((v & SEM_NWAITERS_MASK) != 0)
diff --git a/nptl/tst-sem15.c b/nptl/tst-sem15.c
new file mode 100644
index 0000000..3df9561
--- /dev/null
+++ b/nptl/tst-sem15.c
@@ -0,0 +1,99 @@ 
+/* Test for SEM_VALUE_MAX overflow detection: BZ #18434.
+   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
+   <http://www.gnu.org/licenses/>.  */
+
+#include <errno.h>
+#include <limits.h>
+#include <semaphore.h>
+#include <stdio.h>
+#include <string.h>
+#include <unistd.h>
+
+
+static int
+do_test (void)
+{
+  sem_t s;
+
+  if (sem_init (&s, 0, SEM_VALUE_MAX))
+    {
+      printf ("sem_init: %m\n");
+      return 1;
+    }
+
+  int result = 0;
+
+  int value = 0xdeadbeef;
+  if (sem_getvalue (&s, &value))
+    {
+      printf ("sem_getvalue: %m\n");
+      result = 1;
+    }
+  else
+    {
+      printf ("sem_getvalue after init: %d\n", value);
+      if (value != SEM_VALUE_MAX)
+	{
+	  printf ("\tshould be %d\n", SEM_VALUE_MAX);
+	  result = 1;
+	}
+    }
+
+  errno = 0;
+  if (sem_post(&s) == 0)
+    {
+      puts ("sem_post at SEM_VALUE_MAX succeeded!");
+      result = 1;
+    }
+  else
+    {
+      printf ("sem_post at SEM_VALUE_MAX: %m (%d)\n", errno);
+      if (errno != EOVERFLOW)
+	{
+	  printf ("\tshould be %s (EOVERFLOW = %d)\n",
+		  strerror (EOVERFLOW), EOVERFLOW);
+	  result = 1;
+	}
+    }
+
+  value = 0xbad1d00d;
+  if (sem_getvalue (&s, &value))
+    {
+      printf ("sem_getvalue: %m\n");
+      result = 1;
+    }
+  else
+    {
+      printf ("sem_getvalue after post: %d\n", value);
+      if (value != SEM_VALUE_MAX)
+	{
+	  printf ("\tshould be %d\n", SEM_VALUE_MAX);
+	  result = 1;
+	}
+    }
+
+  if (sem_destroy (&s))
+    {
+      printf ("sem_destroy: %m\n");
+      result = 1;
+    }
+
+  return result;
+}
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"
diff --git a/sysdeps/sparc/sparc32/sem_post.c b/sysdeps/sparc/sparc32/sem_post.c
index 64cd851..c9f85a0 100644
--- a/sysdeps/sparc/sparc32/sem_post.c
+++ b/sysdeps/sparc/sparc32/sem_post.c
@@ -60,19 +60,19 @@  __new_sem_post (sem_t *sem)
   int private = isem->private;
   unsigned int v;
 
-  __sparc32_atomic_do_lock24(&isem->pad);
+  __sparc32_atomic_do_lock24 (&isem->pad);
 
   v = isem->value;
-  if ((v << SEM_VALUE_SHIFT) == SEM_VALUE_MAX)
+  if ((v >> SEM_VALUE_SHIFT) == SEM_VALUE_MAX)
     {
-      __sparc32_atomic_do_unlock24(&isem->pad);
+      __sparc32_atomic_do_unlock24 (&isem->pad);
 
       __set_errno (EOVERFLOW);
       return -1;
     }
   isem->value = v + (1 << SEM_VALUE_SHIFT);
 
-  __sparc32_atomic_do_unlock24(&isem->pad);
+  __sparc32_atomic_do_unlock24 (&isem->pad);
 
   if ((v & SEM_NWAITERS_MASK) != 0)
     futex_wake (&isem->value, 1, private);