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

RUST-1545 Consider removing the take_mut dependency #776

Open
seanpianka opened this issue Nov 12, 2022 · 2 comments
Open

RUST-1545 Consider removing the take_mut dependency #776

seanpianka opened this issue Nov 12, 2022 · 2 comments
Labels
tracked-in-jira Ticket filed in Mongo's Jira system

Comments

@seanpianka
Copy link
Contributor

Versions/Environment

  1. What version of Rust are you using?
    Rust 1.65
  2. What operating system are you using?
    N/A
  3. What versions of the driver and its dependencies are you using? (Run
    cargo pkgid mongodb & cargo pkgid bson)
    Driver v2.3.1
  4. What version of MongoDB are you using? (Check with the MongoDB shell using db.version())
    N/A
  5. What is your MongoDB topology (standalone, replica set, sharded cluster, serverless)?
    N/A

Describe the bug

There is a potential memory leak in take_mut, which I noted from 2 issues filed with cargo-crev. A pull request with a fix was opened several years ago and it hasn't been merged yet, so the repository might also be rightly considered abandoned.

Is this dependency necessary? Something to consider, as it also uses unproven unsafe code.

https://github.com/yvt/crev-proofs and https://github.com/vorner/crev-proofs both reported issues.

@isabelatkinson
Copy link
Contributor

Hi @seanpianka, thank you for bringing this to our attention! The only method from take_mut we use in the driver is take which does not appear to interact with fill, the method with the potential memory leak.

Something to consider, as it also uses unproven unsafe code.

Can you please expand upon this? Is this just in reference to the fill issue, or is there additional unproven unsafe code we should be aware of?

Regardless of whether we're using unproven unsafe code, I think you're right that this crate seems abandoned, so I filed RUST-1545 to consider removing the dependency. Doing so would require some internal refactoring, so we'll need to discuss the priority of that before moving forward.

@seanpianka
Copy link
Contributor Author

Can you please expand upon this? Is this just in reference to the fill issue, or is there additional unproven unsafe code we should be aware of?

I'll repost the relevant content from the two issues raised about take_mut here, since I don't have much to add constructively beyond their reviews (I'll leave that to the smart maintainers here 😄):

  1. https://github.com/vorner/crev-proofs

The whole idea of the crate -- having a variable unitialized for a while,
then return something back later on seems a bit unnatural for the whole Rust
type system. This can be seen by the fact that certain cornercases are
handled by not merely panicking, but outright aborting the whole process.

I didn't manage to find a way to break safety guarantees using the crate and
I tried to find a loophole quite hard. But considering how questionable
things it does, I'd really like to see some kind of proof or semi-formal
argument saying why it is safe. Such thing is not included in the source
code, unfortunately.

The repository doesn't seem to have a recent activity and the last release is
2 years ago, but it's hard to say if it's abandoned or simply considered
finished.

I've also found a resource leak (that is somewhat unlikely to get triggered
in real-world usage).

Therefore, I'd be somewhat wary using this myself and would need a good
reason to reach for this crate -- certainly not just for convenience.

  1. https://github.com/yvt/crev-proofs

While the idea of making a variable temporarily uninitialized may sound scary,
this crate takes necessary precaution to make this sound.

Unfortunately, this crate has [a resource leak issue][1] that has been left
open for two years, hence the negative rating.

This crate looks unmaintained as the last commit is from 2018.

Food for thought, although good to know that the driver doesn't use fill.

@bajanam bajanam changed the title Potential memory leak from take_mut dependency RUST-1545 Potential memory leak from take_mut dependency Nov 29, 2022
@bajanam bajanam added the tracked-in-jira Ticket filed in Mongo's Jira system label Nov 29, 2022
@bajanam bajanam changed the title RUST-1545 Potential memory leak from take_mut dependency RUST-1545 Consider removing the take_mut dependency Nov 29, 2022
@isabelatkinson isabelatkinson removed their assignment Jan 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tracked-in-jira Ticket filed in Mongo's Jira system
Projects
None yet
Development

No branches or pull requests

3 participants