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

When exceptions disabled with JSON_NOEXCEPTION, lib just aborts without any message #2724

Closed
2 of 5 tasks
alecazam opened this issue Apr 19, 2021 · 16 comments
Closed
2 of 5 tasks
Labels
kind: question solution: proposed fix a fix for the issue has been proposed and waits for confirmation

Comments

@alecazam
Copy link

What is the issue you have?

When using this json library, I'm seeing the release build either produce an "exception" at the middle or end of a perfectly valid json document. I'm still tracking that down, but noted that our app would just "abort code 6" without any useful information. That's usually associated with a buffer overrun, but in this case it's a parse error that calls JSON_THROW that then aborts without useful info about where the parse error occurs.

Please describe the steps to reproduce the issue.

  1. Build and run any app with JSON_NOEXCEPTION set
  2. Hit any valid parsing exceptions
  3. Note app just quits without any useful information.

Can you provide a small but working code example?

Proposing the following change to at least report the line, column, and other data from exception.what() before the abort. If you have some sort of log abstraction, then this could go to that. Now I actually see useful data that I can act on, but I suspect that the parser we have is not initializing something. Sometimes I get line 40 column 2, and other times it's the end of my document at line 82, 2.

// allow to disable exceptions
#if (defined(__cpp_exceptions) || defined(__EXCEPTIONS) || defined(_CPPUNWIND)) && !defined(JSON_NOEXCEPTION)
    #define JSON_THROW(exception) throw exception
    ...
#else
    #define JSON_THROW(exc) fprintf(stderr, "%s\n", (exc).what()), std::abort() <-
#endif

What is the expected behavior?

Really with exceptions off, I expect the code to first print error information and then return false. I didn't write this wrapping code, but we still have allow_exceptions set on the parser even though we have JSON_NOEXCEPTIONS set, so the code goes into a series of JSON_THROW calls, but the other path just returns false without useful data.

 bool parse_error(std::size_t /*unused*/, const std::string& /*unused*/,
                     const detail::exception& ex)
    {
        errored = true;
        if (allow_exceptions)
        {
            // determine the proper exception type from the id
            switch ((ex.id / 100) % 100)
            {
                case 1:
                    JSON_THROW(*reinterpret_cast<const detail::parse_error*>(&ex));  <- abort occurs here
                case 4:
                    JSON_THROW(*reinterpret_cast<const detail::out_of_range*>(&ex));
                // LCOV_EXCL_START
                case 2:
                    JSON_THROW(*reinterpret_cast<const detail::invalid_iterator*>(&ex));
                case 3:
                    JSON_THROW(*reinterpret_cast<const detail::type_error*>(&ex));
                case 5:
                    JSON_THROW(*reinterpret_cast<const detail::other_error*>(&ex));
                default:
                    assert(false);
                    // LCOV_EXCL_STOP
            }
        }
        return false;  <- no useful data
    }

And what is the actual behavior instead?

The app just does and abort without any useful info about where parsing failed.

Which compiler and operating system are you using?

  • Compiler: Xcode 12.2
  • Operating system: macOS Big Sur 11.2.3

Which version of the library did you use?

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

If you experience a compilation error: can you compile and run the unit tests?

  • yes
  • no - please copy/paste the error message below
@nlohmann
Copy link
Owner

Switching off exceptions means that abort is called instead, see https://json.nlohmann.me/home/exceptions/#switch-off-exceptions. In particular:

Note that JSON_THROW_USER should leave the current scope (e.g., by throwing or aborting), as continuing after it may yield undefined behavior.

If you want to parse JSON without exceptions, see https://json.nlohmann.me/features/parsing/parse_exceptions/.

@alecazam
Copy link
Author

alecazam commented Apr 19, 2021

Is there even a reason to have the "allow_exception" codepaths that abort() when JSON_NOEXCEPTION is set? Seems like if a parser is just going to abort() without useful info, it would be better to strip that code and return false.

There are many games and apps built with Emscripten where correct json parsing and errors are needed without exceptions enabled. Emulating exceptions in Emscripten is a huge slowdown to perf since they generate so much code, so they're typically disabled. And Win didn't have zero-cost runtime exceptions until Win64, but the cost is still in the codegen.

@nlohmann
Copy link
Owner

The JSON_NOEXCEPTION functionality was not built for the parse function, but as a general mechanism to be able to compile the library while having exceptions disabled in the compiler. The parse function was fundamentally designed to return a value and communicate parse errors with exceptions. All other alternatives are documented in https://json.nlohmann.me/features/parsing/parse_exceptions/.

If you do want diagnostics without exceptions, then the last option (https://json.nlohmann.me/features/parsing/parse_exceptions/#user-defined-sax-interface) should be used.

@nlohmann nlohmann added the solution: proposed fix a fix for the issue has been proposed and waits for confirmation label Apr 21, 2021
@razaqq
Copy link

razaqq commented Apr 29, 2021

even when using the sax interface, json.at() and json.get() still call abort if you try to access a key which does not exist or the value is of the wrong type.

this is a more a curiosity thing than an actual proposal, but why was smth like

template<typename KeyType, typename ValueType>
bool get(const KeyType& key, ValueType& value);

int val;
if (!j.get("someValue", val))
{
  std::cerr << "Failed to get value" << std::endl;
}

(basically just returning false if the key wasnt found or the type is not valid) not considered?

to access it safely with exceptions off you would have to do smth like

int i = 0;
if (j.contains("someValue") && j.at("someValue").is_number_integer())
{
  i = j.at("someValue").get<int>();
}

@nlohmann
Copy link
Owner

The reason is that the library was designed with the assumption that exceptions are enabled.

@razaqq
Copy link

razaqq commented Apr 29, 2021

fair enough, was just curious. thanks for the answer

i got one more question (didnt feel like making a new issue just for this question).
right now you have to check for each type with an individual function is_number_integer() and so on.

with c++ 17's std::is_same_v<T, T> it should be possible to get a single unified

template<typename T>
bool json.is_type<T>();

or am i missing something?

@nlohmann
Copy link
Owner

As we are using a union and an enumerator to implement the JSON types, I'm not sure how to implement this.

@nlohmann
Copy link
Owner

Is there anything left for this issue to do?

@nlohmann nlohmann closed this as completed Aug 1, 2021
@dawinaj
Copy link

dawinaj commented Oct 8, 2023

even when using the sax interface, json.at() and json.get() still call abort if you try to access a key which does not exist or the value is of the wrong type.

this is a more a curiosity thing than an actual proposal, but why was smth like

template<typename KeyType, typename ValueType>
bool get(const KeyType& key, ValueType& value);

int val;
if (!j.get("someValue", val))
{
  std::cerr << "Failed to get value" << std::endl;
}

(basically just returning false if the key wasnt found or the type is not valid) not considered?

to access it safely with exceptions off you would have to do smth like

int i = 0;
if (j.contains("someValue") && j.at("someValue").is_number_integer())
{
  i = j.at("someValue").get<int>();
}

This would be really useful. For example in embedded environments, where you may not want to enable exceptions.
It would be much easier than doing all checks manually or implementing a try catch block.
If everything is fine, write to the specified variable, otherwise just leave the original value.

Also while at it, would be nice to have a template for deserializing into objects which also skips any missing or invalid values.

@alessandromrc
Copy link

Switching off exceptions means that abort is called instead, see https://json.nlohmann.me/home/exceptions/#switch-off-exceptions. In particular:

Note that JSON_THROW_USER should leave the current scope (e.g., by throwing or aborting), as continuing after it may yield undefined behavior.

If you want to parse JSON without exceptions, see https://json.nlohmann.me/features/parsing/parse_exceptions/.

Directly aborting doesn't seem like a great idea at all since it gives little to no-information about what is happening in production, sure it is useful if we currently have a debugger and debug symbols enabled but on a production view it just causes big headaches when trying to understand what is magically closing your software.

Not sure if anything has changed since 2021, but it would be great if there is another way to disable exceptions and still not call "abort()".

@nlohmann
Copy link
Owner

nlohmann commented May 7, 2024

What would you propose instead? There is no way to proceed in the situation where abort is called.

@alessandromrc
Copy link

What would you propose instead? There is no way to proceed in the situation where abort is called.

I don't really have clear what raises this behavior of calling it so I am not that sure about what the possible ways are... The major problem comes when your software uses SEH Exceptions and not C++ ones and so it's not very well possible to manage them in a clean way.

@dawinaj
Copy link

dawinaj commented May 8, 2024

Yeah this whole thing is causing issues again.
For example, ESP32 even with enabled exception handling and task wrappers, still immediately aborts when exception is thrown in a function managed by some internal task, like a webserver.

Idk, maybe let it write to some nlohmann::errno and exit in whatever non destructive way?

@alessandromrc
Copy link

@nlohmann Have you seen the comment from @herhor67, theirs looks like a good idea...

It could either be a callback function that gets called when an exception is happening (when C++ ones are disabled) or pretty much anything you could come up while avoiding to call abort().

@dawinaj
Copy link

dawinaj commented May 11, 2024

I thought about implementing it with the given macros, but uh, to "leave the current scope" one would need to call return, probably multiple times, and probably with at least some default value for functions that return something, so that definitely needs an internal implementation...
Are there any other ways for jumping? goto but it's probably impossible with nested functions...
Im not sure how much nesting exists in the nlohmann:: after compiling, but seeing that it's all templated, probably a bit...

@alessandromrc
Copy link

I thought about implementing it with the given macros, but uh, to "leave the current scope" one would need to call return, probably multiple times, and probably with at least some default value for functions that return something, so that definitely needs an internal implementation...

Are there any other ways for jumping? goto but it's probably impossible with nested functions...

Im not sure how much nesting exists in the nlohmann:: after compiling, but seeing that it's all templated, probably a bit...

I would honestly go with a cleaner path by having the macro being just a boolean toggle and then implement all the code with an actual handler itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: question solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Projects
None yet
Development

No branches or pull requests

5 participants