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 issues with Coverity 2024.6.0 #4430

Open
2 tasks done
alainsanguinetti opened this issue Jul 31, 2024 · 8 comments
Open
2 tasks done

Coverity issues with Coverity 2024.6.0 #4430

alainsanguinetti opened this issue Jul 31, 2024 · 8 comments
Labels
state: needs more info the author of the issue needs to provide more details

Comments

@alainsanguinetti
Copy link

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

960b763

Validation

@nlohmann
Copy link
Owner

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

@nlohmann nlohmann added the state: needs more info the author of the issue needs to provide more details label Nov 19, 2024
@alainsanguinetti
Copy link
Author

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
Copy link
Author

it's based on a version from July

@nlohmann nlohmann self-assigned this Nov 20, 2024
@nlohmann
Copy link
Owner

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

@nlohmann nlohmann removed the state: needs more info the author of the issue needs to provide more details label Nov 20, 2024
@gregmarr
Copy link
Contributor

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.

@nlohmann
Copy link
Owner

Thanks @gregmarr for the analysis!

  • The issue with not moving a parser_callback_t should be fixed with Another desperate try to fix the CI #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 nlohmann added state: needs more info the author of the issue needs to provide more details and removed kind: bug labels Nov 21, 2024
@nlohmann nlohmann removed their assignment Nov 21, 2024
@gregmarr
Copy link
Contributor

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

@alainsanguinetti
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state: needs more info the author of the issue needs to provide more details
Projects
None yet
Development

No branches or pull requests

3 participants