google / moveit

Apache License 2.0
166 stars 18 forks source link

Fix unsoundness due to DerefMove impls for Pin #37

Closed silvanshade closed 1 year ago

silvanshade commented 1 year ago

This PR fixes the soundness issue identified by @SkiFire13 in #34.

I left detailed notes about the changes in the commits, so I won't repeat everything here, except to give a summary of the changes:

With these changes, the unsound example from #34 no longer compiles, instead the compiler complains about unsatisfied Unpin bounds, as expected.

All of the other existing tests still pass, so I believe this should fully address the issue.

cc @mcy @adetaylor

google-cla[bot] commented 1 year ago

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

adetaylor commented 1 year ago

Thanks very much for working on this and especially for fixing it end-to-end with autocxx (https://github.com/google/autocxx/pull/1260). @mcy WDYT?

adetaylor commented 1 year ago

@silvanshade hello! @mcy and I had a chat about this and tried to page back in all our memory about how all this works.

Overall, the main message is - thanks for getting your head around this and fixing this hole especially with such excellently-reviewable code. Very much appreciated.

Here are the changes we'd request:

  1. Your rationales in the commit messages are excellent. Please can you try to move some of them into comments so that they stand the test of time. The main thing we want to see in a comment is the rationale from 8c4ae5ed97fc6ea1b0c7e7c5d3d213398cb96058 for why the bound on AsMove has to be relaxed and thus the definition of Storage has to be repeated, because without your commit message it's not obvious. But please do take a look through your other commit messages and see what other stuff seems sensible to transfer across to comments too.
  2. Please change a58583572255ec36bff49416f754f01291ccc927 to point to the latest rustc.
  3. @mcy really does not like trybuild tests. Specifically, the maintenance hassle of keeping expectation files up to date. Please remove them. Please could you turn them into compile_fail doctests.
silvanshade commented 1 year ago

@adetaylor @mcy I've made several changes and pushed new commits which I believe address all of those points.

Additionally, since this is a breaking change, I went ahead and made another modification which unifies Emplace and EmplaceUnpinned (since they were split at the point that UniquePtr support was introduced). However, this modification (in the last commit) can simply be omitted if you would rather not include that change.

I also wonder whether it might make sense to change Emplace to be an unsafe trait, since the intention is that the emplacement operations produce a Pin-safe "stable address" type (usually Pin<Self>, but not always, viz., UniquePtr), but there is no way to guarantee this notion except to rely on the implementor's judgement about the behavior of the respective pointer type.