diff mbox

Fix struct sigaltstack namespace (bug 21517)

Message ID alpine.DEB.2.20.1705242232210.9182@digraph.polyomino.org.uk
State New, archived
Headers

Commit Message

Joseph Myers May 24, 2017, 10:32 p.m. UTC 
  glibc defines the stack_t type with the tag struct sigaltstack.  This
is not permitted by POSIX; sigaltstack is only reserved with file
scope in the namespace of ordinary identifiers, not the tag namespace,
and in the case where stack_t is obtained from ucontext.h rather than
signal.h, it's not reserved with file scope at all.

This patch removes the tag accordingly and updates uses in glibc of
struct sigaltstack.  This is similar to the removal of the "struct
siginfo" tag a few years ago: C++ name mangling changes are an
unavoidable consequence.  A NEWS item is added to note the changed
mangling.  There is inevitably some risk of breaking builds of
anything that relies on the struct sigaltstack name (though the first
few hits I looked at from codesearch.debian.net generally seemed to
involve code that could use the stack_t name conditionally, so
depending on how they determine the conditionals they may work with
glibc not defining the struct tag anyway).

Tested for x86_64 and x86, and with build-many-glibcs.py.

2017-05-24  Joseph Myers  <joseph@codesourcery.com>

	[BZ #21517]
	* bits/types/stack_t.h (stack_t): Remove struct tag.
	* sysdeps/unix/sysv/linux/bits/types/stack_t.h (stack_t):
	Likewise.
	* sysdeps/unix/sysv/linux/mips/bits/types/stack_t.h (stack_t):
	Likewise.
	* debug/segfault.c (install_handler): Use stack_t instead of
	struct sigaltstack.
	* hurd/hurd/signal.h (struct hurd_sigstate): Likewise.
	* hurd/trampoline.c (_hurd_setup_sighandler): Likewise.
	* include/signal.h (__sigaltstack): Likwise.
	* signal/sigaltstack.c (__sigaltstack): Likewise.
	* signal/signal.h (sigaltstack): Likewise.
	* sysdeps/mach/hurd/i386/signal-defines.sym
	(SIGALTSTACK__SS_SP__OFFSET): Likewise.
	(SIGALTSTACK__SS_SIZE__OFFSET): Likewise.
	(SIGALTSTACK__SS_FLAGS__OFFSET): Likewise.
	* sysdeps/mach/hurd/sigaltstack.c (__sigaltstack): Likewise.
	* sysdeps/mach/hurd/sigstack.c (sigstack): Likewise.
	* sysdeps/unix/sysv/linux/alpha/sys/procfs.h (struct
	elf_prstatus): Likewise.
	* sysdeps/unix/sysv/linux/hppa/____longjmp_chk.c (CHECK_SP):
	Likewise.
	* sysdeps/unix/sysv/linux/ia64/sys/procfs.h (struct elf_prstatus):
	Likewise.
	* sysdeps/unix/sysv/linux/m68k/____longjmp_chk.c (CHECK_SP):
	Likewise.
	* sysdeps/unix/sysv/linux/powerpc/sys/procfs.h (struct
	elf_prstatus): Likewise.
	* sysdeps/unix/sysv/linux/sh/sys/procfs.h (struct elf_prstatus):
	Likewise.
	* sysdeps/unix/sysv/linux/sys/procfs.h (struct elf_prstatus):
	Likewise.

Comments

Joseph Myers June 1, 2017, 11:37 a.m. UTC | #1 
Ping.  This patch 
<https://sourceware.org/ml/libc-alpha/2017-05/msg00744.html> is pending 
review.
Zack Weinberg June 3, 2017, 2:15 p.m. UTC | #2 
On 05/24/2017 06:32 PM, Joseph Myers wrote:
> glibc defines the stack_t type with the tag struct sigaltstack.  This
> is not permitted by POSIX; sigaltstack is only reserved with file
> scope in the namespace of ordinary identifiers, not the tag namespace,
> and in the case where stack_t is obtained from ucontext.h rather than
> signal.h, it's not reserved with file scope at all.
>
> This patch removes the tag accordingly and updates uses in glibc of
> struct sigaltstack.  This is similar to the removal of the "struct
> siginfo" tag a few years ago: C++ name mangling changes are an
> unavoidable consequence.  A NEWS item is added to note the changed
> mangling.  There is inevitably some risk of breaking builds of
> anything that relies on the struct sigaltstack name (though the first
> few hits I looked at from codesearch.debian.net generally seemed to
> involve code that could use the stack_t name conditionally, so
> depending on how they determine the conditionals they may work with
> glibc not defining the struct tag anyway).

I'm not super happy about changing C++ name mangling, but this
particular type is used rarely enough that I think we can get away with
it.  Since no one has objected in over a week, this is OK.

zw
Florian Weimer June 3, 2017, 3:55 p.m. UTC | #3 
On 05/25/2017 12:32 AM, Joseph Myers wrote:
> +* The stack_t type no longer has the name struct sigaltstack.  This changes
> +  the C++ name mangling for interfaces involving this type.

I checked Fedora and we don't seem to have any ABI impact from this
change (no exported symbols matching /^_Z.*sigaltstack/), so this is
probably fine.

Thanks,
Florian
Khem Raj June 12, 2017, 9:15 p.m. UTC | #4 
On Sat, Jun 3, 2017 at 8:55 AM, Florian Weimer <fweimer@redhat.com> wrote:
> On 05/25/2017 12:32 AM, Joseph Myers wrote:
>> +* The stack_t type no longer has the name struct sigaltstack.  This changes
>> +  the C++ name mangling for interfaces involving this type.
>
> I checked Fedora and we don't seem to have any ABI impact from this
> change (no exported symbols matching /^_Z.*sigaltstack/), so this is
> probably fine.

I think gcc sanitizers will have issues since they are using
the forward declaration of sigaltstack

see
https://github.com/gcc-mirror/gcc/blob/master/libsanitizer/sanitizer_common/sanitizer_linux.h#L22

>
> Thanks,
> Florian
Florian Weimer June 13, 2017, 6:34 a.m. UTC | #5 
On 06/12/2017 11:15 PM, Khem Raj wrote:
> On Sat, Jun 3, 2017 at 8:55 AM, Florian Weimer <fweimer@redhat.com> wrote:
>> On 05/25/2017 12:32 AM, Joseph Myers wrote:
>>> +* The stack_t type no longer has the name struct sigaltstack.  This changes
>>> +  the C++ name mangling for interfaces involving this type.
>>
>> I checked Fedora and we don't seem to have any ABI impact from this
>> change (no exported symbols matching /^_Z.*sigaltstack/), so this is
>> probably fine.
> 
> I think gcc sanitizers will have issues since they are using
> the forward declaration of sigaltstack
> 
> see
> https://github.com/gcc-mirror/gcc/blob/master/libsanitizer/sanitizer_common/sanitizer_linux.h#L22

There's already a bug for that:

  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81066

There is no ABI impact, so this is fixable.

Thanks,
Florian
diff mbox

Patch

diff --git a/NEWS b/NEWS
index b4ecd62..6ccc4e6 100644
--- a/NEWS
+++ b/NEWS
@@ -66,6 +66,9 @@  Version 2.26
 * The port to Native Client running on ARMv7-A (--host=arm-nacl) has been
   removed.
 
+* The stack_t type no longer has the name struct sigaltstack.  This changes
+  the C++ name mangling for interfaces involving this type.
+
 Security related changes:
 
 * The DNS stub resolver limits the advertised UDP buffer size to 1200 bytes,
diff --git a/bits/types/stack_t.h b/bits/types/stack_t.h
index 3cf0a40..47149fc 100644
--- a/bits/types/stack_t.h
+++ b/bits/types/stack_t.h
@@ -23,7 +23,7 @@ 
 #include <stddef.h>
 
 /* Structure describing a signal stack.  */
-typedef struct sigaltstack
+typedef struct
   {
     void *ss_sp;
     size_t ss_size;
diff --git a/debug/segfault.c b/debug/segfault.c
index dac9efb..78c1cf5 100644
--- a/debug/segfault.c
+++ b/debug/segfault.c
@@ -156,7 +156,7 @@  install_handler (void)
   if (getenv ("SEGFAULT_USE_ALTSTACK") != 0)
     {
       void *stack_mem = malloc (2 * SIGSTKSZ);
-      struct sigaltstack ss;
+      stack_t ss;
 
       if (stack_mem != NULL)
 	{
diff --git a/hurd/hurd/signal.h b/hurd/hurd/signal.h
index f899e81..e03d53e 100644
--- a/hurd/hurd/signal.h
+++ b/hurd/hurd/signal.h
@@ -70,7 +70,7 @@  struct hurd_sigstate
     sigset_t blocked;		/* What signals are blocked.  */
     sigset_t pending;		/* Pending signals, possibly blocked.  */
     struct sigaction actions[NSIG];
-    struct sigaltstack sigaltstack;
+    stack_t sigaltstack;
 
     /* Chain of thread-local signal preemptors; see <hurd/sigpreempt.h>.
        Each element of this chain is in local stack storage, and the chain
diff --git a/hurd/trampoline.c b/hurd/trampoline.c
index 8216e22..e506fd8 100644
--- a/hurd/trampoline.c
+++ b/hurd/trampoline.c
@@ -28,7 +28,7 @@ 
 struct sigcontext *
 _hurd_setup_sighandler (int flags,
 			__sighandler_t handler,
-			struct sigaltstack *sigaltstack,
+			stack_t *sigaltstack,
 			int signo, int sigcode,
 			void *state)
 {
diff --git a/include/signal.h b/include/signal.h
index e39ddc6..bcf1455 100644
--- a/include/signal.h
+++ b/include/signal.h
@@ -41,8 +41,8 @@  extern int __sigqueue (__pid_t __pid, int __sig,
 #ifdef __USE_MISC
 extern int __sigreturn (struct sigcontext *__scp);
 #endif
-extern int __sigaltstack (const struct sigaltstack *__ss,
-			  struct sigaltstack *__oss);
+extern int __sigaltstack (const stack_t *__ss,
+			  stack_t *__oss);
 extern int __libc_sigaction (int sig, const struct sigaction *act,
 			     struct sigaction *oact);
 libc_hidden_proto (__libc_sigaction)
diff --git a/signal/sigaltstack.c b/signal/sigaltstack.c
index e4d2319..b70afa7 100644
--- a/signal/sigaltstack.c
+++ b/signal/sigaltstack.c
@@ -21,7 +21,7 @@ 
 /* Run signals handlers on the stack specified by SS (if not NULL).
    If OSS is not NULL, it is filled in with the old signal stack status.  */
 int
-sigaltstack (const struct sigaltstack *ss, struct sigaltstack *oss)
+sigaltstack (const stack_t *ss, stack_t *oss)
 {
   __set_errno (ENOSYS);
   return -1;
diff --git a/signal/signal.h b/signal/signal.h
index 947873e..13d4c1a 100644
--- a/signal/signal.h
+++ b/signal/signal.h
@@ -306,8 +306,8 @@  extern int siginterrupt (int __sig, int __interrupt) __THROW;
 
 /* Alternate signal handler stack interface.
    This interface should always be preferred over `sigstack'.  */
-extern int sigaltstack (const struct sigaltstack *__restrict __ss,
-			struct sigaltstack *__restrict __oss) __THROW;
+extern int sigaltstack (const stack_t *__restrict __ss,
+			stack_t *__restrict __oss) __THROW;
 
 #endif /* Use POSIX.1-2008 or X/Open Unix.  */
 
diff --git a/sysdeps/mach/hurd/i386/signal-defines.sym b/sysdeps/mach/hurd/i386/signal-defines.sym
index 9521bd7..e42bbbe 100644
--- a/sysdeps/mach/hurd/i386/signal-defines.sym
+++ b/sysdeps/mach/hurd/i386/signal-defines.sym
@@ -5,6 +5,6 @@ 
 
 HURD_SIGSTATE__SIGALTSTACK__OFFSET	offsetof(struct hurd_sigstate, sigaltstack)
 
-SIGALTSTACK__SS_SP__OFFSET		offsetof(struct sigaltstack, ss_sp)
-SIGALTSTACK__SS_SIZE__OFFSET		offsetof(struct sigaltstack, ss_size)
-SIGALTSTACK__SS_FLAGS__OFFSET		offsetof(struct sigaltstack, ss_flags)
+SIGALTSTACK__SS_SP__OFFSET		offsetof(stack_t, ss_sp)
+SIGALTSTACK__SS_SIZE__OFFSET		offsetof(stack_t, ss_size)
+SIGALTSTACK__SS_FLAGS__OFFSET		offsetof(stack_t, ss_flags)
diff --git a/sysdeps/mach/hurd/sigaltstack.c b/sysdeps/mach/hurd/sigaltstack.c
index 06a6a5c..4fba69e 100644
--- a/sysdeps/mach/hurd/sigaltstack.c
+++ b/sysdeps/mach/hurd/sigaltstack.c
@@ -22,16 +22,16 @@ 
 /* Run signals handlers on the stack specified by SS (if not NULL).
    If OSS is not NULL, it is filled in with the old signal stack status.  */
 int
-__sigaltstack (const struct sigaltstack *argss, struct sigaltstack *oss)
+__sigaltstack (const stack_t *argss, stack_t *oss)
 {
   struct hurd_sigstate *s;
-  struct sigaltstack ss, old;
+  stack_t ss, old;
 
   /* Fault before taking any locks.  */
   if (argss != NULL)
     ss = *argss;
   if (oss != NULL)
-    *(volatile struct sigaltstack *) oss = *oss;
+    *(volatile stack_t *) oss = *oss;
 
   s = _hurd_self_sigstate ();
   __spin_lock (&s->lock);
diff --git a/sysdeps/mach/hurd/sigstack.c b/sysdeps/mach/hurd/sigstack.c
index a3a11f9..484efb6 100644
--- a/sysdeps/mach/hurd/sigstack.c
+++ b/sysdeps/mach/hurd/sigstack.c
@@ -24,7 +24,7 @@ 
 int
 sigstack (struct sigstack *ss, struct sigstack *oss)
 {
-  struct sigaltstack as, oas;
+  stack_t as, oas;
 
   as.ss_sp = ss->ss_sp;
   as.ss_size = 0;
diff --git a/sysdeps/unix/sysv/linux/alpha/sys/procfs.h b/sysdeps/unix/sysv/linux/alpha/sys/procfs.h
index 2cd69d4..abc9fd8 100644
--- a/sysdeps/unix/sysv/linux/alpha/sys/procfs.h
+++ b/sysdeps/unix/sysv/linux/alpha/sys/procfs.h
@@ -71,7 +71,7 @@  struct elf_prstatus
     unsigned long int pr_sigpend;	/* Set of pending signals.  */
     unsigned long int pr_sighold;	/* Set of held signals.  */
 #if 0
-    struct sigaltstack pr_altstack;	/* Alternate stack info.  */
+    stack_t pr_altstack;		/* Alternate stack info.  */
     struct sigaction pr_action;		/* Signal action for current sig.  */
 #endif
     __pid_t pr_pid;
diff --git a/sysdeps/unix/sysv/linux/bits/types/stack_t.h b/sysdeps/unix/sysv/linux/bits/types/stack_t.h
index 497e42b..373c227 100644
--- a/sysdeps/unix/sysv/linux/bits/types/stack_t.h
+++ b/sysdeps/unix/sysv/linux/bits/types/stack_t.h
@@ -23,7 +23,7 @@ 
 #include <stddef.h>
 
 /* Structure describing a signal stack.  */
-typedef struct sigaltstack
+typedef struct
   {
     void *ss_sp;
     int ss_flags;
diff --git a/sysdeps/unix/sysv/linux/hppa/____longjmp_chk.c b/sysdeps/unix/sysv/linux/hppa/____longjmp_chk.c
index 3a04250..48aaeb3 100644
--- a/sysdeps/unix/sysv/linux/hppa/____longjmp_chk.c
+++ b/sysdeps/unix/sysv/linux/hppa/____longjmp_chk.c
@@ -27,7 +27,7 @@ 
        destroyed must all have stack values higher than ours.  */	\
     if ((unsigned long) (sp) > this_sp)					\
       {									\
-        struct sigaltstack oss;						\
+        stack_t oss;							\
         INTERNAL_SYSCALL_DECL (err);					\
         int result = INTERNAL_SYSCALL (sigaltstack, err, 2, NULL, &oss);\
 	/* If we aren't using an alternate stack then we have already	\
diff --git a/sysdeps/unix/sysv/linux/ia64/sys/procfs.h b/sysdeps/unix/sysv/linux/ia64/sys/procfs.h
index 93e569b..afe54fb 100644
--- a/sysdeps/unix/sysv/linux/ia64/sys/procfs.h
+++ b/sysdeps/unix/sysv/linux/ia64/sys/procfs.h
@@ -73,7 +73,7 @@  struct elf_prstatus
     unsigned long int pr_sigpend;	/* Set of pending signals.  */
     unsigned long int pr_sighold;	/* Set of held signals.  */
 #if 0
-    struct sigaltstack pr_altstack;	/* Alternate stack info.  */
+    stack_t pr_altstack;		/* Alternate stack info.  */
     struct sigaction pr_action;		/* Signal action for current sig.  */
 #endif
     __pid_t pr_pid;
diff --git a/sysdeps/unix/sysv/linux/m68k/____longjmp_chk.c b/sysdeps/unix/sysv/linux/m68k/____longjmp_chk.c
index 3539b54..030e463 100644
--- a/sysdeps/unix/sysv/linux/m68k/____longjmp_chk.c
+++ b/sysdeps/unix/sysv/linux/m68k/____longjmp_chk.c
@@ -24,7 +24,7 @@ 
     register unsigned long this_sp asm ("sp");				      \
     if ((unsigned long) (sp) < this_sp)					      \
       {									      \
-	struct sigaltstack oss;						      \
+	stack_t oss;							      \
 	INTERNAL_SYSCALL_DECL (err);					      \
 	int result = INTERNAL_SYSCALL (sigaltstack, err, 2, NULL, &oss);      \
 	if (!INTERNAL_SYSCALL_ERROR_P (result, err)			      \
diff --git a/sysdeps/unix/sysv/linux/mips/bits/types/stack_t.h b/sysdeps/unix/sysv/linux/mips/bits/types/stack_t.h
index ef06072..b9635ad 100644
--- a/sysdeps/unix/sysv/linux/mips/bits/types/stack_t.h
+++ b/sysdeps/unix/sysv/linux/mips/bits/types/stack_t.h
@@ -23,7 +23,7 @@ 
 #include <stddef.h>
 
 /* Structure describing a signal stack.  */
-typedef struct sigaltstack
+typedef struct
   {
     void *ss_sp;
     size_t ss_size;
diff --git a/sysdeps/unix/sysv/linux/powerpc/sys/procfs.h b/sysdeps/unix/sysv/linux/powerpc/sys/procfs.h
index b7b7b0b..61ded22 100644
--- a/sysdeps/unix/sysv/linux/powerpc/sys/procfs.h
+++ b/sysdeps/unix/sysv/linux/powerpc/sys/procfs.h
@@ -83,7 +83,7 @@  struct elf_prstatus
     unsigned long int pr_sigpend;	/* Set of pending signals.  */
     unsigned long int pr_sighold;	/* Set of held signals.  */
 #if 0
-    struct sigaltstack pr_altstack;	/* Alternate stack info.  */
+    stack_t pr_altstack;		/* Alternate stack info.  */
     struct sigaction pr_action;		/* Signal action for current sig.  */
 #endif
     __pid_t pr_pid;
diff --git a/sysdeps/unix/sysv/linux/sh/sys/procfs.h b/sysdeps/unix/sysv/linux/sh/sys/procfs.h
index c41c877..414911a 100644
--- a/sysdeps/unix/sysv/linux/sh/sys/procfs.h
+++ b/sysdeps/unix/sysv/linux/sh/sys/procfs.h
@@ -55,7 +55,7 @@  struct elf_prstatus
     unsigned long int pr_sigpend;	/* Set of pending signals.  */
     unsigned long int pr_sighold;	/* Set of held signals.  */
 #if 0
-    struct sigaltstack pr_altstack;	/* Alternate stack info.  */
+    stack_t pr_altstack;		/* Alternate stack info.  */
     struct sigaction pr_action;		/* Signal action for current sig.  */
 #endif
     __pid_t pr_pid;
diff --git a/sysdeps/unix/sysv/linux/sys/procfs.h b/sysdeps/unix/sysv/linux/sys/procfs.h
index 8dfa0c0..b3b2cf3 100644
--- a/sysdeps/unix/sysv/linux/sys/procfs.h
+++ b/sysdeps/unix/sysv/linux/sys/procfs.h
@@ -58,7 +58,7 @@  struct elf_prstatus
     unsigned long int pr_sigpend;	/* Set of pending signals.  */
     unsigned long int pr_sighold;	/* Set of held signals.  */
 #if 0
-    struct sigaltstack pr_altstack;	/* Alternate stack info.  */
+    stack_t pr_altstack;		/* Alternate stack info.  */
     struct sigaction pr_action;		/* Signal action for current sig.  */
 #endif
     __pid_t pr_pid;