json icon indicating copy to clipboard operation
json copied to clipboard

Coverity issues with Coverity 2024.6.0

Open alainsanguinetti opened this issue 1 year ago • 8 comments

Description

Hi,

I am using the library at a place where Coverity scans the source code.

Recently the scanner was upgraded to 2024.6.0

Some new issues are popping up:

For example:

image

There are around 17 reported issues as far as I can see. I can add the full list in the ticket if interested.

Thanks for this amazing library in any case!

Reproduction steps

Run Coverity 2024.6.0 on json.hpp

Expected vs. actual results

Expect: no errors Results: some errors are reported

Minimal code example

No response

Error messages

No response

Compiler and operating system

Linux/ unknown

Library version

960b763ecd144f156d05ec61f577b04107290137

Validation

alainsanguinetti avatar Jul 31 '24 07:07 alainsanguinetti

@alainsanguinetti Can you provide an updated list of the results?

nlohmann avatar Nov 17 '24 09:11 nlohmann

2024_11_20_coverity_scan.pdf

Hi @nlohmann, here's an "export", sorry it's the only way I could get the information out efficiently..

alainsanguinetti avatar Nov 20 '24 10:11 alainsanguinetti

it's based on a version from July

alainsanguinetti avatar Nov 20 '24 10:11 alainsanguinetti

Thanks! I will see what's still relevant from this.

nlohmann avatar Nov 20 '24 10:11 nlohmann

The only valid issue I see here is parser_callback_t, which is an alias for std::function, being taken by value and not being moved.

The ones about the resource leak are incomplete analysis, Coverity just can't see how they're being deleted.

It is assuming that get() and get_token() return the same thing every time they're called, but they do not.

It is complaining about asserts.

It is complaining that the move constructor is forwarding the argument to the base class constructor. This is how it needs to be done (it could actually just be std::move instead of std::forward, but that doesn't change the behavior, just the amount of text).

It is complaining that the noexcept constructor for nullptr_t calls a function that can throw, but doesn't do the analysis to see that it can't reach any of the throwing code because of the parameter value passed in.

It is complaining about the use of 32 bit time_t instead of 64 bit in a templated function, so the time_t is selected by the caller and not the library code.

It is complaining about using '0' instead of 0 in a memset call, but the function is creating the string form of a number and filling the string with zeros, not nulls.

gregmarr avatar Nov 20 '24 21:11 gregmarr

Thanks @gregmarr for the analysis!

  • The issue with not moving a parser_callback_t should be fixed with #4489
  • I agree with your analysis on resource leaks and the misunderstanding of Coverity about get() and get_token().
  • Asserts are used to avoid undefined behavior in debug builds as well as to document invariants. Great to see some invariants "validated" by a tool.

Maybe @alainsanguinetti can create another check on the latest develop branch.

nlohmann avatar Nov 21 '24 07:11 nlohmann

Agreed that the "not moving" should be fixed by that commit.

gregmarr avatar Nov 21 '24 17:11 gregmarr

Hi, I'll give a try to the update and will report later, could take a few weeks

alainsanguinetti avatar Nov 22 '24 13:11 alainsanguinetti

Any update on this?

nlohmann avatar Jan 19 '25 15:01 nlohmann

2025_01_20_coverity_scan.pdf

Looks like no new issues are reported, and that the "not moving" is fixed :)

the same issues are reported otherwise that were not applicable. I think we can close!

alainsanguinetti avatar Jan 20 '25 09:01 alainsanguinetti

Image

FYI, received a new one today, without any code update otherwise.

alainsanguinetti avatar Feb 20 '25 08:02 alainsanguinetti

Yeah, that line of code hasn't changed in over 7 years. https://github.com/nlohmann/json/blob/a3143f5f2f20e6e368efc4f2792ae8b3b65f92ff/include/nlohmann/detail/json_pointer.hpp#L720

gregmarr avatar Feb 20 '25 16:02 gregmarr