Missing Npth test

[NIIBE Yutaka]

Hello,

Thank you for your bug report.  I also see your bug report 2260.

On 02/11/2016 10:19 PM, Uldis Anšmits wrote:
> I believe, npth project is missing test for resource locking after for.
> Please see patch at the end of message.
>
> gnupg-2.1.11 tests fail on AIX. gpg-agent fails on semaphore operations
> during startup.
>
> Following important npth library functions are executed by gpg-agent on
> startup:
> 1. npth_init
> 2. npth_unportect
> 3. npth_protect
> 4. fork
> 5. npth_pselect <- here things go wrong

I understand the issue.  Here, the unnamed semaphore created by a
parent with sem_init(pshared=0) is used by its children.  POSIX-wise,
this usage of semaphore is undefined.  In GNU/Linux (and possibly
GNU/Hurd), it just works, but I think that we rely on undefined
behaviour.

> For reason unknown to me, on last machine child receives SIGHUP soon
> after fork.
> Workaround for this is to set signal handler right after fork but this
> is nothing to do with Npth.
>
> npth test output:
> PASS: t-mutex
> PASS: t-thread
> Assertion failed: __EX, file  ../../npth-1.2/src/npth.c, line 123
> FAIL: t-fork
>
> If i change sem_init(,pshared=1,) in npth_init, semaphore operations works.

I think that we only create the unnamed semaphore with pshared=0 for
know system (I'd say: a system with fork-safe semaphore).  POSIX-wise,
it is completely valid not to allow access to the semaphore object
after fork (even SIGHUP after fork would be valid because of the
existence of such a shared object between parent and child).

For other systems, it sounds better to create the unnamed semaphore with
pshared=1.

I don't think sem_open is good when we have another option, because
such a semaphore can be accessed by other processes (while unnamed
semaphre can only share among a process and its descendants).


> Please consider following patch for testing Npth behaviour after fork.
>
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 0dd436e..4b39ff6 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -28,7 +28,7 @@
>
>   ## Process this file with automake to produce Makefile.in
>
> -TESTS = t-mutex t-thread
> +TESTS = t-mutex t-thread t-fork

Thanks for your suggestion.  I think that because fork is only
available on Unix systems (not on Windows), it is better to
conditionalize it.

Inspired by your work, here is my attempt to improve current
situation.

I don't change the behavior of GNU/Linux and GNU/Hurd.  It keeps
current behavior of using pshared=0.

I call it HAVE_FORK_SAFE_SEMAPHORE.  I don't think AIX has "broken"
semaphore implementation, it is completely valid.

I'd avoid runtime test in configure (my concern is cross compiling).
I believe it is determined by host operating system.

How do you think?

diff --git a/configure.ac b/configure.ac
index 84d34ff..e5c5049 100644
--- a/configure.ac
+++ b/configure.ac
@@ -118,9 +118,11 @@ have_ld_version_script=no
  case "${host}" in
      *-*-linux*)
  	have_ld_version_script=yes
+	have_fork_safe_semaphore=yes
          ;;
      *-*-gnu*)
  	have_ld_version_script=yes
+	have_fork_safe_semaphore=yes
          ;;
      *-apple-darwin*)
          AC_DEFINE(_XOPEN_SOURCE, 500, Activate POSIX interface on MacOS X)
@@ -128,6 +130,9 @@ case "${host}" in
  esac

  AM_CONDITIONAL(HAVE_LD_VERSION_SCRIPT, test "$have_ld_version_script" 
= "yes")
+if test "$have_fork_safe_semaphore" = yes; then
+   AC_DEFINE(HAVE_FORK_SAFE_SEMAPHORE, 1, [Defined if we have fork-safe 
semaphore])
+fi

  # Set some default values
  config_libs="-lnpth"
diff --git a/src/npth.c b/src/npth.c
index 7de6c9d..37a60d6 100644
--- a/src/npth.c
+++ b/src/npth.c
@@ -61,6 +61,31 @@
  static sem_t sceptre_buffer;
  static sem_t *sceptre = &sceptre_buffer;

+/* Configure defines HAVE_FORK_SAFE_SEMAPHORE if child process can
+   access non-shared unnamed semaphore.  It doesn't define it if not.
+
+   We use unnamed semaphore (if available) for the global lock.  The
+   specific semaphore is only valid for those threads in a process,
+   and it is no use by other processes.  Thus, PSHARED argument for
+   sem_init is naturally 0.
+
+   However, there are daemon-like applications which use fork after
+   npth_init.  In this case, a child process uses the semaphore which
+   was created by its parent process.  In some system (e.g. AIX),
+   access by child process to non-shared unnamed semaphore is
+   prohibited.  For such a system, HAVE_FORK_SAFE_SEMAPHORE should be
+   undefined, so that unnamed semaphore will be created with PSHARED =
+   1.  The purpose of the setting of PSHARED = 1 is only for accessing
+   of the lock by child process.  For NPTH, it does not mean any other
+   interactions between processes.
+
+ */
+#ifdef HAVE_FORK_SAFE_SEMAPHORE
+#define NPTH_SEMAPHORE_PSHARED 0
+#else
+#define NPTH_SEMAPHORE_PSHARED 1
+#endif
+
  /* The main thread is the active thread at the time pth_init was
     called.  As of now it is only useful for debugging.  The volatile
     make sure the compiler does not eliminate this set but not used
@@ -181,8 +206,8 @@ npth_init (void)
       sem_init.  */
    errno = 0;

-  /* The semaphore is not shared and binary.  */
-  res = sem_init (sceptre, 0, 1);
+  /* The semaphore is binary.  */
+  res = sem_init (sceptre, NPTH_SEMAPHORE_PSHARED, 1);
    if (res < 0)
      {
        /* Mac OSX and some AIX versions have sem_init but return
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 0dd436e..2a0542b 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -40,6 +40,7 @@ else
  AM_CPPFLAGS = -I../src -D_POSIX_C_SOURCE=200112L
  AM_LDFLAGS =
  LDADD = ../src/libnpth.la $(LIBSOCKET) $(LIB_CLOCK_GETTIME)
+TESTS += t-fork
  endif

  noinst_HEADERS = t-support.h
diff --git a/tests/t-fork.c b/tests/t-fork.c
new file mode 100644
index 0000000..c6331dc
--- /dev/null
+++ b/tests/t-fork.c
@@ -0,0 +1,49 @@
+/* t-fork.c
+ * Copyright 2016 g10 Code GmbH
+ *
+ * This file is free software; as a special exception the author gives
+ * unlimited permission to copy and/or distribute it, with or without
+ * modifications, as long as this notice is preserved.
+ *
+ * This file is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY, to the extent permitted by law; without even the
+ * implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
+ */
+
+#include <sys/types.h>
+#include <sys/wait.h>
+#include <unistd.h>
+#include "t-support.h"
+
+
+int
+main (int argc, const char *argv[])
+{
+  int rc;
+  pid_t pid;
+
+  if (argc >= 2 && !strcmp (argv[1], "--verbose"))
+    opt_verbose = 1;
+
+  rc = npth_init ();
+  fail_if_err (rc);
+
+  pid = fork ();
+  if (pid == (pid_t)-1)
+    fail_msg ("fork failed");
+  else if (pid)
+   {
+     int status;
+
+     info_msg ("forked");
+     wait (&status);
+     fail_if_err (status);
+   }
+  else
+    {
+      info_msg ("child exit");
+      npth_usleep (1000);     /* Let NPTH enter, sleep, and leave.  */
+    }
+
+  return 0;
+}
--