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

vec::consume/consume_mut are probably not failsafe. #3603

Closed
brson opened this issue Sep 27, 2012 · 10 comments
Closed

vec::consume/consume_mut are probably not failsafe. #3603

brson opened this issue Sep 27, 2012 · 10 comments
Labels
E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.

Comments

@brson
Copy link
Contributor

brson commented Sep 27, 2012

Code looks like this:

fn consume<T>(+v: ~[T], f: fn(uint, +v: T)) unsafe {                                                                                                                          
    do as_imm_buf(v) |p, ln| {                                                                                                                                                
        for uint::range(0, ln) |i| {                                                                                                                                          
            let x <- *ptr::offset(p, i);                                                                                                                                      
            f(i, move x);                                                                                                                                                     
        }                                                                                                                                                                     
    }                                                                                                                                                                         

    raw::set_len(v, 0);                                                                                                                                                       
}         

What happens if the callback fails in the middle of the loop?

@nikomatsakis
Copy link
Contributor

So, I think it's ok today---each item that is moved will be zero'd. However, it's an important consideration if we ever wanted to stop zeroing out things to indicate that they have been moved (which I do).

@brson
Copy link
Contributor Author

brson commented Sep 27, 2012

I thought that might be the situation. Deserves a test case.

@nikomatsakis
Copy link
Contributor

Indeed!

@brson
Copy link
Contributor Author

brson commented Sep 28, 2012

I added failure tests for most of the higher-order vec functions and they all passed. Yay.

@brson brson closed this as completed Sep 28, 2012
@brson brson reopened this Sep 30, 2012
@brson
Copy link
Contributor Author

brson commented Sep 30, 2012

This is still not correct. Even though it doesn't crash a failure leaves the vector filled with nulls pretending to be valid elements (set_len never gets called). I will revisit this later.

@brson
Copy link
Contributor Author

brson commented Sep 30, 2012

Argh! but the frame owns the vector so it's not possible to be referred to anywhere else. This is why we are writing a safe language.

@brson brson closed this as completed Sep 30, 2012
@jruderman
Copy link
Contributor

Do T's destructors get called the correct number of times?

@brson
Copy link
Contributor Author

brson commented Oct 4, 2012

@jruderman I am not sure. Built in destructors definitely do, but I did not test with user-defined dtors.

@brson brson reopened this Oct 4, 2012
@bstrie
Copy link
Contributor

bstrie commented May 13, 2013

Here's how vec::consume looks today:

pub fn consume<T>(mut v: ~[T], f: &fn(uint, v: T)) {
    unsafe {
        do as_mut_buf(v) |p, ln| {
            for uint::range(0, ln) |i| {
                // NB: This unsafe operation counts on init writing 0s to the
                // holes we create in the vector. That ensures that, if the
                // iterator fails then we won't try to clean up the consumed
                // elements during unwinding
                let x = intrinsics::init();
                let p = ptr::mut_offset(p, i);
                f(i, util::replace_ptr(p, x));
            }
        }

        raw::set_len(&mut v, 0);
    }
}

That comment seems somewhat reassuring, but someone more qualified ought to determine if that means this issue is resolved.

@thestinger
Copy link
Contributor

We don't have catchable exceptions so it's safe. It always zeroes the drop flag for custom destructors.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 5, 2019
Changes:
````
Revert "tests: used_underscore_binding_macro: disable random_state lint."
Revert "Auto merge of rust-lang#3603 - xfix:random-state-lint, r=phansch"
rustup rust-lang#56837
rustup (don't know the exact PR unfortunately)
Add itertools to integration tests
tests: used_underscore_binding_macro: disable random_state lint.
Trigger `use_self` lint in local macros
Add run-rustfix where it already passes
rustup: rust-lang#55517
Make clippy work with parallel rustc
Add ui/for_kv_map test for false positive in rust-lang#1279
Update to latest compiletest-rs release
add testcase for rust-lang#3462
deps: bump rustc_tools_util version from 0.1.0 to 0.1.1 just in case...
Use compiletest's aux-build header instead of include macro
rustc_tool_utils: fix failure to create proper non-repo version string when used in crates on crates.io, bump version
rustfmt
UI test cleanup: Extract ifs_same_cond tests
Extract IteratorFalsePositives into option_helpers.rs
UI test cleanup: Extract for_kv_map lint tests
UI test cleanup: Extract lint from methods.rs test
Fix test for rust-lang#57250
Limit infinite_iter collect() check to known types
Some improvements to util documentation
Use hashset for name blacklist
Reformat random_state tests
Use node_id_to_type_opt instead of node_it_to_type in random_state
Check pattern equality while checking declaration equality
random_state lint
Move constant write checks to temporary_assignment lint
Use an FxHashSet for valid idents in documentation lint
Fix suggestion for unnecessary_ref lint
Update CONTRIBUTING.md for rustfix tests
Update .fixed files via update-references.sh
Run rustfix on first UI test
Use WIP branch for compiletest_rs
````
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 7, 2019
Changes:
````
Revert "tests: used_underscore_binding_macro: disable random_state lint."
Revert "Auto merge of rust-lang#3603 - xfix:random-state-lint, r=phansch"
rustup rust-lang#56837
rustup (don't know the exact PR unfortunately)
Add itertools to integration tests
tests: used_underscore_binding_macro: disable random_state lint.
Trigger `use_self` lint in local macros
Add run-rustfix where it already passes
rustup: rust-lang#55517
Make clippy work with parallel rustc
Add ui/for_kv_map test for false positive in rust-lang#1279
Update to latest compiletest-rs release
add testcase for rust-lang#3462
deps: bump rustc_tools_util version from 0.1.0 to 0.1.1 just in case...
Use compiletest's aux-build header instead of include macro
rustc_tool_utils: fix failure to create proper non-repo version string when used in crates on crates.io, bump version
rustfmt
UI test cleanup: Extract ifs_same_cond tests
Extract IteratorFalsePositives into option_helpers.rs
UI test cleanup: Extract for_kv_map lint tests
UI test cleanup: Extract lint from methods.rs test
Fix test for rust-lang#57250
Limit infinite_iter collect() check to known types
Some improvements to util documentation
Use hashset for name blacklist
Reformat random_state tests
Use node_id_to_type_opt instead of node_it_to_type in random_state
Check pattern equality while checking declaration equality
random_state lint
Move constant write checks to temporary_assignment lint
Use an FxHashSet for valid idents in documentation lint
Fix suggestion for unnecessary_ref lint
Update CONTRIBUTING.md for rustfix tests
Update .fixed files via update-references.sh
Run rustfix on first UI test
Use WIP branch for compiletest_rs
````
RalfJung pushed a commit to RalfJung/rust that referenced this issue May 19, 2024
Give `FileDescription::{read, write}` access to the `MiriInterpCx `

fixes rust-lang#3572
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.
Projects
None yet
Development

No branches or pull requests

5 participants