Use latest version of cppcheck (2.15.0) to analyze codebase

[eduar-hte]

what

• Update the cppcheck GitHub workflow to run on the latest version of that tool (2.15.0) for better codebase analysis (more aligned with that of Sonarcloud).
  • NOTE: This PR references cppcheck version 2.14.2 because that was the last version at the time it was created. However, the GitHub macOS runner will now install 2.15.0 (the latest version), which has been released since then (Aug 31, 2024).
• Update the cppcheck configuration to have the analysis be performed using C++17, which is aligned with the build configuration for the project.
• Address warnings identified by the newer version of the tool.
  • This should also address some similar Sonarcloud issues as well.

why

The latest version of cppcheck includes improved analysis and address some limitations of the version currently used (2.7.1, from Feb, 2022).

For example, std::string_view instances are expected to be passed by value (see https://quuxplusone.github.io/blog/2021/11/09/pass-string-view-by-value/), but the older version of cppcheck would report a warning incorrectly stating that the argument should be passed by reference instead. This was addressed in danmar/cppcheck#3817

notes

The cppcheck GitHub workflow now runs on the macOS image (instead of the Ubuntu/Linux image previously used) for two reasons:

• The Ubuntu/Linux image didn't have the latest version of cppcheck available, so we needed to manually checkout the cppcheck repository and compile the tool, which would make the workflow more complex and take more time.
• The cppcheck build in Ubuntu/Linux would run much slower than the package available on macOS (probably due to some additional build instructions being required to improve performance which I couldn't determine/find).

[eduar-hte]

Failed conditions B Maintainability Rating on New Code (required ≥ A)

I added two commits (unrelated to this PR) to address Sonarcloud reports. The first blocked analysis of the commits due to Sonarcloud finding (existing) duplicate code in ValidateSchema & ValidateDTD where initially there was a single line change to include the override keyword in those classes' destructors (and then the destructor was removed altogether).

The set of "new" (between quotes) issues reported by Sonarcloud after these changes are:

• src/actions/exec.h
• Add a using-declaration to this derived class to inherit the constructors of "Action", and remove the ones you manually duplicated. Note that this may add other constructors to your derived class. * Addressing this issue would include additional constructors to Exec, which are not currently supported, so it's not an equivalent change.
• src/operators/validate_dtd.h & src/operators/validate_schema.h
  • Remove use of ellipsis notation (2)
  • Replace this use of "void *" with a more meaningful type. (6)
  • Make the type of this parameter a pointer-to-const. The current type of this unnamed parameter is "void *" (3)
  • Replace "reinterpret_cast" with a "static_cast" (2)
    • All these are required by the signature of libxml2 C-style callback functions.
• test/regression/regression.cc
  • Refactor this function to reduce its Cognitive Complexity from ... (5)
    • Existing code, unrelated to these changes.

[airween]

Hi @eduar-hte,

many thanks for this great PR. Also thanks for adding false positive labels to cppcheck suppression's.

I have only one question regarding the new inline suppression's: are those there because of the new cppcheck? Why those haven't appeared until now? (I marked the first two items and later I saw that there are many others there)

About SonarCloud: I already sent you an invitation to have an admin role on SonarCloud - haven't you received that? With that you could mark those issues (that you explain every time really accurately) as FP and suppress them.

[eduar-hte]

I have only one question regarding the new inline suppression's: are those there because of the new cppcheck? Why those haven't appeared until now? (I marked the first two items and later I saw that there are many others there)

Yes, the new version reported some new issues, some of which were addressed with changes to the code as they were correct (see commits related to using the override keyword or adding const for example).

At the same time, the new version also introduced a (limited) number of inaccurate issues, which I had to address by adding the inline suppressions you noticed (those are grouped in commit d053ec6).

I understand that while they may add new features and address bugs in newer versions, some of these changes may have introduced inaccurate reports as well. 🤷‍♂️

I've spent some time today on this PR again to do an experiment and remove all current inline suppressions and check which ones are still reported, because it may be the case that they're no longer necessary. As you can see in a0490bd, I could do away with a few of them, which is nice.

Additionally, when reviewing every one that was still reported by cppcheck I realized I could implement the suggestion reported by the tool instead of suppressing it, so a few more of them could be removed from the codebase.

About SonarCloud: I already sent you an invitation to have an admin role on SonarCloud - haven't you received that? With that you could mark those issues (that you explain every time really accurately) as FP and suppress them.

Sorry, I didn't see that. In any case, I'm not sure whether we should remove some of the issues reported by Sonarcloud that I detail in some of the PRs. My goal with that is to document which issues are unrelated to changes in the PR (they currently exist in the codebase and Sonarcloud reports them in the PR because some code has been changed around the existing issue).

I would group the issues in those lists in at least three groups:

• There are some that are false positives like the ones related to the libxml2 callback functions.
• There are some that may be sensible, but addressing them would be unrelated to the current PR, like the ones about code complexity (which could also lead to issues by trying to address an issue on stable code that has not changed in years, so the tradeoff for addressing an issue there is tricky).
• Then there are some that are opinionated, like this one that just came up after commit 651536f. I think that c++11 default member initializers are nice, but a matter of style whether to use them or not.

About the first group, I think it would make sense to flag them, but I think it'd be better to do so through a mechanism similar to cppcheck's inline suppressions. I tried to look for that, but I couldn't find it (this Sonarcloud community issue seems to be about that).

[airween]

Yes, the new version reported some new issues, some of which were addressed with changes to the code as they were correct (see commits related to using the override keyword or adding const for example).

I saw and I though you aligned those because of this reason.

At the same time, the new version also introduced a (limited) number of inaccurate issues, which I had to address by adding the inline suppressions you noticed (those are grouped in commit d053ec6).

Thanks,

I understand that while they may add new features and address bugs in newer versions, some of these changes may have introduced inaccurate reports as well. 🤷‍♂️

Thanks again.

I saw you added some more stuff (removed some inline suppression). Let me know if you finish this PR.

[sonarqubecloud[bot]]

Quality Gate passed

Issues
21 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[eduar-hte]

I saw you added some more stuff (removed some inline suppression). Let me know if you finish this PR.

That's right, I did away with a few of the cppcheck inline suppressions, as I ended up explaining in an edit of my previous reply (which I was in the process of writing when the build with the new changes completed so I decided to merge them into the PR and then resume the lengthy reply 😁).

PS: I ended up even addressing the new issue Sonarcloud reported and that I mentioned here:

Then there are some that are opinionated, like this one that just came up after commit https://github.com/owasp-modsecurity/ModSecurity/commit/651536fa0eacd60ca5b99ca0cbe675f0674e327b. I think that c++11 default member initializers are nice, but a matter of style whether to use them or not.

just not to add up to the number of outstanding Sonarcloud issues.

I'm now done with this PR.

[airween]

Thanks for the PR again and for the detailed explanations.

I'm merging this now.