Emscripten metadata file size is wrong

[juntyr]

I tried this code:

std::fs::read("my-file")

for a file that is <1MB large.

I expected to see this happen: The read should allocate a vec that is as large as the file content. The heap size in Emscripten should not meaningfully change.

Instead, this happened: The Emscripten heap grows by 1.8GB

I looked into the implementation of fs::read. Since it uses the file metadata size to pre-allocate the output vector, I debug printed the file size, which is around 1.8GB.

Since my Rust code is running inside Pyodide 0.25, I also checked Python's os.stat of the file, which reports the correct file size. Reading the file in Python also succeeds without the enormous over-allocation. It seems that this is a Rust-specific bug in getting the correct file size information from Emscripten.

Meta

My Rust toolchain is pinned to nightly-2024-07-21, the last nightly for 1.81.

[workingjubilee]

uhh does this reproduce on the latest nightly?

[juntyr]

uhh does this reproduce on the latest nightly?

Yes, unfortunately it still reproduces on rustc 1.83.0-nightly (eb4e23467 2024-10-09).

My current workaround is the following:

- let bytes = std::fs::read(path)?;
+ struct FileNoSize(std::fs::File);
+
+ impl std::io::Read for FileNoSize {
+     fn read(&mut self, buf: &mut [u8]) -> std::io::Result<usize> {
+         self.0.read(buf)
+     }
+ }
+
+ let mut file = FileNoSize(std::fs::File::open(path)?);
+
+ let mut bytes = Vec::new();
+ std::io::Read::read_to_end(&mut file, &mut bytes)?;

[workingjubilee]

It seems we just call fstat64? It's not clear how that's not working out for us.

[workingjubilee]

We could have an incorrect definition of the Emscripten stat64 structure?

[kadiwa4]

emscripten's definition of stat64 changed in version 3.1.42, which is accounted for in the libc crate. Maybe that can lead to version-mismatch problems? Which version are you using? From this file, it seems like the Rust standard library still targets 2.0.5.

[juntyr]

Pyodide v0.25 uses Emscripten 3.1.46, so that would be after the change. The fix to libc was in rust-lang/libc@63b0d67 and released in v0.2.148, which Rust is already using. Huh.

[kadiwa4]

When building with a fixed version of libc, it does a check in the build script to see if the installed version of emscripten uses the old or new definition. I assume you downloaded the emscripten target from rustup, which would mean that you're using a precompiled standard library. So the question is really which version of emscripten is installed on the builders that built the standard library you are using.

[juntyr]

Do you think that using -Zbuild-std would help here?

[kadiwa4]

Sure, give it a try!

[juntyr]

Yes, -Zbuild-std works both on the latest nightly and my pin.

How could this footgun be avoided?

[kadiwa4]

Not sure; maybe the emscripten version used in src/ci/docker/scripts/emscripten.sh could be updated and then a minimum version requirement for the emscripten toolchain could be added.
Note that I'm not a target maintainer or anything, just a fellow contributor :)

[juntyr]

Perhaps we could add some warning to the target list about this issue? Could someone point me in the direction of which file to edit?

Cc @hoodmane: Pyodide packages using Rust should be built with build-std to ensure that they use the correct Emscripten ABI, otherwise e.g. file size metadata is garbage

[workingjubilee]

@juntyr Here, you would need to add an entry because the emscripten target doesn't have a doc for it yet, but you can add appropriate caveats to this page and in this folder:

• https://github.com/rust-lang/rust/blob/master/src/doc/rustc/src/platform-support.md
• https://github.com/rust-lang/rust/tree/master/src/doc/rustc/src/platform-support

[workingjubilee]

@kleisauke It seems you needed to also update https://github.com/rust-lang/rust/blob/master/src/ci/docker/scripts/emscripten.sh to finish the patch

[kleisauke]

Ah, I think this is also the reason why #116655 failed without -Zbuild-std.

I just opened (draft) PR rust-lang/libc#3962 for this in libc to bump emsdk and make the changes in rust-lang/libc@63b0d67 unconditional.

[hoodmane]

Thanks @juntyr for reporting this! I will add a test for this to Pyodide.

Pyodide packages using Rust should be built with build-std to ensure that they use the correct Emscripten ABI, otherwise e.g. file size metadata is garbage

Yeah does sound like we should add -Zbuild-std back in to the rust flags. It's definitely not safe to build against one version of Emscripten and then link against a different one. I thought we'd been passing it all along, but looking through Pyodide's git history I got rid of it almost immediately after adding rust support. We should probably encourage everyone targeting Emscripten to pass -Zbuild-std. Not doing that requires ABI stability.

Seems like I got rid of -Zbuild-std because I was hoping to get off of nightly, but we're stuck on nightly anyways because we have to pass -Z link-native-libraries=no because Emscripten fails if you pass -lc when linking a shared library.
emscripten-core/emscripten#16680 (comment)

It would be really nice if Emscripten could adopt an abi versioning scheme so that other projects could know what is going on.
@sbc100
emscripten-core/emscripten#15917

[hoodmane]

By the way, is it possible for us to build the rust standard library once against our emscripten compiler and point all packages we build to this one copy of the rust stdlib? That way we would only have to rebuild when we change the version of rust or emscripten as opposed to every time we build any rust package. It would save a lot of CI time.

[juntyr]

By the way, is it possible for us to build the rust standard library once against our emscripten compiler and point all packages we build to this one copy of the rust stdlib? That way we would only have to rebuild when we change the version of rust or emscripten as opposed to every time we build any rust package. It would save a lot of CI time.

When building the Rust crates, you could pass --sysroot <PATH> to a prebuilt sysroot. To each crate, it would look again just normal (no build-std anymore).

I'm not sure how to prepare the sysroot itself, but there is a crate https://crates.io/crates/cargo-sysroot that supports building with custom sysroots, so it might be useful to look at.

[apiraino]

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-high

[kleisauke]

PR #131533 unfortunately didn't resolve the issue. https://github.com/rust-lang/rust/blob/master/src/ci/docker/scripts/emscripten.sh looks like a remnant of the now-removed asmjs-unknown-emscripten target (see PR #117338). As a result, the emscripten_new_stat_abi cfg added in rust-lang/libc@63b0d67 was not enabled when the prebuilt std was built.

We may need to either install emsdk in this Dockerfile:
https://github.com/rust-lang/rust/blob/master/src/ci/docker/host-x86_64/dist-various-1/Dockerfile

Or alternatively, always enforce the emscripten_new_stat_abi cfg in libc and require Emscripten 3.1.42 or later in Rust, see:
rust-lang/libc#3962 (comment)

Standalone reproducer:

$ curl https://sh.rustup.rs -sSf | sh -s -- -y --profile minimal --target wasm32-unknown-emscripten --default-toolchain nightly-2024-12-09 --component rust-src
$ source "$HOME/.cargo/env"
$ cargo new foo
$ cd foo
$ cat <<EOT > src/main.rs
use std::io;
use std::fs;

fn main() -> io::Result<()> {
    let metadata = fs::metadata("hello.txt")?;
    println!("{}", metadata.len());
    Ok(())
}
EOT
$ RUSTFLAGS="-Clink-arg=-sNODERAWFS" cargo build --release -Zbuild-std=panic_abort,std --target wasm32-unknown-emscripten
$ echo "Hello, world!" > hello.txt
$ node target/wasm32-unknown-emscripten/release/foo.js
14
$ RUSTFLAGS="-Clink-arg=-sNODERAWFS" cargo build --release --target wasm32-unknown-emscripten
$ node target/wasm32-unknown-emscripten/release/foo.js
1733771096