Use latest version of cppcheck (2.15.0) to analyze codebase #3283

eduar-hte · 2024-10-19T13:56:07Z

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).

- Run cppcheck on MacOS to use a newer version of cppcheck

- Do not scan third-party libraries (others dir) - Use standard C++17 for checks (defaults to C++20)

…sis of branches. Use --check-level=exhaustive to analyze all branches.)

…des a function in a base class but is identical to the overridden function)

warning: seclang-parser.hh,2116,warning,duplInheritedMember,The struct 'basic_symbol < by_kind >' defines member function with name 'clear' also defined in its parent struct 'by_kind'. warning: seclang-parser.hh,2376,warning,duplInheritedMember,The struct 'basic_symbol < by_kind >' defines member function with name 'type_get' also defined in its parent struct 'by_kind'. warning: seclang-parser.hh,2116,warning,duplInheritedMember,The struct 'basic_symbol < by_state >' defines member function with name 'clear' also defined in its parent struct 'by_state'. warning: seclang-parser.hh,2120,style,constVariableReference,Variable 'yysym' can be declared as reference to const

…stions - The following two warnings were generated after introducing the change to instantiate the DigestImpl template with the address of mbedtls_md5 or mbedtls_sha1: - warning: src/utils/sha1.h,62,error,danglingTemporaryLifetime,Using pointer that is a temporary. - warning: src/utils/sha1.h,60,style,constVariablePointer,Variable 'ret' can be declared as pointer to const - See owasp-modsecurity#3231 (comment)

eduar-hte · 2024-10-19T16:29:19Z

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.

- Reported by Sonarcloud

src/actions/transformations/parity_zero_7bit.cc

src/compat/msvc.h

airween · 2024-10-21T14:14:55Z

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 · 2024-10-21T19:38:17Z

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 · 2024-10-21T19:46:33Z

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.

- This is correct because base class is initialized before members are initialized. - Removes cppcheck suppression by addressing reported issue. - Leverage C++11's 'default member initializer' to initialize m_provider & m_demandsPassword and address Sonarcloud issue.

…as this->m_variables - The name of the local variable would clash with the namespace of the same name, which may have lead cppcheck to think the variable was not used.

…sion

…by cppcheck.

sonarqubecloud · 2024-10-21T20:08:39Z

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 · 2024-10-21T20:27:05Z

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 651536f. 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

LGTM

airween · 2024-10-22T11:54:46Z

Thanks for the PR again and for the detailed explanations.

I'm merging this now.

eduar-hte added 9 commits October 19, 2024 10:32

Use latest version of cppcheck (2.14.2)

e0c5823

- Run cppcheck on MacOS to use a newer version of cppcheck

Updated cppcheck config

193a000

- Do not scan third-party libraries (others dir) - Use standard C++17 for checks (defaults to C++20)

Ignore cppcheck warnings: normalCheckLevelMaxBranches (Limiting analy…

1eed8b9

…sis of branches. Use --check-level=exhaustive to analyze all branches.)

Added missing override keyword as reported by cppcheck 2.14

da38f20

Address cppcheck warnings: uselessOverride (The function '...' overri…

7d9c80d

…des a function in a base class but is identical to the overridden function)

Add cppcheck suppressions for false positives

d053ec6

Added const reported by cppcheck 2.14

bbef22b

eduar-hte force-pushed the cppcheck2142 branch 2 times, most recently from 8b865f3 to 60dad6a Compare October 19, 2024 16:14

eduar-hte force-pushed the cppcheck2142 branch from 60dad6a to 049e436 Compare October 19, 2024 16:32

eduar-hte added 2 commits October 19, 2024 15:27

Refactor to remove duplicate code in ValidateSchema & ValidateDTD

d1e7e7b

- Reported by Sonarcloud

Avoid this unnecessary copy by using a "const" reference.

b0497d9

- Reported by Sonarcloud

eduar-hte force-pushed the cppcheck2142 branch from 049e436 to b0497d9 Compare October 19, 2024 18:27

airween reviewed Oct 21, 2024

View reviewed changes

src/actions/transformations/parity_zero_7bit.cc Show resolved Hide resolved

src/compat/msvc.h Show resolved Hide resolved

eduar-hte added 5 commits October 21, 2024 17:03

Remove cppcheck suppression by replacing use of local variable to ali…

cdaf32f

…as this->m_variables - The name of the local variable would clash with the namespace of the same name, which may have lead cppcheck to think the variable was not used.

Replace usage of sscanf with strtol to remove cppcheck inline suppres…

4e68edf

…sion

Make GeoLookup::debug function static (and non-member), as suggested …

7ec50eb

…by cppcheck.

Remove no longer needed cppcheck inline suppressions.

aca93f5

eduar-hte force-pushed the cppcheck2142 branch from a0490bd to aca93f5 Compare October 21, 2024 20:05

airween approved these changes Oct 22, 2024

View reviewed changes

airween merged commit 29a86b1 into owasp-modsecurity:v3/master Oct 22, 2024
49 checks passed

eduar-hte deleted the cppcheck2142 branch October 22, 2024 12:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use latest version of cppcheck (2.15.0) to analyze codebase #3283

Use latest version of cppcheck (2.15.0) to analyze codebase #3283

eduar-hte commented Oct 19, 2024 •

edited

Loading

eduar-hte commented Oct 19, 2024 •

edited

Loading

airween commented Oct 21, 2024

eduar-hte commented Oct 21, 2024 •

edited

Loading

airween commented Oct 21, 2024

sonarqubecloud bot commented Oct 21, 2024

eduar-hte commented Oct 21, 2024

airween left a comment

airween commented Oct 22, 2024

Use latest version of cppcheck (2.15.0) to analyze codebase #3283

Use latest version of cppcheck (2.15.0) to analyze codebase #3283

Conversation

eduar-hte commented Oct 19, 2024 • edited Loading

what

why

notes

eduar-hte commented Oct 19, 2024 • edited Loading

airween commented Oct 21, 2024

eduar-hte commented Oct 21, 2024 • edited Loading

airween commented Oct 21, 2024

sonarqubecloud bot commented Oct 21, 2024

Quality Gate passed

eduar-hte commented Oct 21, 2024

airween left a comment

Choose a reason for hiding this comment

airween commented Oct 22, 2024

eduar-hte commented Oct 19, 2024 •

edited

Loading

eduar-hte commented Oct 19, 2024 •

edited

Loading

eduar-hte commented Oct 21, 2024 •

edited

Loading