Fix critical/high-severity correctness and security bugs found in an audit - #432
Draft
franv314 wants to merge 7 commits into
Draft
Fix critical/high-severity correctness and security bugs found in an audit#432franv314 wants to merge 7 commits into
franv314 wants to merge 7 commits into
Conversation
mono/node treat the first non-option token as the program to load, but runtime_args() appended the executable/script path after the user's own arguments instead of before them. As soon as any argv is passed - which task-maker's own checkers, generators, validators and Terry solutions always do - mono/node try to load the first argument as the program and fail immediately, making every C#/JS checker, generator, validator or Terry solution non-functional.
franv314
marked this pull request as draft
August 26, 2026 18:58
CacheEntry::is_compatible() computed self.extra_memory - extra_memory (both u64) assuming the cached value is never smaller than the query's, but --extra-memory legitimately varies between runs. When it doesn't hold, the subtraction underflows and (in release builds, with no overflow-checks) corrupts the following limit comparison, letting a stale, incompatible cache entry be served for a grading run. Fixed by comparing left + left_extra > right + right_extra directly instead of pre-computing a delta, which works regardless of which side is larger. Also fixes the extra_readable_dirs check, which only rejected a strict superset relationship; two differing, non-nested sets of granted directories (neither a subset of the other) were incorrectly treated as compatible even though the cached run could have depended on access the new run doesn't grant.
While resolving a subtask's included groups, testcase ids were appended for every group visited and only sorted afterwards, never deduplicated. A testcase reachable through more than one included group (its own group and a group_name override both ending up in the same subtask) ended up listed twice in subtask.testcases. Since Sum aggregation divides by the length of that list, the duplicate silently doubled that testcase's weight in the subtask's average score.
FileStoreIndex::flush() popped candidates from a BinaryHeap ordered by last_access, but BinaryHeap is a max-heap, so pop() returned the *most* recently used entry first - the opposite of the documented "least recently used" eviction contract. Whenever the store needed to reclaim space with more than one unlocked candidate, it deleted the most recently produced or reused files while ancient, untouched ones survived, defeating the cache and causing avoidable recomputation. Fixed by wrapping heap entries in Reverse so pop() returns the oldest entry first. Added a regression test with explicit, well-separated timestamps set directly on the index entries; the existing tests weren't sensitive enough to catch this (they still pass against the old, unfixed logic too - likely because their back-to-back SystemTime::now() calls don't reliably produce distinct timestamps, falling back to an incidental tie-break).
The javac/jar invocation was built as a single shell string with the binary name and main class spliced in unescaped, then run through sh -c. A source filename containing shell metacharacters (backticks, \$(...), ;) could execute arbitrary commands during compilation; this is reachable over the network through the eval_server tool, whose filename validation doesn't reject shell metacharacters or spaces. Even without malice, a legitimate filename containing a space broke compilation. Every other compiled language builds an argv array instead of a shell string and was unaffected. Fixed by single-quoting both interpolated values before splicing them into the shell command (the glob-based javac/jar invocation itself still needs a shell, so it can't be switched to a plain argv command).
print_subtasks() indexed evaluation_results[testcase] with a usize taken directly from the checker's own JSON stdout, with no bounds check anywhere in the pipeline - unlike the safe .zip() used a few lines above for the analogous feedback loop. Any Terry checker that reports a subtask testcase index past the end of the real testcase list crashed the whole evaluation after every execution had already finished, instead of surfacing a diagnostic.
first_chunk is only populated for non-empty chunks, so it stays None for a zero-byte output file, but the check unconditionally unwrapped it once it decided the outputs were identical. A subtask whose testcases all have a byte-identical empty official output - a legitimate "print nothing" task, or exactly the broken-generator situation this check exists to catch - made the sanity-check pass panic instead of warning about it.
franv314
force-pushed
the
fix/audit-critical-high
branch
from
August 26, 2026 21:16
b3a0e70 to
df4604d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A focused audit of the codebase surfaced 23 issues; this PR fixes the 8 rated critical/high severity (the remaining medium/low findings, including one described below, are left for a follow-up):
task-maker-lang):runtime_args()appended the executable/script path after the user's own arguments instead of before, somono/nodetry to load the wrong thing as soon as any argv is passed - which task-maker's own checkers/generators/validators/Terry solutions always do. Verified against realmono/nodebinaries: old order fails with "file not found" /MODULE_NOT_FOUND, new order works.u64underflow in cache compatibility check (task-maker-cache):self.extra_memory - extra_memorycould underflow when--extra-memorydiffers between the cached and current run, corrupting the following limit comparison and letting a stale cache entry be served.gen.tomlsubtasks (task-maker-format): a testcase reachable through two included groups was counted twice, silently doubling its weight inSum-aggregated subtask scores.task-maker-store):flush()used aBinaryHeap(max-heap) and evicted the most recently used files first instead of the least recently used, defeating the file-store cache.task-maker-lang): thejavac/jarinvocation spliced the binary name and main class unescaped into ash -cstring; reachable via the network-facingeval_servertool.task-maker-format, Terry): a checker reporting a subtask testcase index past the end of the real list crashed the whole evaluation.AllOutputsEqualon empty outputs (task-maker-format, IOI): a subtask whose testcases all have byte-identical empty outputs crashed the exact sanity check meant to catch that situation.extra_readable_dirs(task-maker-cache): the cache compatibility check only rejected a strict superset relationship, missing the case of two differing, non-nested sets of granted directories.Each fix has a dedicated commit with a full explanation; see individual commit messages for details, root cause, and trigger scenario.
Dropped from this PR after further investigation
An earlier revision of this PR also included a fix for a theoretical race condition in
ControllerKeeper::run()(task-maker-exec), where reserving a result slot and writing the corresponding config to the controller's stdin were two independently-locked steps. On paper, concurrent solution starts could have their results/pids cross-wired. However, tracing (and empirically confirming with real timing instrumentation) the only caller,sandbox_manager, showed it always blocks onwait_for_pipes()after eachSTART_SOLUTIONrequest - which can't return until the correspondingrun()call has already completed its critical section - so the two-thread overlap this would require never actually occurs, regardless of how the controller program itself behaves. The underlying code pattern is still fragile (relies on an implementation detail of its sole caller rather than an explicit invariant), but it's not an active bug, so it's been reclassified as low severity and removed from this PR.Test plan
cargo build --workspaceandcargo test --workspace --lib(all green)cargo clippy --workspace --all-targets -- -D warnings(clean)cargo fmt --check(clean)mono/nodebinaries