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

Raw pointer #51

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Raw pointer #51

wants to merge 10 commits into from

Conversation

ricked-twice
Copy link

During an internship studying Rust compilation phase, I encounter this problem with std::ptr::read function.
As I read this guide, I thought I could contribute to it.
So this is a presentation of my contribution to this guide (english and french).

I extended the chapter 4 of the book, and added a rule about the usage of the std::ptr::read function.
As explained, a usage with a type implementing the Drop trait could lead to double-drop and possibly a double-free if the value is stored on the heap and freed during drop execution.

This kind of contribution should have followed an issue, but I have been told that it would be easier for maintainers to directly go for a pull request. So here it is! 😄

Copy link
Contributor

@danielhenrymantilla danielhenrymantilla left a comment

Choose a reason for hiding this comment

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

I like the idea to insist on this point 💯

We could maybe even mention that in the rare case where ptr::read is needed, then:

  • either use a pre-emptive ManuallyDrop of the original value;

  • or ptr::write-overwrite the original value before it is used. This last case should be handled with great caution, though, given how minimal exception safety / unwind safety may be broken otherwise (e.g., a panic before the write will still lead to a double drop; thus the write should happen in a "finally" like block, such as one from ::scopeguard (or using catch_unwind)); code snippets of https://docs.rs/replace_with being very educational in that regard.


I still think the suggested documentation should be a bit more harsh as to how bad misusing this function is, given how many things can go wrong, and how wrong that can be (UAF / double free can cause remote code execution), hence my request for changes 😉

  • (Note that I am just a random user, I am not part of the maintainers of the repo).

src/en/04_language.md Outdated Show resolved Hide resolved
src/en/04_language.md Outdated Show resolved Hide resolved
src/en/04_language.md Outdated Show resolved Hide resolved
src/en/04_language.md Show resolved Hide resolved
src/en/04_language.md Show resolved Hide resolved
src/fr/04_language.md Outdated Show resolved Hide resolved
@ricked-twice
Copy link
Author

ricked-twice commented May 20, 2022

Okay, so finally I should have took last reviews into account (with a lot of commits, sorry about that...).
It only took me a year or so 👍

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.

3 participants