Skip to content

Validate the container id and invoke runc without a shell in reap-oci-container - #9008

Open
reiabreu wants to merge 3 commits into
masterfrom
worker-launcher-reap-oci-container
Open

Validate the container id and invoke runc without a shell in reap-oci-container#9008
reiabreu wants to merge 3 commits into
masterfrom
worker-launcher-reap-oci-container

Conversation

@reiabreu

Copy link
Copy Markdown
Contributor

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. Adds test_validate_container_id.

…-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 rzo1 added this to the 3.1.0 milestone Aug 23, 2026

@rzo1 rzo1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
@reiabreu

Copy link
Copy Markdown
Contributor Author

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.

  • 1 (--): cleanup_oci_container now execs runc delete -- <id>, so an option-shaped id can't be parsed as a flag.
  • 2 (test nit / coverage): Replaced the 24-char case (rejected by the length check) with a 40-char invalid-charset case that actually exercises the character check. cleanup_oci_container and the main.c rejection branch still aren't run because main.c isn't linked into the test binary — left for a separate change.

@reiabreu

Copy link
Copy Markdown
Contributor Author

Following up on the test-coverage point: cleanup_oci_container and the main.c reject branch aren't run because main.c isn't linked into the test binary. I'll address this in a single worker-launcher test-coverage follow-up PR once these PRs are on master (extracting the dispatch logic into a test-linked source), together with the parse-level tests noted on #9010.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants