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.

Sign up for GitHub

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

Jump to bottom

Types with inaccessible destructors may still be freed by Rust #829

Open
bsilver8192 opened this issue Feb 23, 2022 · 1 comment · May be fixed by #1329
Open

Types with inaccessible destructors may still be freed by Rust #829

bsilver8192 opened this issue Feb 23, 2022 · 1 comment · May be fixed by #1329
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@bsilver8192
Copy link
Contributor

bsilver8192 commented Feb 23, 2022 •
edited

Expected Behavior

Refuse to allow freeing C++ objects with inaccessible destructors.

Constructible C++ classes with non-public destructors are uncommon in my experience, but the ones that do exist are usually very deliberate. Note the restricted situations where this is a problem:

  • If constructors are all private/deleted, and no functions in the class (or friends of it, bla bla etc) create an instance, then there is never an instance to destroy. Flatbuffers' Table does this, for example (it's intended to cast pointers to subclasses of it, and then they do pointer arithmetic based on this to find values).
  • Public destructors are easy enough to call when an owning Rust object is dropped.

Actual Behavior

The memory is just freed, without running any C++ destructor.

I'm working on calling implicit destructors (which may be nontrivial, currently they don't get called) as part of cleaning up a lot of things with implicit and defaulted constructors (almost done). However, I don't have a good solution for inaccessible destructors. For now, I'm just going to leave the current behavior, but it seems likely to create memory leaks.

Possible solutions I see:

  • Refuse to generate any functions for these types. Seems overly restrictive.
  • Something like https://www.reddit.com/r/rust/comments/my3ipa/comment/gw0g636/ to make a type which Rust can't drop, but can create in a UniquePtr and then pass that to C++. Probably too tricky, and prone to link failures based on Rust optimizer decisions.
  • Don't generate any APIs which allow Rust to own these types by value, but still support calling members if Rust gets a pointer from C++ or by casting. This provides maximum flexibility, but it seems prone to new autocxx features opening up new ways to fail to drop them.
  • Generate a Drop which panics. Definitely sound (under the semantics of both languages), but prone to writing Rust code that compiles but never works. Also a possibility of enabling Rust code that works normally, but panics in some error paths (where it fails to hand off ownership and allows the Rust object to be dropped), which are always harder to test.
@adetaylor adetaylor added the bug Something isn't working label Feb 24, 2022
bsilver8192 added a commit to bsilver8192/autocxx that referenced this issue Feb 27, 2022
@bsilver8192
Implement most of the C++ special member function rules
64297d8 
This now tracks most of the information about which C++ special member
functions are implicit/explicit/deleted/etc for most of the common
cases. This information was needed in several places, which were each
using different approximations that failed in different ways, so unify
those to get it all working. Also add a bunch of tests around the
various cases to keep this working.

This assumes that any non-analyzed types (except some built-in ones
which are handled specially) have no implicit special member functions,
instead of the previous behavior which assumed they all existed if the
analyzed type had explicit declarations. This should generate
functional code for more situations, but it will skip some optional
things (such as moveit traits and make_unique) for additional types. If
you run into issues with those things disappearing after this change,
make sure all dependencies of the type (superclasses and member types)
have a `generate!`/`generate_pod!`.

Added TODOs for the following unhandled parts:
* google#815 (this is a Clang warning anyways, TODOs show all
  the places to change to fix it)
* google#816 (this just means we ignore some implicit
  constructors which do exist)

Also added TODOs related to the followig issues, which limit what can be
tested but aren't made better or worse by this change:
* google#832 (this one affects lots of areas)
* google#829 (this one's pretty prone to unexpected behavior)

Also fixed some existing bugs which are surfaced by generating more
trait implementations for types in the existing tests:
* Use the correct C++ name for destructors of nested types
* Handle trait methods for types that end up ignored
@bsilver8192 bsilver8192 mentioned this issue Feb 27, 2022
Merged
2 tasks
bsilver8192 added a commit to bsilver8192/autocxx that referenced this issue Feb 28, 2022
@bsilver8192
Implement most of the C++ special member function rules
8943de2 
This now tracks most of the information about which C++ special member
functions are implicit/explicit/deleted/etc for most of the common
cases. This information was needed in several places, which were each
using different approximations that failed in different ways, so unify
those to get it all working. Also add a bunch of tests around the
various cases to keep this working.

This assumes that any non-analyzed types (except some built-in ones
which are handled specially) have no implicit special member functions,
instead of the previous behavior which assumed they all existed if the
analyzed type had explicit declarations. This should generate
functional code for more situations, but it will skip some optional
things (such as moveit traits and make_unique) for additional types. If
you run into issues with those things disappearing after this change,
make sure all dependencies of the type (superclasses and member types)
have a `generate!`/`generate_pod!`.

Added TODOs for the following unhandled parts:
* google#815 (this is a Clang warning anyways, TODOs show all
  the places to change to fix it)
* google#816 (this just means we ignore some implicit
  constructors which do exist)

Also added TODOs related to the followig issues, which limit what can be
tested but aren't made better or worse by this change:
* google#832 (this one affects lots of areas)
* google#829 (this one's pretty prone to unexpected behavior)

Also fixed some existing bugs which are surfaced by generating more
trait implementations for types in the existing tests:
* Use the correct C++ name for destructors of nested types
* Handle trait methods for types that end up ignored
@adetaylor
Copy link
Collaborator

Next steps here: add a test and see what actually happens. Adding 'good first issue' for taking that step.

@adetaylor adetaylor added the good first issue Good for newcomers label Apr 2, 2023
adetaylor added a commit that referenced this issue Sep 12, 2023
@adetaylor
Fix deleted destructors.
d11f977 
Fixes #829.
@adetaylor adetaylor linked a pull request Sep 12, 2023 that will close this issue
Open
adetaylor added a commit that referenced this issue Sep 14, 2023
@adetaylor
Fix deleted destructors.
7131ec0 
Fixes #829.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix deleted destructors. google/autocxx
2 participants
@adetaylor @bsilver8192