Fix critical/high-severity correctness and security bugs found in an audit

task-maker-cache/src/entry.rs

@@ -150,30 +150,53 @@ impl CacheEntry {
     /// Checks whether a given execution is compatible with the limits stored in this entry. See the
     /// docs of the crate for the definition of _compatible_.
     pub fn is_compatible(&self, group: &ExecutionGroup) -> bool {
-        // makes sure that $left <= $right where None = inf
-        // if $left is less restrictive than $right, return false
+        // makes sure that $left + $left_extra <= $right + $right_extra, where None = inf.
+        // The two extra amounts are added separately (rather than pre-subtracted into a single
+        // delta) so this works for unsigned limits too: $left_extra and $right_extra can each
+        // legitimately be larger than the other, which would underflow a `u64` subtraction.
+        // If $left is less restrictive than $right, return false.
         macro_rules! check_limit {
-            ($left:expr, $right:expr, $extra_time:expr) => {
+            ($left:expr, $right:expr, $left_extra:expr, $right_extra:expr) => {
                 match ($left, $right) {
-                    (Some(left), Some(right)) if left + $extra_time > right => {
+                    (Some(left), Some(right)) if left + $left_extra > right + $right_extra => {
                         return false;
                     }
                     (None, Some(_)) => return false,
                     _ => {}
                 }
             };
         }
-        // will return false if $less is less restrictive of $right
+        // will return false if $left is less restrictive of $right
         macro_rules! check_limits {
-            ($left:expr, $right:expr, $extra_time:expr, $extra_memory:expr) => {
-                check_limit!($left.cpu_time, $right.cpu_time, $extra_time);
-                check_limit!($left.sys_time, $right.sys_time, $extra_time);
-                check_limit!($left.wall_time, $right.wall_time, $extra_time);
-                check_limit!($left.memory, $right.memory, $extra_memory);
-                check_limit!($left.nofile, $right.nofile, 0);
-                check_limit!($left.fsize, $right.fsize, 0);
-                check_limit!($left.memlock, $right.memlock, 0);
-                check_limit!($left.stack, $right.stack, 0);
+            ($left:expr, $right:expr, $left_extra_time:expr, $right_extra_time:expr, $left_extra_memory:expr, $right_extra_memory:expr) => {
+                check_limit!(
+                    $left.cpu_time,
+                    $right.cpu_time,
+                    $left_extra_time,
+                    $right_extra_time
+                );
+                check_limit!(
+                    $left.sys_time,
+                    $right.sys_time,
+                    $left_extra_time,
+                    $right_extra_time
+                );
+                check_limit!(
+                    $left.wall_time,
+                    $right.wall_time,
+                    $left_extra_time,
+                    $right_extra_time
+                );
+                check_limit!(
+                    $left.memory,
+                    $right.memory,
+                    $left_extra_memory,
+                    $right_extra_memory
+                );
+                check_limit!($left.nofile, $right.nofile, 0, 0);
+                check_limit!($left.fsize, $right.fsize, 0, 0);
+                check_limit!($left.memlock, $right.memlock, 0, 0);
+                check_limit!($left.stack, $right.stack, 0, 0);
                 if $left.allow_multiprocess > $right.allow_multiprocess {
                     return false;
                 }
@@ -187,9 +210,10 @@ impl CacheEntry {
                     $left.extra_readable_dirs.iter().cloned().collect();
                 let right_readable_dirs: HashSet<PathBuf> =
                     $right.extra_readable_dirs.iter().cloned().collect();
-                if left_readable_dirs != right_readable_dirs
-                    && left_readable_dirs.is_superset(&right_readable_dirs)
-                {
+                // Incompatible as soon as $left was granted access to anything $right doesn't
+                // grant, not just when $left is a strict superset of $right: two differing,
+                // non-nested sets (e.g. left={A,B}, right={B,C}) are just as incompatible.
+                if !left_readable_dirs.is_subset(&right_readable_dirs) {
                     return false;
                 }
             };
@@ -203,17 +227,21 @@ impl CacheEntry {
                     check_limits!(
                         item.limits,
                         exec.limits,
-                        self.extra_time - extra_time,
-                        self.extra_memory - extra_memory
+                        self.extra_time,
+                        extra_time,
+                        self.extra_memory,
+                        extra_memory
                     );
                 }
                 _ => {
                     // require that the new limits are more restrictive
                     check_limits!(
                         exec.limits,
                         item.limits,
-                        extra_time - self.extra_time,
-                        extra_memory - self.extra_memory
+                        extra_time,
+                        self.extra_time,
+                        extra_memory,
+                        self.extra_memory
                     );
                 }
             }
@@ -230,8 +258,8 @@ mod tests {
     use std::path::{Path, PathBuf};
 
     use task_maker_dag::{
-        Execution, ExecutionCommand, ExecutionDAGConfig, ExecutionResourcesUsage, ExecutionResult,
-        ExecutionStatus,
+        Execution, ExecutionCommand, ExecutionDAGConfig, ExecutionGroup, ExecutionResourcesUsage,
+        ExecutionResult, ExecutionStatus,
     };
     use task_maker_store::{FileStore, FileStoreHandle, FileStoreKey, ReadFileIterator};
 
@@ -468,4 +496,46 @@ mod tests {
         exec2.limits.read_only = true;
         assert!(entry.is_compatible(&exec2.into()));
     }
+
+    #[test]
+    fn test_compatible_success_extra_memory_smaller_than_query_does_not_underflow() {
+        // Cached with no extra memory bonus at all, queried with the (larger) default one:
+        // `self.extra_memory - extra_memory` used to underflow the `u64` subtraction here.
+        let (mut entry, mut exec) = empty_entry();
+        entry.items[0].result.status = ExecutionStatus::Success;
+        entry.items[0].limits.memory = Some(1000);
+        entry.extra_memory = 0;
+        exec.limits.memory = Some(1000);
+        // exec keeps the default (larger) extra_memory from `ExecutionDAGConfig::new()`.
+        assert!(entry.is_compatible(&exec.into()));
+    }
+
+    #[test]
+    fn test_compatible_fail_extra_memory_larger_than_query_does_not_underflow() {
+        // Cached with the (larger) default extra memory bonus, queried with none: the fail-branch
+        // `extra_memory - self.extra_memory` used to underflow here.
+        let (mut entry, mut exec) = empty_entry();
+        entry.items[0].result.status = ExecutionStatus::ReturnCode(1);
+        entry.items[0].limits.memory = Some(1000);
+        exec.limits.memory = Some(1000);
+        let mut group: ExecutionGroup = exec.into();
+        group.config.extra_memory = 0;
+        assert!(entry.is_compatible(&group));
+    }
+
+    #[test]
+    fn test_compatible_extra_readable_dirs_incomparable_sets() {
+        // Neither a subset nor a superset of the other: `left` (cached) was granted `/a`, which
+        // `right` (queried) doesn't grant, even though `right` grants `/c` which `left` lacks.
+        let (mut entry, mut exec) = empty_entry();
+        entry.items[0].result.status = ExecutionStatus::Success;
+        entry.items[0]
+            .limits
+            .extra_readable_dirs
+            .extend([PathBuf::from("/a"), PathBuf::from("/b")]);
+        exec.limits
+            .extra_readable_dirs
+            .extend([PathBuf::from("/b"), PathBuf::from("/c")]);
+        assert!(!entry.is_compatible(&exec.into()));
+    }
 }

task-maker-format/src/ioi/format/italian_toml/gen_toml.rs

@@ -467,6 +467,10 @@ pub(super) fn parse(
         }
 
         testcases.sort();
+        // A testcase can be reachable through more than one included group (e.g. its own
+        // group and a `group_name` override both ending up in the same subtask); without
+        // deduplicating it would be counted twice when averaging the subtask's score.
+        testcases.dedup();
         testcases_owned.sort();
 
         subtasks.insert(

task-maker-format/src/ioi/sanity_checks/subtasks.rs

@@ -164,6 +164,23 @@ impl OutputHasher {
     }
 }
 
+/// Describe, for a diagnostic note, what a group of identical outputs start with.
+///
+/// `first_chunk` is `None` when the (identical) outputs are all empty, e.g. a "print nothing"
+/// task or a broken generator producing empty output everywhere; that must not be treated the
+/// same as "the chunk wasn't valid UTF-8" (in which case there's simply nothing to show).
+fn describe_common_prefix(first_chunk: Option<&[u8]>) -> Option<String> {
+    match first_chunk {
+        None => Some("They are all empty".to_string()),
+        Some(chunk) => std::str::from_utf8(chunk).ok().map(|contents| {
+            format!(
+                "They all start with: {}",
+                contents.chars().take(20).join("")
+            )
+        }),
+    }
+}
+
 #[derive(Debug, Default)]
 pub struct AllOutputsEqual {
     outputs: Arc<Mutex<HashMap<SubtaskId, Vec<TestcaseOutput>>>>,
@@ -221,9 +238,8 @@ impl SanityCheck for AllOutputsEqual {
                 let message = format!("All outputs for subtask {id}{name} are identical");
 
                 let mut diag = Diagnostic::warning(message);
-                if let Ok(contents) = std::str::from_utf8(first.first_chunk.as_ref().unwrap()) {
-                    let contents = contents.chars().take(20).join("");
-                    diag = diag.with_note(format!("They all start with: {contents}"));
+                if let Some(note) = describe_common_prefix(first.first_chunk.as_deref()) {
+                    diag = diag.with_note(note);
                 }
                 eval.add_diagnostic(diag)?;
             }
@@ -233,6 +249,34 @@ impl SanityCheck for AllOutputsEqual {
     }
 }
 
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_describe_common_prefix_empty_outputs_does_not_panic() {
+        // Regression test: used to `.unwrap()` a `None` here and panic instead of
+        // reporting that the (identical, empty) outputs are all empty.
+        assert_eq!(
+            describe_common_prefix(None),
+            Some("They are all empty".to_string())
+        );
+    }
+
+    #[test]
+    fn test_describe_common_prefix_text_content() {
+        assert_eq!(
+            describe_common_prefix(Some(b"hello world, this is long")),
+            Some("They all start with: hello world, this is".to_string())
+        );
+    }
+
+    #[test]
+    fn test_describe_common_prefix_non_utf8_content() {
+        assert_eq!(describe_common_prefix(Some(&[0xff, 0xfe])), None);
+    }
+}
+
 /// Check if any subtask uses the deprecated `description` field.
 #[derive(Debug, Default)]
 pub struct DeprecatedDescriptionInSubtask;

task-maker-format/src/terry/finish_ui.rs

@@ -213,10 +213,13 @@ impl FinishUI {
             print!(" | ");
 
             for &testcase in &subtask.testcases {
-                if evaluation_results[testcase].correct {
-                    cwrite!(self, GREEN, "{} ", testcase);
-                } else {
-                    cwrite!(self, RED, "{} ", testcase);
+                // `testcase` is a checker-reported index (from the solution's own JSON stdout),
+                // not something task-maker validated; a buggy checker can report an index past
+                // the end of `evaluation_results`, so don't let that panic the whole evaluation.
+                match evaluation_results.get(testcase) {
+                    Some(result) if result.correct => cwrite!(self, GREEN, "{} ", testcase),
+                    Some(_) => cwrite!(self, RED, "{} ", testcase),
+                    None => cwrite!(self, YELLOW, "{}? ", testcase),
                 }
             }
 

task-maker-lang/src/languages/csharp.rs

@@ -68,7 +68,9 @@ impl Language for LanguageCSharp {
         write_to: Option<&Path>,
         mut args: Vec<String>,
     ) -> Vec<String> {
-        args.push(
+        // will run for example: mono program.exe args...
+        args.insert(
+            0,
             self.executable_name(path, write_to)
                 .to_string_lossy()
                 .to_string(),
@@ -83,3 +85,21 @@ impl Language for LanguageCSharp {
             .allow_multiprocess();
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_runtime_args_executable_comes_first() {
+        // mono requires the assembly to load to be the first non-option argument;
+        // regression test for it being appended after the program's own args instead.
+        let lang = LanguageCSharp::new();
+        let args = lang.runtime_args(
+            Path::new("solution.cs"),
+            None,
+            vec!["input".into(), "output".into()],
+        );
+        assert_eq!(args, vec!["solution", "input", "output"]);
+    }
+}

task-maker-lang/src/languages/java.rs

@@ -52,6 +52,11 @@ impl Language for LanguageJava {
             .to_string_lossy()
             .to_string();
 
+        // binary_name and main_class are interpolated into a shell command (needed for the
+        // `*.java`/`*.class` globs); quote them so a source filename with spaces or shell
+        // metacharacters can't break or inject into the compilation command.
+        let binary_name = shell_quote(&binary_name);
+        let main_class = shell_quote(&main_class);
         metadata.add_arg("-c").add_arg(format!(
             "javac -encoding UTF-8 -d . *.java && jar cfe {binary_name} {main_class} *.class"
         ));
@@ -92,3 +97,23 @@ impl Language for LanguageJava {
             .add_extra_readable_dir("/etc");
     }
 }
+
+/// Quotes a string for safe, literal inclusion inside single quotes in a POSIX shell command.
+fn shell_quote(s: &str) -> String {
+    format!("'{}'", s.replace('\'', r"'\''"))
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_shell_quote_neutralizes_metacharacters() {
+        assert_eq!(shell_quote("solution"), "'solution'");
+        assert_eq!(
+            shell_quote("a$(touch /tmp/pwned).java"),
+            r"'a$(touch /tmp/pwned).java'"
+        );
+        assert_eq!(shell_quote("it's"), r"'it'\''s'");
+    }
+}

task-maker-lang/src/languages/javascript.rs

@@ -42,7 +42,9 @@ impl Language for LanguageJS {
         write_to: Option<&Path>,
         mut args: Vec<String>,
     ) -> Vec<String> {
-        args.push(
+        // will run for example: node script.js args...
+        args.insert(
+            0,
             self.executable_name(path, write_to)
                 .to_string_lossy()
                 .to_string(),
@@ -54,3 +56,21 @@ impl Language for LanguageJS {
         limits.allow_multiprocess();
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_runtime_args_executable_comes_first() {
+        // node requires the script to load to be the first non-option argument;
+        // regression test for it being appended after the program's own args instead.
+        let lang = LanguageJS::new();
+        let args = lang.runtime_args(
+            Path::new("solution.js"),
+            None,
+            vec!["input".into(), "output".into()],
+        );
+        assert_eq!(args, vec!["solution", "input", "output"]);
+    }
+}