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

<exception>: Invalid destruction of non-copyable throwables during exception_ptr rethrow #2466

Closed
hofst opened this issue Jan 11, 2022 · 12 comments
Labels
compiler Compiler work involved external This issue is unrelated to the STL

Comments

@hofst
Copy link

hofst commented Jan 11, 2022

Describe the bug
If you rethrow an object with deleted copy-constructor via an exception_ptr, it will corrupt the memory and crash.

Command-line test case

C:\Temp>type repro.cpp
#include <exception>
#include <string>
using namespace std;

class C {
public:
  C() = default;
  C(const C&) = delete;
  C& operator=(const C&) = delete;
  C(C&&) = default;
  C& operator=(C&&) = default;

private:
  string m;
};

int main() {
  exception_ptr eptr;
  try {
    throw C();
  } catch (const C&) {
    eptr = std::current_exception();
  }
  try {
    rethrow_exception(eptr);
  } catch (const C&) {
  }
}

C:\Temp>cl /EHsc /W4 /WX .\repro.cpp
Microsoft (R) C/C++ Optimizing Compiler Version 19.23.28019.1 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

repro.cpp
Microsoft (R) Incremental Linker Version 14.23.28019.1
Copyright (C) Microsoft Corporation.  All rights reserved.

/out:repro.exe
repro.obj

C:\Temp>.\repro.exe
test failure

Expected behavior
Does not crash.

STL version
MSVC build tools 14.28, 14.29, 14.30, 14.31 (reproes in all of them).

@fsb4000

This comment has been minimized.

@fsb4000
Copy link
Contributor

fsb4000 commented Jan 11, 2022

I found some links: https://en.cppreference.com/w/cpp/language/throw

Even if copy initialization selects the move constructor, copy initialization from lvalue must be well-formed, and the destructor must be accessible

https://wg21.cmeerw.net/cwg/issue1863

When the thrown object is a class object, the constructor selected for the copy-initialization as well as the constructor selected for a copy-initialization considering the thrown object as an lvalue shall be non-deleted and accessible, even if the copy/move operation is elided

https://eel.is/c++draft/except.throw#5

@CaseyCarter CaseyCarter added compiler Compiler work involved external This issue is unrelated to the STL labels Jan 11, 2022
@CaseyCarter
Copy link
Contributor

CaseyCarter commented Jan 11, 2022

The bug here is - per [except.throw]/5 - the compiler failing to diagnose that C is not copyable at throw C();. Please report this as a compiler bug at https://developercommunity.visualstudio.com/report?space=62&entry=problem.

@hofst
Copy link
Author

hofst commented Jan 11, 2022

@CaseyCarter

Are you positive that this is a compiler bug and the deleted/non-existent copy constructor is called? How did you verify that?

I have some doubts because I did more tests with other compiler / STL combinations:
The exact same problem does reproduce with clang13+msvcSTL but the problem does not reproduce with clang13+libcxx. So the msvcSTL is a common denominator. If it is indeed a compiler bug, that observation would be quite the coincidence. It would mean that clang has a similar problem but libcxx somehow sidesteps the problem.
I just want to make sure we blame the correct culprit before I start opening defects against both the MSVC compiler and the clang compiler.

@CaseyCarter
Copy link
Contributor

Are you positive that this is a compiler bug and the deleted/non-existent copy constructor is called? How did you verify that?

Yes, it's clear: the standard says "the constructor selected for a copy-initialization considering the thrown object as an lvalue shall be non-deleted and accessible." This program violates that diagnosable constraint, so the compilers should diagnose it.

I have some doubts because I did more tests with other compiler / STL combinations: The exact same problem does reproduce with clang13+msvcSTL but the problem does not reproduce with clang13+libcxx.

"The problem" is the lack of diagnostic, which does reproduce with Clang + libc++. And GCC + libstdc++, for that matter.

@hofst
Copy link
Author

hofst commented Jan 12, 2022

@CaseyCarter
What do you mean with lack of diagnostics? To my understanding, the copy-initialization does not necessarily need to be performed by a copy constructor but can also be performed by a move constructor (the "copy" in "copy initialization" is a misnomer). So the word "constructor" that is referred in the standard is the move constructor in this case. I still believe the program is well-formed and it works perfectly well in clang+libc++ or GCC+libstdc++ as I would expect.

@fsb4000
Copy link
Contributor

fsb4000 commented Jan 12, 2022

What do you mean with lack of diagnostics?

I think a conforming C++ compiler must give a compilation error for the initial code.

Even if copy initialization selects the move constructor, copy initialization from lvalue must be well-formed

@hofst
Copy link
Author

hofst commented Jan 12, 2022

@fsb4000

Even if copy initialization selects the move constructor, copy initialization from lvalue must be well-formed

Interesting! This citation is from "cppreference.com", right? Do you happen to know the original citation in the c++ standard? I don't seem to find it.

@fsb4000
Copy link
Contributor

fsb4000 commented Jan 12, 2022

Do you happen to know the original citation in the c++ standard?

I think that line from cppreference is paraphrased the line from the standard:

the constructor selected for a copy-initialization considering the thrown object as an lvalue shall be non-deleted and accessible

Cubbi added the line after cwg-1863 was accepted.

https://en.cppreference.com/mwiki/index.php?title=cpp%2Flanguage%2Fthrow&diff=81680&oldid=74819

@fsb4000
Copy link
Contributor

fsb4000 commented Jan 16, 2022

I created issues:

DevCom-1638273

LLVM-53224

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104046

@fsb4000
Copy link
Contributor

fsb4000 commented Jan 24, 2022

Someone from Microsoft also ran into the issue: DevCom-1645768

@frederick-vs-ja
Copy link
Contributor

@fsb4000

Even if copy initialization selects the move constructor, copy initialization from lvalue must be well-formed

Interesting! This citation is from "cppreference.com", right? Do you happen to know the original citation in the c++ standard? I don't seem to find it.

Off-topic: it seems that we (editors of cppreference) should complete the "Reference" sections of cpp pages (which cite the corresponding sections in all related revisions of C++ standard).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler Compiler work involved external This issue is unrelated to the STL
Projects
None yet
Development

No branches or pull requests

4 participants