cmu-sei / redemption

Redemption is a tool that automatically repairs C/C++ code given a set of static-analysis alerts
Other
3 stars 1 forks source link

Need Clarification on 3rd Alert Category #4

Open donaldmurf opened 2 months ago

donaldmurf commented 2 months ago

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 commented 2 months ago

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.