This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Fix sigval namespace (bug 21944)



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.  */
> 


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]