-
Notifications
You must be signed in to change notification settings - Fork 7.9k
ext/posix: posix_kill() process_id range check. #18944
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
before going into a pid_t handling "killing spree" all over the extension, let me know if this is something wishable. |
996cc69
to
e9402c6
Compare
cc @Girgias |
I think this makes sense, as anything outside of this range seems to point to a bug |
a84a097
to
c540a26
Compare
What actually happens on -2 at the moment? |
hmmm looking at it now, negative numbers < -1 might be relevant cases (for kill), they normally aim for process groups in this case e.g. -2 -> process group 2 but will have a look later. |
36f9542
to
bc69cef
Compare
pid_t is, for the most part, represented by a signed int, by overflowing it, we end up being in the -1 case which affect all accessible processes.
bc69cef
to
a94236c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you going to do the same for group ids, etc.?
Also UPGRADING entry :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah breaking group ids would be an issue so it cannot get merged untill this is sorted.
I might ... we ll see |
What s the issue @bukka ? would any future plans be a blocker ? |
Oh you updated it to min int. Sorry missed that part. That's fine then. |
pid_t is, for the most part, represented by a signed int, by overflowing it, we end up being in the -1 case which affect all accessible processes.