search

owasp-modsecurity / ModSecurity

ModSecurity is an open source, cross platform web application firewall (WAF) engine for Apache, IIS and Nginx. It has a robust event-based programming language which provides protection from a range of attacks against web applications and allows for HTTP traffic monitoring, logging and real-time analysis.
https://www.modsecurity.org
Apache License 2.0
7.7k stars 1.54k forks source link

Bump the C++ version from C++11 to C++17 #3079

Closed MirkoDziadzka closed 3 months ago

MirkoDziadzka commented 3 months ago

This will allow the usage of more modern features in the future.

sonarcloud[bot] commented 3 months ago

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

gberkes commented 3 months ago

Hi Mirko!

Before last xmas I built ModSec with these flags: CXXFLAGS=-std=c++20 -Wall -Wextra -Wconversion -Wshadow -Wpedantic -fsanitize=undefined,address LDFLAGS=-fsanitize=undefined,address It produced a huge amount of warnings. I read about secure coding and found, that one of the best practices is to fully utilize the language and the compiler provided features to enhance code clarity and security before using 3rd party static and dynamic analyzers.

I kindly ask what is your opinion not to stop at C++17 but bump it to C++20, and introduce more compiler and linker flags to promote higher level code quality?

My goal is not to use new language features as much as possible at all cost, but let the language and the compiler help us write nicer, cleaner and more secure code.

References: https://wiki.sei.cmu.edu/confluence/display/seccode/Top+10+Secure+Coding+Practices https://learning.oreilly.com/library/view/c-coding-standards/0321113586/ch02.html https://www.sonarsource.com/blog/modernizing-your-code-with-cpp20/

airween commented 3 months ago

Hi @MirkoDziadzka, thanks for sent this patch.

I took a quick review on QA logs and compared them with a previous build results. Here are some details just FYI:

Perhaps this one is because of the changed of the flags, but I guess those are also not relevant. So it looks like there are no limiting factors to switch to C++17. Let us wait for some other opinions.

mirkodziadzka-avi commented 3 months ago

I kindly ask what is your opinion not to stop at C++17 but bump it to C++20, and introduce more compiler and linker flags to promote higher level code quality?

I'm all in to add all compiler warnings and static analyzer flags we can. But from experience, this is a slow process because you have to fix a lot on the way. But yes, it is the right way.

Regarding compiler version: We can bump it to C++20. 17 is a compromise where a lot of modern C++ is already there and what should be supported on all modern systems. 20 may be too young for some build environments. I don't really know. YMMV

This PR is more or less: I want to fix another problem, for performance reasons, I would prefer to have std::string_view and not only std::string and therefore I did open this PR.

Maybe we need to distinguish between the production build compiler version which dictates the language feature versions we can use and the dev compiler build which can used the newest compiler and the best static analyzer tools.

gberkes commented 3 months ago

Maybe we need to distinguish between the production build compiler version which dictates the language feature versions we can use and the dev compiler build which can used the newest compiler and the best static analyzer tools.

That's a great idea. I'll work that way. Thank you very much.

zimmerle commented 3 months ago

Building on our previous discussions in 2020 regarding the adoption of std::string_view, as captured in the GitHub pull request here, it's worth noting that there might already exist a branch where this development has been undertaken. At that time, we encountered compiler-related issues on several widely-used distributions. Additionally, we explored the option of integrating standalone implementations of std::string_view, such as the one available at martinmoene/string-view-lite, to mitigate these challenges. We dropped as won't be wide spread, some vendors adopted as they had infra for it. I voluntear to review anything on that segment. The std::string_view is particullary interesting with the utilization of an API expansion that would allow strings be constructed with buffer + len. As oppose to only buffer. That would avoid copies on the connectors as well.

@majordaw @airween you can count on me to review the std::string_view implemetation.