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

Add debug assertions to raw pointer methods testing for unaligned/NULL pointers #53871

Open
RalfJung opened this issue Aug 31, 2018 · 8 comments

Comments

@RalfJung
Copy link
Member

commented Aug 31, 2018

In #53783, we document more precisely the rules for the various methods that can be used to access memory through raw pointers. In particular, we clarify that the pointer must be non-NULL and aligned even when the access has size 0.

This issue is about helping people find bugs in libstd by adding a debug_assert! to all these methods testing that condition, similar to what I did for from_raw_parts. I suggest to add a helper method to raw pointers to test this, and also use that for from_raw_parts and from_raw_parts_mut.

This may uncover issues in libstd, uncovering misuses of these methods. Those should then be fixed.

@nitnelave

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2019

I might be interested in working on that. Could you help me with a starting point for that?

@RalfJung

This comment has been minimized.

Copy link
Member Author

commented Jan 21, 2019

You might want to start with https://doc.rust-lang.org/nightly/std/ptr/fn.write_bytes.html. It could get a check similar to

debug_assert!(data as usize % mem::align_of::<T>() == 0, "attempt to create unaligned slice");

To avoid code duplication, consider adding a (private) helper method in ptr.rs for this purpose, and using it in both the old places (from_raw_parts{,_mut}) and write_bytes.

Other candidates besides write_bytes are copy and copy_nonoverlapping. We could also go further, but maybe we should involve the libs team there... so let's get started with these 3.

@nitnelave

This comment has been minimized.

Copy link
Contributor

commented Jan 28, 2019

Sorry for the delay, it took me a while to free up some time. Quick questions:

  • I found write_bytes in libcore/ptr.rs. However, one is just a delegation to the intrinsic, and the other one is for *const T, which is just a call to the previous one. Should I hunt the intrinsic in the C code, or provide a rust wrapper that will check the assumptions and call the intrinsic?
  • I couldn't find a from_raw_parts in ptr.rs. The only ones I could find were for vec, string, slice and in libstd/sys/sgx/abi/usercalls/alloc.rs. Am I missing something?
@nitnelave

This comment has been minimized.

Copy link
Contributor

commented Jan 28, 2019

Ah, intrinsics are still written in Rust, but as code generation functions. Well, I guess adding debug assertions everywhere in the intrinsics would be counter-productive, so we're going for the wrapper? Although, I'm not sure how to cleanly hijack the interface: I have to keep using the same function name for the callers, so that I don't have to change their code, but unless I don't understand how it works, I also need to have the same signature including the function name for the code generation. How should I proceed?

@RalfJung

This comment has been minimized.

Copy link
Member Author

commented Jan 29, 2019

Ah good point, I forgot about these reexports.

I think the right way forward is to replace this line by a wrapper function that just calls the intrinsic. The docs should be moved from intrinsics.rs to ptr.rs. That move should be a total NOP.

The, in the 2nd step, you can add the debug_assert! in the delegating wrapper you just added.

couldn't find a from_raw_parts in ptr.rs

Sorry, I meant the one in slice.rs. It already has a debug_assert!, but that does not test as much as it could (i.e., it accepts NULL).

nitnelave added a commit to nitnelave/rust that referenced this issue Jan 30, 2019

Wrap write_bytes in a function. Move docs
This will allow us to add debug assertions.
See issue rust-lang#53871.

bors added a commit that referenced this issue Jan 30, 2019

Auto merge of #57997 - nitnelave:master, r=<try>
Wrap write_bytes in a function. Move docs

This will allow us to add debug assertions.
See issue #53871.

nitnelave added a commit to nitnelave/rust that referenced this issue Feb 5, 2019

Wrap write_bytes in a function. Move docs
This will allow us to add debug assertions.
See issue rust-lang#53871.

bors added a commit that referenced this issue Feb 15, 2019

Auto merge of #57997 - nitnelave:master, r=RalfJung
Wrap write_bytes in a function. Move docs

This will allow us to add debug assertions.
See issue #53871.

bors added a commit that referenced this issue Feb 15, 2019

Auto merge of #57997 - nitnelave:master, r=RalfJung
Wrap write_bytes in a function. Move docs

This will allow us to add debug assertions.
See issue #53871.

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Feb 17, 2019

Rollup merge of rust-lang#57997 - nitnelave:master, r=RalfJung
Wrap write_bytes in a function. Move docs

This will allow us to add debug assertions.
See issue rust-lang#53871.

Centril added a commit to Centril/rust that referenced this issue Feb 20, 2019

Rollup merge of rust-lang#57997 - nitnelave:master, r=RalfJung
Wrap write_bytes in a function. Move docs

This will allow us to add debug assertions.
See issue rust-lang#53871.

bors added a commit that referenced this issue Feb 22, 2019

Auto merge of #57997 - nitnelave:master, r=RalfJung
Wrap write_bytes in a function. Move docs

This will allow us to add debug assertions.
See issue #53871.
@RalfJung

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2019

@nitnelave Congrats, your patch landed. :) So with the preparation out of the way, let me know if you need any help with adding the debug assertions.

@nitnelave

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

@RalfJung

This comment has been minimized.

Copy link
Member Author

commented Apr 21, 2019

Cc #51713

Centril added a commit to Centril/rust that referenced this issue Jun 10, 2019

Rollup merge of rust-lang#61501 - RalfJung:intrinsics, r=rkruppe
get rid of real_intrinsics module

instead import intrinsics locally in their wrapper functions.

(These functions are wrapper functions as a preparation to fixing rust-lang#53871.)

Centril added a commit to Centril/rust that referenced this issue Jun 26, 2019

Rollup merge of rust-lang#62103 - RalfJung:debug-assert, r=alexcrichton
Add debug assertions to write_bytes and copy*

Looks like @nitnelave  went MIA in rust-lang#58783, so I am re-submitting their PR, tweaked just a bit. I took care to preserve commit authorship.

Cc rust-lang#53871

Centril added a commit to Centril/rust that referenced this issue Jun 27, 2019

Rollup merge of rust-lang#62103 - RalfJung:debug-assert, r=alexcrichton
Add debug assertions to write_bytes and copy*

Looks like @nitnelave  went MIA in rust-lang#58783, so I am re-submitting their PR, tweaked just a bit. I took care to preserve commit authorship.

Cc rust-lang#53871

Centril added a commit to Centril/rust that referenced this issue Jun 27, 2019

Rollup merge of rust-lang#62103 - RalfJung:debug-assert, r=alexcrichton
Add debug assertions to write_bytes and copy*

Looks like @nitnelave  went MIA in rust-lang#58783, so I am re-submitting their PR, tweaked just a bit. I took care to preserve commit authorship.

Cc rust-lang#53871

Manishearth added a commit to Manishearth/rust that referenced this issue Jul 2, 2019

Rollup merge of rust-lang#62103 - RalfJung:debug-assert, r=alexcrichton
Add debug assertions to write_bytes and copy*

Looks like @nitnelave  went MIA in rust-lang#58783, so I am re-submitting their PR, tweaked just a bit. I took care to preserve commit authorship.

Cc rust-lang#53871

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Jul 3, 2019

Rollup merge of rust-lang#62103 - RalfJung:debug-assert, r=alexcrichton
Add debug assertions to write_bytes and copy*

Looks like @nitnelave  went MIA in rust-lang#58783, so I am re-submitting their PR, tweaked just a bit. I took care to preserve commit authorship.

Cc rust-lang#53871

Centril added a commit to Centril/rust that referenced this issue Jul 4, 2019

Rollup merge of rust-lang#62103 - RalfJung:debug-assert, r=alexcrichton
Add debug assertions to write_bytes and copy*

Looks like @nitnelave  went MIA in rust-lang#58783, so I am re-submitting their PR, tweaked just a bit. I took care to preserve commit authorship.

Cc rust-lang#53871

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Jul 4, 2019

Rollup merge of rust-lang#62103 - RalfJung:debug-assert, r=alexcrichton
Add debug assertions to write_bytes and copy*

Looks like @nitnelave  went MIA in rust-lang#58783, so I am re-submitting their PR, tweaked just a bit. I took care to preserve commit authorship.

Cc rust-lang#53871

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Jul 4, 2019

Rollup merge of rust-lang#62103 - RalfJung:debug-assert, r=alexcrichton
Add debug assertions to write_bytes and copy*

Looks like @nitnelave  went MIA in rust-lang#58783, so I am re-submitting their PR, tweaked just a bit. I took care to preserve commit authorship.

Cc rust-lang#53871

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Jul 15, 2019

Rollup merge of rust-lang#62103 - RalfJung:debug-assert, r=alexcrichton
Add debug assertions to write_bytes and copy*

Looks like @nitnelave  went MIA in rust-lang#58783, so I am re-submitting their PR, tweaked just a bit. I took care to preserve commit authorship.

Cc rust-lang#53871
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.