type_error factory creates a dangling pointer (in VisualStudio 2019)

[ghost]

What is the issue you have?

I get a non-catchable C00000409 exception from the error handling code.

I have a piece of code which mistakenly tries to call at() on a string.

This triggers this error handling code:

        JSON_THROW(type_error::create(304, "cannot use at() with " + std::string(type_name())));

Which in turn, calls the type_error factory:

    static type_error create(int id_, const std::string& what_arg)
    {
        std::string w = exception::name("type_error", id_) + what_arg;
        return type_error(id_, w.c_str());
    }

Which calls the type_error constructor:

    type_error(int id_, const char* what_arg) : exception(id_, what_arg) {}

Which calls the exception contructor:

    exception(int id_, const char* what_arg) : id(id_), m(what_arg) {}

Which (in VisualStudio 2019) calls the std::runtime_error constructor:

    explicit runtime_error(const char* _Message) : _Mybase(_Message) {}

Which then calls (in VisualStudio 2019) the std::exception constructor:

    explicit exception(char const* const _Message) noexcept
        : _Data()
    {
        __std_exception_data _InitData = { _Message, true };
        __std_exception_copy(&_InitData, &_Data);
    }

Which finally stores the exception message in this struct:

struct __std_exception_data
{
    char const* _What;
    bool        _DoFree;
};

The issue is that the message is stored as a pointer, which in this case is the c_str() of a string which will be destroyed as soon as we exit the type_error factory.
When we then throw the exception object, the vcruntime kills the program.

Which compiler and operating system are you using?

• Compiler: VisualStudio 2019
• Operating system: Windows 10

Which version of the library did you use?

• latest release version 3.9.1
• other release - please state the version: 3.6.1
• the develop branch

[gregmarr]

@nlohmann runtime_error has a constructor that takes a const std::string & and presumably that one handles the lifetime properly rather than assuming that the pointer has a lifetime at least as long as the exception. I'm not sure what the standard says about this case.

[nickaein]

It seems exception constructor doesn't have a overload taking const char* and MSFT has deviated from the standard if they have implemented one.

Update: Under the hood we are using std::runtime_error and this has nothing to do with std::exception.

runtime_error has a constructor accepting const char*, but it also should make a copy:

Because copying std::runtime_error is not permitted to throw exceptions, this message is typically stored internally as a separately-allocated reference-counted string. This is also why there is no constructor taking std::string&&: it would have to copy the content anyway.

To me this seems a library bug in MSVC, but I'm not sure the wording is enforcing the compiler vendors to make a copy of const char*

[nickaein]

Ah, once again an MS specific extension. It has been available since 2015 though. I wonder whether they have done some optimisation recently that is causing this issue.

[nickaein]

@evolv-bricerive What is exact version of Visual Studio/MSVC that you are using? I tested the following code on and it worked as expected. I tried it on Visual Studio 2019 ver. 16.6.2 which ships msvc 19.26.28806.

#include <iostream>
#include "json.hpp"

int main()
{
    nlohmann::json j = "Hello";

    try
    {
        j.at(0);
    }
    catch (std::exception& exp)
    {
        std::cout << "Caught the exception: " << exp.what() << std::endl;
    }
}

Output:

Caught the exception: [json.exception.type_error.304] cannot use at() with string

P.S. I would be nice if you send a minimal code snippet that reproduces this issue.

[ghost]

@nickaein
I am using latest VisualStudio Version 16.8.1

It is hard to reproduce because the actual issue is a dangling pointer to freed memory.
It can go through happily or not depending on what happens next.

[ghost]

@gregmarr
I don't think the second ctor is any better:

    explicit runtime_error(const string& _Message) : _Mybase(_Message.c_str()) {}

It also assumes the string will be there during exception processing.
I think the basic issue is with the pattern in the factory.
Why not just use a public constructor and store the message in the type_error object?

[nickaein]

@evolv-bricerive Can you try the sample code above and see whay happens? It should trigger the same exception-generation path, I guess.

I doubt this is a bug in library as it is continuously under test and this bug is trivial to catch for current tests. This could be either caused by unexpected behaviour of MSCV library (not making a copy of error message in error_runtime constructor) or a memory corruption in app that has happened earlier in code and is manifesting itself at this point.

[gregmarr]

@evolv-bricerive I don't believe that an expectation that the std::string passed to runtime_error outlives the exception itself is reasonable or required by the standard.

[nlohmann]

I don't know how MSVC handles this, but the implementation makes the assumptions that are described here: https://stackoverflow.com/a/38482856.

[ghost]

@gregmarr
The expectation is that the string passed to runtime_error lives as long as the exception,, not outlives it.
The crash I get is in the runtime exception throwing code itself.

@nickaein
I tried code similar to yours and it works fine. That is because the exception handling is trivial.
In my case, the exception handling triggers some stack unwinding and that is when the memory collision occurs (still during the exception processing by the runtime) leading to a STACK_BUFFER_OVERRUN exception.
Suppose that, during the unwinding, I allocate a bit of memory, and by luck, I get the memory that the sting lives in, then I write over the end of string \0 and free the memory. What the rest of the exception handling gets is a C string that runs into unallocated memory. That's a buffer overrun condition and is detected by the runtime as a possible attack. Not saying this is what happens to me as I didn't debug that far but that's a possible scenario.
I'll try to trigger a simple example of that when I have some more time.

Not trying to make a point, here; Just letting you guys know about the story. Feel free to ignore it.
In any case, thank you for this excellent library.

[gregmarr]

@evolv-bricerive You can't technically have something that lives as long as something else but doesn't outlive it because there is an order to the destruction of the two objects, so there is no real difference between "lives at least as long as" and "outlives".

Having said that, I still don't believe that this is required by the standard. Do you have a reference that says otherwise? All the references that we've found say that the runtime_error is responsible for the ownership of the string. Both libc++ and libstdc++ create a copy of the string.

Even MSVC's documentation says that the value returned by what() is a copy of what's passed in. https://docs.microsoft.com/en-us/cpp/standard-library/runtime-error-class?view=msvc-160

[ghost]

@gregmarr I see your point about "outlives".

Having said that, If I understand you correctly, you believe the library is correct according to the standard, so the crash is MS's problem. Fine with me.