From 359b8015b24916eac45eb5fe326f3ac55d440e66 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Fri, 25 Sep 2026 08:12:23 +0200 Subject: [PATCH 1/2] fix(cli): make --dry-run refuse what the real run refuses, and answer help - An impossible --at (month 13, 30 February, 25:61) is now checked by the driver with the parser the core itself uses, so a dry run exits 1 in the core's words instead of printing it as the plan and exiting 0. - A file Windows will not start (no PE image, not a batch script) is a new plan state, not_a_program, and exits 2 like the real launch failure. A batch script is still planned, because Windows starts it. - The target file date that fills a date parameter is midnight, as a date parameter is everywhere else. date-before-install carried the time of day the file was written. - help, run --help and calc --help (and -h) print the usage and exit 0, and --at followed by another flag says the moment is missing. Tests: a unit test for each (257 in --bins), dry_run.rs gains the plan refusals with a batch-script control and a real-run control for the text file, and usage.rs is new. Every new test was seen to fail with its fix reverted. The two real-session tests in dry_run.rs now share one lock, because the core allows one session at a time. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 17 +++++++ crates/cli/src/cli.rs | 17 +++++++ crates/cli/src/main.rs | 5 +- crates/cli/src/pe.rs | 76 +++++++++++++++++++++++------ crates/cli/src/preset.rs | 33 ++++++++++++- crates/cli/src/run/moment.rs | 41 +++++++++++++++- crates/cli/src/run/plan.rs | 66 +++++++++++++++++++++++-- crates/cli/tests/dry_run.rs | 95 ++++++++++++++++++++++++++++++++++++ crates/cli/tests/network.rs | 13 +++-- crates/cli/tests/usage.rs | 66 +++++++++++++++++++++++++ 10 files changed, 402 insertions(+), 27 deletions(-) create mode 100644 crates/cli/tests/usage.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 37e976d..25f9e09 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,23 @@ Notable changes to Chrono Mock, newest first. The format follows ### Fixed +- **`--dry-run` approved sessions the real run refuses.** An impossible `--at` (month 13, 30 February, + 25:61) was printed as the plan and exited 0, while the real run was refused by the core with exit + 1. A file Windows will not start (a text file, an empty or truncated `.exe`) was planned as "native + injection" and exited 0, while the real run failed to launch it with exit 2. The plan now refuses + both with the code the real run gives, the moment in the core's own words. A batch script is still + planned, because Windows starts it through the command interpreter. `chronomock.plan/1` names that + target state `not_a_program`. +- **A preset that dates from the application's file carried the time of day the file was written.** + `chrono run --preset date-before-install` without `--param` set the session to 23:00:35 the day + before installation, where the same preset given the date by hand sets midnight. A date parameter + is a date, so the file's creation date now arrives as midnight too. The trial presets were not + affected, because they set their own time. +- **Asking for help was answered as a mistake.** `chrono help` said "unknown command", and + `chrono calc --help` said "unknown flag" and exited 1, while `chrono --help` exited 0. `help`, + `run --help` and `calc --help` (and `-h`) now print the usage and exit 0. `--help` further along the + command line is still passed to the application. `--at` followed by another flag now says that + `--at` is missing its moment, instead of a sentence about a "shift". - **Ending a session sent the application's timers back, and left its web pages on the session date.** A session often ends while the application keeps running: `--ticks` ran out, or the session was stopped from the window. The application was then handed back the real value of every diff --git a/crates/cli/src/cli.rs b/crates/cli/src/cli.rs index 5811ffc..1b19fcb 100644 --- a/crates/cli/src/cli.rs +++ b/crates/cli/src/cli.rs @@ -235,6 +235,23 @@ pub(crate) fn print_usage() { eprintln!("usage: chrono license [--components] (also --license) the licence, the warranty disclaimer, and every bundled component with its version"); } +/// Whether the words after a command ask for its help: `run --help`, `calc -h`. Only the FIRST word +/// counts, because `--help` further along can be meant for the target (`--args --help`). +pub(crate) fn asks_for_help(rest: &[String]) -> bool { + matches!(rest.first().map(String::as_str), Some("--help" | "-h")) +} + +/// The usage a command was asked for, and exit 0 - it is an answer, not a refusal. `calc --help` +/// used to say "unknown flag" and exit 1 while `chrono --help` beside it exited 0. +pub(crate) fn print_help_for(command: &str) -> i32 { + if command == "calc" { + print_calc_usage(); + } else { + print_usage(); + } + 0 +} + pub(crate) fn print_calc_usage() { eprintln!("usage: chrono calc [--base ] [--base-utc ] [--shift <±N>]... [--set-time ] [--snap ] [--nearest ] [--to-zone <+HH:MM>] [--zone <+HH:MM>] [--calendar ] [--format ] [--json]"); eprintln!(" or: chrono calc --preset [--param id=value]... (named moment, e.g. month-end, trial-first-day-after)"); diff --git a/crates/cli/src/main.rs b/crates/cli/src/main.rs index bf10b03..c1c10b3 100644 --- a/crates/cli/src/main.rs +++ b/crates/cli/src/main.rs @@ -63,7 +63,7 @@ mod zone; use calc::calc_run; use cdp_probe::{cdp_date_probe, cdp_launch_probe, cdp_probe, cdp_shim_probe}; -use cli::{print_license, print_usage, print_version}; +use cli::{asks_for_help, print_help_for, print_license, print_usage, print_version}; use core::core_mode; use run::driver_run; @@ -71,6 +71,7 @@ fn main() { let args: Vec = std::env::args().collect(); let code = match args.get(1).map(String::as_str) { Some("__core") => core_mode(), + Some(command @ ("run" | "calc")) if asks_for_help(&args[2..]) => print_help_for(command), Some("run") => driver_run(&args[2..]), Some("calc") => calc_run(&args[2..]), Some("__cdp-probe") => cdp_probe(&args[2..]), @@ -83,7 +84,7 @@ fn main() { 0 } Some("license") | Some("--license") => print_license(&args[2..]), - Some("--help") | Some("-h") | None => { + Some("--help") | Some("-h") | Some("help") | None => { print_usage(); 0 } diff --git a/crates/cli/src/pe.rs b/crates/cli/src/pe.rs index 078a117..90cf257 100644 --- a/crates/cli/src/pe.rs +++ b/crates/cli/src/pe.rs @@ -1,6 +1,6 @@ //! A bounded, read-only look inside the target's own executable, for the facts the file names around -//! it cannot give: whether the Go toolchain linked it, and whether it is a .NET executable that leaves -//! no runtime file beside it to say so. +//! it cannot give: whether the Go toolchain linked it, whether it is a .NET executable that leaves +//! no runtime file beside it to say so, and whether it is a PE image at all. //! //! Every read is capped and every offset goes through `get`, so a truncated or hostile file answers //! "no" rather than panicking, and a half-gigabyte target costs a few kilobytes to ask. The one read @@ -224,6 +224,39 @@ struct ImageSpan { pointer: usize, } +/// Whether the file is a PE image at all. `Some(false)` only for a file that was READ and does not +/// carry the two signatures, `None` for one that could not be read. +/// +/// The questions above lean to "no" on any doubt, because a false fingerprint accuses a target of +/// something it does not do. This one is asked in order to REFUSE a target, so its doubt leans the +/// other way: a file this could not read is not called "not a program" (untouchable rule 4). What it +/// does not say is the bitness - `run::plan` explains why the header's machine field is not trusted. +pub(crate) fn is_pe_image(target_path: &Path) -> Option { + let (_, head) = read_head(target_path)?; + Some(pe_header_offset(&head).is_some()) +} + +/// The file and the first `HEADER_WINDOW` bytes of it. +fn read_head(path: &Path) -> Option<(File, Vec)> { + let mut file = File::open(path).ok()?; + let mut head: Vec = Vec::new(); + // `take` + `read_to_end` rather than one `read`: a single read may return fewer bytes than + // asked for, and a short header would send the section-table offsets somewhere arbitrary. + file.by_ref().take(HEADER_WINDOW).read_to_end(&mut head).ok()?; + Some((file, head)) +} + +/// Where the PE header starts, when `head` carries the two signatures every PE image has: `MZ` at +/// the front and `PE\0\0` where the DOS header's pointer says. One definition for both questions this +/// module asks of the start of a file. +fn pe_header_offset(head: &[u8]) -> Option { + if head.get(..2)? != b"MZ" { + return None; + } + let pe = u32_at(head, 0x3C)? as usize; + (head.get(pe..pe.checked_add(4)?)? == b"PE\0\0").then_some(pe) +} + /// The first four kilobytes of a PE file, checked for the two signatures, plus the open handle so the /// sections and directories they describe can be read. struct PeFile { @@ -236,18 +269,8 @@ struct PeFile { impl PeFile { fn open(path: &Path) -> Option { - let mut file = File::open(path).ok()?; - let mut head: Vec = Vec::new(); - // `take` + `read_to_end` rather than one `read`: a single read may return fewer bytes than - // asked for, and a short header would send the section-table offsets somewhere arbitrary. - file.by_ref().take(HEADER_WINDOW).read_to_end(&mut head).ok()?; - if head.get(..2)? != b"MZ" { - return None; - } - let pe = u32_at(&head, 0x3C)? as usize; - if head.get(pe..pe.checked_add(4)?)? != b"PE\0\0" { - return None; - } + let (file, head) = read_head(path)?; + let pe = pe_header_offset(&head)?; let section_count = u16_at(&head, pe.checked_add(6)?)? as usize; let optional_size = u16_at(&head, pe.checked_add(20)?)? as usize; let optional = pe.checked_add(24)?; @@ -672,6 +695,31 @@ mod tests { } } + /// The one question here whose doubt leans the other way: a file read in full and missing either + /// signature is "not a PE", and a file that could not be read is "do not know" - never "not a PE". + #[test] + fn only_a_file_read_in_full_is_called_not_a_pe_image() { + let pe = write_probe("is-pe", &synthetic_pe(0, b".data\0\0\0", b"anything", None)); + assert_eq!(is_pe_image(&pe), Some(true)); + let text = write_probe("text", b"this is a note, not a program"); + assert_eq!(is_pe_image(&text), Some(false)); + let empty = write_probe("empty", b""); + assert_eq!(is_pe_image(&empty), Some(false)); + // Both signatures are required: `MZ` alone is a DOS stub or a truncated file, and Windows + // refuses to start either (measured: two bytes of MZ exit 2 with 0x800700D8). + let stub = write_probe("mz-only", b"MZ\0\0"); + assert_eq!(is_pe_image(&stub), Some(false)); + let mut no_pe = synthetic_pe(0, b".data\0\0\0", b"anything", None); + let at = u32_at(&no_pe, 0x3C).unwrap() as usize; + no_pe[at..at + 4].copy_from_slice(b"NE\0\0"); + let no_pe = write_probe("mz-without-pe", &no_pe); + assert_eq!(is_pe_image(&no_pe), Some(false)); + assert_eq!(is_pe_image(Path::new("no such file anywhere.exe")), None); + for p in [pe, text, empty, stub, no_pe] { + let _ = std::fs::remove_file(p); + } + } + /// Every doubt answers "not Go". A target we cannot read must not be accused of a gap it may not /// have - an audit that invents one is no better than an audit that hides one (rule 4). #[test] diff --git a/crates/cli/src/preset.rs b/crates/cli/src/preset.rs index d8b289e..7451cac 100644 --- a/crates/cli/src/preset.rs +++ b/crates/cli/src/preset.rs @@ -624,7 +624,13 @@ pub(crate) fn read_target_creation_date( return None; } let wall = filetime_utc_to_wall(created as i64, tz_bias_min.unwrap_or(0)); - chrono_core::calc::parse_civil_datetime(&wall).ok() + // The DATE, at midnight. A `date` parameter is a bare date (docs/04 4.2), and the calculator's + // `--param install_date=...` is one. The time of day the file was written leaked through here into + // every preset without a `set_time` step: `date-before-install` put the session at 23:00:35 the + // day before, where the same preset with the same date given by hand gives midnight. + chrono_core::calc::parse_civil_datetime(&wall) + .ok() + .map(|d| chrono_core::calc::CivilDateTime { hour: 0, minute: 0, second: 0, ..d }) } /// Locate a preset file: next to the executable (portable layout), else in ./presets. @@ -1112,6 +1118,31 @@ mod tests { assert!(matches!(resolve_moment(p.moment, &values), Err(PresetError::BadFile(_)))); } + /// The file date is a DATE: midnight of the day the file was created, in the session zone, the + /// same value `--param install_date=YYYY-MM-DD` gives. It used to carry the time of day too. + #[test] + fn the_target_file_date_is_midnight_of_the_day_it_was_created() { + use std::os::windows::fs::MetadataExt; + let dir =crate::testutil::unique_temp_dir("chrono-preset-file-date"); + std::fs::create_dir_all(&dir).expect("scratch dir"); + let file = dir.join("app.exe"); + std::fs::write(&file, b"MZ").expect("file"); + for bias in [0, -330, 480] { + let d = read_target_creation_date(&file.display().to_string(), Some(bias)).expect("a creation date"); + assert_eq!((d.hour, d.minute, d.second), (0, 0, 0), "bias {bias}: {d:?}"); + } + // The day itself still follows the session zone: the file was created a moment ago, so in UTC + // the date is today's UTC date. + let today_utc = chrono_core::calc::parse_civil_datetime(&filetime_utc_to_wall( + std::fs::metadata(&file).unwrap().creation_time() as i64, + 0, + )) + .unwrap(); + let d = read_target_creation_date(&file.display().to_string(), Some(0)).unwrap(); + assert_eq!((d.year, d.month, d.day), (today_utc.year, today_utc.month, today_utc.day)); + let _ = std::fs::remove_dir_all(&dir); + } + /// The target_file_creation hint fills a date parameter from the target's file date (run only) - /// without a target it is the honest not-built - a duration slot or an unbuilt hint is refused. #[test] diff --git a/crates/cli/src/run/moment.rs b/crates/cli/src/run/moment.rs index fb2938a..0ef0fc6 100644 --- a/crates/cli/src/run/moment.rs +++ b/crates/cli/src/run/moment.rs @@ -201,16 +201,29 @@ pub(super) fn resolve_time_spec(ra: &RunArgs, now_bias: i32) -> Result) -> Result { + // `--at` takes the next word whatever it is, so `--at --dry-run` handed the flag over as the + // moment, and its leading '-' made that a relative one: the refusal then spoke of a "shift" + // nobody had written. No moment starts with two dashes, so the word is named as what it is. + if raw.starts_with("--") { + return Err(format!( + "--at needs a moment after it, but the next word is the flag '{raw}' - write --at YYYY-MM-DDTHH:MM:SS, or a relative +N" + )); + } if raw.starts_with(['+', '-']) { let now = resolve_now_civil(tz_bias_min)?; resolve_relative_at(raw, now) } else { + // The core parses this same string with this same function before it starts anything, and + // refused 2038-13-45 there - AFTER the driver had already let a dry run approve it with exit + // 0 and a plan naming that moment. Docs/08 section 8 says a dry run's 0 means "this plan is + // sound", so a moment the run would refuse is refused here, in the core's own words. + chrono_core::calc::parse_civil_datetime(raw)?; Ok(raw.to_string()) } } @@ -264,6 +277,30 @@ mod tests { ); } + /// A moment the core would refuse is refused here, before anything starts, and in the core's own + /// words - the three impossible fields a dry run used to approve with exit 0. + #[test] + fn an_absolute_at_the_core_would_refuse_is_refused_before_anything_starts() { + for (raw, words) in [ + ("2038-13-45T00:00:00", "month out of range"), + ("2030-02-30T00:00:00", "day 30 out of range for month 2"), + ("2030-02-28T25:61:00", "hour 25 out of range"), + ] { + let e = resolve_at(raw, Some(0)).expect_err(raw); + assert!(e.contains(words), "{raw}: {e}"); + } + // The space the core accepts in place of the T stays accepted - one parser, one answer. + assert_eq!(resolve_at("2038-01-19 03:14:07", Some(0)).unwrap(), "2038-01-19 03:14:07"); + } + + /// `--at --dry-run` took the flag as the moment and answered with a sentence about a "shift". + #[test] + fn a_flag_where_the_moment_should_be_is_named_as_a_flag() { + let e = resolve_at("--dry-run", Some(0)).unwrap_err(); + assert!(e.contains("--at needs a moment") && e.contains("'--dry-run'"), "{e}"); + assert!(!e.contains("shift"), "{e}"); + } + #[test] fn at_relative_resolves_to_absolute_wall() { // Value is now-dependent, but a valid delta must produce a wall string. diff --git a/crates/cli/src/run/plan.rs b/crates/cli/src/run/plan.rs index eacb526..14f93e2 100644 --- a/crates/cli/src/run/plan.rs +++ b/crates/cli/src/run/plan.rs @@ -56,6 +56,10 @@ const LABEL: usize = 12; enum TargetPath { /// A file is there, at the absolute path the session would use. Found(PathBuf), + /// A file is there, and Windows will not start it: no PE image and no batch script. Measured: + /// the real run exits 2 with `CreateProcessW` failing on a text file, an empty `.exe` and two + /// bytes of `MZ`, while this plan used to call all three sound and exit 0. + NotAProgram(PathBuf), /// No file of that name, and the mechanism that would run it does not search anywhere else. Missing, /// A bare name on the Chromium path, which resolves it through PATH itself. @@ -67,6 +71,7 @@ impl TargetPath { fn key(&self) -> &'static str { match self { TargetPath::Found(_) => "found", + TargetPath::NotAProgram(_) => "not_a_program", TargetPath::Missing => "missing", TargetPath::Unchecked => "unchecked", } @@ -86,8 +91,8 @@ fn inspect_target(target: &str, chromium: bool) -> TargetPath { if path.is_file() { // The absolute path, so the plan names the file the session would open rather than whatever // the shell's current directory made of it. - let canonical = std::fs::canonicalize(path).unwrap_or_else(|_| path.to_path_buf()); - return TargetPath::Found(readable(canonical)); + let canonical = readable(std::fs::canonicalize(path).unwrap_or_else(|_| path.to_path_buf())); + return if windows_would_start(path) { TargetPath::Found(canonical) } else { TargetPath::NotAProgram(canonical) }; } if chromium && is_bare_name(target) { TargetPath::Unchecked @@ -96,6 +101,21 @@ fn inspect_target(target: &str, chromium: bool) -> TargetPath { } } +/// Whether `CreateProcessW` would start this file, which both mechanisms end in: a PE image, or a +/// batch script, which it hands to the command interpreter itself (measured: `chrono run x.bat` starts +/// the script and exits 12 when it ends at once). A file that could not be read gets the benefit of +/// the doubt - the plan does not know, so it does not refuse (untouchable rule 4). +/// +/// Only WHETHER it is a PE image, never its bitness - see `mechanism_text` for why the header's +/// machine field is not trusted here. +fn windows_would_start(path: &Path) -> bool { + let batch = path + .extension() + .and_then(|e| e.to_str()) + .is_some_and(|e| e.eq_ignore_ascii_case("bat") || e.eq_ignore_ascii_case("cmd")); + batch || crate::pe::is_pe_image(path) != Some(false) +} + /// The canonical path without the extended-length prefix Windows answers `canonicalize` with. That /// prefix is correct and unreadable, and this line is read by a person first - `\\?\C:\app.exe` names /// the same file as `C:\app.exe`. @@ -129,8 +149,9 @@ struct Plan<'a> { zone_bias_min: i32, } -/// Print the plan and start nothing. Exit 0, except for a path that definitely leads to no file, -/// which exits 2 - the code a real run would give for the same fact (docs/08 section 8). +/// Print the plan and start nothing. Exit 0, except for a path that definitely leads to no file or +/// to a file Windows will not start, which exits 2 - the code a real run gives for the same fact +/// (docs/08 section 8). pub(super) fn dry_run(ra: &RunArgs, spec: &TimeSpec, origin: &TimeOrigin, now_bias: i32) -> i32 { // The same pure function the core calls, so the plan names the mechanism the core would choose // and not one worked out a second way (ADR-9). @@ -160,6 +181,13 @@ pub(super) fn dry_run(ra: &RunArgs, spec: &TimeSpec, origin: &TimeOrigin, now_bi ); return 2; } + if let TargetPath::NotAProgram(path) = &plan.target { + eprintln!( + "chrono: '{}' is not a program Windows can start - no executable header, and not a batch script - so a real run would fail to launch it (exit 2)", + path.display() + ); + return 2; + } 0 } @@ -177,6 +205,10 @@ fn target_block(p: &Plan) -> String { let mut out = String::new(); match &p.target { TargetPath::Found(path) => out.push_str(&line("target", &path.display().to_string())), + TargetPath::NotAProgram(path) => { + out.push_str(&line("target", &path.display().to_string())); + out.push_str(¬e("not a program Windows can start - no executable header, and not a batch script")); + } TargetPath::Missing => { out.push_str(&line("target", &p.ra.target)); out.push_str(¬e("there is no file here by that name")); @@ -208,6 +240,9 @@ fn mechanism_text(p: &Plan) -> String { if p.target == TargetPath::Missing { return "not decided - a mechanism is chosen from the target's own folder".to_string(); } + if matches!(p.target, TargetPath::NotAProgram(_)) { + return "none - neither mechanism can start this file".to_string(); + } if p.chromium { return "Chromium or Electron over CDP - the folder carries the Chromium runtime".to_string(); } @@ -428,7 +463,7 @@ fn render_json(p: &Plan) -> String { target: TargetJson { path: &p.ra.target, resolved: match &p.target { - TargetPath::Found(path) => Some(path.display().to_string()), + TargetPath::Found(path) | TargetPath::NotAProgram(path) => Some(path.display().to_string()), _ => None, }, state: p.target.key(), @@ -544,6 +579,27 @@ mod tests { /// name through PATH and so declined to judge one. It does not: the native mechanism passes the /// target to CreateProcessW as lpApplicationName, which Microsoft documents as never using the /// search path, and `chrono run notepad` exits 2 on this machine with notepad.exe on PATH twice. + /// A file that is there but that Windows will not start is its own state, and a batch script is + /// not one of them - `CreateProcessW` starts it through the command interpreter, measured. + #[test] + fn a_file_windows_will_not_start_is_not_a_program_and_a_batch_script_is() { + let dir = crate::testutil::unique_temp_dir("chrono-plan-not-a-program"); + std::fs::create_dir_all(&dir).expect("scratch dir"); + let note = dir.join("note.txt"); + std::fs::write(¬e, "not a program").expect("text file"); + assert!(matches!(inspect_target(¬e.display().to_string(), false), TargetPath::NotAProgram(_))); + assert!(matches!(inspect_target(¬e.display().to_string(), true), TargetPath::NotAProgram(_))); + for script in ["run.bat", "RUN.CMD"] { + let path = dir.join(script); + std::fs::write(&path, "@echo off\r\n").expect("batch file"); + assert!( + matches!(inspect_target(&path.display().to_string(), false), TargetPath::Found(_)), + "{script} is started by CreateProcessW, so the plan must not refuse it" + ); + } + let _ = std::fs::remove_dir_all(&dir); + } + #[test] fn a_name_that_is_not_a_file_here_is_missing_whether_or_not_it_looks_like_a_path() { assert_eq!(inspect_target(r"C:\definitely\not\here\nothing.exe", false), TargetPath::Missing); diff --git a/crates/cli/tests/dry_run.rs b/crates/cli/tests/dry_run.rs index edf59b7..eccb848 100644 --- a/crates/cli/tests/dry_run.rs +++ b/crates/cli/tests/dry_run.rs @@ -15,6 +15,19 @@ use std::path::PathBuf; use std::process::Command; +use std::sync::{Mutex, MutexGuard}; + +/// Held by every test here that drives a REAL session. The core allows one session at a time, and +/// the tests of one file run on parallel threads, so two real runs side by side had the second +/// refused with "another session's core is running" - seen on the third run of this file after the +/// second real session came in, having passed the two before it. +static REAL_SESSION: Mutex<()> = Mutex::new(()); + +/// The lock, whether or not a test holding it before has failed - a failure there says nothing +/// about the next session, and a poisoned lock would turn one red test into several. +fn one_real_session_at_a_time() -> MutexGuard<'static, ()> { + REAL_SESSION.lock().unwrap_or_else(std::sync::PoisonError::into_inner) +} /// The command interpreter, by full path. fn command_interpreter() -> String { @@ -100,6 +113,7 @@ fn the_same_command_line_without_the_flag_does_start_the_target() { library.display() ); + let _session = one_real_session_at_a_time(); let dir = scratch("starts-something"); let marker = dir.join("the-target-ran"); @@ -194,6 +208,87 @@ fn every_committed_runner_builds_the_debug_artifacts_before_it_runs_the_tests() } } +/// A plan refuses what the run would refuse, with the code the run gives for it (docs/08 section 8): +/// a moment the core cannot read exits 1 in the core's own words, and a file Windows will not start +/// exits 2. Both used to exit 0 with a plan calling them sound - measured, while the real run refused +/// both. A batch script is the control: `CreateProcessW` starts one through the command interpreter, +/// so a plan that refused it would be wrong the other way. +#[test] +fn a_plan_refuses_what_the_run_would_refuse_and_nothing_else() { + let target = command_interpreter(); + for (at, words) in [ + ("2038-13-45T00:00:00", "month out of range"), + ("2030-02-30T00:00:00", "day 30 out of range for month 2"), + ("2030-02-28T25:61:00", "hour 25 out of range"), + ] { + let out = Command::new(env!("CARGO_BIN_EXE_chrono")) + .args(["run", &target, "--at", at, "--dry-run"]) + .output() + .expect("the tool must run"); + let stderr = String::from_utf8_lossy(&out.stderr); + assert_eq!(out.status.code(), Some(1), "{at}: {stderr}"); + assert!(stderr.contains(words), "{at}: the refusal must name the field: {stderr}"); + assert!( + !String::from_utf8_lossy(&out.stdout).contains("Nothing was started"), + "{at}: no plan is printed for a moment that cannot be" + ); + } + + let dir = scratch("not-a-program"); + let note = dir.join("note.txt"); + std::fs::write(¬e, "a note, not a program").expect("a text file"); + let out = Command::new(env!("CARGO_BIN_EXE_chrono")) + .args(["run", ¬e.display().to_string(), "--at", "2038-01-19T03:14:07", "--dry-run"]) + .output() + .expect("the tool must run"); + assert_eq!(out.status.code(), Some(2), "{}", String::from_utf8_lossy(&out.stdout)); + assert!( + String::from_utf8_lossy(&out.stderr).contains("not a program Windows can start"), + "the reason must be on stderr: {}", + String::from_utf8_lossy(&out.stderr) + ); + + let script = dir.join("script.bat"); + std::fs::write(&script, "@echo off\r\n").expect("a batch script"); + let out = Command::new(env!("CARGO_BIN_EXE_chrono")) + .args(["run", &script.display().to_string(), "--at", "2038-01-19T03:14:07", "--dry-run"]) + .output() + .expect("the tool must run"); + assert_eq!(out.status.code(), Some(0), "a batch script is started by Windows: {}", String::from_utf8_lossy(&out.stderr)); + + let _ = std::fs::remove_dir_all(&dir); +} + +/// The premise under the refusal above, checked on a real run: the text file really does fail to +/// launch with exit 2. Should the core ever start such a file, the plan's refusal would become the +/// lie, and this is what would say so. +#[test] +fn a_real_run_of_a_file_windows_will_not_start_exits_two() { + let library = injected_library(); + assert!( + library.is_file(), + "this probe drives a real session and needs {}, which `cargo test` does not build. \ + Run `cargo build --workspace` first - CI and tools/gates.ps1 both do that now.", + library.display() + ); + let _session = one_real_session_at_a_time(); + let dir = scratch("real-not-a-program"); + let note = dir.join("note.txt"); + std::fs::write(¬e, "a note, not a program").expect("a text file"); + let out = Command::new(env!("CARGO_BIN_EXE_chrono")) + .args(["run", ¬e.display().to_string(), "--at", "2038-01-19T03:14:07", "--ticks", "1"]) + .output() + .expect("the tool must run"); + assert_eq!( + out.status.code(), + Some(2), + "stdout: {} stderr: {}", + String::from_utf8_lossy(&out.stdout), + String::from_utf8_lossy(&out.stderr) + ); + let _ = std::fs::remove_dir_all(&dir); +} + /// A path with a directory component that holds no file is a fact the plan can establish without /// starting anything, and it exits 2 - the code a real run gives for the same fact. That is what /// makes `--dry-run` usable as a pre-flight check rather than a pretty printer. diff --git a/crates/cli/tests/network.rs b/crates/cli/tests/network.rs index 2dd7227..2f29e79 100644 --- a/crates/cli/tests/network.rs +++ b/crates/cli/tests/network.rs @@ -158,9 +158,16 @@ const ALLOWED: &[(&str, &str, &str)] = &[ ( "crates/cli/tests/dry_run.rs", "spawn", - "the dry-run guard runs the built binary twice over: once with --dry-run, which must start \ - nothing, and once without it, which must start the target - the second is what proves the \ - first can fail. Neither reaches past this machine", + "the dry-run guard runs the built binary: with --dry-run, which must start nothing and must \ + refuse what a real run refuses, and without it, which must start the target or fail to \ + launch a file Windows will not start - the real runs are what prove the plans can fail. \ + None reaches past this machine", + ), + ( + "crates/cli/tests/usage.rs", + "spawn", + "runs the built binary to ask it for help and to give it a flag without a value, and reads \ + what it answers. It starts no target and reaches nothing past this machine", ), ( "crates/cli/tests/session_clock.rs", diff --git a/crates/cli/tests/usage.rs b/crates/cli/tests/usage.rs new file mode 100644 index 0000000..d92555d --- /dev/null +++ b/crates/cli/tests/usage.rs @@ -0,0 +1,66 @@ +//! What a person gets when they ask the tool for help, or leave a flag without its value: an answer +//! in words that match what they typed. +//! +//! Found by the release checks, which drive the packaged tool the way a person would: `chrono help` +//! said "unknown command", `chrono calc --help` said "unknown flag" and exited 1 while `chrono --help` +//! beside it exited 0, and `--at --dry-run` was answered with a sentence about a "shift" nobody wrote. + +use std::process::{Command, Output}; + +fn chrono(args: &[&str]) -> Output { + Command::new(env!("CARGO_BIN_EXE_chrono")) + .args(args) + .output() + .expect("the tool must run") +} + +fn text(out: &Output) -> String { + format!("{}{}", String::from_utf8_lossy(&out.stdout), String::from_utf8_lossy(&out.stderr)) +} + +/// Asking is answered: usage, exit 0, no word about anything being unknown. +#[test] +fn every_way_of_asking_for_help_is_answered_with_usage_and_exit_zero() { + let asks: [&[&str]; 7] = [ + &["--help"], + &["-h"], + &["help"], + &["run", "--help"], + &["run", "-h"], + &["calc", "--help"], + &["calc", "-h"], + ]; + for args in asks { + let out = chrono(args); + let said = text(&out); + assert_eq!(out.status.code(), Some(0), "{args:?}: {said}"); + assert!(said.contains("usage:"), "{args:?} must print the usage: {said}"); + assert!(!said.contains("unknown"), "{args:?} is a question, not a mistake: {said}"); + } + // The calculator's help is the calculator's usage, not the whole tool's. + let calc = text(&chrono(&["calc", "--help"])); + assert!(calc.contains("chrono calc") && !calc.contains("chrono run "), "{calc}"); +} + +/// Only the first word after a command asks for its help. Further along, `--help` can belong to the +/// target, and taking it as a question would replace the session with a usage text. +#[test] +fn a_help_flag_meant_for_the_target_is_passed_on_and_not_answered() { + let out = chrono(&["run", r"C:\definitely\not\here\app.exe", "--args", "--help", "--dry-run"]); + assert_eq!( + out.status.code(), + Some(2), + "the plan must be resolved (and refuse the missing target), not replaced by usage: {}", + text(&out) + ); +} + +/// `--at` takes the next word, so `--at --dry-run` gave it a flag. The refusal names the flag. +#[test] +fn a_flag_where_the_moment_should_be_is_named_as_a_flag() { + let out = chrono(&["run", r"C:\definitely\not\here\app.exe", "--at", "--dry-run"]); + let said = text(&out); + assert_eq!(out.status.code(), Some(1), "{said}"); + assert!(said.contains("--at needs a moment") && said.contains("'--dry-run'"), "{said}"); + assert!(!said.contains("shift needs"), "{said}"); +} From e61d0875cc61e36661e8002915ac4e250ab34a5c Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Fri, 25 Sep 2026 08:50:01 +0200 Subject: [PATCH 2/2] fix(cli): read the PE header where it is, and refuse what Windows refuses Review round on the dry-run fix. - The plan looked for the PE signature in the first four kilobytes only, so a program whose header sits further in was refused as "not a program" with exit 2. Windows starts such an image with its header 4 KiB, 32 KiB and 1 MiB in (measured). The header is now read where the DOS header points, and the fingerprints read it there too, so such a target keeps its runtime caution. - A file cut short right behind the signature, a library, an image without the executable flag, with an unknown optional header magic or with none, or whose section table the file does not hold, was planned as sound while the real run refused it with exit 2. Each refusal was measured on synthetic images through the real run. The machine, the subsystem and the section data stay the real run's to judge. - `--at -h` is named as a flag, like `--at --dry-run`, instead of being answered as a shift with no number. - The file-date test sets a creation time next to midnight UTC and checks the day in three zones, so a date that ignored the zone now fails it. The help-forwarding test reads the arguments the plan hands the application. It used to pass with `--help` dropped. - The batch-script comment says what was measured: the script runs and gets its arguments, except when an argument carries quotes. - CHANGELOG: the entry names what is checked, and no line of it starts with "1." any more, which Markdown renders as a list. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 18 +-- crates/cli/src/cli.rs | 7 +- crates/cli/src/pe.rs | 257 ++++++++++++++++++++++++++--------- crates/cli/src/preset.rs | 31 +++-- crates/cli/src/run/moment.rs | 17 ++- crates/cli/src/run/plan.rs | 27 ++-- crates/cli/tests/dry_run.rs | 42 ++++-- crates/cli/tests/usage.rs | 31 +++-- 8 files changed, 295 insertions(+), 135 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 25f9e09..052d20b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,12 +44,14 @@ Notable changes to Chrono Mock, newest first. The format follows ### Fixed - **`--dry-run` approved sessions the real run refuses.** An impossible `--at` (month 13, 30 February, - 25:61) was printed as the plan and exited 0, while the real run was refused by the core with exit - 1. A file Windows will not start (a text file, an empty or truncated `.exe`) was planned as "native - injection" and exited 0, while the real run failed to launch it with exit 2. The plan now refuses - both with the code the real run gives, the moment in the core's own words. A batch script is still - planned, because Windows starts it through the command interpreter. `chronomock.plan/1` names that - target state `not_a_program`. + 25:61) was printed as the plan and exited 0, while the real run was refused by the core with + exit 1. A file Windows will not start (a text file, an empty `.exe`, one cut short inside its + header, a library) was planned as "native injection" and exited 0, while the real run failed to + launch it with exit 2. The plan now refuses both with the code the real run gives, the moment in + the core's own words. It reads the header where the file says it is, so a program whose header + sits far into the file is still planned. What it leaves to the real run is whether the rest of the + file is intact. A batch script is still planned, because Windows starts it through the command + interpreter. `chronomock.plan/1` names that target state `not_a_program`. - **A preset that dates from the application's file carried the time of day the file was written.** `chrono run --preset date-before-install` without `--param` set the session to 23:00:35 the day before installation, where the same preset given the date by hand sets midnight. A date parameter @@ -58,8 +60,8 @@ Notable changes to Chrono Mock, newest first. The format follows - **Asking for help was answered as a mistake.** `chrono help` said "unknown command", and `chrono calc --help` said "unknown flag" and exited 1, while `chrono --help` exited 0. `help`, `run --help` and `calc --help` (and `-h`) now print the usage and exit 0. `--help` further along the - command line is still passed to the application. `--at` followed by another flag now says that - `--at` is missing its moment, instead of a sentence about a "shift". + command line is still passed to the application. `--at` followed by another flag, or by `-h`, now + says that `--at` is missing its moment, instead of a sentence about a "shift". - **Ending a session sent the application's timers back, and left its web pages on the session date.** A session often ends while the application keeps running: `--ticks` ran out, or the session was stopped from the window. The application was then handed back the real value of every diff --git a/crates/cli/src/cli.rs b/crates/cli/src/cli.rs index 1b19fcb..3731556 100644 --- a/crates/cli/src/cli.rs +++ b/crates/cli/src/cli.rs @@ -235,10 +235,15 @@ pub(crate) fn print_usage() { eprintln!("usage: chrono license [--components] (also --license) the licence, the warranty disclaimer, and every bundled component with its version"); } +/// Whether a word is the help flag, which every command answers. +pub(crate) fn is_help_flag(word: &str) -> bool { + matches!(word, "--help" | "-h") +} + /// Whether the words after a command ask for its help: `run --help`, `calc -h`. Only the FIRST word /// counts, because `--help` further along can be meant for the target (`--args --help`). pub(crate) fn asks_for_help(rest: &[String]) -> bool { - matches!(rest.first().map(String::as_str), Some("--help" | "-h")) + rest.first().is_some_and(|word| is_help_flag(word)) } /// The usage a command was asked for, and exit 0 - it is an answer, not a refusal. `calc --help` diff --git a/crates/cli/src/pe.rs b/crates/cli/src/pe.rs index 90cf257..0e5a451 100644 --- a/crates/cli/src/pe.rs +++ b/crates/cli/src/pe.rs @@ -1,6 +1,6 @@ //! A bounded, read-only look inside the target's own executable, for the facts the file names around //! it cannot give: whether the Go toolchain linked it, whether it is a .NET executable that leaves -//! no runtime file beside it to say so, and whether it is a PE image at all. +//! no runtime file beside it to say so, and whether its header describes a program at all. //! //! Every read is capped and every offset goes through `get`, so a truncated or hostile file answers //! "no" rather than panicking, and a half-gigabyte target costs a few kilobytes to ask. The one read @@ -14,10 +14,30 @@ use std::fs::File; use std::io::{Read, Seek, SeekFrom}; use std::path::Path; -/// How much of the PE headers to read. The section table sits right behind the optional header, and -/// four kilobytes covers it with room to spare on every binary we have measured. +/// How much of the PE header to read, counted from where the DOS header points. The section table sits +/// right behind the optional header, and four kilobytes covers it with room to spare on every binary +/// we have measured. const HEADER_WINDOW: u64 = 4096; +/// The DOS header, whose last field says where the PE header starts. +const DOS_HEADER: u64 = 0x40; + +/// The four bytes the PE header starts with. +const PE_SIGNATURE: &[u8; 4] = b"PE\0\0"; + +/// Where the optional header starts inside the PE header: behind the signature and the twenty-byte +/// file header. +const OPTIONAL_AT: usize = 24; + +/// How much of the PE header `is_pe_image` reads: the signature, the file header, and the optional +/// header's first field, its magic. +const PROGRAM_HEADER: u64 = 26; + +/// The two file header flags that say what kind of image this is, `IMAGE_FILE_EXECUTABLE_IMAGE` and +/// `IMAGE_FILE_DLL` in the PE format. +const FILE_IS_EXECUTABLE: u16 = 0x0002; +const FILE_IS_LIBRARY: u16 = 0x2000; + /// The size of one section header, and of one data directory entry, as the PE format fixes them. const SECTION_HEADER: usize = 40; const DATA_DIRECTORY_ENTRY: usize = 8; @@ -224,60 +244,103 @@ struct ImageSpan { pointer: usize, } -/// Whether the file is a PE image at all. `Some(false)` only for a file that was READ and does not -/// carry the two signatures, `None` for one that could not be read. +/// Whether the file's header describes a program Windows can start. `Some(false)` only for a file that +/// was READ and whose header is missing, cut short, or a library's, `None` for one that could not be +/// read. +/// +/// Every refusal here is one Windows makes too, measured on synthetic images through the real run +/// (`CreateProcessW` failing with 0x800700C1 or 0x800700D8): no `MZ`, no `PE\0\0` where the DOS header +/// points, a file ending inside the file header, an optional header declared shorter than its magic, +/// a magic other than PE32 or PE32+, the executable flag missing, the library flag set, a section +/// table the file does not hold in full. The header is read where the DOS header points, however far +/// in: Windows starts an image whose header is 4 KiB, 32 KiB and 1 MiB into the file, which the first +/// version of this check, reading only the first four kilobytes, called "not a program". What it leaves +/// to the real run: the machine (`run::plan` explains why that field is not trusted), the subsystem, +/// and whether the sections behind the header are all there. /// /// The questions above lean to "no" on any doubt, because a false fingerprint accuses a target of /// something it does not do. This one is asked in order to REFUSE a target, so its doubt leans the -/// other way: a file this could not read is not called "not a program" (untouchable rule 4). What it -/// does not say is the bitness - `run::plan` explains why the header's machine field is not trusted. +/// other way: a file this could not read is not called "not a program" (untouchable rule 4). pub(crate) fn is_pe_image(target_path: &Path) -> Option { - let (_, head) = read_head(target_path)?; - Some(pe_header_offset(&head).is_some()) + let mut file = File::open(target_path).ok()?; + let Some(pe) = pe_pointer(&read_bytes(&mut file, 0, DOS_HEADER)?) else { + return Some(false); + }; + let header = read_bytes(&mut file, pe, PROGRAM_HEADER)?; + let length = file.metadata().ok()?.len(); + Some(names_a_program(&header, pe, length)) } -/// The file and the first `HEADER_WINDOW` bytes of it. -fn read_head(path: &Path) -> Option<(File, Vec)> { - let mut file = File::open(path).ok()?; - let mut head: Vec = Vec::new(); - // `take` + `read_to_end` rather than one `read`: a single read may return fewer bytes than - // asked for, and a short header would send the section-table offsets somewhere arbitrary. - file.by_ref().take(HEADER_WINDOW).read_to_end(&mut head).ok()?; - Some((file, head)) +/// Whether `header`, read at file offset `pe`, starts the PE header of a program: see `is_pe_image` +/// for what each condition is and that Windows refuses a file failing it. A header shorter than +/// `PROGRAM_HEADER` is a file ending inside it, which is a "no". +fn names_a_program(header: &[u8], pe: u64, length: u64) -> bool { + names_a_program_checked(header, pe, length) == Some(true) +} + +/// `names_a_program` with every read that can fail spelled `?`, so a short header is a "no". +fn names_a_program_checked(header: &[u8], pe: u64, length: u64) -> Option { + let sections = u64::from(u16_at(header, 6)?); + let optional_size = u16_at(header, 20)?; + let flags = u16_at(header, 22)?; + let magic = u16_at(header, OPTIONAL_AT)?; + let table_end = pe + OPTIONAL_AT as u64 + u64::from(optional_size) + sections * SECTION_HEADER as u64; + Some( + header.get(..4)? == PE_SIGNATURE + && optional_size >= 2 + && matches!(magic, 0x10B | 0x20B) + && flags & FILE_IS_EXECUTABLE != 0 + && flags & FILE_IS_LIBRARY == 0 + && table_end <= length, + ) } -/// Where the PE header starts, when `head` carries the two signatures every PE image has: `MZ` at -/// the front and `PE\0\0` where the DOS header's pointer says. One definition for both questions this -/// module asks of the start of a file. -fn pe_header_offset(head: &[u8]) -> Option { - if head.get(..2)? != b"MZ" { +/// Where the PE header starts, when `dos` begins with `MZ` and holds the DOS header's pointer. +fn pe_pointer(dos: &[u8]) -> Option { + if dos.get(..2)? != b"MZ" { return None; } - let pe = u32_at(head, 0x3C)? as usize; - (head.get(pe..pe.checked_add(4)?)? == b"PE\0\0").then_some(pe) + u32_at(dos, 0x3C).map(u64::from) +} + +/// Up to `limit` bytes of the file from `offset`: fewer where the file ends first, `None` when it +/// cannot be read. +fn read_bytes(file: &mut File, offset: u64, limit: u64) -> Option> { + let mut bytes: Vec = Vec::new(); + file.seek(SeekFrom::Start(offset)).ok()?; + // `take` + `read_to_end` rather than one `read`: a single read may return fewer bytes than + // asked for, and a short header would send the section-table offsets somewhere arbitrary. + file.by_ref().take(limit).read_to_end(&mut bytes).ok()?; + Some(bytes) } -/// The first four kilobytes of a PE file, checked for the two signatures, plus the open handle so the -/// sections and directories they describe can be read. +/// The first four kilobytes of the PE header, read from where the DOS header points and checked for +/// its signature, plus the open handle so the sections and directories it describes can be read. +/// +/// Offsets into `head` count from the signature. The ones the header stores (a section's raw data) +/// count from the start of the file, and are read through the handle. The header is usually a few +/// hundred bytes in, but Windows starts an image whose header is a megabyte in (measured), and reading +/// from where it is rather than from the start of the file keeps such a target fingerprinted. struct PeFile { file: File, head: Vec, - optional: usize, optional_size: usize, section_count: usize, } impl PeFile { fn open(path: &Path) -> Option { - let (file, head) = read_head(path)?; - let pe = pe_header_offset(&head)?; - let section_count = u16_at(&head, pe.checked_add(6)?)? as usize; - let optional_size = u16_at(&head, pe.checked_add(20)?)? as usize; - let optional = pe.checked_add(24)?; + let mut file = File::open(path).ok()?; + let pe = pe_pointer(&read_bytes(&mut file, 0, DOS_HEADER)?)?; + let head = read_bytes(&mut file, pe, HEADER_WINDOW)?; + if head.get(..4)? != PE_SIGNATURE { + return None; + } + let section_count = u16_at(&head, 6)? as usize; + let optional_size = u16_at(&head, 20)? as usize; Some(Self { file, head, - optional, optional_size, section_count, }) @@ -285,7 +348,7 @@ impl PeFile { /// The section headers in table order, up to the first one the header window does not hold. fn sections(&self) -> impl Iterator + '_ { - let table = self.optional.saturating_add(self.optional_size); + let table = OPTIONAL_AT.saturating_add(self.optional_size); (0..self.section_count).map_while(move |index| { let entry = table.checked_add(index.checked_mul(SECTION_HEADER)?)?; Some(Section { @@ -301,12 +364,12 @@ impl PeFile { /// The image base and size from the optional header. PE32 keeps a four-byte base at offset 28 and /// PE32+ an eight-byte one at 24, and both keep the size of the image at 56. fn image_span(&self) -> Option { - let (base, pointer) = match u16_at(&self.head, self.optional)? { - 0x10B => (u64::from(u32_at(&self.head, self.optional.checked_add(28)?)?), 4), - 0x20B => (u64_at(&self.head, self.optional.checked_add(24)?)?, 8), + let (base, pointer) = match u16_at(&self.head, OPTIONAL_AT)? { + 0x10B => (u64::from(u32_at(&self.head, OPTIONAL_AT + 28)?), 4), + 0x20B => (u64_at(&self.head, OPTIONAL_AT + 24)?, 8), _ => return None, }; - let size = u32_at(&self.head, self.optional.checked_add(56)?)?; + let size = u32_at(&self.head, OPTIONAL_AT + 56)?; Some(ImageSpan { base, end: base.checked_add(u64::from(size))?, @@ -326,20 +389,17 @@ impl PeFile { /// four stack and heap sizes before it are eight bytes wide in PE32+. Any other magic is a shape /// this module does not know, so it has no directories. fn directory(&self, index: usize) -> Option<(u32, u32)> { - let (count_at, table_at) = match u16_at(&self.head, self.optional)? { + let (count_at, table_at) = match u16_at(&self.head, OPTIONAL_AT)? { 0x10B => (92, 96), 0x20B => (108, 112), _ => return None, }; - if index >= u32_at(&self.head, self.optional.checked_add(count_at)?)? as usize { + if index >= u32_at(&self.head, OPTIONAL_AT + count_at)? as usize { return None; } - let entry = self - .optional - .checked_add(table_at)? - .checked_add(index.checked_mul(DATA_DIRECTORY_ENTRY)?)?; + let entry = (OPTIONAL_AT + table_at).checked_add(index.checked_mul(DATA_DIRECTORY_ENTRY)?)?; // The table lives inside the optional header, so an entry past its declared size is not one. - if entry.checked_add(DATA_DIRECTORY_ENTRY)? > self.optional.checked_add(self.optional_size)? { + if entry.checked_add(DATA_DIRECTORY_ENTRY)? > OPTIONAL_AT + self.optional_size { return None; } Some((u32_at(&self.head, entry)?, u32_at(&self.head, entry.checked_add(4)?)?)) @@ -358,10 +418,7 @@ impl PeFile { } fn read_at(&mut self, offset: u64, limit: usize) -> Option> { - let mut bytes: Vec = Vec::new(); - self.file.seek(SeekFrom::Start(offset)).ok()?; - self.file.by_ref().take(limit as u64).read_to_end(&mut bytes).ok()?; - Some(bytes) + read_bytes(&mut self.file, offset, limit as u64) } /// Up to `window` bytes from the start of the named section, or `None` when there is no such @@ -695,27 +752,91 @@ mod tests { } } - /// The one question here whose doubt leans the other way: a file read in full and missing either - /// signature is "not a PE", and a file that could not be read is "do not know" - never "not a PE". + /// A synthetic image whose file header says what a linker says about a program: an executable + /// image, not a library. + fn program(magic: u16) -> Vec { + let mut pe = synthetic_pe(magic, b".text\0\0\0", b"\x31\xc0\xc3", None); + pe[0x80 + 22..0x80 + 24].copy_from_slice(&FILE_IS_EXECUTABLE.to_le_bytes()); + pe + } + + /// The same image with its PE header moved to `to`, where the DOS header then points. The section + /// table still names the section where it was, so only the header's place changes - the shape of + /// an image Windows starts with its header 4 KiB, 32 KiB and 1 MiB in (measured). + fn header_moved(pe: &[u8], to: usize) -> Vec { + let header = pe[0x80..RAW].to_vec(); + let mut moved = pe.to_vec(); + moved[0x80..RAW].fill(0); + moved.resize(moved.len().max(to + header.len()), 0); + moved[to..to + header.len()].copy_from_slice(&header); + moved[0x3C..0x40].copy_from_slice(&(to as u32).to_le_bytes()); + moved + } + + /// The one question here whose doubt leans the other way: a file read in full whose header is + /// missing, cut short or a library's is "not a program", every one of them a file Windows refuses + /// to start (measured), and a file that could not be read is "do not know" - never "not a program". #[test] - fn only_a_file_read_in_full_is_called_not_a_pe_image() { - let pe = write_probe("is-pe", &synthetic_pe(0, b".data\0\0\0", b"anything", None)); - assert_eq!(is_pe_image(&pe), Some(true)); - let text = write_probe("text", b"this is a note, not a program"); - assert_eq!(is_pe_image(&text), Some(false)); - let empty = write_probe("empty", b""); - assert_eq!(is_pe_image(&empty), Some(false)); - // Both signatures are required: `MZ` alone is a DOS stub or a truncated file, and Windows - // refuses to start either (measured: two bytes of MZ exit 2 with 0x800700D8). - let stub = write_probe("mz-only", b"MZ\0\0"); - assert_eq!(is_pe_image(&stub), Some(false)); - let mut no_pe = synthetic_pe(0, b".data\0\0\0", b"anything", None); - let at = u32_at(&no_pe, 0x3C).unwrap() as usize; - no_pe[at..at + 4].copy_from_slice(b"NE\0\0"); - let no_pe = write_probe("mz-without-pe", &no_pe); - assert_eq!(is_pe_image(&no_pe), Some(false)); + fn only_a_file_whose_header_describes_a_program_is_called_one() { + let mut probes = Vec::new(); + let mut says = |name: &str, bytes: &[u8], expected: Option| { + let path = write_probe(name, bytes); + assert_eq!(is_pe_image(&path), expected, "{name}"); + probes.push(path); + }; + says("pe32plus", &program(0x20B), Some(true)); + says("pe32", &program(0x10B), Some(true)); + // Wherever the DOS header points: the first version read only the first four kilobytes, and + // called an image Windows starts "not a program". + says("header-at-4k", &header_moved(&program(0x20B), 0x1100), Some(true)); + says("header-at-1m", &header_moved(&program(0x20B), 0x10_0000), Some(true)); + + says("text", b"this is a note, not a program", Some(false)); + says("empty", b"", Some(false)); + // `MZ` alone is a DOS stub or a truncated file (measured: 0x800700D8). + says("mz-only", b"MZ\0\0", Some(false)); + let mut ne = program(0x20B); + ne[0x80..0x84].copy_from_slice(b"NE\0\0"); + says("mz-without-pe", &ne, Some(false)); + + // Cut short: right behind the signature, inside the file header, inside the section table. + let whole = program(0x20B); + says("cut-after-signature", &whole[..0x80 + 4], Some(false)); + says("cut-in-file-header", &whole[..0x80 + 20], Some(false)); + says("cut-in-section-table", &whole[..0x80 + 24 + 240 + 20], Some(false)); + + // Whole, and not a program. + let with_flags = |flags: u16| { + let mut pe = program(0x20B); + pe[0x80 + 22..0x80 + 24].copy_from_slice(&flags.to_le_bytes()); + pe + }; + says("library", &with_flags(FILE_IS_EXECUTABLE | FILE_IS_LIBRARY), Some(false)); + says("not-executable", &with_flags(0x0020), Some(false)); + for magic in [0u16, 0x107] { + let mut other = program(0x20B); + other[0x80 + 24..0x80 + 26].copy_from_slice(&magic.to_le_bytes()); + says(&format!("magic-{magic:x}"), &other, Some(false)); + } + let mut no_optional = program(0x20B); + no_optional[0x80 + 20..0x80 + 22].copy_from_slice(&0u16.to_le_bytes()); + says("optional-size-zero", &no_optional, Some(false)); + assert_eq!(is_pe_image(Path::new("no such file anywhere.exe")), None); - for p in [pe, text, empty, stub, no_pe] { + for p in probes { + let _ = std::fs::remove_file(p); + } + } + + /// The fingerprints read the header where the DOS header points too. Reading only the first four + /// kilobytes left an image Windows starts, with its header further in, without its caution. + #[test] + fn a_header_far_into_the_file_is_still_fingerprinted() { + let go = write_probe("go-far", &header_moved(&synthetic_pe(0, b".data\0\0\0", &go_blob(), None), 0x1100)); + assert!(is_go_binary(&go), "the header 4 KiB in"); + let aot = write_probe("aot-far", &header_moved(&exporting(0x20B, &[b"DotNetRuntimeDebugHeader"]), 0x8000)); + assert!(is_dotnet_executable(&aot), "the header 32 KiB in"); + for p in [go, aot] { let _ = std::fs::remove_file(p); } } diff --git a/crates/cli/src/preset.rs b/crates/cli/src/preset.rs index 7451cac..eaa45d0 100644 --- a/crates/cli/src/preset.rs +++ b/crates/cli/src/preset.rs @@ -1120,26 +1120,29 @@ mod tests { /// The file date is a DATE: midnight of the day the file was created, in the session zone, the /// same value `--param install_date=YYYY-MM-DD` gives. It used to carry the time of day too. + /// + /// The file is given a creation time half an hour from midnight UTC on either side, so the day + /// itself moves with the zone: a date that ignored the zone, or read its sign backwards, lands on + /// the wrong day for at least one of the three. #[test] fn the_target_file_date_is_midnight_of_the_day_it_was_created() { - use std::os::windows::fs::MetadataExt; - let dir =crate::testutil::unique_temp_dir("chrono-preset-file-date"); + use std::os::windows::fs::FileTimesExt; + use std::time::{Duration, UNIX_EPOCH}; + let dir = crate::testutil::unique_temp_dir("chrono-preset-file-date"); std::fs::create_dir_all(&dir).expect("scratch dir"); let file = dir.join("app.exe"); std::fs::write(&file, b"MZ").expect("file"); - for bias in [0, -330, 480] { - let d = read_target_creation_date(&file.display().to_string(), Some(bias)).expect("a creation date"); - assert_eq!((d.hour, d.minute, d.second), (0, 0, 0), "bias {bias}: {d:?}"); + // 2030-06-15T23:30:00Z and 2030-06-15T00:30:00Z. Bias is UTC minus local: -330 is UTC+05:30, + // 480 is UTC-08:00. + let late = 1_907_796_600; + for (created, days) in [(late, [(0, 15), (-330, 16), (480, 15)]), (late - 23 * 3600, [(0, 15), (-330, 15), (480, 14)])] { + let times = std::fs::FileTimes::new().set_created(UNIX_EPOCH + Duration::from_secs(created)); + std::fs::File::options().write(true).open(&file).expect("open").set_times(times).expect("set the creation time"); + for (bias, day) in days { + let d = read_target_creation_date(&file.display().to_string(), Some(bias)).expect("a creation date"); + assert_eq!((d.year, d.month, d.day, d.hour, d.minute, d.second), (2030, 6, day, 0, 0, 0), "created {created}, bias {bias}"); + } } - // The day itself still follows the session zone: the file was created a moment ago, so in UTC - // the date is today's UTC date. - let today_utc = chrono_core::calc::parse_civil_datetime(&filetime_utc_to_wall( - std::fs::metadata(&file).unwrap().creation_time() as i64, - 0, - )) - .unwrap(); - let d = read_target_creation_date(&file.display().to_string(), Some(0)).unwrap(); - assert_eq!((d.year, d.month, d.day), (today_utc.year, today_utc.month, today_utc.day)); let _ = std::fs::remove_dir_all(&dir); } diff --git a/crates/cli/src/run/moment.rs b/crates/cli/src/run/moment.rs index 0ef0fc6..c1cd3c4 100644 --- a/crates/cli/src/run/moment.rs +++ b/crates/cli/src/run/moment.rs @@ -209,8 +209,9 @@ pub(super) fn resolve_time_spec(ra: &RunArgs, now_bias: i32) -> Result) -> Result { // `--at` takes the next word whatever it is, so `--at --dry-run` handed the flag over as the // moment, and its leading '-' made that a relative one: the refusal then spoke of a "shift" - // nobody had written. No moment starts with two dashes, so the word is named as what it is. - if raw.starts_with("--") { + // nobody had written. No moment starts with two dashes, and none is spelled like the help flag + // (`-h` has no number), so the word is named as what it is. + if raw.starts_with("--") || crate::cli::is_help_flag(raw) { return Err(format!( "--at needs a moment after it, but the next word is the flag '{raw}' - write --at YYYY-MM-DDTHH:MM:SS, or a relative +N" )); @@ -293,12 +294,16 @@ mod tests { assert_eq!(resolve_at("2038-01-19 03:14:07", Some(0)).unwrap(), "2038-01-19 03:14:07"); } - /// `--at --dry-run` took the flag as the moment and answered with a sentence about a "shift". + /// `--at --dry-run` took the flag as the moment and answered with a sentence about a "shift", and + /// so did `--at -h`. A relative moment with a number stays one. #[test] fn a_flag_where_the_moment_should_be_is_named_as_a_flag() { - let e = resolve_at("--dry-run", Some(0)).unwrap_err(); - assert!(e.contains("--at needs a moment") && e.contains("'--dry-run'"), "{e}"); - assert!(!e.contains("shift"), "{e}"); + for flag in ["--dry-run", "-h", "--help"] { + let e = resolve_at(flag, Some(0)).unwrap_err(); + assert!(e.contains("--at needs a moment") && e.contains(&format!("'{flag}'")), "{flag}: {e}"); + assert!(!e.contains("shift"), "{flag}: {e}"); + } + assert!(resolve_at("-5h", Some(0)).is_ok(), "five hours back is a moment"); } #[test] diff --git a/crates/cli/src/run/plan.rs b/crates/cli/src/run/plan.rs index 14f93e2..10156fe 100644 --- a/crates/cli/src/run/plan.rs +++ b/crates/cli/src/run/plan.rs @@ -56,9 +56,10 @@ const LABEL: usize = 12; enum TargetPath { /// A file is there, at the absolute path the session would use. Found(PathBuf), - /// A file is there, and Windows will not start it: no PE image and no batch script. Measured: - /// the real run exits 2 with `CreateProcessW` failing on a text file, an empty `.exe` and two - /// bytes of `MZ`, while this plan used to call all three sound and exit 0. + /// A file is there, and Windows will not start it: its header is missing, cut short or a + /// library's, and it is not a batch script. Measured: the real run exits 2 with `CreateProcessW` + /// failing on a text file, an empty `.exe`, two bytes of `MZ` and this tool's own hook library, + /// while this plan used to call all four sound and exit 0. NotAProgram(PathBuf), /// No file of that name, and the mechanism that would run it does not search anywhere else. Missing, @@ -101,12 +102,18 @@ fn inspect_target(target: &str, chromium: bool) -> TargetPath { } } -/// Whether `CreateProcessW` would start this file, which both mechanisms end in: a PE image, or a -/// batch script, which it hands to the command interpreter itself (measured: `chrono run x.bat` starts -/// the script and exits 12 when it ends at once). A file that could not be read gets the benefit of -/// the doubt - the plan does not know, so it does not refuse (untouchable rule 4). +/// Whether `CreateProcessW` would start this file, which both mechanisms end in: a program's PE image, +/// or a batch script. A file that could not be read gets the benefit of the doubt - the plan does not +/// know, so it does not refuse (untouchable rule 4). /// -/// Only WHETHER it is a PE image, never its bitness - see `mechanism_text` for why the header's +/// A batch script it starts through the command interpreter itself, although Microsoft Learn says the +/// caller must start `cmd.exe /c` for it. Measured with a script that writes down what it received: +/// it runs and gets its arguments, spaces in its path or name included. Except when an argument +/// carries quotes - the interpreter Windows starts then strips the first quote and the last one on +/// the line, and cannot find the script. That is a fact about the launch, not about the file, so the +/// plan does not refuse it here. +/// +/// Only WHETHER it is a program, never its bitness - see `mechanism_text` for why the header's /// machine field is not trusted here. fn windows_would_start(path: &Path) -> bool { let batch = path @@ -183,7 +190,7 @@ pub(super) fn dry_run(ra: &RunArgs, spec: &TimeSpec, origin: &TimeOrigin, now_bi } if let TargetPath::NotAProgram(path) = &plan.target { eprintln!( - "chrono: '{}' is not a program Windows can start - no executable header, and not a batch script - so a real run would fail to launch it (exit 2)", + "chrono: '{}' is not a program Windows can start - its header is missing, cut short or a library's, and it is not a batch script - so a real run would fail to launch it (exit 2)", path.display() ); return 2; @@ -207,7 +214,7 @@ fn target_block(p: &Plan) -> String { TargetPath::Found(path) => out.push_str(&line("target", &path.display().to_string())), TargetPath::NotAProgram(path) => { out.push_str(&line("target", &path.display().to_string())); - out.push_str(¬e("not a program Windows can start - no executable header, and not a batch script")); + out.push_str(¬e("not a program Windows can start - its header is missing, cut short or a library's, and it is not a batch script")); } TargetPath::Missing => { out.push_str(&line("target", &p.ra.target)); diff --git a/crates/cli/tests/dry_run.rs b/crates/cli/tests/dry_run.rs index eccb848..bc574b8 100644 --- a/crates/cli/tests/dry_run.rs +++ b/crates/cli/tests/dry_run.rs @@ -256,12 +256,24 @@ fn a_plan_refuses_what_the_run_would_refuse_and_nothing_else() { .expect("the tool must run"); assert_eq!(out.status.code(), Some(0), "a batch script is started by Windows: {}", String::from_utf8_lossy(&out.stderr)); + // A library is a whole PE image, and still not a program: its header says so, and Windows + // refuses it. The state, not only the code, because a missing file exits 2 as well. + let library = injected_library(); + assert!(library.is_file(), "this needs {}, which `cargo test` does not build", library.display()); + let out = Command::new(env!("CARGO_BIN_EXE_chrono")) + .args(["run", &library.display().to_string(), "--at", "2038-01-19T03:14:07", "--dry-run", "--json"]) + .output() + .expect("the tool must run"); + let plan = String::from_utf8_lossy(&out.stdout); + assert_eq!(out.status.code(), Some(2), "{plan}"); + assert!(plan.contains(r#""state":"not_a_program""#), "a library is not a program: {plan}"); + let _ = std::fs::remove_dir_all(&dir); } -/// The premise under the refusal above, checked on a real run: the text file really does fail to -/// launch with exit 2. Should the core ever start such a file, the plan's refusal would become the -/// lie, and this is what would say so. +/// The premise under the refusal above, checked on a real run: the text file and the library really +/// do fail to launch with exit 2. Should the core ever start such a file, the plan's refusal would +/// become the lie, and this is what would say so. #[test] fn a_real_run_of_a_file_windows_will_not_start_exits_two() { let library = injected_library(); @@ -275,17 +287,19 @@ fn a_real_run_of_a_file_windows_will_not_start_exits_two() { let dir = scratch("real-not-a-program"); let note = dir.join("note.txt"); std::fs::write(¬e, "a note, not a program").expect("a text file"); - let out = Command::new(env!("CARGO_BIN_EXE_chrono")) - .args(["run", ¬e.display().to_string(), "--at", "2038-01-19T03:14:07", "--ticks", "1"]) - .output() - .expect("the tool must run"); - assert_eq!( - out.status.code(), - Some(2), - "stdout: {} stderr: {}", - String::from_utf8_lossy(&out.stdout), - String::from_utf8_lossy(&out.stderr) - ); + for target in [note.display().to_string(), library.display().to_string()] { + let out = Command::new(env!("CARGO_BIN_EXE_chrono")) + .args(["run", &target, "--at", "2038-01-19T03:14:07", "--ticks", "1"]) + .output() + .expect("the tool must run"); + assert_eq!( + out.status.code(), + Some(2), + "{target}: stdout: {} stderr: {}", + String::from_utf8_lossy(&out.stdout), + String::from_utf8_lossy(&out.stderr) + ); + } let _ = std::fs::remove_dir_all(&dir); } diff --git a/crates/cli/tests/usage.rs b/crates/cli/tests/usage.rs index d92555d..1f38042 100644 --- a/crates/cli/tests/usage.rs +++ b/crates/cli/tests/usage.rs @@ -43,24 +43,27 @@ fn every_way_of_asking_for_help_is_answered_with_usage_and_exit_zero() { } /// Only the first word after a command asks for its help. Further along, `--help` can belong to the -/// target, and taking it as a question would replace the session with a usage text. +/// target, and taking it as a question would replace the session with a usage text. The plan names +/// the arguments the application would receive, so it shows `--help` arriving there - the tool itself +/// as the target, because the plan has to find a program to print one. #[test] fn a_help_flag_meant_for_the_target_is_passed_on_and_not_answered() { - let out = chrono(&["run", r"C:\definitely\not\here\app.exe", "--args", "--help", "--dry-run"]); - assert_eq!( - out.status.code(), - Some(2), - "the plan must be resolved (and refuse the missing target), not replaced by usage: {}", - text(&out) - ); + let out = chrono(&["run", env!("CARGO_BIN_EXE_chrono"), "--args", "--help", "--dry-run", "--json"]); + let said = text(&out); + assert_eq!(out.status.code(), Some(0), "the plan, not the usage: {said}"); + assert!(said.contains(r#""args":["--help"]"#), "the application must be handed --help: {said}"); + assert!(!said.contains("usage:"), "{said}"); } -/// `--at` takes the next word, so `--at --dry-run` gave it a flag. The refusal names the flag. +/// `--at` takes the next word, so `--at --dry-run` gave it a flag, and `--at -h` the help flag, whose +/// dash made it a relative moment. The refusal names the flag. #[test] fn a_flag_where_the_moment_should_be_is_named_as_a_flag() { - let out = chrono(&["run", r"C:\definitely\not\here\app.exe", "--at", "--dry-run"]); - let said = text(&out); - assert_eq!(out.status.code(), Some(1), "{said}"); - assert!(said.contains("--at needs a moment") && said.contains("'--dry-run'"), "{said}"); - assert!(!said.contains("shift needs"), "{said}"); + for flag in ["--dry-run", "-h"] { + let out = chrono(&["run", r"C:\definitely\not\here\app.exe", "--at", flag]); + let said = text(&out); + assert_eq!(out.status.code(), Some(1), "{flag}: {said}"); + assert!(said.contains("--at needs a moment") && said.contains(&format!("'{flag}'")), "{flag}: {said}"); + assert!(!said.contains("shift needs"), "{flag}: {said}"); + } }