Conversation
mtrmac
left a comment
There was a problem hiding this comment.
The changes here LGTM.
Note that this by no means handles everything from #582 , e.g. see the example below. But, also, I’m not going to be able to follow up on everything there, so the resolution, if any, is entirely up to you.
src/conmon.c
Outdated
| pexit("seccomp notify socket specified without any plugin"); | ||
| seccomp_listener = setup_seccomp_socket(opt_seccomp_notify_socket); | ||
| #else | ||
| pexit("seccomp support not present"); |
There was a problem hiding this comment.
E.g. this is still printing an errno-derived value.
8ec43cd to
769b735
Compare
|
Ephemeral COPR build failed. @containers/packit-build please check. |
882bca5 to
e7c5880
Compare
|
Thanks, I just replaced the |
src/cli.c
Outdated
| "Path to the socket where the seccomp notification fd is received", NULL}, | ||
| {"seccomp-notify-plugins", 0, 0, G_OPTION_ARG_STRING, &opt_seccomp_notify_plugins, | ||
| "Plugins to use for managing the seccomp notifications", NULL}, | ||
| {"seccomp-notify-allowed", 0, 0, G_OPTION_ARG_NONE, &opt_seccomp_notify_allowed, "Allow seccomp notify annotations", NULL}, |
There was a problem hiding this comment.
Does the man page need updating for the new option?
(If this didn’t work for … years, my first instinct would be to delete the code wholesale, but I know nothing of the history.)
There was a problem hiding this comment.
I don't know much about the history either. @giuseppe ?
There was a problem hiding this comment.
Anyway, I'd prefer to keep it for now unless @giuseppe has different opinion - I added the --seccomp-notify-allowed option to the man page.
There was a problem hiding this comment.
this was added to experiment with crun-style seccomp notification plugins: https://github.com/containers/crun/blob/main/contrib/seccomp-notify-plugin-example/full.c
Not sure if anyone is using it, we didn't follow up to expose it in Podman because using them is quite difficult (i.e. requires a binary handler)
There was a problem hiding this comment.
but why do we need opt_seccomp_notify_allowed?
If they are specified in opt_seccomp_notify_plugins we use them, otherwise not. Why another bool?
If anything, it should be a separate commit
There was a problem hiding this comment.
Ok, I removed the seccomp-notify-allowed as opt_seccomp_notify_plugins already handles that.
- Fix strncpy missing null termination in conn_sock.c - Replace malloc with g_malloc for consistency in ctr_exit.c - Add bounds validation for sscanf parsing in ctrl.c - Fix signal safety: change volatile pid_t to sig_atomic_t - Add null check for cgroup2_path before use - Clean up outdated TODO/FIXME comments These changes address potential buffer safety, memory allocation consistency, signal safety, and logic issues flagged during code review. Fixes: containers#582 Signed-off-by: Jindrich Novy <jnovy@redhat.com>
|
I removed all seccomp related bits from this PR and filed issue #591 to address seccomp related fixes separately. |
These changes address potential buffer safety, memory allocation consistency, signal safety, and logic issues flagged during code review.
Fixes: #582