Validate the container id and invoke runc without a shell in reap-oci-container - #9008
Validate the container id and invoke runc without a shell in reap-oci-container#9008reiabreu wants to merge 3 commits into
Conversation
…-container reap-oci-container now checks the container id with validate_container_id (hex digits and dashes, 36-42 chars) before it is used. validate_container_id is exported from oci_launch_cmd.c. cleanup_oci_container runs "runc delete" via fork/execv with an explicit argument vector instead of system(). Adds test_validate_container_id. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
profile-docker-container now checks the worker id with validate_container_id before get_docker_container_pid builds the docker command line, and get_docker_container_pid returns pid -1 instead of dereferencing a NULL stream when popen fails. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
rzo1
left a comment
There was a problem hiding this comment.
Currently low on time, thus a LLM based review.
Direction is good (dropping the shell is the right call). Two minor points:
1. A validated container id can still begin with -, and it's passed to runc delete without a -- separator (minor).
validate_container_id checks only length (36–42) and charset (all_uuid_digit = hex + -), with no positional/UUID-structure check, so an all-dash or leading-- string passes. In cleanup_oci_container the child execs { runc_path, "delete", container_id, NULL } — runc's CLI parser then treats a leading-dash id as a flag rather than the positional argument. No harmful flag is spellable in [0-9a-fA-F-] at that length, so it fails safe (misparsed/failed delete, rc=1) rather than escalating, but it doesn't fully deliver the "safe on the runc command line" goal. Inserting -- before container_id, or rejecting ids whose first char is -, closes the gap. (Same option-shaped ids also reach docker inspect ... <worker_id>, which likewise fails safely to pid=-1.)
2. Tests cover only the validator, not the branches the fix adds (minor).
test_validate_container_id() exercises the validator in isolation, but the substance of the PR — the new rejection branches in main.c and replacing system() with fork/execv in cleanup_oci_container — isn't tested: main.c isn't linked into the test binary and no test invokes cleanup_oci_container, so reverting either would still pass the whole suite. Minor nit: the "6702-x nope; other stuff" case is 24 chars and is rejected by the length check, not the character check — only the slash case actually exercises all_uuid_digit.
…r check at a valid length validate_container_id accepts a leading dash, so end runc option parsing with -- before the id in the delete argument vector. The test string that stood in for a disallowed-character id was short enough to be rejected on length; replace it with a 40-character one so it exercises the character check. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thank you for the feedback, @rzo1. I'm afraid I haven't touched C in many years, so I relied entirely on Claude for these fixes.
|
|
Following up on the test-coverage point: |
reap-oci-container now checks the container id with validate_container_id (hex digits and dashes, 36-42 chars) before it is used. validate_container_id is exported from oci_launch_cmd.c. cleanup_oci_container runs "runc delete" via fork/execv with an explicit argument vector instead of system().
How this was tested
Built the worker-launcher native tree with
autoreconf -i && ./configure && make check(autotools, compiled with-Werror); the test suite passes. Addstest_validate_container_id.