This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: Fix sigval namespace (bug 21944)
- From: Adhemerval Zanella <adhemerval dot zanella at linaro dot org>
- To: libc-alpha at sourceware dot org
- Date: Wed, 16 Aug 2017 15:04:55 -0300
- Subject: Re: Fix sigval namespace (bug 21944)
- Authentication-results: sourceware.org; auth=none
- References: <alpine.DEB.2.20.1708111812040.2884@digraph.polyomino.org.uk>
On 11/08/2017 15:12, Joseph Myers wrote:
> XPG4.2 defines the siginfo_t type, but not union sigval or its
> contents (which were added in the 1993 edition of POSIX.1), resulting
> in namespace violations for sigval, sival_int and sival_ptr for
> signal.h and sys/wait.h for that standard because those headers
> incorrectly expose those names in that case.
>
> This patch fixes this problem. The public type in this case is union
> sigval, but various places in the headers use the sigval_t name for
> it; direct uses of union sigval are already properly guarded or in
> headers not in XPG4.2. Now, sigval_t, although not a standard name,
> does seem to be widely used outside glibc. The approach taken by this
> patch is to make installed headers use the name __sigval_t instead.
> __sigval_t is then defined to either union sigval or union __sigval
> (where union __sigval has __-prefixed member names as well), depending
> on whether there are any namespace issues with the union sigval name
> and its members. In the case where union __sigval is used, sigval_t
> is not defined at all, to avoid the problem of sigval_t having a C++
> mangled name that depends on feature test macros. sigval_t is still
> defined by signal.h if __USE_MISC (reflecting the nonstandard nature
> of that name).
>
> Tested for x86_64.
>
> 2017-08-11 Joseph Myers <joseph@codesourcery.com>
>
> [BZ #21944]
> * signal/bits/types/__sigval_t.h: New file.
> * signal/Makefile (headers): Add bits/types/__sigval_t.h.
> * signal/bits/types/sigval_t.h: Include <bits/types/__sigval_t.h>
> and define sigval_t using __sigval_t.
> * include/bits/types/__sigval_t.h: New file.
> * bits/types/sigevent_t.h: Include <bits/types/__sigval_t.h>
> instead of <bits/types/__sigval_t.h>.
> (struct sigevent): Use __sigval_t instead of sigval_t.
> * bits/types/siginfo_t.h: Include <bits/types/__sigval_t.h>
> instead of <bits/types/__sigval_t.h>.
> (siginfo_t): Use __sigval_t instead of sigval_t.
> * sysdeps/unix/sysv/linux/bits/types/sigevent_t.h: Include
> <bits/types/__sigval_t.h> instead of <bits/types/__sigval_t.h>.
> (struct sigevent): Use __sigval_t instead of sigval_t.
> * sysdeps/unix/sysv/linux/bits/types/siginfo_t.h: Include
> <bits/types/__sigval_t.h> instead of <bits/types/__sigval_t.h>.
> (siginfo_t): Use __sigval_t instead of sigval_t.
> * signal/signal.h [__USE_MISC]: Include <bits/types/sigval_t.h>.
I can't really voucher for the standard wording, but patch itself LGTM
with just some doubt regarding new header copyright headers below.
>
> diff --git a/bits/types/sigevent_t.h b/bits/types/sigevent_t.h
> index 7b8cb05..5611268 100644
> --- a/bits/types/sigevent_t.h
> +++ b/bits/types/sigevent_t.h
> @@ -2,15 +2,15 @@
> #define __sigevent_t_defined 1
>
> #include <bits/types.h>
> -#include <bits/types/sigval_t.h>
> +#include <bits/types/__sigval_t.h>
>
> /* Structure to transport application-defined values with signals. */
> typedef struct sigevent
> {
> - sigval_t sigev_value;
> + __sigval_t sigev_value;
> int sigev_signo;
> int sigev_notify;
> - void (*sigev_notify_function) (sigval_t); /* Function to start. */
> + void (*sigev_notify_function) (__sigval_t); /* Function to start. */
> void *sigev_notify_attributes; /* Really pthread_attr_t.*/
> } sigevent_t;
>
> diff --git a/bits/types/siginfo_t.h b/bits/types/siginfo_t.h
> index ab6bf18..1ac2a98 100644
> --- a/bits/types/siginfo_t.h
> +++ b/bits/types/siginfo_t.h
> @@ -2,7 +2,7 @@
> #define __siginfo_t_defined 1
>
> #include <bits/types.h>
> -#include <bits/types/sigval_t.h>
> +#include <bits/types/__sigval_t.h>
>
> typedef struct siginfo
> {
> @@ -15,7 +15,7 @@ typedef struct siginfo
> void *si_addr; /* Address of faulting instruction. */
> int si_status; /* Exit value or signal. */
> long int si_band; /* Band event for SIGPOLL. */
> - sigval_t si_value; /* Signal value. */
> + __sigval_t si_value; /* Signal value. */
> } siginfo_t;
>
> #endif
> diff --git a/include/bits/types/__sigval_t.h b/include/bits/types/__sigval_t.h
> new file mode 100644
> index 0000000..62f8e48
> --- /dev/null
> +++ b/include/bits/types/__sigval_t.h
> @@ -0,0 +1 @@
> +#include <signal/bits/types/__sigval_t.h>
Shouldn't we have a copyright definition here?
> diff --git a/signal/Makefile b/signal/Makefile
> index 8c9a7d1..a6a1289 100644
> --- a/signal/Makefile
> +++ b/signal/Makefile
> @@ -30,7 +30,8 @@ headers := signal.h sys/signal.h \
> bits/types/__sigset_t.h bits/types/sig_atomic_t.h \
> bits/types/sigevent_t.h bits/types/siginfo_t.h \
> bits/types/sigset_t.h bits/types/sigval_t.h \
> - bits/types/stack_t.h bits/types/struct_sigstack.h
> + bits/types/stack_t.h bits/types/struct_sigstack.h \
> + bits/types/__sigval_t.h
>
> routines := signal raise killpg \
> sigaction sigprocmask kill \
> diff --git a/signal/bits/types/__sigval_t.h b/signal/bits/types/__sigval_t.h
> new file mode 100644
> index 0000000..90cba5b
> --- /dev/null
> +++ b/signal/bits/types/__sigval_t.h
> @@ -0,0 +1,23 @@
> +#ifndef ____sigval_t_defined
> +#define ____sigval_t_defined
Same as before. I noted also for signal/bits/types only struct_sigstack.h
contains the header and it is based on old kernel version. Should we omit
the copyright header for these new types definitions headers?
> +
> +/* Type for data associated with a signal. */
> +#ifdef __USE_POSIX199309
> +union sigval
> +{
> + int sival_int;
> + void *sival_ptr;
> +};
> +
> +typedef union sigval __sigval_t;
> +#else
> +union __sigval
> +{
> + int __sival_int;
> + void *__sival_ptr;
> +};
> +
> +typedef union __sigval __sigval_t;
> +#endif
> +
> +#endif
> diff --git a/signal/bits/types/sigval_t.h b/signal/bits/types/sigval_t.h
> index 666598f..a05d7f4 100644
> --- a/signal/bits/types/sigval_t.h
> +++ b/signal/bits/types/sigval_t.h
> @@ -1,13 +1,18 @@
> #ifndef __sigval_t_defined
> #define __sigval_t_defined
>
> -/* Type for data associated with a signal. */
> -union sigval
> -{
> - int sival_int;
> - void *sival_ptr;
> -};
> -
> -typedef union sigval sigval_t;
> +#include <bits/types/__sigval_t.h>
> +
> +/* To avoid sigval_t (not a standard type name) having C++ name
> + mangling depending on whether the selected standard includes union
> + sigval, it should not be defined at all when using a standard for
> + which the sigval name is not reserved; in that case, headers should
> + not include <bits/types/sigval_t.h> and should use only the
> + internal __sigval_t name. */
> +#ifndef __USE_POSIX199309
> +# error "sigval_t defined for standard not including union sigval"
> +#endif
> +
> +typedef __sigval_t sigval_t;
>
> #endif
> diff --git a/signal/signal.h b/signal/signal.h
> index c8f6100..416c5a2 100644
> --- a/signal/signal.h
> +++ b/signal/signal.h
> @@ -58,6 +58,10 @@ typedef __uid_t uid_t;
> # include <bits/siginfo-consts.h>
> #endif
>
> +#ifdef __USE_MISC
> +# include <bits/types/sigval_t.h>
> +#endif
> +
> #ifdef __USE_POSIX199309
> # include <bits/types/sigevent_t.h>
> # include <bits/sigevent-consts.h>
> diff --git a/sysdeps/unix/sysv/linux/bits/types/sigevent_t.h b/sysdeps/unix/sysv/linux/bits/types/sigevent_t.h
> index 0d4857b..e8b28de 100644
> --- a/sysdeps/unix/sysv/linux/bits/types/sigevent_t.h
> +++ b/sysdeps/unix/sysv/linux/bits/types/sigevent_t.h
> @@ -3,7 +3,7 @@
>
> #include <bits/wordsize.h>
> #include <bits/types.h>
> -#include <bits/types/sigval_t.h>
> +#include <bits/types/__sigval_t.h>
>
> #define __SIGEV_MAX_SIZE 64
> #if __WORDSIZE == 64
> @@ -21,7 +21,7 @@ typedef union pthread_attr_t pthread_attr_t;
> /* Structure to transport application-defined values with signals. */
> typedef struct sigevent
> {
> - sigval_t sigev_value;
> + __sigval_t sigev_value;
> int sigev_signo;
> int sigev_notify;
>
> @@ -35,7 +35,7 @@ typedef struct sigevent
>
> struct
> {
> - void (*_function) (sigval_t); /* Function to start. */
> + void (*_function) (__sigval_t); /* Function to start. */
> pthread_attr_t *_attribute; /* Thread attributes. */
> } _sigev_thread;
> } _sigev_un;
> diff --git a/sysdeps/unix/sysv/linux/bits/types/siginfo_t.h b/sysdeps/unix/sysv/linux/bits/types/siginfo_t.h
> index bed6914..33766d1 100644
> --- a/sysdeps/unix/sysv/linux/bits/types/siginfo_t.h
> +++ b/sysdeps/unix/sysv/linux/bits/types/siginfo_t.h
> @@ -3,7 +3,7 @@
>
> #include <bits/wordsize.h>
> #include <bits/types.h>
> -#include <bits/types/sigval_t.h>
> +#include <bits/types/__sigval_t.h>
>
> #define __SI_MAX_SIZE 128
> #if __WORDSIZE == 64
> @@ -64,7 +64,7 @@ typedef struct
> {
> int si_tid; /* Timer ID. */
> int si_overrun; /* Overrun count. */
> - sigval_t si_sigval; /* Signal value. */
> + __sigval_t si_sigval; /* Signal value. */
> } _timer;
>
> /* POSIX.1b signals. */
> @@ -72,7 +72,7 @@ typedef struct
> {
> __pid_t si_pid; /* Sending process ID. */
> __uid_t si_uid; /* Real user ID of sending process. */
> - sigval_t si_sigval; /* Signal value. */
> + __sigval_t si_sigval; /* Signal value. */
> } _rt;
>
> /* SIGCHLD. */
>