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

fixing std::shared_ptr<void> initialization from void* #3873

Merged
merged 6 commits into from
Jul 20, 2023

Conversation

MiguelBarro
Copy link
Contributor

class shared_ptr is optimized to avoid keeping a deleter object for non-primitive types which are detected using _Can_scalar_delete. This class relies on SFINAE to detect if the operator delete is valid for a given pointer type. It turns out that delete a void* is valid in msvc, though, according to the standard is undefined behaviour (7.6.2.9). By specializing for void is possible to prevent direct initialization of shared_ptr<void> from a void* and prevent bugs. Note that default_deleter<void> is not valid too.

class `shared_ptr` is optimized to avoid keeping a deleter object for non-primitive types which are detected using `_Can_scalar_delete`.
This class relies on SFINAE to detect if the operator delete is valid for a given pointer type.
It turns out that delete a void* is valid in msvc, though, according to the standard is undefined behaviour (7.6.2.9). 
By specializing for void is possible to prevent direct initialization of `shared_ptr<void>` from a void* and prevent bugs.
Note that `default_deleter<void>` is not valid too.
@MiguelBarro MiguelBarro requested a review from a team as a code owner July 13, 2023 13:57
@achabense
Copy link
Contributor

achabense commented Jul 13, 2023

Is this a compiler bug?

#include <memory>

constexpr int foo() {
  void* p = new int;
  delete p;
  return 0;
}

int main() { constexpr auto f = foo(); }

/std:c++14
/std:c++20
my bad

@fsb4000
Copy link
Contributor

fsb4000 commented Jul 13, 2023

Is this a compiler bug?

@achabense , no. We have constexpr new only since C++20. https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p0784r7.html

@achabense
Copy link
Contributor

Hmm... _Can_scalar_delete

@frederick-vs-ja
Copy link
Contributor

though, according to the standard is undefined behaviour (7.6.2.9)

It seems that the standard wording in [expr.delete] is defective. In the core spec, "shall" generally means that requirements for well-formedness, not for the well-definedness of the behavior. However, "shall" sometimes introduces UB in [expr.delete], and it's not clear whether deleting a void* can be well-formed.

  • Clang rejects it when passing -pedantic-errors.
  • GCC accepts it with -pedantic-errors, even in constant evaluation since C++20, but says that it's undefined.
  • MSVC silently accepts it.

@CaseyCarter CaseyCarter added the bug Something isn't working label Jul 13, 2023
stl/inc/memory Outdated Show resolved Hide resolved
@CaseyCarter
Copy link
Contributor

FYI, I'm calling this "bug" rather than "enhancement" since [util.smartptr.shared.const]/4 says we should be diagnosing this as ill-formed.

MiguelBarro and others added 3 commits July 13, 2023 23:45
Sticking to metaprogramming available in C++14
avoid using resolution for std namespace types
stl/inc/memory Outdated Show resolved Hide resolved
@fsb4000

This comment was marked as resolved.

@StephanTLavavej

This comment was marked as resolved.

@StephanTLavavej StephanTLavavej self-assigned this Jul 20, 2023
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit e52dda3 into microsoft:main Jul 20, 2023
@StephanTLavavej
Copy link
Member

Thanks for fixing this bug and congratulations on your first microsoft/STL commit! 🚀 🎉 🥳

@MiguelBarro MiguelBarro deleted the patch-1 branch July 21, 2023 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants