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

str::buf_as_slice includes (and reads) uninitialized memory in the slice #3843

Closed
brson opened this issue Oct 23, 2012 · 6 comments
Closed

Comments

@brson
Copy link
Contributor

brson commented Oct 23, 2012

As long as string slices expect to have an extra byte at the end this function cannot be expressed without copying. Here's how it's written:

1964-    /// Form a slice from a *u8 buffer of the given length without copying.
1965:    pub unsafe fn buf_as_slice<T>(buf: *u8, len: uint,
1966-                              f: fn(v: &str) -> T) -> T {
1967-        let v = (buf, len + 1);
1968-        assert is_utf8(::cast::reinterpret_cast(&v));
1969-        f(::cast::transmute(move v))
1970-    }
1971-

That extra byte in len + 1 is just random memory, which is_utf8 immediately reads.

@graydon
Copy link
Contributor

graydon commented Mar 25, 2013

curiously it seems nobody uses this at the moment; I agree it shouldn't read possibly-invalid memory, but can we either say len should be one-longer than the intended length, and/or len+1 must be valid memory (as a precondition)? Or is this a general relative of the 'maybe str should not include a trailing byte' bug?

@graydon
Copy link
Contributor

graydon commented Mar 25, 2013

non-critical for 0.6, de-milestoning

1 similar comment
@graydon
Copy link
Contributor

graydon commented Mar 25, 2013

non-critical for 0.6, de-milestoning

@brson
Copy link
Contributor Author

brson commented Mar 25, 2013

@graydon We can state some precondition in the docs, yes. It will be error prone, but I suppose that's the nature of unsafe.

Not really related - with region pointers I think these types of functions that temporarily cast an unsafe pointer to another type in a closure aren't a best practice. We ought to just fabricate a region (would be nice to have a dedicated 'unsafe region maybe?) and return it.

@bblum
Copy link
Contributor

bblum commented Jun 10, 2013

@brson An "unsafe" region wouldn't be necessary for that; you'd want to write fn buf_as_slice<'a>(&'a str, uint len) -> &'a str.

But I suggest this function shouldn't be removed entirely (perhaps replaced with a safe copy_slice(&str, uint) -> ~str). The only place it's used is in uv_error_to_io_error, in a helper function called c_str_to_static_slice -- which itself should be moved to the str module and written without using buf_as_slice.

@bblum
Copy link
Contributor

bblum commented Jun 10, 2013

I also note that the function is "currently safe" because its only caller ensures that there's a null in the appropriate spot.

@bblum bblum closed this as completed Jun 11, 2013
flip1995 pushed a commit to flip1995/rust that referenced this issue May 27, 2020
Add common lint tools doc

This PR starts adding some documentation about linting tools.

`Retrieving all methods of a type` is not covered at this time.

fixes partially: rust-lang#3843

changelog: none
tesuji pushed a commit to tesuji/rustc that referenced this issue Jun 9, 2020
…hansch,flip1995

Add doc for checking if type defines specific method

This PR adds documentation on how:
- check if a type defines a specific method
- check an expr is calling a specific method

closes: rust-lang#3843

changelog: none
RalfJung pushed a commit to RalfJung/rust that referenced this issue Aug 30, 2024
… r=RalfJung

Make TB tree traversal bottom-up

In preparation for rust-lang#3837, the tree traversal needs to be made bottom-up, because the current top-down tree traversal, coupled with that PR's changes to the garbage collector, can introduce non-deterministic error messages if the GC removes a parent tag of the accessed tag that would have triggered the error first.

This is a breaking change for the diagnostics emitted by TB. The implemented semantics stay the same.
RalfJung pushed a commit to RalfJung/rust that referenced this issue Aug 30, 2024
…e-gc, r=RalfJung

Make Tree Borrows Provenance GC compact the tree

Follow-up on rust-lang#3833 and rust-lang#3835. In these PRs, the TB GC was fixed to no longer cause a stack overflow. One test that motivated it was the test `fill::horizontal_line` in [`tiny-skia`](https://github.com/RazrFalcon/tiny-skia). But not causing stack overflows was not a large improvents, since it did not fix the fundamental issue: The tree was too large. The test now ran, but it required gigabytes of memory and hours of time (only for it to be OOM-killed 🤬), whereas it finishes within 24 seconds in Stacked Borrows. With this merged, it finishes in about 40 seconds under TB.

The problem in that test was that it used [`slice::chunked`](https://doc.rust-lang.org/std/primitive.slice.html#method.chunks) to iterate a slice in chunks. That iterator is written to reborrow at each call to `next`, which creates a linear tree with a bunch of intermediary nodes, which also fragments the `RangeMap` for that allocation.

The solution is to now compact the tree, so that these interior nodes are removed. Care is taken to not remove nodes that are protected, or that otherwise restrict their children.

I am currently only 99% sure that this is sound, and I do also think that this could compact even more. So `@Vanille-N` please also have a look at whether I got the compacting logic right.

For a more visual comparison, [here is a gist](https://gist.github.com/JoJoDeveloping/ae4a7f7c29335a4c233ef42d2f267b01) of what the tree looks like at one point during that test, with and without compacting.

This new GC requires a different iteration order during accesses (since the current one can make the error messages non-deterministic), so it is rebased on top of rust-lang#3843 and requires that PR to be merged first.
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

No branches or pull requests

3 participants