node
node copied to clipboard
nodejs
add missing sigemptyset() to init sigset_t
Checklist
- [X]
make -j4 test(UNIX), orvcbuild test(Windows) passes - [ ] commit message follows commit guidelines
This conforms to posix saying that sigset_t cannot be initialized through memset but rather sigemptyset() or sigfillset()
In struct sigaction , sa_handler is being set explicitly and now sa_mask is being set via sigemptyset(), so memset() really only inits sa_flags to 0. So memset() could be replaced by sa_flags = 0 which should be more readable.
This conforms to posix saying that sigset_t cannot be initialized through memset
Where in the spec does it say that?
Leaving out the memset() leaves other fields uninitialized. That's probably going to cause trouble sooner or later.
(I fixed a bug to that effect years ago but I can't find the commit anymore.)
Where in the spec does it say that?
I was refering to the book "The Linux programming interface". Relevent quotes:
On Linux, as on most UNIX implementations, the sigset_t data type is a bit mask. However, SUSv3 doesn’t require this. A signal set could conceivably be represented using some other kind of structure. SUSv3 requires only that the type of sigset_t be assignable. [...] One of sigemptyset() or sigaddset() must be used to initialize a signal set. [...] the initialization of static variables to 0 can’t portably be relied upon as indicating an empty signal set, since signal sets may be implemented using structures other than bit masks. (For the same reason, it is incorrect to use memset(3) to zero the contents of a signal set in order to mark it as empty.)
Leaving out the memset() leaves other fields uninitialized. That's probably going to cause trouble sooner or later.
sigaction only has 3 members (for application usage) and I've made sure these are always initialized. This is made the same way as examples in mentioned book.
Ah, okay. I wouldn't equate TLPI with POSIX but POSIX.2008 mentions that:
[..] it would be reasonable to initialize part of the structure, such as a version field, to permit binary-compatibility between releases where the size of the set varies. For such reasons, either sigemptyset() or sigfillset() must be called prior to any other use of the signal set
So okay, from a correctness perspective, sigemptyset() is an improvement over memset().
This however:
sigaction only has 3 members (for application usage) and I've made sure these are always initialized.
Assumes that struct sigaction is the same everywhere but that's not necessarily true. POSIX only describes greatest common denominator behavior. I'd like to see memset() + sigemptyset().
Assumes that
struct sigactionis the same everywhere but that's not necessarily true. POSIX only describes greatest common denominator behavior. I'd like to see memset() + sigemptyset().
That is pretty unusual honestly, seems "overdoing" to me, but I won't argue if it gets the upper vote.
The same can be said about changing memset() to sigemptyset(). There's no platform we support where it makes a functional difference.
Not properly initializing all struct sigaction fields on the other hand has been the cause of at least one bug in the past.
This however:
sigaction only has 3 members (for application usage) and I've made sure these are always initialized.
Assumes that
struct sigactionis the same everywhere but that's not necessarily true. POSIX only describes greatest common denominator behavior. I'd like to see memset() + sigemptyset().
If it contains additional fields, then how do you know that 0 is the right value for them? Unless posix says to initialize them to 0 if not used, then such implementation would be non-conformant. However I'll restore memsets if you're willing to.
There's no platform we support where it makes a functional difference.
We support right now + implementations are free to change this (not that it's very likely but save than sorry story).
I wouldn't equate TLPI with POSIX
Actually TLPI just quotes sus so ~ posix.
If it contains additional fields, then how do you know that 0 is the right value for them?
I'd say the odds are > 50% that 0 is more likely to do the right thing than whatever random value is on the stack at the time of the call. :-)
If nothing else, it stops tools like valgrind from overzealously complaining about uninitialized padding.
I would say finally let s go with memset + sigemptyset @bnoordhuis made a valid point.
@mcpiroman would you be so kind and rebase and force push instead of merging? Our CI does not work otherwise. You can do that along of these commands:
git fetch upstream master
git rebase -i upstream/master
git push --force-with-lease
upstream depends on your settings.
heya @mcpiroman, any chance you'd be able to address the above requests? Would love to get this merged if possible.
@bnb Not soon, at best after 3 weeks. But github now allows 'changes from the maintainers' so trival changes you may attempt to do yourself.
@mcpiroman can you have a look to make sure the rebase I made aligns with what you had in mind for this PR? I'd like to have at least another pair of eyes validating the code before merging in case I made a mistake.
Otherwise LGTM, though I it looks like force push covered my original commit and I don't remember that closely how it looked.
Your original commits were f9c16b87eff60858efa0b6977fa1d253be538589...905dcc90a7e5e9cad3cf8db72bfcede4ba66e975.