diff mbox

BZ# 18125: setcontext: Call exit, not _exit, after last linked context executes.

Message ID 55035374.6060906@redhat.com
State Superseded
Headers show

Commit Message

Carlos O'Donell March 13, 2015, 9:15 p.m. UTC
There appears to be a discrepancy among the implementations
of setcontext with regards to the function called once the last
linked-to context has finished executing via setcontext.

The POSIX standard says:
~~~
If the uc_link member of the ucontext_t structure pointed to by 
the ucp argument is equal to 0, then this context is the main 
context, and the thread will exit when this context returns. 
~~~

It says "exit" not "exit immediately" nor "exit without running
functions registered with atexit or on_exit."

Therefore the AArch64, ARM, hppa and NIOS II implementations are
wrong and no test detects it.

It is questionable if this should even be fixed or just documented
that the above 4 targets are wrong. The functions are deprecated
and nobody should be using them, but at the same time it silly to
have cross-target differences that make it hard to port old applications
from say x86_64 to AArch64.

Therefore I'm inclined to fix the 4 arches, and checkin a regression
test to prevent it from changing again.

Tested on x86_64 and hppa.

OK to commit?

2015-03-13  Carlos O'Donell  <carlos@redhat.com>

	[BZ #18125]
	* stdlib/tst-setcontext2.c: New file.
	* stdlib/tst-setcontext2.sh: New file.
	* Makefile (tests): Add tst-setcontext2.
	(tst-setcontext2.out): Custom rule to run tst-setcontext2.sh
	to verify test program created output file.
	* sysdeps/unix/sysv/linux/aarch64/setcontext.S: Call exit.
	* sysdeps/unix/sysv/linux/arm/setcontext.S: Likewise.
	* sysdeps/unix/sysv/linux/hppa/setcontext.S: Likewise.
	* sysdeps/unix/sysv/linux/nios2/setcontext.S: Likewise.

---

Cheers,
Carlos.

Comments

Mike Frysinger March 13, 2015, 11:31 p.m. UTC | #1
On 13 Mar 2015 17:15, Carlos O'Donell wrote:

just nits ... actual changes look fine

> +#ifndef PATH_MAX
> +# define PATH_MAX 4096
> +#endif
> +char filename[PATH_MAX];

considering you just strcpy from argv[1], why not do one of:
	- assign filename to argv[1] directly
	- use strdup
that avoids the static bounds ugliness

> +/* It is intended that this function does nothing.  */
> +static void
> +cf (void)
> +{
> +  printf ("called context function\n");
> +  return;
> +}

return is kind of pointless

> +  char buf[] = "Called exit function\n";

doesn't really matter, but const ?

> +  printf ("PASS: %s", buf);
> +  res = close (fd);
> +  if (res == -1)
> +    {
> +      printf ("FAIL: Failed to close test file.\n");
> +      exit (1);
> +    }

seems weird ot print PASS and then a FAIL ... maybe move the PASS after the 
close ?

> +  return;
> +}

pointless return

> +#define TEST_FUNCTION do_test (argc, argv)

this is the default

> +# We want to run the test program and see if secontext called
> +# exit() and wrote out the test file we specified.  If the
> +# test exits with a non-zero status this will fail because we
> +# are using `set -e`.
> +$test_pre $test $tempfile

quote the paths ?  you did everywhere else :)

> +# Look for resulting file.
> +if [ -e "$tempfile" ]; then
> +  echo "PASS: tst-setcontext2 ran exit() and created $tempfile"
> +  cleanup
> +  exit 0

your trap should kick in here, so the explicit cleanup should not be needed
-mike
Andreas Schwab March 14, 2015, 8:22 a.m. UTC | #2
"Carlos O'Donell" <carlos@redhat.com> writes:

> +  makecontext (&ctx, (void (*) (void)) cf, 0);

Pointless cast.

> +  if ((ret = setcontext (&ctx)) != 0)

No assignment inside condition.

Andreas.
Pedro Alves March 14, 2015, 1:21 p.m. UTC | #3
OOC, and for my own education, why bother with writing a file in
the exit handler, and then checking that in the script on success
exit, instead of just printf("PASS: ...") directly in the test
program's exit handler?

(I wondered that because I noticed that the exit handler
will run in many of the FAIL cases too, though I realize
that those exit with exit code != 0 and thus won't return
in PASS from the script).

Thanks,
Pedro Alves
Rich Felker March 14, 2015, 2:30 p.m. UTC | #4
On Fri, Mar 13, 2015 at 05:15:32PM -0400, Carlos O'Donell wrote:
> There appears to be a discrepancy among the implementations
> of setcontext with regards to the function called once the last
> linked-to context has finished executing via setcontext.
> 
> The POSIX standard says:
> ~~~
> If the uc_link member of the ucontext_t structure pointed to by 
> the ucp argument is equal to 0, then this context is the main 
> context, and the thread will exit when this context returns. 
> ~~~
> 
> It says "exit" not "exit immediately" nor "exit without running
> functions registered with atexit or on_exit."

It says "the thread will exit", so you need to call pthread_exit, not
_exit or exit. The latter cause the process to exit. Note that
pthread_exit in the last thread results in exit being called.

Rich
Pedro Alves March 14, 2015, 2:49 p.m. UTC | #5
On 03/14/2015 01:21 PM, Pedro Alves wrote:
> OOC, and for my own education, why bother with writing a file in
> the exit handler, and then checking that in the script on success
> exit, instead of just printf("PASS: ...") directly in the test
> program's exit handler?

Oh sorry, "D'oh" moment...  If setcontext calls _exit then the
atexit handler won't run, and then the process exits with
exit code 0 and we'd miss issuing FAIL...

> 
> (I wondered that because I noticed that the exit handler
> will run in many of the FAIL cases too, though I realize
> that those exit with exit code != 0 and thus won't return
> in PASS from the script).

Thanks,
Pedro Alves
Carlos O'Donell March 16, 2015, 3:24 p.m. UTC | #6
On 03/14/2015 10:30 AM, Rich Felker wrote:
> On Fri, Mar 13, 2015 at 05:15:32PM -0400, Carlos O'Donell wrote:
>> There appears to be a discrepancy among the implementations
>> of setcontext with regards to the function called once the last
>> linked-to context has finished executing via setcontext.
>>
>> The POSIX standard says:
>> ~~~
>> If the uc_link member of the ucontext_t structure pointed to by 
>> the ucp argument is equal to 0, then this context is the main 
>> context, and the thread will exit when this context returns. 
>> ~~~
>>
>> It says "exit" not "exit immediately" nor "exit without running
>> functions registered with atexit or on_exit."
> 
> It says "the thread will exit", so you need to call pthread_exit, not
> _exit or exit. The latter cause the process to exit. Note that
> pthread_exit in the last thread results in exit being called.

That kind of change is outside the scope of this bug fix.

While in retrospect I agree with you, the interface in question is
deprecated and should not be used with pthreads, instead pthreads
should be used directly.

Thus we won't be 100% POSIX compliant until this gets fixed, but
we'll at least run registered exit functions.

Cheers,
Carlos.
Carlos O'Donell March 16, 2015, 8:27 p.m. UTC | #7
On 03/14/2015 10:49 AM, Pedro Alves wrote:
> On 03/14/2015 01:21 PM, Pedro Alves wrote:
>> OOC, and for my own education, why bother with writing a file in
>> the exit handler, and then checking that in the script on success
>> exit, instead of just printf("PASS: ...") directly in the test
>> program's exit handler?
> 
> Oh sorry, "D'oh" moment...  If setcontext calls _exit then the
> atexit handler won't run, and then the process exits with
> exit code 0 and we'd miss issuing FAIL...

That is correct.

Cheers,
Carlos.
diff mbox

Patch

diff --git a/stdlib/Makefile b/stdlib/Makefile
index 92ceca7..1179400 100644
--- a/stdlib/Makefile
+++ b/stdlib/Makefile
@@ -73,7 +73,8 @@  tests		:= tst-strtol tst-strtod testmb testrand testsort testdiv   \
 		   tst-makecontext2 tst-strtod6 tst-unsetenv1		    \
 		   tst-makecontext3 bug-getcontext bug-fmtmsg1		    \
 		   tst-secure-getenv tst-strtod-overflow tst-strtod-round   \
-		   tst-tininess tst-strtod-underflow tst-tls-atexit
+		   tst-tininess tst-strtod-underflow tst-tls-atexit	    \
+		   tst-setcontext2
 tests-static	:= tst-secure-getenv
 
 modules-names	= tst-tls-atexit-lib
@@ -157,3 +158,9 @@  tst-tls-atexit-lib.so-no-z-defs = yes
 
 $(objpfx)tst-tls-atexit: $(shared-thread-library) $(libdl)
 $(objpfx)tst-tls-atexit.out: $(objpfx)tst-tls-atexit-lib.so
+
+$(objpfx)tst-setcontext2.out: tst-setcontext2.sh $(objpfx)tst-setcontext2
+	$(SHELL) $< $(common-objpfx) '$(test-program-prefix-before-env)' \
+		 '$(run-program-env)' '$(test-program-prefix-after-env)' \
+		 $(common-objpfx)stdlib/; \
+	$(evaluate-test)
diff --git a/stdlib/tst-setcontext2.c b/stdlib/tst-setcontext2.c
new file mode 100644
index 0000000..1ead75d
--- /dev/null
+++ b/stdlib/tst-setcontext2.c
@@ -0,0 +1,137 @@ 
+/* Bug 18125: Verify setcontext calls exit() and not _exit().
+   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 <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <ucontext.h>
+#include <unistd.h>
+#include <limits.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+
+static ucontext_t ctx;
+#ifndef PATH_MAX
+# define PATH_MAX 4096
+#endif
+char filename[PATH_MAX];
+
+/* It is intended that this function does nothing.  */
+static void
+cf (void)
+{
+  printf ("called context function\n");
+  return;
+}
+
+static void
+exit_called (void)
+{
+  int fd;
+  ssize_t res;
+  char buf[] = "Called exit function\n";
+  fd = open (filename, O_WRONLY | O_CREAT, S_IRUSR | S_IWUSR);
+  if (fd == -1)
+    {
+      printf ("FAIL: Unable to create test file %s\n", filename);
+      exit (1);
+    }
+  res = write (fd, buf, sizeof (buf));
+  if (res != sizeof (buf))
+    {
+      printf ("FAIL: Expected to write test file in one write call.\n");
+      exit (1);
+    }
+  printf ("PASS: %s", buf);
+  res = close (fd);
+  if (res == -1)
+    {
+      printf ("FAIL: Failed to close test file.\n");
+      exit (1);
+    }
+  return;
+}
+
+/* The test expects a filename given by the wrapper calling script.
+   The test then registers an atexit handler that will create the
+   file to indicate that the atexit handler ran. Then the test
+   creates a context, modifies it with makecontext, and sets it.
+   The context has only a single context which then must exit.
+   If it incorrectly exits via _exit then the atexit handler is
+   not run, the file is not created, and the wrapper detects this
+   and fails the test.  This test cannot be done using an _exit
+   interposer since setcontext avoids the PLT and calls _exit
+   directly.  */
+static int
+do_test (int argc, char **argv)
+{
+  int ret;
+  char st1[32768];
+  ucontext_t tempctx = ctx;
+
+  if (argc < 2)
+    {
+      printf ("FAIL: Test missing filename argument.\n");
+      exit (1);
+    }
+
+  strcpy (filename, argv[1]);
+
+  atexit (exit_called);
+
+  puts ("making contexts");
+  if (getcontext (&ctx) != 0)
+    {
+      if (errno == ENOSYS)
+	{
+	  /* Exit with 77 to mark the test as UNSUPPORTED.  */
+	  printf ("UNSUPPORTED: getcontext not implemented.\n");
+	  exit (77);
+	}
+
+      printf ("FAIL: getcontext failed.\n");
+      exit (1);
+    }
+
+  ctx.uc_stack.ss_sp = st1;
+  ctx.uc_stack.ss_size = sizeof (st1);
+  ctx.uc_link = 0;
+  makecontext (&ctx, (void (*) (void)) cf, 0);
+
+  /* Without this check, a stub makecontext can make us spin forever.  */
+  if (memcmp (&tempctx, &ctx, sizeof ctx) == 0)
+    {
+      puts ("UNSUPPORTED: makecontext was a no-op, presuming not implemented");
+      return 77;
+    }
+
+  if ((ret = setcontext (&ctx)) != 0)
+    {
+      printf ("FAIL: setcontext returned with %d and errno of %d.\n", ret, errno);
+      exit (1);
+    }
+
+  printf ("FAIL: Impossibly returned to main.\n");
+  return 1;
+}
+
+#define TEST_FUNCTION do_test (argc, argv)
+#include "../test-skeleton.c"
diff --git a/stdlib/tst-setcontext2.sh b/stdlib/tst-setcontext2.sh
new file mode 100644
index 0000000..c82e4ba
--- /dev/null
+++ b/stdlib/tst-setcontext2.sh
@@ -0,0 +1,55 @@ 
+#! /bin/bash
+# Bug 18125: Test the exit functionality of setcontext().
+# 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/>.
+
+set -e
+
+common_objpfx=$1
+test_program_prefix_before_env=$2
+run_program_env=$3
+test_program_prefix_after_env=$4
+objpfx=$5
+
+test_pre="${test_program_prefix_before_env} ${run_program_env}"
+test="${test_program_prefix_after_env} ${objpfx}tst-setcontext2"
+out=${objpfx}tst-setcontext2.out
+
+tempfiles=()
+cleanup() {
+    rm -f "${tempfiles[@]}"
+}
+trap cleanup 0
+
+tempfile=$(mktemp "tst-setcontext2.XXXXXXXXXX")
+tempfiles+=("$tempfile")
+
+# We want to run the test program and see if secontext called
+# exit() and wrote out the test file we specified.  If the
+# test exits with a non-zero status this will fail because we
+# are using `set -e`.
+$test_pre $test $tempfile
+
+# Look for resulting file.
+if [ -e "$tempfile" ]; then
+  echo "PASS: tst-setcontext2 ran exit() and created $tempfile"
+  cleanup
+  exit 0
+else
+  echo "FAIL: tst-setcontext2 did not create $tempfile"
+  exit 1
+fi
diff --git a/sysdeps/unix/sysv/linux/aarch64/setcontext.S b/sysdeps/unix/sysv/linux/aarch64/setcontext.S
index 6dd7836..ae67581 100644
--- a/sysdeps/unix/sysv/linux/aarch64/setcontext.S
+++ b/sysdeps/unix/sysv/linux/aarch64/setcontext.S
@@ -125,5 +125,5 @@  weak_alias (__setcontext, setcontext)
 ENTRY (__startcontext)
 	mov	x0, x19
 	cbnz	x0, __setcontext
-1:	b       HIDDEN_JUMPTARGET (_exit)
+1:	b       HIDDEN_JUMPTARGET (exit)
 END (__startcontext)
diff --git a/sysdeps/unix/sysv/linux/arm/setcontext.S b/sysdeps/unix/sysv/linux/arm/setcontext.S
index 5268e06..24c7294 100644
--- a/sysdeps/unix/sysv/linux/arm/setcontext.S
+++ b/sysdeps/unix/sysv/linux/arm/setcontext.S
@@ -91,7 +91,7 @@  ENTRY(__startcontext)
 	bne     PLTJMP(__setcontext)
 
 	@ New context was 0 - exit
-	b       PLTJMP(HIDDEN_JUMPTARGET(_exit))
+	b       PLTJMP(HIDDEN_JUMPTARGET(exit))
 END(__startcontext)
 
 #ifdef PIC
diff --git a/sysdeps/unix/sysv/linux/hppa/setcontext.S b/sysdeps/unix/sysv/linux/hppa/setcontext.S
index 7271410..abe87a9 100644
--- a/sysdeps/unix/sysv/linux/hppa/setcontext.S
+++ b/sysdeps/unix/sysv/linux/hppa/setcontext.S
@@ -18,6 +18,7 @@ 
    <http://www.gnu.org/licenses/>.  */
 
 #include <sysdep.h>
+#include <libc-symbols.h>
 
 #include "ucontext_i.h"
 
@@ -139,7 +140,7 @@  ENTRY(__setcontext)
 	nop
 
 	/* No further context available. Exit now.  */
-	bl	_exit, %r2
+	bl	HIDDEN_JUMPTARGET(exit), %r2
 	ldi	-1, %r26
 
 
diff --git a/sysdeps/unix/sysv/linux/nios2/setcontext.S b/sysdeps/unix/sysv/linux/nios2/setcontext.S
index 9a8dd87..f40b733 100644
--- a/sysdeps/unix/sysv/linux/nios2/setcontext.S
+++ b/sysdeps/unix/sysv/linux/nios2/setcontext.S
@@ -89,15 +89,15 @@  ENTRY(__startcontext)
 	mov	r4, r16
 	bne	r4, zero, __setcontext
 
-	/* If uc_link == zero, call _exit.  */
+	/* If uc_link == zero, call exit.  */
 #ifdef PIC
 	nextpc	r22
 1:	movhi	r8, %hiadj(_gp_got - 1b)
 	addi	r8, r8, %lo(_gp_got - 1b)
 	add	r22, r22, r8
-	ldw	r8, %call(HIDDEN_JUMPTARGET(_exit))(r22)
+	ldw	r8, %call(HIDDEN_JUMPTARGET(exit))(r22)
 	jmp	r8
 #else
-	jmpi	_exit
+	jmpi	exit
 #endif
 END(__startcontext)