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>: constructor does not copy what_arg if _HAS_EXCEPTIONS=0 #2114

Open
nlohmann opened this issue Aug 11, 2021 · 4 comments
Open

<exception>: constructor does not copy what_arg if _HAS_EXCEPTIONS=0 #2114

nlohmann opened this issue Aug 11, 2021 · 4 comments
Labels
bug Something isn't working vNext Breaks binary compatibility

Comments

@nlohmann
Copy link

Describe the bug

When _HAS_EXCEPTIONS is defined to 0, the constructors of the exception classes do not copy the what_arg, but only store the pointer:

explicit __CLR_OR_THIS_CALL exception(const char* _Message = "unknown", int = 1) noexcept : _Ptr(_Message) {}

This leads to errors if the pointee is a temporary string.

If _HAS_EXCEPTIONS is set to 1, a copy is made just as the standard would require it.

Command-line test case

The following code outputs [json.exception.parse_error] foo if _HAS_EXCEPTIONS is set to 1, and some garbage if _HAS_EXCEPTIONS is set to 0:

#include <exception> // exception
#include <stdexcept> // runtime_error
#include <string> // string
#include <iostream> // cout

namespace nlohmann
{
namespace detail2
{

class exception : public std::exception
{
  public:
    const char* what() const noexcept override
    {
        return m.what();
    }

  protected:
    exception(const char* what_arg) : m(what_arg) {}

  private:
    std::runtime_error m;
};

class parse_error : public exception
{
  public:
    static parse_error create(const std::string& what_arg)
    {
        std::string w = "[json.exception.parse_error] " + what_arg;
        return parse_error(w.c_str());
    }

  private:
    parse_error(const char* what_arg) : exception(what_arg) {}
};

}  // namespace detail2
}  // namespace nlohmann


int main()
{
    auto error = nlohmann::detail2::parse_error::create("foo");
    std::cout << error.what() << std::endl;
}

The example comes from https://github.com/nlohmann/json where exceptions are created by a SAX parser, but it is up to the client whether they are thrown or not. Therefore, using exception classes while disabling throwing exceptions is a usecase.

Expected behavior

The exception makes a copy of the what_arg argument, regardless of the value of _HAS_EXCEPTIONS.

STL version

I could reproduce the issue with MSVC 19.16.27045.0 and MSVC 19.29.30040.0.

Additional context

The issue was originally found here: nlohmann/json#2824

@StephanTLavavej StephanTLavavej added bug Something isn't working vNext Breaks binary compatibility decision needed We need to choose something before working on this labels Aug 11, 2021
@StephanTLavavej
Copy link
Member

_HAS_EXCEPTIONS=0 is quasi-supported (it's not officially documented, and has various design/usability issues like this one, although we've tried to avoid breaking how it works). I've marked this as vNext because I believe that dynamically allocating/deallocating memory in the _HAS_EXCEPTIONS=0 implementation of exception would be ABI-breaking. Also marked as decision needed - as @mnatsuhara mentioned, we need to decide whether to change this long-standing behavior (e.g. attempting to malloc memory, and storing a string literal as a fallback if we're unable to; that would also require storing whether the string is dynamically allocated, which is super-definitely ABI breaking).

Finally, @CaseyCarter reminded me that this constructor itself is non-Standard (this applies to the normal _HAS_EXCEPTIONS=1 implementation too) - that's tracked by #882.

@nlohmann
Copy link
Author

Thanks for the quick response!

About the non-standard issue: I think it's not about the constructor of std::exception, but rather about the behavior of the derived classes like runtime_error(const char* what_arg) - there I would still expect a copy of the argument to be made.

@CaseyCarter
Copy link
Member

About the non-standard issue: I think it's not about the constructor of std::exception, but rather about the behavior of the derived classes like runtime_error(const char* what_arg) - there I would still expect a copy of the argument to be made.

Ah, I see - thanks for the clarification. It hadn't occurred to me the the standard constructors in the derived classes are implemented in terms of this non-standard constructor in exception.

@StephanTLavavej StephanTLavavej removed the decision needed We need to choose something before working on this label Apr 19, 2023
@StephanTLavavej
Copy link
Member

We talked about this at the weekly maintainer meeting and we believe that in the _HAS_EXCEPTIONS=0 case we should do what we do for _HAS_EXCEPTIONS=1: try to copy the string, and if we can't allocate memory, then just store nothing. (Separately we should get rid of that non-Standard base class constructor although doing so will be an epic undertaking.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working vNext Breaks binary compatibility
Projects
None yet
Development

No branches or pull requests

3 participants