Skip to content

rustc: Prepend a length to all metadata #19563

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

Closed
wants to merge 1 commit into from

Conversation

alexcrichton
Copy link
Member

One of the causes of #19501 was that the metadata on OSX was getting corrupted.
For any one particular invocation of the compiler the metadata file inside of an
rlib archive would have extra bytes appended to the end of it. These extra bytes
end up confusing rbml and have it run off the end of the array (resulting in the
out of bounds detected).

This commit prepends the length of metadata to the start of the metadata to
ensure that we always slice the precise amount that we want, and it also
un-ignores the test from #19502.

Closes #19501

v.insert(0, (len >> 8) as u8);
v.insert(0, (len >> 16) as u8);
v.insert(0, (len >> 24) as u8);
return v;
Copy link
Member

Choose a reason for hiding this comment

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

aren't four separate calls to insert going to move the contents over to the right four separate times?

If your intent is that this be temporary code that we remove once the source of the garbage is identified, then that cost may be fine. But I suspect we would be better off leaving the length in even after we identify the current source of the garbage.

@pnkfelix
Copy link
Member

pnkfelix commented Dec 5, 2014

r=me after you either replace the four v.insert(0, ...) calls with something less expensive (or provide an argument for why the cost is reasonable here -- I'm working under the assumption that the vector can be quite large).

@alexcrichton
Copy link
Member Author

I'm not actually quite sure if we have a better way of doing so sadly. I don't know of a way to push many items onto a vector from the front, and we can't reserve space ahead-of-time as it may corrupt the rbml data. I do suspect that metadata would be quite large, but when compared to the runtime of the rest of the compiler I suspect it will not be so bad.

Do you have an idea for how to do so here? We could allocate two vectors, but that does represent a significant portion of memory (maybe not so bad), and resorting to unsafe code seems like overkill for this.

@lifthrasiir
Copy link
Contributor

@alexcrichton How about simply reserving four bytes before writing the actual metadata? Something in the line of this:

pub fn encode_metadata(parms: EncodeParams, krate: &ast::Crate) -> Vec<u8> {
    let mut wr = SeekableMemWriter::new();

    // Reserve 4 bytes for the length.
    wr.write_be_u32(0).unwrap();

    encode_metadata_inner(&mut wr, parms, krate);

    // Now go back to the beginning and fill the length.
    let len = (wr.tell().unwrap() - 4).to_u32().unwrap();
    wr.seek(0, SeekStyle::SeekSet).unwrap();
    wr.write_be_u32(len).unwrap();

    wr.unwrap()
}

@alexcrichton
Copy link
Member Author

That was what I initially thought, but sadly if we reserve it up front then all of the offsets in the RBML tags will be off-by-four and probably corrupted :(

@alexcrichton
Copy link
Member Author

Relatively speaking, shifting 4 bytes onto a 100MB array takes ~47ms on my machine:

extern crate test;                                                

#[bench]                                                          
fn foo(b: &mut test::Bencher) {                                   
    b.iter(|| {                                                   
        let mut v = Vec::from_fn(100 * 1024 * 1024, |_| 0u8);     
        v.insert(0, 1);                                           
        v.insert(0, 1);                                           
        v.insert(0, 1);                                           
        v.insert(0, 1);                                           
        v                                                         
    });                                                           
}                                                                 
running 1 test
test foo ... bench:  47444184 ns/iter (+/- 343621)

test result: ok. 0 passed; 0 failed; 0 ignored; 1 measured

@lifthrasiir
Copy link
Contributor

@alexcrichton Ah, I forgot the existence of the index tables. :S Still, it might be possible to have a pseudo-RBML tag to do that, i.e. tag_total_size followed by 84 (the length, 4 bytes) followed by exactly four bytes of the total size of the metadata. tag_total_size is still a part of the metadata, so it wouldn't disrupt the indices.

@pnkfelix
Copy link
Member

pnkfelix commented Dec 6, 2014

@alexcrichton ah yes; I can think of new methods to add to Vec to support this case, but we need not block this PR on that high level design work

One of the causes of rust-lang#19501 was that the metadata on OSX was getting corrupted.
For any one particular invocation of the compiler the metadata file inside of an
rlib archive would have extra bytes appended to the end of it. These extra bytes
end up confusing rbml and have it run off the end of the array (resulting in the
out of bounds detected).

This commit prepends the length of metadata to the start of the metadata to
ensure that we always slice the precise amount that we want, and it also
un-ignores the test from rust-lang#19502.

Closes rust-lang#19501
bors added a commit that referenced this pull request Dec 9, 2014
One of the causes of #19501 was that the metadata on OSX was getting corrupted.
For any one particular invocation of the compiler the metadata file inside of an
rlib archive would have extra bytes appended to the end of it. These extra bytes
end up confusing rbml and have it run off the end of the array (resulting in the
out of bounds detected).

This commit prepends the length of metadata to the start of the metadata to
ensure that we always slice the precise amount that we want, and it also
un-ignores the test from #19502.

Closes #19501
@bors bors closed this Dec 10, 2014
@alexcrichton alexcrichton deleted the issue-19501 branch December 28, 2014 06:45
lnicola pushed a commit to lnicola/rust that referenced this pull request Apr 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

issue-13560.rs is flaky on 64-bit OSX
4 participants