V3/remove this throw call transaction h #3104
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…atements. - SonarCloud analysis identified standalone `throw;` calls without accompanying `try-catch` blocks, used inconsistently as placeholders or for premature termination under specific conditions. - Removed these `throw;` instances to prevent potential runtime issues in future development phases, where such configurations might inadvertently be created. - Introduced `assert` statements as a more appropriate mechanism for asserting preconditions in the affected class member functions, ensuring clearer intent and safer code behavior during development.
Implemented a new configuration option --enable-assertions=[yes|no] within config.ac, enabling controlled inclusion of -DNDEBUG in CPPFLAGS. The default setting suppresses assertions (by adding -DNDEBUG to CPPFLAGS), preserving the original behavior. This enhancement allows for the optional enabling of assertions during development or debugging by setting --enable-assertions=yes, thereby excluding -DNDEBUG from CPPFLAGS.
…call_transaction_h
…call_transaction_h
added 2 commits
March 27, 2024 14:42
Resolved a null pointer dereference error identified by `make check-static` due to an outdated suppression in 'test/cppcheck_suppressions.txt'. The suppression at line 57 was incorrectly referencing 'src/rule_with_actions.cc:244'. Updated this to 'src/rule_with_actions.cc:243' to align with the current codebase. Additionally, removed suppressions for 'rethrowNoCurrentException' as identified by SonarCloud. This was possible due to the removal of standalone 'throw;' statements in the code.
airween
reviewed
Mar 29, 2024
airween
reviewed
Mar 29, 2024
|
Just for the record: this PR fixes two SonarCloud issues in files: I also added these references as conversation. |
airween
requested changes
Mar 29, 2024
…call_transaction_h
…elopment and usage of assertions.
…ains; add assertion in default case.
|
This was referenced Apr 28, 2024
eduar-hte
added a commit
to eduar-hte/ModSecurity
that referenced
this pull request
Apr 30, 2024
- These were initially not included in these changes, as they were other PRs (owasp-modsecurity#3104 & owasp-modsecurity#3132) that address them.
eduar-hte
added a commit
to eduar-hte/ModSecurity
that referenced
this pull request
May 4, 2024
- These were initially not included in these changes, as they were other PRs (owasp-modsecurity#3104 & owasp-modsecurity#3132) that address them.
|
@gberkes |
eduar-hte
added a commit
to eduar-hte/ModSecurity
that referenced
this pull request
Aug 6, 2024
- These were initially not included in these changes, as they were other PRs (owasp-modsecurity#3104 & owasp-modsecurity#3132) that address them.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Introducing the use of assertions to address throw; calls that lack try-catch blocks. Upon examining the caller code that utilized methods containing the questioned throw; calls, it became clear that, in the current state of development, there are no scenarios where execution could reach these throw; calls. However, we cannot guarantee this for future development. For instance, if someone attempts to use getCurrentMarker() without first verifying isInsideAMarker(), ModSecurity would encounter the throw; and terminate. The issue with the other throw; call is similar in that it is, fortunately, unreachable at the moment. However, it differs because this throw; is intended to handle a case that has not yet been developed.
Fortunately, I found an article titled "Effective Use of Assertions in C++" by Mike A. Martin in (ACM SIGPLAN Language Tips, page 3), which offers a neat way to handle such cases, specifically regarding argument validation and unreachable code. Link to the article.
Following the guidance from this article, I addressed the issues and also included modifications to enrich assert error messages. Furthermore, I updated configure.ac to maintain the usual build procedure and modified README.md to introduce the new configure flag.