fix: resolved invalid binaries caused by NUL padding in copy_and_replace_cstring_placeholder (#2342) - #2503
fix: resolved invalid binaries caused by NUL padding in copy_and_replace_cstring_placeholder (#2342)#2503doraem-on wants to merge 8 commits into
Conversation
…erve suffix merging and prevent embedded NULs
|
Hi @doraem-on thank you for working on this! I think it's a really interesting approach. Do you have any idea on how compatible this is? Are we confident this works everywhere? We have thought that maybe we could add a flag to paths.json / rattler-build to enable this only when Rust / Go are involved for the time being. |
|
Hi @wolfv, Thanks for bringing up the compatibility concerns. You are absolutely right—applying slash padding would break compatibility with older C/C++ packages that strictly rely on \0 string terminators to identify path lengths in memory. To fix this securely, I've implemented the exact approach you suggested: I've added an optional c_string boolean flag to the paths.json entry schema (exposed across both Rust and the Python bindings).
The changes have been pushed to the branch and updated description ,let me know what you think of this implementation and really grateful for your time in reviewing the code. |
Description
Fixes #2342
Motivation and Context:
This PR fixes the issue where
copy_and_replace_cstring_placeholderresulted in invalid binaries due to\0string padding, specifically breaking Go and Rust packages built withrattler-build.When replacing a longer prefix placeholder with a shorter target prefix, we must pad the size difference so the total binary byte size doesn't change.
The core issue with the old approach (NUL padding at the end):
Padding the end of the replaced C-string with
\0bytes caused two major bugs in modern binaries:SHF_MERGEOptimization: The linker optimizes overlapping string suffixes to save space. Because the old code shifted the suffix to the left and padded\0at the end, any references pointing to the optimized suffix offset would now point to trailing NUL bytes (\0\0\0) instead of the characters.CString::new()would panic because the statically sized slice bounds (&[u8]) now enclosed embedded NULs in the middle of length-prefixed strings.The Implementation:
Applying Slash (
/) padding universally breaks backward compatibility for standard C/C++ binaries that strictly rely on\0string terminators to identify path lengths in memory. To solve this securely:c_string): We've added an optionalc_stringboolean flag to thepaths.jsonentry schema (exposed in both Rust and the Python bindings).c_stringisNoneorTrue, the prefix replacement algorithm safely falls back to standard NUL padding. This preserves 100% legacy behavior and guarantees absolute backward compatibility for existing C/C++ packages.c_string: false, we pad the difference directly on the target prefix using slashes (/). For example,PLACEHOLDER/depbecomes/short///////dep.depstays at its exact original memory offset, preserving all linker optimizations.rattler-buildcan now selectively enable this exclusively for modern languages.How Has This Been Tested?
crates/rattler/src/install/link.rsto rigorously test both padding behaviors down to the byte array.pytest --doctest-modulespasses for Python bindings by conditionally outputting thec_stringflag only when explicitly passed.cargo test --workspaceto ensure no regressions.cargo fmt,cargo clippy) and Python (ruff format,ruff check).AI Disclosure
Tools: Gemini
Checklist: