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

box: into_raw, from_raw functions #21318

Merged
merged 1 commit into from
Feb 2, 2015
Merged

Conversation

stepancheg
Copy link
Contributor

Functions are needed for safety and convenience.

It is a common pattern to use mem::transmute to convert between
Box and raw pointer, like this:

let b = Box::new(3);
let p = mem::transmute(b);
// pass `p` to some C library

After this commit, conversion can be written as:

let p = b.into_raw();

into_raw and from_raw functions are still unsafe, but they are
much safer than mem::transmute, because *raw functions do not
convert between incompatible pointers. For example, this likely
incorrect code can be successfully compiled:

let p: *mut u64 = ...
let b: Box<u32> = mem::transmute(p);

Using from_raw results in compile-time error:

let p: *mut u64 = ...
let b: Box<u32> = Box::from_raw(p); // compile-time error

into_raw and from_raw functions are similar to C++ std::unique_ptr
release function [1] and constructor from pointer [2].

[1] http://en.cppreference.com/w/cpp/memory/unique_ptr/release
[2] http://en.cppreference.com/w/cpp/memory/unique_ptr/unique_ptr

@rust-highfive
Copy link
Collaborator

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

@brson
Copy link
Contributor

brson commented Jan 17, 2015

Agree this is a common pattern.

r? @alexcrichton are there any alternate approaches to solving this problem?

If accepted this should have tests for boxed DSTs as well - I'm not actually sure whether they will work or not here.

@rust-highfive rust-highfive assigned alexcrichton and unassigned brson Jan 17, 2015
@Gankra
Copy link
Contributor

Gankra commented Jan 17, 2015

I am very in favour of this. transmutes are at best awful for semantics.

@stepancheg
Copy link
Contributor Author

I suddenly realized, that boxed is not tested, at least not with make check-stage0-alloc. Because mod is declared as:

#[cfg(not(test))]
pub mod boxed;

int liballoc/lib.rs.

How it should be dealt with?

@brson
Copy link
Contributor

brson commented Jan 18, 2015

@stepancheg This is a tricky case because Box must be a lang item because it currently relies on compiler magic. I'd suggest moving everything but the test mod into an inner impl module with cfg(test). Put these reexports in the boxed module:

#[cfg(not(test))] pub use self::impl::{HEAP, Box};
#[cfg(test)] pub use std::boxed::{HEAP, Box};

This will cause the lang item to be present at the desired path under normal builds and test builds.

@alexcrichton
Copy link
Member

We need to be pretty cautious adding inherent methods to Box<T> due to it shadowing any method on T itself. The current practice in the std::rc module is to add std::boxed::foo instead of an inherent method. Adding Box::from_raw seems fine to me, but I find it unusual that *const T is used instead of *mut T.

Could the documentation of these methods also be expanded? It would be useful to explain why the function is unsafe as well.

@stepancheg
Copy link
Contributor Author

Created separate PR that enables tests: #21446
@brson I decided to move tests into separate mod alloc::boxed_test to avoid reimports that complicate things too much.

@stepancheg
Copy link
Contributor Author

Updated PR with:

  • better docs
  • test for Box<Traits> conversion to and from raw pointer
  • const replaced with mut in from_raw
  • into_raw made global function instead of Box member

///
/// Function is unsafe, because improper use of this function may
/// lead to memory problems like double-free, for example if the
/// function is called twice on the same raw pointer.
Copy link
Member

Choose a reason for hiding this comment

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

It may be worth mentioning that the only valid pointers to pass to this function are those created from boxed::into_raw. I don't think we'd want to bake in any assumptions about how Box is allocated and letting others think that it can be used to free any old malloc memory.

@alexcrichton
Copy link
Member

cc @aturon, do you have thoughts about these APIs? Am I perhaps too paranoid about inherent methods on smart pointers?

Functions are needed for safety and convenience.

It is a common pattern to use `mem::transmute` to convert between
`Box` and raw pointer, like this:

```
let b = Box::new(3);
let p = mem::transmute(b);
// pass `p` to some C library
```

After this commit, conversion can be written as:

```
let p = boxed::into_raw(b);
```

`into_raw` and `from_raw` functions are still unsafe, but they are
much safer than `mem::transmute`, because *raw functions do not
convert between incompatible pointers. For example, this likely
incorrect code can be successfully compiled:

```
let p: *mut u64 = ...
let b: Box<u32> = mem::transmute(p);
```

Using `from_raw` results in compile-time error:

```
let p: *mut u64 = ...
let b: Box<u32> = Box::from_raw(p); // compile-time error
```

`into_raw` and `from_raw` functions are similar to C++ `std::unique_ptr`
`release` function [1] and constructor from pointer [2].

[1] http://en.cppreference.com/w/cpp/memory/unique_ptr/release
[2] http://en.cppreference.com/w/cpp/memory/unique_ptr/unique_ptr
@stepancheg
Copy link
Contributor Author

Updated PR with more docs and rebased to master.

@aturon
Copy link
Member

aturon commented Feb 1, 2015

I'm happy to move forward with this as an #[unstable] pair of free/static fns, as they are now.

@alexcrichton
Copy link
Member

@bors: r+ dadd97c

@alexcrichton
Copy link
Member

Thanks!

/// lead to memory problems like double-free, for example if the
/// function is called twice on the same raw pointer.
#[unstable(feature = "alloc",
reason = "may be renamed of moved out of Box scope")]
Copy link
Contributor

Choose a reason for hiding this comment

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

s/of/or ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sinistersnare fixed, thanks.

@stepancheg
Copy link
Contributor Author

Updated with typo fixed. @alexcrichton please, re-r+.

@alexcrichton
Copy link
Member

@bors: r+ 5a722f8

bors added a commit that referenced this pull request Feb 1, 2015
Functions are needed for safety and convenience.

It is a common pattern to use `mem::transmute` to convert between
`Box` and raw pointer, like this:

```
let b = Box::new(3);
let p = mem::transmute(b);
// pass `p` to some C library
```

After this commit, conversion can be written as:

```
let p = b.into_raw();
```

`into_raw` and `from_raw` functions are still unsafe, but they are
much safer than `mem::transmute`, because *raw functions do not
convert between incompatible pointers. For example, this likely
incorrect code can be successfully compiled:

```
let p: *mut u64 = ...
let b: Box<u32> = mem::transmute(p);
```

Using `from_raw` results in compile-time error:

```
let p: *mut u64 = ...
let b: Box<u32> = Box::from_raw(p); // compile-time error
```

`into_raw` and `from_raw` functions are similar to C++ `std::unique_ptr`
`release` function [1] and constructor from pointer [2].

[1] http://en.cppreference.com/w/cpp/memory/unique_ptr/release
[2] http://en.cppreference.com/w/cpp/memory/unique_ptr/unique_ptr
@bors
Copy link
Contributor

bors commented Feb 1, 2015

⌛ Testing commit 5a722f8 with merge ca4b967...

@bors bors merged commit 5a722f8 into rust-lang:master Feb 2, 2015
@phil-opp
Copy link
Contributor

phil-opp commented Feb 5, 2015

Why *mut T and not Unique<T>? From the documentation of Unique:

A wrapper around a raw *mut T that indicates that the possessor of this wrapper owns the referent.

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.

9 participants