Skip to content
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.

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

rustc index out of bounds libserialize\leb128.rs #37803

Closed
fralalonde opened this issue Nov 16, 2016 · 22 comments · Fixed by #37931
Closed

rustc index out of bounds libserialize\leb128.rs #37803

fralalonde opened this issue Nov 16, 2016 · 22 comments · Fixed by #37931
Labels
regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@fralalonde
Copy link

thread 'rustc' panicked at 'index out of bounds: the len is 37008 but the index is 37008', ../src/libserialize\leb128.rs:47

kind of expected, after switching toolchain from 1.15 back to 1.14. cargo clean && cargo build "repaired it"

@michaelwoerister
Copy link
Member

Do you have some more information on how to reproduce this?

@seanmonstar
Copy link
Contributor

I hit this compiling hyper with 1.13 stable.

@michaelwoerister
Copy link
Member

Which version of hyper is that? I can't reproduce with latest master.

@seanmonstar
Copy link
Contributor

It was with master. I tried cargo clean and now it works. So rustc/cargo were using libs compiled when I tried this with 1.15nightly?

@michaelwoerister
Copy link
Member

Sounds a bit like cargo was re-using libraries that it should have re-compiled because the compiler has changed. cc @alexcrichton

@fralalonde
Copy link
Author

Sorry if I did not save the full backtrace - It was my first issue report.

I switched away from 1.15 nightly back to 1.14 nightly in order to solve
a serde macro issue.

Cargo cleaning and rebuilding fixed the issue.

I will make sure to run again and capture the output if it happens again.

On 11/16/2016 4:54 PM, Michael Woerister wrote:

Sounds a bit like cargo was re-using libraries that it should have
re-compiled because the compiler has changed. cc @alexcrichton
https://github.com/alexcrichton


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#37803 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AF50tGACE60wWhxk5GxEE67R_GOYeX3Lks5q-3uugaJpZM4K0TyF.

@alexcrichton
Copy link
Member

Sounds suspiciously like a Cargo bug! If there's a reproduction I'd be more than willing to dig in.

@alexcrichton
Copy link
Member

I believe I just reproduced this in Cargo itself:

$ cargo +nightly build
# ...
$ cargo +stable build -p cfg-if -v
   Compiling cfg-if v0.1.0
     Running `rustc /home/alex/.cargo/registry/src/github.com-1ecc6299db9ec823/cfg-if-0.1.0/src/lib.rs --crate-name cfg_if --crate-type lib -g -C metadata=72c1f992b13d5087 -C extra-filename=-72c1f992b13d5087 --out-dir /home/alex/code/cargo/wut/debug/deps --emit=dep-info,link -L dependency=/home/alex/code/cargo/wut/debug/deps --cap-lints allow`
error: internal compiler error: unexpected panic

note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports

thread 'rustc' panicked at 'index out of bounds: the len is 382871 but the index is 382871', ../src/libserialize/leb128.rs:46
note: Run with `RUST_BACKTRACE=1` for a backtrace.

error: Could not compile `cfg-if`.

Caused by:
  process didn't exit successfully: `rustc /home/alex/.cargo/registry/src/github.com-1ecc6299db9ec823/cfg-if-0.1.0/src/lib.rs --crate-name cfg_if --crate-type lib -g -C metadata=72c1f992b13d5087 -C extra-filename=-72c1f992b13d5087 --out-dir /home/alex/code/cargo/wut/debug/deps --emit=dep-info,link -L dependency=/home/alex/code/cargo/wut/debug/deps --cap-lints allow` (exit code: 101)

When executing with RUST_LOG=rustc_metadata the last few lines are:

INFO:rustc_metadata::loader: Rejecting via crate name
INFO:rustc_metadata::loader: metadata mismatch
INFO:rustc_metadata::creader: register crate `extern crate alloc as alloc`
INFO:rustc_metadata::creader: resolving crate `extern crate core as core`
INFO:rustc_metadata::creader: resolving crate `extern crate rustc_unicode as rustc_unicode`
INFO:rustc_metadata::creader: resolving crate `extern crate alloc as alloc`
INFO:rustc_metadata::creader: resolving crate `extern crate rand as rand`
INFO:rustc_metadata::creader: falling back to a load
INFO:rustc_metadata::loader: lib candidate: /home/alex/code/cargo/wut/debug/deps/librand-91edd4ba23bc52f2.rlib
INFO:rustc_metadata::loader: lib candidate: /home/alex/.multirust/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/librand-a4729905.rlib
INFO:rustc_metadata::loader: rlib reading metadata from: /home/alex/code/cargo/wut/debug/deps/librand-91edd4ba23bc52f2.rlib
INFO:rustc_metadata::loader: reading "librand-91edd4ba23bc52f2.rlib" => Duration { secs: 0, nanos: 54751 }

So what's happening here is:

  1. Cargo is populating the entire crate cache with a bunch of libraries on the nightly compiler.
  2. When switching compilers, Cargo decides to recompile everything.
  3. Cargo doesn't delete everything as it recompiles, rather incrementally builds up a new crate cache
  4. The compiler, given -L to that directory, will read crates in that directory.
  5. The compiler panics when reading a crate compiled by a future compiler.

So I take back what I said earlier about this likely being a Cargo bug, it seems like this may be a bug in the compiler trying to read stale metadata? I thought we had like 10 checks in place to ensure we didn't do that, but maybe it's still leaking through?

@alexcrichton alexcrichton added regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 17, 2016
@alexcrichton
Copy link
Member

I'm going to tag this as a stable-to-nightly regression as this appears to have been introduced in nightly. If I build cargo with beta and then switch to beta, the compiler doesn't ICE.

@alexcrichton
Copy link
Member

The panic on stable happens in MetadataBlob::get_root, just before we check version information.

Did we change how we encode version information?

cc @rust-lang/compiler

@eddyb
Copy link
Member

eddyb commented Nov 17, 2016

I incremented the header version number though... It should be impossible for older rustc to get that far.

@alexcrichton
Copy link
Member

@eddyb on stable it looks the same on master.

Perhaps we just need a PR to bump that?

@eddyb
Copy link
Member

eddyb commented Nov 17, 2016

Oh it looks like my changes have reached stable. I wonder if perhaps the rustc_version is in a precarious spot and/or has been moved or the serialization otherwise changed. This requires further investigation.

@michaelwoerister
Copy link
Member

Why is this even calling leb128? Are we using variable-length integers in the metadata header?

@nrc
Copy link
Member

nrc commented Nov 18, 2016

@michaelwoerister I assumed leb128 was the crate being built, not one used by the compiler.

@arielb1
Copy link
Contributor

arielb1 commented Nov 19, 2016

@eddyb

Sure enough. Now every change to CrateRoot requires us to update the LLVM version number, because checking the rustc version number requires parsing CrateRoot.

I think we should go back to JSON version guard.

@eddyb
Copy link
Member

eddyb commented Nov 19, 2016

Ahh that's what it was. We can put it at the start then, before/after the CrateRoot absolute address. I'd forgotten that the whole struct would have to be decoded.

@brson
Copy link
Contributor

brson commented Nov 22, 2016

What was the PR that modified the versioning scheme? I gather that something has changed about it. Can somebody explain more clearly what is going wrong here?

@eddyb
Copy link
Member

eddyb commented Nov 22, 2016

@brson I mistakenly put the version string inside a larger struct, somehow thinking that it being at the start helped, but if the whole struct (that is, CrateRoot) can't be decoded (e.g. because its definition changed) then you don't get to see the version string, whereas before it was separate.

The fix would be to separate it again, put it back at the start and leave it there. Or just encode/decode it before CrateRoot instead of as part of it. Either will work as long as it can't be affected by changes to the metadata schema.

The PR that introduced the new schema was #36551, although breakage didn't happen until CrateRoot changed later, for procedural macros and/or the name resolution changes (@jseyfried would know).

@jseyfried
Copy link
Contributor

#37463 removed the field macro_defs from CrateRoot for macro modularization.

@eddyb
Copy link
Member

eddyb commented Nov 22, 2016

@alexcrichton @brson I believe I have a fix ready. Does this affect beta too now, or just nightly?

@alexcrichton
Copy link
Member

@eddyb AFAIK it's just nightly, not beta or stable

bors added a commit that referenced this issue Nov 23, 2016
rustc_metadata: don't break the version check when CrateRoot changes.

In #36551 I made `rustc_version` a field of `CrateRoot`, but despite it being the first field, one could still break the version check by changing `CrateRoot` so older compilers couldn't fully decode it (e.g. #37463).

This PR fixes #37803 by moving the version string back at the beginning of metadata, right after the 32-bit big-endian absolute position of `CrateRoot`, and by incrementing `METADATA_VERSION`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants