RUST-1545 Consider removing the take_mut dependency

[seanpianka]

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]

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]

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.

2. 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.