Use mmap for faster warn_missing_py_init

[orlp]

One of my colleagues noticed that warn_missing_py_init is quite slow, reading the full file into memory (for context: the Polars debug .so is ~4GB).

I think it's quite likely that goblin only needs to parse a portion of the file, so I think using mmap here makes sense.

[konstin]

Is that sound, does it guarantee non of the rust invariants are being violated? If so, can you write a safety comment on why it it safe to do?

[orlp]

@konstin Please see https://docs.rs/memmap2/0.9.9/memmap2/struct.Mmap.html#safety.

I'm not exactly sure what kind of safety comment you'd be looking for. It is technically inherently 'unsound' by the Rust abstract model if someone edits the file in the file system at the same time. But mmap is very widely used regardless, including in the Rust project itself, e.g. https://github.com/rust-lang/rust/blob/94a0cd15f5976fa35e5e6784e621c04e9f958e57/src/tools/rust-analyzer/crates/proc-macro-srv/src/dylib.rs#L110.

[konstin]

I'm afraid I'm not comfortable with using mmap like that in maturin. For context, the only production usage of unsafe we have in maturin currently is env::set_var and that also only because it used to be safe in a previous edition.

Applications must consider the risk and take appropriate precautions when using file-backed maps. Solutions such as file permissions, locks or process-private (e.g. unlinked) files exist but are platform specific and limited.

As long as we can't guarantee that the artifact is not actually modified by some means, we risk UB, and that's a risk that's not worth it for this performance win.

[orlp]

@konstin Which risks are you worried about? 'UB' is not a risk in of itself, it's a binary property of a program that states the abstract model of Rust was violated, a risk needs an actual attacker model, consequence and probability of exploitation.

Mmap is already widely used in the Rust compiler: https://github.com/search?q=repo%3Arust-lang%2Frust%20mmap&type=code. I'd really like to know which risks in particular you're worried about that are unacceptable for maturin but are apparently acceptable for the Rust compiler.

[konstin]

I don't know which measure or soundness considerations the rust-lang/rust repo has around mmap, but the documentation you linked at https://docs.rs/memmap2/0.9.9/memmap2/struct.Mmap.html#safety is very clear that unless we guarantee that the underlying file doesn't change, this is unsound. I assume those mmap calls ensure this, but here I don't see that we've ensured that (yet).

With rust being safe by default, using unsafe inverts the language contract: We need to proof and document at the site where we use the unsafe that we're keeping up the contract, and should also document why the unsafe is necessary in the first place, usually with a // SAFETY: comment.

[orlp]

@konstin "Measure of soundness" is the wrong measure. The API is fundamentally unsound. You can not prevent someone from mutating the file. There also is no alternative without re-writing the entire parser to be Read based rather than &[u8] based.

I'm perfectly aware of how Rust works with unsafe. You are however holding me to an impossible standard. There is no safety comment I could possibly write containing the contract we uphold, because we can not possibly guarantee that the contract is upheld. This is no different in the Rust compiler.

However, would you be satisfied with the following logic?

// SAFETY: technically this is unsound if the file gets modified while we hold the
// immutable &[u8] slice. However, considering we're reading a dynamic library
// with executable code intended to be run, should a malicious agent have write
// access to it, arbitrary code execution is already trivially achieved. So there
// is no reasonable attacker model where this would lead to exploits that are
// otherwise impossible.
let mmap = unsafe { memmap2::Mmap::map(&fd).context("mmap failed")? };

[konstin]

I meant more in the sense of, can we ensure that this is not a file that changes underneath the mmap if you do another cargo build while maturin is running? As I understand it, there's a lot of ecosystem usage, where generally those can either assume that the file doesn't change underneath them (they hold a lock, it's a temp dir, they are the only process in that docker container, etc.) or the file changing isn't catastrophic. I know this is unlikely with cargo being slow and how this check is timed, but the uv issue tracker has scared me that every timing and non-determinism bug that can happen, will happen.

[orlp]

can we ensure that this is not a file that changes underneath the mmap if you do another cargo build while maturin is running?

No, you can not in general. And should this be a problem (it's not, really), you're already in the woods because you call

maturin/src/auditwheel/repair.rs

Line 15 in e7922ab

let dep_analyzer = DependencyAnalyzer::new(sysroot).library_paths(ld_paths);

which does... you guessed it, mmap:

https://github.com/messense/lddtree-rs/blob/e3ab19edf39b4d2d9cc90058e53a7ec966c271ac/src/lib.rs#L133

I must re-iterate that I feel you're holding me to an impossibly high standard for this PR, considering your tool already does the exact same strategy I proposed elsewhere (as well as your dependencies in the Rust compiler, probably others).

[messense]

LGTM, thanks

[konstin]

for context: the Polars debug .so is ~4GB

While I agree that we should speed up maturin for large binaries, have you considered reducing the debug info, such as https://doc.rust-lang.org/cargo/guide/build-performance.html#reduce-amount-of-generated-debug-information? I found those changes to make building a lot faster, producing a 4GB file is a large task for rustc and the linker too.

[orlp]

The result would still be an 1.7 GB binary, so still significant overhead to fully read in warn_missing_py_init. Plus having to wait several minutes for a complete recompile with debug symbols should a debugger be needed is rather undesirable.