Skip to content
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.

Sign up for GitHub

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

Jump to bottom

(minor) podman kill: should do input validation #4746

Closed
edsantiago opened this issue Dec 23, 2019 · 1 comment · Fixed by #4749
Closed

(minor) podman kill: should do input validation #4746

edsantiago opened this issue Dec 23, 2019 · 1 comment · Fixed by #4749
Labels
Good First Issue This issue would be a good issue for a first time contributor to undertake. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.

Comments

@edsantiago
Copy link
Collaborator

Looks like podman is accepting any int, casting to ulong, and passing it along:

# podman run -d --name foo alpine sleep 60
5eafd476443109ab52af53cc68a08e81f3eb7a2249c81993193b01590cd1d5b2
# podman kill -s -1 foo
ERRO[0000] unknown signal "18446744073709551615"
unknown signal "18446744073709551615"
Error: error sending signal to container 5eafd476443109ab52af53cc68a08e81f3eb7a2249c81993193b01590cd1d5b2: `/usr/bin/runc kill 5eafd476443109ab52af53cc68a08e81f3eb7a2249c81993193b01590cd1d5b2 18446744073709551615` failed: exit status 1
# podman kill -s 999 foo
ERRO[0000] container_linux.go:389: signaling init process caused "invalid argument"
container_linux.go:389: signaling init process caused "invalid argument"
Error: error sending signal to container 5eafd476443109ab52af53cc68a08e81f3eb7a2249c81993193b01590cd1d5b2: `/usr/bin/runc kill 5eafd476443109ab52af53cc68a08e81f3eb7a2249c81993193b01590cd1d5b2 999` failed: exit status 1

------
# [ expected something more like ]:
Error: Invalid signal: -1 (or 999)

Would it make sense to validate against SIGRTMAX before accepting it?

And, for the sake of discussion, I feel it might be a nice courtesy to accept a signed int and run abs() on it. Those of us with fingers trained to type kill -N would appreciate it.

Oh, also, kill -s 0 throws an error (invalid signal). This is what docker does, so we might need to keep it for compatibility, but it could also be nice to replicate standard kill -0 behavior (exit 0 if container exists).
@rhatdan rhatdan added the Good First Issue This issue would be a good issue for a first time contributor to undertake. label Dec 24, 2019
@rhatdan
Copy link
Member

rhatdan commented Dec 24, 2019

I like to get a fix on all of these.

Anyone out there looking for a quick git project should grab this.

edsantiago added a commit to edsantiago/libpod that referenced this issue Dec 26, 2019
@edsantiago
signal parsing - better input validation
64e0d1a 
The helper function we use for signal name mapping does not
check for negative numbers nor invalid (too-high) ones. This
can yield unexpected error messages:

   # podman kill -s -1 foo
   ERRO[0000] unknown signal "18446744073709551615"

This PR introduces a small wrapper for it that:

  1) Strips off a leading dash, allowing '-1' or '-HUP'
     as valid inputs; and
  2) Rejects numbers <1 or >64 (SIGRTMAX)

Also adds a test suite checking signal handling as well as
ensuring that invalid signals are rejected by the command line.

Fixes: containers#4746

Signed-off-by: Ed Santiago <santiago@redhat.com>
edsantiago added a commit to edsantiago/libpod that referenced this issue Dec 26, 2019
@edsantiago
signal parsing - better input validation
9d2aa9c 
The helper function we use for signal name mapping does not
check for negative numbers nor invalid (too-high) ones. This
can yield unexpected error messages:

   # podman kill -s -1 foo
   ERRO[0000] unknown signal "18446744073709551615"

This PR introduces a small wrapper for it that:

  1) Strips off a leading dash, allowing '-1' or '-HUP'
     as valid inputs; and
  2) Rejects numbers <1 or >64 (SIGRTMAX)

Also adds a test suite checking signal handling as well as
ensuring that invalid signals are rejected by the command line.

Fixes: containers#4746

Signed-off-by: Ed Santiago <santiago@redhat.com>
@edsantiago edsantiago mentioned this issue Dec 26, 2019
Merged
edsantiago added a commit to edsantiago/libpod that referenced this issue Dec 26, 2019
@edsantiago
signal parsing - better input validation
a10269d 
The helper function we use for signal name mapping does not
check for negative numbers nor invalid (too-high) ones. This
can yield unexpected error messages:

   # podman kill -s -1 foo
   ERRO[0000] unknown signal "18446744073709551615"

This PR introduces a small wrapper for it that:

  1) Strips off a leading dash, allowing '-1' or '-HUP'
     as valid inputs; and
  2) Rejects numbers <1 or >64 (SIGRTMAX)

Also adds a test suite checking signal handling as well as
ensuring that invalid signals are rejected by the command line.

Fixes: containers#4746

Signed-off-by: Ed Santiago <santiago@redhat.com>
edsantiago added a commit to edsantiago/libpod that referenced this issue Dec 26, 2019
@edsantiago
signal parsing - better input validation
40f55ca 
The helper function we use for signal name mapping does not
check for negative numbers nor invalid (too-high) ones. This
can yield unexpected error messages:

   # podman kill -s -1 foo
   ERRO[0000] unknown signal "18446744073709551615"

This PR introduces a small wrapper for it that:

  1) Strips off a leading dash, allowing '-1' or '-HUP'
     as valid inputs; and
  2) Rejects numbers <1 or >64 (SIGRTMAX)

Also adds a test suite checking signal handling as well as
ensuring that invalid signals are rejected by the command line.

Fixes: containers#4746

Signed-off-by: Ed Santiago <santiago@redhat.com>
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 23, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Good First Issue This issue would be a good issue for a first time contributor to undertake. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

signal parsing - better input validation edsantiago/libpod
2 participants
@edsantiago @rhatdan