Build libModSecurity with std C++20

[eduar-hte]

what

Update build configuration to build the library using std C++20

why

The goal of this PR is not necessarily to update this configuration at this time, but rather to document that the library builds correctly at this stage using std C++20 on all platforms.

why not C++23

This PR is limited to C++20 because moving to C++23 is not yet possible on all platforms, as the compilers available in some of the images do not include support for this C++ std. Additionally, the current version of the ax_cxx_compile_stdcxx autoconf macro doesn't support C++23 yet either.

NOTE: Building the library using std C++23 on Windows found the issue found in PR #3220.

a note on adoption of C++20 ranges

C++20 introduces the ranges library (see here). The library offers a powerful and expressive way to handle data due to its declarative approach and composability.

However, adoption of the library needs to consider potential performance drawbacks when compared to existing approaches (such as STL algorithms), as library implementations may not be as efficient yet.

For the sake of reference, see:

• Performance benchmark: Ranges VS STL algorithms VS Smart output iterators
• Performance of C++20's ranges
• C++ ranges are better than iterators?

sonarcloud code analysis on PRs usually recommend replacing existing code with C++20 ranges, but that feedback should be taken with a grain of salt (such as considering performance implications of the changes).

NOTE: This is feedback is unrelated to the Range-based for loop feature introduced in C++11.

references

The library was updated to build with std C++17 in PR #3079 (and the configuration was simplified in PR #3219 to make it easier to upgrade in the future).

[gberkes]

I don't know whether all LTS Linux distributions containing/supporting ModSecurity have GCC capable of compiling with -std=C++20. I guess users of ModSecurity have little interest in which standard we are using.

As a user of a security product, I would be much more interested in the quality of the codebase the product is built from. First of all, IMHO, we should not only focus on what SonarCloud reports but on what the G++ compiler would report with the appropriate compiler and linker flags used throughout development and testing. Currently, we are using almost none of what G++ could provide regarding the quality of our code. Instead, we are relying on Cppcheck, with many suppressions, some without explanations as to why they are false positives.

So, IMHO, it would be a better idea to first use something like: g++ -std=c++17 -g -Wall -Wextra -Wconversion -Wshadow -Wpedantic -fsanitize=undefined,address during development and testing, and refactor the code to build warning-free. Besides this, making an effort to eliminate or explain the Cppcheck suppressions would be beneficial.

IMHO, we developers and the end users of ModSecurity will benefit more from these refactorings than from using newer language features. For example, until the next release, one of the goals could be to achieve a warning-free build, extend test coverage, and plan which parts of the code would be good to refactor using the new standard's constructs. After that, there's nothing against bumping to C++20, except the supported OSes' GCC standard coverage.

Reference:
https://wiki.sei.cmu.edu/confluence/display/c/MSC00-C.+Compile+cleanly+at+high+warning+levels

[eduar-hte]

I don't disagree. As I stated in the description to this PR, the goal is not to have this integrated now, but rather to document how to update the compiler version in the future and also to provide visibility on whether the codebase compiles with C++20 (as I mentioned, it didn't with C++23 due to a couple of minor issues -that apply in C++17 as well-):

The goal of this PR is not necessarily to update this configuration at this time, but rather to document that the library builds correctly at this stage using std C++20 on all platforms.

I wouldn't minimize the current state of the codebase, though, as compiling with no warnings and cppcheck (with few suppressions) is significant. Of course, raising the bar is always good and could be a focus for improvement. Earlier this year I proposed using the latest version of cppcheck and have a branch with that, which I've not submitted because it takes significantly longer to analyze the codebase and there's a trade-off to consider there.

With regards to sonarcloud, it's a nice addition as well to the analysis of the codebase. I think it's helpful to focus on issues related to security (and performance) though. In my experience, it's reporting many suggestions on leveraging modern C++ features such as structured binding or replacing NULL with nullptr, which are nice and can be adopted gradually as the codebase evolves with actual functional changes and fixes.

[airween]

Hi @eduar-hte,

I don't disagree. As I stated in the description to this PR, the goal is not to have this integrated now, but rather to document how to update the compiler version in the future and also to provide visibility on whether the codebase compiles with C++20 (as I mentioned, it didn't with C++23 due to a couple of minor issues -that apply in C++17 as well-):

then may be you should add a small block into the README.

With this PR you requested to change the c++ version from 17 to 20.

I downloaded your branch into a Debian 11 (which is still a supported system) and I got:

...
checking pkg-config is at least version 0.9.0... yes
checking whether g++ supports C++20 features with -std=c++20... no
checking whether g++ supports C++20 features with +std=c++20... no
checking whether g++ supports C++20 features with -h std=c++20... no
checking whether g++ supports C++20 features with -std:c++20... no
configure: error: *** A compiler with support for C++20 language features is required.

So I'm not able to build the library.

Note: Debian 11 uses gcc 10.2, Ubuntu Focal (20.04 - this is an LTS system) uses gcc 9.3.

The goal of this PR is not necessarily to update this configuration at this time, but rather to document that the library builds correctly at this stage using std C++20 on all platforms.

But this PR definitely updates the configuration, doesn't it?

[eduar-hte]

But this PR definitely updates the configuration, doesn't it?

Yes, but I'm not proposing integrating this PR right now, but at some point in the future.

I don't think there's any urgency to move to C++20. Moreover, my main concern would be upgrading to that version and start adopting the ranges library suggestions by sonarcloud and having a negative impact on performance, just because the tool said so.

I've been building the library myself using the default configuration, but also with newer versions of the standard as well, in order to detect any potential and unnecessary roadblock to upgrading in the future. This PR can easily be rebased as well to keep track of this until the moment is right to integrate it.

[airween]

But this PR definitely updates the configuration, doesn't it?

Yes, but I'm not proposing integrating this PR right now, but at some point in the future.

Ahh, right - then please feel free to add labels on the right side of the page (now I added [do not merge]), which help us to discover the real reason 😃.

[eduar-hte]

Ahh, right - then please feel free to add labels on the right side of the page (now I added [do not merge]), which help us to discover the real reason 😃.

@airween, I cannot set labels. I thought it was clear from the start, or that's what I wrote on the why section when submitting the PR anyway.

[airween]

Ahh, right - then please feel free to add labels on the right side of the page (now I added [do not merge]), which help us to discover the real reason 😃.

@airween, I cannot set labels.

uhm, sorry. Yes, now I see a triage access needs to apply any label (and let me take the opportunity again and call your attention your pending invitation to developers group :) - with that, you are able to apply labels)

I thought it was clear from the start, or that's what I wrote on the why section when submitting the PR anyway.

Sorry, I was a bit confused, because the sent modifications would have changed the build system.

Sorry again.

[sonarqubecloud]

Quality Gate passed

Issues
0 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