Skip to content

Strip basedirs from cmdline args and direct-mode hash key #2711

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
solarispika wants to merge 3 commits into
base: main
Choose a base branch
from
+320 −4
Open

Strip basedirs from cmdline args and direct-mode hash key #2711

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ee7df57
hash: strip basedirs from cmdline args in HashKeyParams::compute
solarispika May 15, 2026
9049e7e
hash: collapse escaped backslashes in preprocessor output on Windows
solarispika May 15, 2026
456bef3
hash: strip basedirs from cmdline args in direct-mode hash key
solarispika May 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
149 changes: 146 additions & 3 deletions src/compiler/c.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1448,6 +1448,38 @@ impl pkg::ToolchainPackager for CToolchainPackager {
/// The cache is versioned by the inputs to `HashKeyParams::compute`.
pub const CACHE_VERSION: &[u8] = b"12";

/// Collapse `\\` byte pairs into a single `\`.
#[cfg(target_os = "windows")]
fn collapse_escaped_backslashes(input: &[u8]) -> Vec<u8> {
let mut iter = input.iter().peekable();
std::iter::from_fn(move || {
let &b = iter.next()?;
if b == b'\\' {
iter.next_if_eq(&&b'\\');
}
Some(b)
})
.collect()
}

/// On Windows, collapse C/C++ source-form `\\` escapes in preprocessor output
/// before basedirs prefix-stripping. MSVC keeps `__FILE__`-style paths in
/// source form, so `normalize_win_path` would otherwise map each `\\` to
/// `//` and basedirs (stored as `/...`) would never match.
#[cfg(target_os = "windows")]
fn maybe_collapse_escaped_backslashes<'a>(input: &'a [u8], basedirs: &[Vec<u8>]) -> Cow<'a, [u8]> {
if basedirs.iter().any(|b| !b.is_empty()) {
Cow::Owned(collapse_escaped_backslashes(input))
} else {
Cow::Borrowed(input)
}
}

#[cfg(not(target_os = "windows"))]
fn maybe_collapse_escaped_backslashes<'a>(input: &'a [u8], _basedirs: &[Vec<u8>]) -> Cow<'a, [u8]> {
Cow::Borrowed(input)
}

/// Environment variables that are factored into the cache key.
static CACHED_ENV_VARS: LazyLock<HashSet<&'static OsStr>> = LazyLock::new(|| {
[
Expand Down Expand Up @@ -1566,7 +1598,11 @@ impl<'a> HashKeyParams<'a> {
m.update(CACHE_VERSION);
m.update(self.language.as_str().as_bytes());
for arg in self.arguments {
arg.hash(&mut HashToDigest { digest: &mut m });
let arg_bytes = arg.as_encoded_bytes();

@06393993 06393993 May 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I am not a maintainer nor a contributor, but I am a user of sccache.

I am wondering if we should use OsString::as_encoded_bytes here with strip_basedirs and self.basedirs.

arg_bytes here seems to be bytes with platform specific encoding, while self.basedirs is in UTF-8, and can cause problems on Windows.

This is the doc for OsString::as_encoded_bytes:

The byte encoding is an unspecified, platform-specific, self-synchronizing superset of UTF-8. By being a self-synchronizing superset of UTF-8, this encoding is also a superset of 7-bit ASCII.

self.basedirs is initialized by storage.basedirs(). Let's follow how <DiskCache as Storage>::basedirs is implemented and initialized, we can tell it's ultimately from config.basedirs. Checking Config::from_env_and_file_configs, we can tell basedirs are populated from basedirs_raw by converting the path to Utf8TypedPathBuf before normalization and converting to bytes. Therefore, basedirs are encoded in UTF-8 not in a platform specific way.

I am wondering if the following is better:

            let arg = arg.to_str()
                .and_then(|arg| {
                    let arg = match strip_basedirs(arg.as_bytes(), self.basedirs) {
                        Cow::Owned(arg) => {
                            let arg = String::from_utf8(arg).ok()?;
                            let arg = OsString::from(arg);
                            Cow::Owned(arg)
                        }
                        Cow::Borrowed(arg) => {
                            let arg = str::from_utf8(arg).ok()?;
                            let arg = OsStr::new(arg);
                            Cow::Borrowed(arg)
                        }
                    };
                    Some(arg)
                })
                .unwrap_or(Cow::Borrowed(arg));
            arg.hash(&mut HashToDigest { digest: &mut m });

The same applies to the similar change in src/compiler/preprocessor_cache.rs. Thanks.

@solarispika solarispika May 25, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@06393993 Good catch on the encoding mismatch. On Windows OsString::as_encoded_bytes() is WTF-8 and self.basedirs is plain UTF-8, so formally they don't agree. In practice I don't think it bites, and the rewrite has costs that aren't obvious:

  1. WTF-8 only diverges from UTF-8 on unpaired surrogates. Paths coming out of real build tools are valid Unicode, so the bytes are the same there.
  2. strip_basedirs already normalises the input on Windows (lowercases ASCII, \ → /) and the non-ASCII branch decodes UTF-8 with an explicit fall-through for invalid sequences (src/util.rs:1090-1095). An unpaired surrogate just fails to match; it doesn't mis-match.
  3. Basedirs themselves can't represent non-Unicode paths. They go through String (file) or to_string_lossy() (env) into Utf8TypedPathBuf (src/config.rs:1175), so the needle is guaranteed UTF-8 and cannot match an ill-formed segment of the haystack on either path. Whatever mismatch the rewrite "fixes" isn't fixable at this layer anyway.
  4. The proposed shape changes the cache key. Hashing via OsString's Hash impl writes a length prefix; the current code writes raw bytes plus a null separator. Not behaviour-preserving, so it'd need CACHE_VERSION / FORMAT_VERSION bumps to avoid silent cache corruption.

If we want to be defensive about it without paying that price, the narrowest version I can see is:

arg.to_str()
   .map(str::as_bytes)
   .unwrap_or_else(|| arg.as_encoded_bytes())

Same bytes for valid Unicode, current behaviour otherwise. I'm not convinced it earns the extra ceremony, since the basedirs side already constrains what can ever match.

@sylvestre what do you prefer — leave it as-is, or take the to_str() fallback?

let stripped = strip_basedirs(arg_bytes, self.basedirs);
m.update(&stripped);
// Separator so adjacent args don't blur together.
m.update(&[0u8]);
}
for hash in self.extra_hashes {
m.update(hash.as_bytes());
Expand All @@ -1580,8 +1616,9 @@ impl<'a> HashKeyParams<'a> {
}
}

// Strip basedirs from preprocessor output if configured
let preprocessor_output_to_hash = strip_basedirs(self.preprocessor_output, self.basedirs);
let preprocessor_for_strip =
maybe_collapse_escaped_backslashes(self.preprocessor_output, self.basedirs);
let preprocessor_output_to_hash = strip_basedirs(&preprocessor_for_strip, self.basedirs);

m.update(&preprocessor_output_to_hash);
m.finish()
Expand Down Expand Up @@ -1761,6 +1798,112 @@ mod test {
assert_eq!(h1, hash_with_basedirs(preprocessed1, &multi_basedirs));
}

#[cfg(target_os = "windows")]
#[test]
fn test_hash_key_basedirs_windows_escaped_backslash_paths() {
// MSVC `__FILE__` stringification produces source-form paths with
// doubled backslashes; basedirs are stored already normalised to
// forward slashes by `Config::basedir`.
let args = ovec!["/c"];
let preprocessed1 =
b"_wassert(L\"x\", L\"c:\\\\users\\\\user1\\\\work\\\\xyz\\\\file.c\", 42);\n";
let preprocessed2 =
b"_wassert(L\"x\", L\"d:\\\\actions\\\\runner\\\\hex\\\\repo\\\\file.c\", 42);\n";
let basedirs = [
b"c:/users/user1/work/xyz".to_vec(),
b"d:/actions/runner/hex/repo".to_vec(),
];

let h1 = HashKeyParams::new("abcd", Language::C, &args, preprocessed1)
.with_basedirs(&basedirs)
.compute();
let h2 = HashKeyParams::new("abcd", Language::C, &args, preprocessed2)
.with_basedirs(&basedirs)
.compute();
assert_eq!(
h1, h2,
"Hashes should match after collapsing escaped backslashes"
);
}

#[cfg(target_os = "windows")]
#[test]
fn test_collapse_escaped_backslashes() {
// Trivial cases
assert_eq!(collapse_escaped_backslashes(b""), b"");
assert_eq!(collapse_escaped_backslashes(b"abc"), b"abc");

// The headline case: source-form path emitted by MSVC for __FILE__
// inside _wassert macros.
assert_eq!(
collapse_escaped_backslashes(b"\"c:\\\\users\\\\user1\\\\file.h\""),
b"\"c:\\users\\user1\\file.h\""
);

// Odd-length runs collapse pair-by-pair, leftmost first. A lone
// trailing backslash (no pair partner) stays untouched.
assert_eq!(collapse_escaped_backslashes(b"a\\b"), b"a\\b");
assert_eq!(collapse_escaped_backslashes(b"a\\\\\\b"), b"a\\\\b");
}

#[test]
fn test_hash_key_args_basedirs() {
// Args carry absolute paths into the cache key for any flag whose
// disposition ends up in `common_args` with the path on its own
// (Separated disposition). On MSVC, `-external:I <path>` is the
// canonical case. Verify that basedirs strip neutralises the
// per-workspace difference.
let basedirs = [
b"/home/user1/project".to_vec(),
b"/home/user2/project".to_vec(),
];
let preprocessed = b"int main() { return 0; }";

let args1: Vec<OsString> = ovec![
"-c",
"-external:I",
"/home/user1/project/include",
"-o",
"/home/user1/project/build/main.o"
];
let args2: Vec<OsString> = ovec![
"-c",
"-external:I",
"/home/user2/project/include",
"-o",
"/home/user2/project/build/main.o"
];

let h1 = HashKeyParams::new("abcd", Language::C, &args1, preprocessed)
.with_basedirs(&basedirs)
.compute();
let h2 = HashKeyParams::new("abcd", Language::C, &args2, preprocessed)
.with_basedirs(&basedirs)
.compute();
assert_eq!(
h1, h2,
"Args with workspace-relative absolute paths should hash equal under basedirs"
);

// Without basedirs, same args produce divergent hashes — proves args
// (not just preprocessor output) feed into the key.
let h1_nb = HashKeyParams::new("abcd", Language::C, &args1, preprocessed).compute();
let h2_nb = HashKeyParams::new("abcd", Language::C, &args2, preprocessed).compute();
assert_neq!(h1_nb, h2_nb);

// Sanity: adjacent args must not blur — the inter-arg separator
// matters. `["-Ia", "b"]` and `["-I", "ab"]` are distinct cmdlines.
let args_a: Vec<OsString> = ovec!["-Ia", "b"];
let args_b: Vec<OsString> = ovec!["-I", "ab"];
let h_a = HashKeyParams::new("abcd", Language::C, &args_a, preprocessed)
.with_basedirs(&basedirs)
.compute();
let h_b = HashKeyParams::new("abcd", Language::C, &args_b, preprocessed)
.with_basedirs(&basedirs)
.compute();
assert_neq!(h_a, h_b);
}

#[test]
fn test_language_from_file_name() {
fn t(extension: &str, expected: Language) {
Expand Down
175 changes: 174 additions & 1 deletion src/compiler/preprocessor_cache.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,7 +392,11 @@ pub fn preprocessor_cache_entry_hash_key(
m.update(&[FORMAT_VERSION]);
m.update(language.as_str().as_bytes());
for arg in arguments {
arg.hash(&mut HashToDigest { digest: &mut m });
let arg_bytes = arg.as_encoded_bytes();
let stripped = strip_basedirs(arg_bytes, basedirs);
m.update(&stripped);
// Separator so adjacent args don't blur together.
m.update(&[0u8]);
}
for hash in extra_hashes {
m.update(hash.as_bytes());
Expand Down Expand Up @@ -772,4 +776,173 @@ mod test {
"Hashes should be different without basedirs for files in different directories"
);
}

#[test]
fn test_preprocessor_cache_entry_hash_key_args_basedirs() {
#[cfg(target_os = "windows")]
use crate::util::normalize_win_path;
use std::fs;
use tempfile::TempDir;

// Two distinct workspace roots simulating two CI workdirs / worktrees.
let dir1 = TempDir::new().unwrap();
let dir2 = TempDir::new().unwrap();

let dir_bytes = |p: &Path| -> Vec<u8> {
let bytes = p.to_string_lossy().into_owned().into_bytes();
#[cfg(target_os = "windows")]
{
normalize_win_path(&bytes)
}
#[cfg(not(target_os = "windows"))]
{
bytes
}
};

let basedirs = vec![dir_bytes(dir1.path()), dir_bytes(dir2.path())];

// Same source content placed in each workspace at the same relative path.
let file1_path = dir1.path().join("test.c");
let file2_path = dir2.path().join("test.c");
let content = b"int main() { return 0; }";
fs::write(&file1_path, content).unwrap();
fs::write(&file2_path, content).unwrap();

// Args that embed the absolute workspace path. We test the
// Separated-disposition shape: `["-external:I", "<workspace>/include"]`
// — this is the form MSVC system-include flags take after sccache's
// arg parser normalizes them, and is the realistic case in CI today.
//
// (The Concatenated shape — `["-IC:\workspace\include"]` — is NOT
// caught by `strip_basedirs` since its boundary check rejects matches
// where the previous byte is alphanumeric. Real MSVC builds rarely
// hit this in `common_args` because `/I` goes to `preprocessor_args`,
// not `common_args`. Concatenated stripping is a future enhancement.)
let inc1_path: OsString = format!("{}/include", dir1.path().display()).into();
let inc2_path: OsString = format!("{}/include", dir2.path().display()).into();
let out1_path: OsString = format!("{}/build/test.o", dir1.path().display()).into();
let out2_path: OsString = format!("{}/build/test.o", dir2.path().display()).into();
let args1 = vec![
OsString::from("-c"),
OsString::from("-external:I"),
inc1_path,
OsString::from("-o"),
out1_path,
];
let args2 = vec![
OsString::from("-c"),
OsString::from("-external:I"),
inc2_path,
OsString::from("-o"),
out2_path,
];

let config = PreprocessorCacheModeConfig::activated();

// With basedirs, args carrying different absolute paths hash equal.
let h1 = preprocessor_cache_entry_hash_key(
"test_digest",
Language::C,
&args1,
&[],
&[],
&file1_path,
false,
config,
&basedirs,
)
.unwrap()
.unwrap();

let h2 = preprocessor_cache_entry_hash_key(
"test_digest",
Language::C,
&args2,
&[],
&[],
&file2_path,
false,
config,
&basedirs,
)
.unwrap()
.unwrap();

assert_eq!(
h1, h2,
"Direct-mode hash should be equal across workspaces when args paths are covered by basedirs"
);

// Without basedirs, the same args produce divergent hashes — proves
// the args (not just the input_file path) feed into the key.
let h1_nb = preprocessor_cache_entry_hash_key(
"test_digest",
Language::C,
&args1,
&[],
&[],
&file1_path,
false,
config,
&[],
)
.unwrap()
.unwrap();

let h2_nb = preprocessor_cache_entry_hash_key(
"test_digest",
Language::C,
&args2,
&[],
&[],
&file2_path,
false,
config,
&[],
)
.unwrap()
.unwrap();

assert_ne!(
h1_nb, h2_nb,
"Direct-mode hash should differ across workspaces when basedirs are not provided"
);

// Sanity: adjacent args don't blur into each other. `["-Ia", "b"]`
// must hash differently from `["-I", "ab"]`. Catches a regression
// where someone removes the inter-arg separator.
let args_a = vec![OsString::from("-Ia"), OsString::from("b")];
let args_b = vec![OsString::from("-I"), OsString::from("ab")];
let h_a = preprocessor_cache_entry_hash_key(
"test_digest",
Language::C,
&args_a,
&[],
&[],
&file1_path,
false,
config,
&basedirs,
)
.unwrap()
.unwrap();
let h_b = preprocessor_cache_entry_hash_key(
"test_digest",
Language::C,
&args_b,
&[],
&[],
&file1_path,
false,
config,
&basedirs,
)
.unwrap()
.unwrap();
assert_ne!(
h_a, h_b,
"Adjacent args must not blur — inter-arg separator missing?"
);
}
}