Bad interaction between signal constants and -Wsign-conversion

Bad interaction between signal constants and -Wsign-conversion

[Zack Weinberg]

This test program does something perfectly normal.  Well, it's cut down
from a program that does something perfectly normal.  SA_RESETHAND is
one of the bit flags intended to be used with the sa_flags field of
struct sigaction.

#include <signal.h>

struct sigaction sa = { 
  .sa_flags = SA_RESETHAND,
};

However ... on Linux, SA_RESETHAND happens to have the value 0x80000000,
and therefore, the type of that constant is 'unsigned int'.  And the
type of sa_flags is 'int'.  So if you compile this test program with
-Wconversion you get a warning:

$ gcc -fsyntax-only -Wconversion test.c
test.c:4:15: warning: signed conversion from ‘unsigned int’ to ‘int’
   changes value from ‘2147483648’ to ‘-2147483648’ [-Wsign-conversion]
    4 |  .sa_flags = SA_RESETHAND,
      |              ^~~~~~~~~~~~

This strikes me as a (minor) bug in our headers.  Given the ABI constraints
(this is a flags word passed to the kernel), I think the most straightforward
fix is probably to change the #define in bits/sigaction.h to

# define SA_RESETHAND ((int)0x80000000) /* Reset to SIG_DFL on entry to handler.  */

Anyone got a better idea?

zw

Re: Bad interaction between signal constants and -Wsign-conversion

[Paul Eggert]

On 3/27/23 18:23, Zack Weinberg via Libc-alpha wrote:
> # define SA_RESETHAND ((int)0x80000000) /* Reset to SIG_DFL on entry to handler.  */
> 
> Anyone got a better idea?

Perhaps this instead:

   #define SA_RESETHAND (-1 - 0x7fffffff)

so that it is usable in preprocessor expressions. This is the sort of 
thing that <limits.h> does for INT_MIN, and what 
glibc/sysdeps/sparc/fpu/bits/fenv.h does for FE_UPWARD.

Are there other instances of 0x80000000 in user-visible glibc headers 
that could cause similar problems?

Re: Bad interaction between signal constants and -Wsign-conversion

[Cyril Hrubis]

Hi!
> > # define SA_RESETHAND ((int)0x80000000) /* Reset to SIG_DFL on entry to handler.  */
> > 
> > Anyone got a better idea?
> 
> Perhaps this instead:
> 
>    #define SA_RESETHAND (-1 - 0x7fffffff)

Why not (1<<31) ?

-- 
Cyril Hrubis
chrubis@suse.cz

Re: Bad interaction between signal constants and -Wsign-conversion

[Florian Weimer]

* Cyril Hrubis:

> Hi!
>> > # define SA_RESETHAND ((int)0x80000000) /* Reset to SIG_DFL on entry to handler.  */
>> > 
>> > Anyone got a better idea?
>> 
>> Perhaps this instead:
>> 
>>    #define SA_RESETHAND (-1 - 0x7fffffff)
>
> Why not (1<<31) ?

Some compilers warn about it, too.  Shifting into the sign position is
undefined in the standard.

I suggest to write out the negative constants explicitly, as a decimal
integer.

Re: Bad interaction between signal constants and -Wsign-conversion

[Andreas Schwab]

On Mär 28 2023, Florian Weimer wrote:

> I suggest to write out the negative constants explicitly, as a decimal
> integer.

That doesn't work either, because you cannot write a negative literal.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: Bad interaction between signal constants and -Wsign-conversion

[Florian Weimer]

* Andreas Schwab via Libc-alpha:

> On Mär 28 2023, Florian Weimer wrote:
>
>> I suggest to write out the negative constants explicitly, as a decimal
>> integer.
>
> That doesn't work either, because you cannot write a negative literal.

Oh.  Yes, then the more complicated expression is actually needed.

Re: Bad interaction between signal constants and -Wsign-conversion

[Alejandro Colomar]

[-- Attachment #1.1: Type: text/plain, Size: 2156 bytes --]

Hi Zack,

On 3/28/23 03:23, Zack Weinberg via Libc-alpha wrote:
> This test program does something perfectly normal.  Well, it's cut down
> from a program that does something perfectly normal.  SA_RESETHAND is
> one of the bit flags intended to be used with the sa_flags field of
> struct sigaction.
> 
> #include <signal.h>
> 
> struct sigaction sa = { 
>   .sa_flags = SA_RESETHAND,
> };
> 
> However ... on Linux, SA_RESETHAND happens to have the value 0x80000000,
> and therefore, the type of that constant is 'unsigned int'.  And the
> type of sa_flags is 'int'.  So if you compile this test program with
> -Wconversion you get a warning:
> 
> $ gcc -fsyntax-only -Wconversion test.c
> test.c:4:15: warning: signed conversion from ‘unsigned int’ to ‘int’
>    changes value from ‘2147483648’ to ‘-2147483648’ [-Wsign-conversion]
>     4 |  .sa_flags = SA_RESETHAND,
>       |              ^~~~~~~~~~~~
> 
> This strikes me as a (minor) bug in our headers.  Given the ABI constraints
> (this is a flags word passed to the kernel), I think the most straightforward
> fix is probably to change the #define in bits/sigaction.h to
> 
> # define SA_RESETHAND ((int)0x80000000) /* Reset to SIG_DFL on entry to handler.  */
> 
> Anyone got a better idea?

Well, probably not doable now, but is switching to the unsigned counterpart
still possible now?  It would require investigating how this member is
being used out there, to know if we're going to break anyones code, and
integer conversions are very tricky, so it's hard to know, but for a flags
field that is to be used in bitwise operations, I don't expect anyone to
actually use it as an int, but rather as an unsigned that for historic
reasons happens to be signed.

This is also a reminder that new such fields in structures and function
parameters should always be of unsigned types if they're going to hold
flags (or in general, if they're to be treated as something that requires
bitwise operations).

Cheers,
Alex

> 
> zw

-- 
<http://www.alejandro-colomar.es/>
GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Bad interaction between signal constants and -Wsign-conversion

[Zack Weinberg]

On Tue, Mar 28, 2023, at 8:08 PM, Alejandro Colomar via Libc-alpha wrote:
> On 3/28/23 03:23, Zack Weinberg via Libc-alpha wrote:
...
>> the type of sa_flags is 'int'
...
> is switching to the unsigned counterpart possible?

Normal usage of struct sigaction wouldn't be affected.  Whether
there are abnormal uses that would ... my feeling is almost
certainly *yes* (and therefore this is not an option), but I have no
idea how to find them.

zw

Re: Bad interaction between signal constants and -Wsign-conversion

[Vincent Lefevre]

On 2023-03-29 02:08:39 +0200, Alejandro Colomar via Libc-alpha wrote:
[about sa_flags in struct sigaction]
> Well, probably not doable now, but is switching to the unsigned counterpart
> still possible now?  It would require investigating how this member is
> being used out there, to know if we're going to break anyones code, and
> integer conversions are very tricky, so it's hard to know, but for a flags
> field that is to be used in bitwise operations, I don't expect anyone to
> actually use it as an int, but rather as an unsigned that for historic
> reasons happens to be signed.
> 
> This is also a reminder that new such fields in structures and function
> parameters should always be of unsigned types if they're going to hold
> flags (or in general, if they're to be treated as something that requires
> bitwise operations).

I entirely agree.

But concerning struct sigaction in particular, shouldn't this be fixed
in POSIX?

-- 
Vincent Lefèvre <vincent@vinc17.net> - Web: <https://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)

Re: Bad interaction between signal constants and -Wsign-conversion

[Alejandro Colomar]

[-- Attachment #1.1: Type: text/plain, Size: 1706 bytes --]

Hi Vincent,

On 3/30/23 15:59, Vincent Lefevre wrote:
> On 2023-03-29 02:08:39 +0200, Alejandro Colomar via Libc-alpha wrote:
> [about sa_flags in struct sigaction]
>> Well, probably not doable now, but is switching to the unsigned counterpart
>> still possible now?  It would require investigating how this member is
>> being used out there, to know if we're going to break anyones code, and
>> integer conversions are very tricky, so it's hard to know, but for a flags
>> field that is to be used in bitwise operations, I don't expect anyone to
>> actually use it as an int, but rather as an unsigned that for historic
>> reasons happens to be signed.
>>
>> This is also a reminder that new such fields in structures and function
>> parameters should always be of unsigned types if they're going to hold
>> flags (or in general, if they're to be treated as something that requires
>> bitwise operations).
> 
> I entirely agree.
> 
> But concerning struct sigaction in particular, shouldn't this be fixed
> in POSIX?

IMHO, POSIX, and in general, standards, should follow implementations
rather than lead them.  I think changes should start as vendor
extensions.

For example, timespec::tv_nsec was mandated to be long by POSIX and
C11, but glibc used 'long long' in some cases, because it was
necessary.  I would say using 'unsigned int' here would be a nice GNU
extension, and if it proves its value, POSIX could later pick it.

However, as Zack said, I'm not sure how we could check if any programs
out there are using this member unreasonably.

Cheers,

Alex
-- 
<http://www.alejandro-colomar.es/>
GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Bad interaction between signal constants and -Wsign-conversion

[Vincent Lefevre]

On 2023-03-30 16:09:12 +0200, Alejandro Colomar wrote:
> On 3/30/23 15:59, Vincent Lefevre wrote:
> > But concerning struct sigaction in particular, shouldn't this be fixed
> > in POSIX?
> 
> IMHO, POSIX, and in general, standards, should follow implementations
> rather than lead them.  I think changes should start as vendor
> extensions.

However, there could be a discussion concerning POSIX at the same time.

> For example, timespec::tv_nsec was mandated to be long by POSIX and
> C11, but glibc used 'long long' in some cases, because it was
> necessary.  I would say using 'unsigned int' here would be a nice GNU
> extension, and if it proves its value, POSIX could later pick it.
> 
> However, as Zack said, I'm not sure how we could check if any programs
> out there are using this member unreasonably.

But on the opposite, it is possible that some programs assume that
flags are positive and these programs have not been tested with a
negative flag. So they could already be buggy, and an unsigned int
could fix them.

BTW, some systems seem to have an unsigned sa_flags:

  https://github.com/github/git-msysgit/blob/master/compat/mingw.h
  https://www.google.com/search?q=%22unsigned+long+sa_flags%22
  https://www.google.com/search?q=%22unsigned+int+sa_flags%22

-- 
Vincent Lefèvre <vincent@vinc17.net> - Web: <https://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)