Remove cppcheck suppressions with line numbers in test/cppcheck_suppressions.txt

eduar-hte commented 2 weeks ago

The goal of this PR is to smooth out the process of updating libModSecurity v3 by avoiding the need to update test/cppcheck_suppressions.txt due to line number changes when doing work unrelated to the suppressions.

Additionally, inlining the suppressions also help removing the suppressions when the code where they're present is refactored/removed. Notice that a number of suppressions currently present in test/cppcheck_suppressions.txt seem to reference issues no longer present in the code and that could be removed.

The changes in this PR remove almost (*) all the current suppressions that have a line number reference at this time (commit 07e5a70):

Some of the suppressions have been addressed by minor code changes to implement the suggestion reported by cppcheck.
Other suppressions have been moved inline into the code (after enabling cppcheck to use inline suppressions, see commit 4288f5a)
A number of suppressions were removed because they were not currently used.

(*) Three suppressions with line numbers remain after these changes because one is removed in the context of PR #3132 and two are removed in PR #3104.

eduar-hte commented 2 weeks ago

I implemented two of the three trivial sonarcloud suggestions in commit 869e369.

I actually did the third one as well, about using C++17 structured binding, but I couldn't include it in the PR. It triggers a cppcheck false positive in the version included in Ubuntu 22.04 (cppcheck 2.7.1). This seems to be addressed in cppcheck 2.8 (see here & here).

eduar-hte commented 2 weeks ago

Quick question, is it necessary to run check-static on every Linux build (I noticed that the macOS builds don't include this step) or could this be done on just one build (or a separate workflow)? I was wondering about that because it takes double the time to execute this step than to build the library, and there are so many combinations of Linux builds!

PS: I originally asked this in a comment in PR #3132 but I'm moving it here, where it's more on topic.

eduar-hte commented 2 weeks ago

Quick question, is it necessary to run check-static on every Linux build (I noticed that the macOS builds don't include this step) or could this be done on just one build (or a separate workflow)? I was wondering about that because it takes double the time to execute this step than to build the library, and there are so many combinations of Linux builds!

In my development branches I ended up creating a separate cppcheck workflow to execute it only once:

  cppcheck:
    runs-on: [ubuntu-22.04]
    steps:
      - name: Setup Dependencies
        run: |
          sudo apt-get update -y -qq
          sudo apt-get install -y cppcheck
      - uses: actions/checkout@v4
        with:
          submodules: true
      - name: build.sh
        run: ./build.sh
      - name: configure
        run: ./configure
      - name: check-static
        run: make check-static

If there are no differences in the source code analyzed in each Linux configuration (is this related to the configurations that enable parser generation?), I guess that step could be removed from the Linux builds and replaced with this workflow. This would improve overall execution time.

BTW, updating the version of the checkout action to v4 would remove most of the annotations in the builds (I think only a warning about using sprintf on macOS builds would remain).

eduar-hte commented 2 weeks ago

I actually did the third one as well, about using C++17 structured binding, but I couldn't include it in the PR. It triggers a cppcheck false positive in the version included in Ubuntu 22.04 (cppcheck 2.7.1). This seems to be addressed in cppcheck 2.8 (see here & here).

I found how to add a step to build the latest version of cppcheck (2.14.0) in a couple other repositories, which follow the instructions to build with GNU make:

      - name: Build cppcheck 2.14 # https://github.com/LessRekkless/Community-Patch-DLL/blob/788f2d67fae77be5f59fb10dc585e4a0a7871419/.github/workflows/cppcheck.yml#L24C1-L34C27
        run: |
          cd /tmp
          git clone https://github.com/danmar/cppcheck.git
          cd cppcheck 
          git checkout 2.14.0
          sudo make MATCHCOMPILER=yes FILESDIR=/usr/share/cppcheck HAVE_RULES=yes CXXFLAGS="-O2 -DNDEBUG -Wall -Wno-sign-compare -Wno-unused-function" install
          cd /tmp
          sudo rm -rf /tmp/cppcheck
          sudo ldconfig

I configured my development branch to run cppcheck using this version and it takes more time to execute (about 20 minutes) and reports a larger number of warnings than the current version. Most of them are recommendations to use declare variables/arguments as const (a few of which are performance improvements by avoiding copies of std::string objects).

I've implemented the recommended updates in order to pass the check-static step if cppcheck is upgraded.

airween commented 2 weeks ago

Hi @eduar-hte,

many thanks again for this awesome work!

The goal of this PR is to smooth out the process of updating libModSecurity v3 by avoiding the need to update test/cppcheck_suppressions.txt due to line number changes when doing work unrelated to the suppressions.

Additionally, inlining the suppressions also help removing the suppressions when the code where they're present is refactored/removed. Notice that a number of suppressions currently present in test/cppcheck_suppressions.txt seem to reference issues no longer present in the code and that could be removed.

If there could be removed, would you mind to remove them? Or is there any reason why haven't you removed them yet?

The changes in this PR remove almost (*) all the current suppressions that have a line number reference at this time (commit 07e5a70):
* Some of the suppressions have been addressed by minor code changes to implement the suggestion reported by cppcheck.

* Other suppressions have been moved inline into the code (after enabling cppcheck to use inline suppressions, see commit [4288f5a](https://github.com/owasp-modsecurity/ModSecurity/commit/4288f5a009b3c64c9baf23637efd2837ff3bd8dc))

* A number of suppressions were removed because they were not currently used.
(*) Three suppressions with line numbers remain after these changes because one is removed in the context of PR #3132 and two are removed in PR #3104.

Right, I think we should merge them first and then - probably you should pick up those modifications - this PR. I'm going to answer the other comments.

airween commented 2 weeks ago

I implemented two of the three trivial sonarcloud suggestions in commit 869e369.

I actually did the third one as well, about using C++17 structured binding, but I couldn't include it in the PR. It triggers a cppcheck false positive in the version included in Ubuntu 22.04 (cppcheck 2.7.1). This seems to be addressed in cppcheck 2.8 (see here & here).

Well, this is also a very useful modification, but as I wrote at your other PR, it would be fine to split the commits by features. I mean if you work on a feature what does something, don't mix that with other features. Now you restructured the suppression lists (again: this is great!), but this commit does not connect the main feature.

Could you revert that commit?

airween commented 2 weeks ago

Quick question, is it necessary to run check-static on every Linux build (I noticed that the macOS builds don't include this step) or could this be done on just one build (or a separate workflow)? I was wondering about that because it takes double the time to execute this step than to build the library, and there are so many combinations of Linux builds!

No, I don't think so.

In fact, I think we must reorganize the whole CI workflow of the library. The issues with that:

it seems like it uses three architectures: x64, x32 (AMD/Intel) and MacOS; but as I know, Github CI does not add native x32 VM. Even though it is set, the workflow uses an AMD64 VM - see what packages are installed (all packages come from amd64 repository) (this line is from an x32 workflow!)
it seems like it uses gcc and clang, but even though the current flow should use clang, each of them uses gcc/g++ - see this part
check-static is independent from the configure-option matrix, so I agree, we don't need to run this step in case of all items in the matrix

As you can see, it's time to reorganize the workflow :)

PS: I originally asked this in a comment in PR #3132 but I'm moving it here, where it's more on topic.

Thanks.

airween commented 2 weeks ago

In my development branches I ended up creating a separate cppcheck workflow to execute it only once: ... If there are no differences in the source code analyzed in each Linux configuration (is this related to the configurations that enable parser generation?), I guess that step could be removed from the Linux builds and replaced with this workflow. This would improve overall execution time.

yes, totally agree.

BTW, updating the version of the checkout action to v4 would remove most of the annotations in the builds (I think only a warning about using sprintf on macOS builds would remain).

Thanks for letting know that.

Please don't send any related PR until we don't merge this one at least :)

eduar-hte commented 2 weeks ago

The goal of this PR is to smooth out the process of updating libModSecurity v3 by avoiding the need to update test/cppcheck_suppressions.txt due to line number changes when doing work unrelated to the suppressions. Additionally, inlining the suppressions also help removing the suppressions when the code where they're present is refactored/removed. Notice that a number of suppressions currently present in test/cppcheck_suppressions.txt seem to reference issues no longer present in the code and that could be removed.

If there could be removed, would you mind to remove them? Or is there any reason why haven't you removed them yet?

I already did! :-)

(*) Three suppressions with line numbers remain after these changes because one is removed in the context of PR #3132 and two are removed in PR #3104.

Right, I think we should merge them first and then - probably you should pick up those modifications - this PR. I'm going to answer the other comments.

I agree, if and when this PR is approved, I'd have to rebase the Windows port PR (#3132) to sync with these changes.

airween commented 2 weeks ago

I found how to add a step to build the latest version of cppcheck (2.14.0) in a couple other repositories, which follow the instructions to build with GNU make:

may be we can take a look to Github's "official" cppcheck-action.

I configured my development branch to run cppcheck using this version and it takes more time to execute (about 20 minutes) and reports a larger number of warnings than the current version. Most of them are recommendations to use declare variables/arguments as const (a few of which are performance improvements by avoiding copies of std::string objects).

Nice catch!

I've implemented the recommended updates in order to pass the check-static step if cppcheck is upgraded.

Just one question again: this is an awesome work, but please walk on the way step by step :)

If we can merge the "clean and pure" suppression fix, then we can focusing the next one, namely add the newr version of cppcheck, and fix the new warnings.

And one more thing (again): would you like to join to us on the Slack channel? (#project-modsecurity)

eduar-hte commented 2 weeks ago

Well, this is also a very useful modification, but as I wrote at your other PR, it would be fine to split the commits by features. I mean if you work on a feature what does something, don't mix that with other features. Now you restructured the suppression lists (again: this is great!), but this commit does not connect the main feature.

Could you revert that commit?

I didn't include that change here because it wouldn't work until cppcheck is upgraded, as it'd introduce additional warnings that would require suppressions, so I thought it was not worth it.

airween commented 2 weeks ago

I didn't include that change here because it wouldn't work until cppcheck is upgraded, as it'd introduce additional warnings that would require suppressions, so I thought it was not worth it.

I thought about 869e3695, which is part of this PR.

eduar-hte commented 2 weeks ago

I didn't include that change here because it wouldn't work until cppcheck is upgraded, as it'd introduce additional warnings that would require suppressions, so I thought it was not worth it.

I thought about 869e3695, which is part of this PR.

I see.

I think we can get this PR to be just the move to using inline cppcheck suppressions to remove all the entries with line numbers in test/cppcheck_suppressions.txt. This would require getting the PR back up to 95ce3a7.

PS: The sonarcloud reported issues actually look like cppcheck warnings that it didn't happen to detect.

airween commented 2 weeks ago

Notice that a number of suppressions currently present in test/cppcheck_suppressions.txt seem to reference issues no longer present in the code and that could be removed.

If there could be removed, would you mind to remove them? Or is there any reason why haven't you removed them yet?

I already did! :-)

Sorry, may be I was confused (and I'm still that), but test/cppcheck_suppressions.txt still contains few items, eg these.

These suppressions still presented in the mentioned file - now are they removable?

Right, I think we should merge them first and then - probably you should pick up those modifications - this PR. I'm going to answer the other comments.

I agree, if and when this PR is approved, I'd have to rebase the Windows port PR (#3132) to sync with these changes.

Sounds good!

eduar-hte commented 2 weeks ago

Sorry, may be I was confused (and I'm still that), but test/cppcheck_suppressions.txt still contains few items, eg these.

These suppressions still presented in the mentioned file - now are they removable?

Oh, I only meant entries that have references to line numbers, those that require people to review and adjust test/cppcheck_suppressions.txt to update line numbers.

I didn't review suppressions for whole files because they don't trigger this issue (of having to update line numbers). I also assumed that they had too many suppressions and the decision was made not to bother with a one-by-one approach, and ignore them altogether.

In the scope of this PR, I left only three entries with line numbers because they're already going away in other PRs, and making them inline would just introduce conflicts. Though I guess these changes will require merge work on those PRs as well, so I could go ahead and inline them if we want to clear them all out when this PR is approved.

airween commented 2 weeks ago

I didn't include that change here because it wouldn't work until cppcheck is upgraded, as it'd introduce additional warnings that would require suppressions, so I thought it was not worth it.

I thought about 869e3695, which is part of this PR.

I see.

I think we can get this PR to be just the move to using inline cppcheck suppressions to remove all the entries with line numbers in test/cppcheck_suppressions.txt. This would require getting the PR back up to 95ce3a7.

You don't need to move there - as I know locally you can revert any commit and then push the current state, then Github removes that commit from PR too.

May be you can take a look to b872f11 too, I think that's a similar commit (you can send that in a separate PR or part of a PR next time).

PS: The sonarcloud reported issues actually look like cppcheck warnings that it didn't happen to detect.

Under this PR there are two SonarCloud notices:

the first one is "just" a warning for using of strlen() here; this is not a new code, but SonarCloud can't detect that's the same as in the previous version
the others are the code duplication, eg like here, which is also not your code, you just modified something there.

I'm sure we can mark these SonarCloud issues as false positive.

airween commented 2 weeks ago

Sorry, may be I was confused (and I'm still that), but test/cppcheck_suppressions.txt still contains few items, eg these. These suppressions still presented in the mentioned file - now are they removable?

Oh, I only meant entries that have references to line numbers, those that require people to review and adjust test/cppcheck_suppressions.txt to update line numbers.

ah, thanks, now it's clear!

I didn't review suppressions for whole files because they don't trigger this issue (of having to update line numbers). I also assumed that they had too many suppressions and the decision was made not to bother with a one-by-one approach, and ignore them altogether.

This makes sense, thanks.

In the scope of this PR, I left only three entries with line numbers because they're already going away in other PRs, and making them inline would just introduce conflicts. Though I guess these changes will require merge work on those PRs as well, so I could go ahead and inline them if we want to clear them all out when this PR is approved.

Okay, this question is clear for me, thank you.

eduar-hte commented 2 weeks ago

May be you can take a look to b872f11 too, I think that's a similar commit (you can send that in a separate PR or part of a PR next time).

Hmm... If I could push back a little in this case. The change is about addressing the memory leak reported by cppcheck, which currently has a suppression with line number (and which I actually had to update when I made the example compile on Windows for PR #3132). This being an example, I think there is little risk in actually fixing the leak. Additionally, the alternative looks worse, that is, an example with a memory leak and an inline cppcheck suppression flagging it! :-)

eduar-hte commented 2 weeks ago

I found how to add a step to build the latest version of cppcheck (2.14.0) in a couple other repositories, which follow the instructions to build with GNU make:

may be we can take a look to Github's "official" cppcheck-action.

Yes, I saw it too when I was trying to run a newer version of cppcheck in my development branch's workflow. However, I decided against introducing an action to do this because then you'd have to maintain both the configuration to run cppcheck in the build configuration (which you can also use when downloading the repository and building the library yourself) and the one for the CI workflow.

I didn't look into it in detail, but it also meant that I'd have to learn about the cppcheck parameters and see if they were supported and how they'd map into the action.

Additionally, it seems that the action relies on a Docker file, so you depend on that project to be updated. For example, it's currently at 2.12.1, while I was able to just use the current release 2.14. So instead of having three dependencies (GitHub action, Docker file & cppcheck project), it seems simpler to depend on the source itself, given it's simple enough.

eduar-hte commented 2 weeks ago

As discussed, I rolled back the additional changes for the issues reported by cppcheck 2.14.0 I worked on during the weekend, and left those specifically related to removing suppressions entries with line numbers in test/cppcheck_suppressions.txt.

I kept the other changes in a separate branch in my fork so that I can create a PR when this one is merged, as most of the changes are simple enough (add some const qualifiers, add missing override specifiers, and remove unnecessary overrides -as cppcheck detects when the base class implementation is the same-).

eduar-hte commented 2 weeks ago

(*) Three suppressions with line numbers remain after these changes because one is removed in the context of PR #3132 and two are removed in PR #3104.

In the end I removed these suppressions with line numbers in test/cppcheck_suppressions.txt for the sake of completeness, and because work to merge that file with those PRs was already inevitable (because of all its changes).

airween commented 2 weeks ago

I see that you added some const modifier after method definitions, eg. here.

Would you mind then add it where the SonarCloud shows too?

After that I think we are done with this - many thanks again. The request change refers only this fix.

eduar-hte commented 2 weeks ago

Would you mind then add it where the SonarCloud shows too?

Oh, those were the ones I initially fixed in commit 869e369 (see comment), and ended up rolling back with the rest of the changes (triggered by trying to address the third one as well, which required upgrading cppcheck).

I'll add them again, no worries.

sonarcloud[bot] commented 2 weeks ago

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

eduar-hte commented 2 weeks ago

Notice that SonarCloud still reports 3 issues, but now the three are about using structured binding.

airween commented 2 weeks ago

Oh, those were the ones I initially fixed in commit 869e369 (see comment), and ended up rolling back with the rest of the changes (triggered by trying to address the third one as well, which required upgrading cppcheck).

Oh my bad, really sorry. I didn't realize that.

I'll add them again, no worries.

Many thanks.

Seems like it's done. I wait for CI will finished and merge this PR.

airween commented 2 weeks ago

Notice that SonarCloud still reports 3 issues, but now the three are about using structured binding.

Yes, I see. Anyway, next time... :smiley:

airween commented 2 weeks ago

Thanks @eduar-hte, this was a huge step.

owasp-modsecurity / ModSecurity

Remove cppcheck suppressions with line numbers in test/cppcheck_suppressions.txt #3134

Quality Gate passed