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

Coverity Scan reports an UNCAUGHT_EXCEPT issue #1400

Closed
nlohmann opened this issue Dec 22, 2018 · 5 comments
Closed

Coverity Scan reports an UNCAUGHT_EXCEPT issue #1400

nlohmann opened this issue Dec 22, 2018 · 5 comments

Comments

@nlohmann
Copy link
Owner

  • What is the issue you have?

Coverity Scan reports an UNCAUGHT_EXCEPT issue.

  • Please describe the steps to reproduce the issue. Can you provide a small but working code example?

No. Instead here is the report:

** CID 184857:    (UNCAUGHT_EXCEPT)
/single_include/nlohmann/json.hpp: 1709 in nlohmann::detail::iteration_proxy_value<nlohmann::detail::iter_impl<const nlohmann::basic_json<std::map, std::vector, std::basic_string<char, std::char_traits<char>, std::allocator<char>>, bool, long, unsigned long, double, std::allocator, nlohmann::adl_serializer>>>::operator !=(const nlohmann::detail::iteration_proxy_value<nlohmann::detail::iter_impl<const nlohmann::basic_json<std::map, std::vector, std::basic_string<char, std::char_traits<char>, std::allocator<char>>, bool, long, unsigned long, double, std::allocator, nlohmann::adl_serializer>>>&) const()
/single_include/nlohmann/json.hpp: 1709 in nlohmann::detail::iteration_proxy_value<nlohmann::detail::iter_impl<nlohmann::basic_json<std::map, std::vector, std::basic_string<char, std::char_traits<char>, std::allocator<char>>, bool, long, unsigned long, double, std::allocator, nlohmann::adl_serializer>>>::operator !=(const nlohmann::detail::iteration_proxy_value<nlohmann::detail::iter_impl<nlohmann::basic_json<std::map, std::vector, std::basic_string<char, std::char_traits<char>, std::allocator<char>>, bool, long, unsigned long, double, std::allocator, nlohmann::adl_serializer>>>&) const()


________________________________________________________________________________________________________
*** CID 184857:    (UNCAUGHT_EXCEPT)
/single_include/nlohmann/json.hpp: 1709 in nlohmann::detail::iteration_proxy_value<nlohmann::detail::iter_impl<const nlohmann::basic_json<std::map, std::vector, std::basic_string<char, std::char_traits<char>, std::allocator<char>>, bool, long, unsigned long, double, std::allocator, nlohmann::adl_serializer>>>::operator !=(const nlohmann::detail::iteration_proxy_value<nlohmann::detail::iter_impl<const nlohmann::basic_json<std::map, std::vector, std::basic_string<char, std::char_traits<char>, std::allocator<char>>, bool, long, unsigned long, double, std::allocator, nlohmann::adl_serializer>>>&) const()
1703         bool operator==(const iteration_proxy_value& o) const noexcept
1704         {
1705             return anchor == o.anchor;
1706         }
1707     
1708         /// inequality operator (needed for range-based for)
   CID 184857:    (UNCAUGHT_EXCEPT)
   An exception of type "nlohmann::detail::invalid_iterator" is thrown but the throw list "throw()" doesn't allow it to be thrown. This will cause a call to unexpected() which usually calls terminate().
1709         bool operator!=(const iteration_proxy_value& o) const noexcept
1710         {
1711             return anchor != o.anchor;
1712         }
1713     
1714         /// return key of the iterator
/single_include/nlohmann/json.hpp: 1709 in nlohmann::detail::iteration_proxy_value<nlohmann::detail::iter_impl<nlohmann::basic_json<std::map, std::vector, std::basic_string<char, std::char_traits<char>, std::allocator<char>>, bool, long, unsigned long, double, std::allocator, nlohmann::adl_serializer>>>::operator !=(const nlohmann::detail::iteration_proxy_value<nlohmann::detail::iter_impl<nlohmann::basic_json<std::map, std::vector, std::basic_string<char, std::char_traits<char>, std::allocator<char>>, bool, long, unsigned long, double, std::allocator, nlohmann::adl_serializer>>>&) const()
1703         bool operator==(const iteration_proxy_value& o) const noexcept
1704         {
1705             return anchor == o.anchor;
1706         }
1707     
1708         /// inequality operator (needed for range-based for)
   CID 184857:    (UNCAUGHT_EXCEPT)
   An exception of type "nlohmann::detail::invalid_iterator" is thrown but the throw list "throw()" doesn't allow it to be thrown. This will cause a call to unexpected() which usually calls terminate().
1709         bool operator!=(const iteration_proxy_value& o) const noexcept
1710         {
1711             return anchor != o.anchor;
1712         }
1713     
1714         /// return key of the iterator
  • What is the expected behavior?

No reported issue.

  • And what is the actual behavior instead?

Reported issue.

N/A

  • Did you use a released version of the library or the version from the develop branch?

develop/ 3.5.0 release.

N/A


The problem is that the iteration_proxy_value class's comparison operators are marked noexcept. They are implemented by the underlying iterator class's comparison operators which may throw json::invalid_iterator when iterators of different JSON values are compared. Coverity Scan warns that if such an exception is thrown, the program would terminate.

@nlohmann nlohmann added the state: please discuss please discuss the issue or vote for your favorite option label Dec 22, 2018
@nlohmann
Copy link
Owner Author

I think such an exception cannot occur - at least not in the case of range-for. I am wondering whether it would make sense to try to (1) silence this issue, (2) somehow cope with the exception, or (3) just remove the noexcept annotation. What do you think?

@gregmarr
Copy link
Contributor

gregmarr commented Dec 22, 2018

Unless there is a way to prevent these from being called from outside range-for, then I don't think they can be noexcept, unless you first test that o is valid and isn't going to throw.

Since iteration_proxy_value is a friend of iter_impl, it could compare anchor.m_object and o.anchor.other.m_object first, thus eliminating the potential for throwing.

@nlohmann
Copy link
Owner Author

In general, we may have to discuss the design decision in case of a violated precondition. Comparing iterators of different containers is, to my knowledge, undefined behavior. There are some choices to implement the comparison:

  1. Check if the precondition is met and throw if it is not (current behavior).
  2. Use an assertion for the precondition (as done, for instance, in the const version of operator[]).
  3. Do not check the precondition.

While (1) is the safest approach, (2) and (3) would have the benefit of being noexcept.

What do you think?

@nlohmann
Copy link
Owner Author

Any idea?

@jaredgrubb
Copy link
Contributor

Comparing iterators into different containers is definitely UB. I think either #1 or #2 are valid approaches (and if #1 maybe we can quiet Coverity?). I don't have a strong opinion here.

@nlohmann nlohmann self-assigned this Jan 13, 2019
@nlohmann nlohmann added release item: 🔨 further change and removed state: please discuss please discuss the issue or vote for your favorite option labels Jan 13, 2019
@nlohmann nlohmann added this to the Release 3.5.1 milestone Jan 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants