Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Need Clarification on 3rd Alert Category #4

Open
donaldmurf opened this issue Jun 24, 2024 · 1 comment
Open

Need Clarification on 3rd Alert Category #4

donaldmurf opened this issue Jun 24, 2024 · 1 comment

Comments

@donaldmurf
Copy link

donaldmurf commented Jun 24, 2024

Screenshot from 2024-06-24 16-00-46

The third category for alerts is "Ineffective Code CERT ID MSC12-C CWE ID 561".

MSC12-C says "Code that has no effect or is never executed (that is, dead or unreachable code)"

CWE 561 says "Dead code is code that can never be executed in a running program. The surrounding code makes it impossible for a section of code to ever be executed."

In the Alert Categories it seems to jump from MSC12-C to EXP12-C. EXP12-C say "Do not ignore values returned by functions".

I've put a red box around the deadcode part and a blue box around EXP12-C.

I ran example 1 from EXP12-C and cppcheck does not recognize EXP12-C, so no repairs were made to it.
I ran some examples of dead code, which cppcheck detected, but no fixes were made to the deadcode.

Can you please clarify exactly what the 3rd Alert is, and how redemption fixes it? Is redemption supposed to remove deadcode from the sourcefile?

In the SEI Research and Review, it appears the 3rd category is supposed to be MSC12-C.
Screenshot from 2024-06-24 16-45-23

Here is an example of some deadcode I tried to have repaired. The left is repaired. You can see myint was changed to myint =0. However, none of the deadcode was changed.
image

Here is a snippet from the alerts.json showing cppcheck found the error and it was converted into a .json file.
image

@sei-dsvoboda
Copy link
Contributor

Your understanding of what we consider 'dead code' appears to be correct.

We did an analysis of the 'dead-code' alerts that cppcheck and clang-tidy gave us, it is published here:
https://github.com/cmu-sei/redemption/blob/main/doc/dead-code.md

There are many reasons why code may be dead, but the two most prominent sources of dead code are (1) a variable is assigned, but never read, and (2) checking that an unsigned int is less than 0, which is always false. These are the only types of dead code that we currently repair right now.

We could extend Redemption to repair always-true or always-false relations such as the ones in your example code; we just haven't encountered enough such examples of code when scanning git or zeek.

I should add that MSC12-C is a controversial recommendation to repair (because many times you can get dead-code alerts that should NOT be repaired). This is because it is a CERT recommendation, not a rule. Repairing CERT rule violations always makes the code better, but repairing CERT recommendations may make the code harder to maintain...that is part of what makes MSC12-C a recommendation, rather than a rule.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants