Skip to content

C++: Fix some FPs in cpp/missing-check-scanf (third attempt!) #18207

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Dec 16, 2024
Merged

C++: Fix some FPs in cpp/missing-check-scanf (third attempt!) #18207

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
20dfbdc
Revert "Merge pull request #18057 from jketema/codeql-cli-2.19.4"
MathiasVP Dec 4, 2024
6f73aa5
C++: Convert IRGuards to use final abstract classes.
MathiasVP Dec 4, 2024
404dd33
C++: Move the main recursion into to a cached module.
MathiasVP Dec 4, 2024
3bdfdd0
C++: Change all the 'ensures' and (and most 'compares') predicates to…
MathiasVP Dec 4, 2024
5327847
C++: No need to exclude static and global initializers now that we in…
MathiasVP Dec 4, 2024
5a90b25
C++: Remove the released change note and add a new change note.
MathiasVP Dec 4, 2024
913357b
C++: Fix incorrect QLDoc.
MathiasVP Dec 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 14 additions & 30 deletions cpp/ql/lib/semmle/code/cpp/controlflow/IRGuards.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,28 +142,15 @@ private class LogicalNotValueNumber extends ValueNumber {
/**
* A Boolean condition in the AST that guards one or more basic blocks. This includes
* operands of logical operators but not switch statements.
*
* For performance reasons conditions inside static local initializers or
* global initializers are not considered `GuardCondition`s.
*/
cached
class GuardCondition extends Expr {
cached
GuardCondition() {
exists(IRGuardCondition ir | this = ir.getUnconvertedResultExpression())
or
// no binary operators in the IR
this.(BinaryLogicalOperation).getAnOperand() instanceof GuardCondition
}
Comment on lines -67 to -72
Copy link
Contributor

@jketema jketema Dec 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this removed?

Copy link
Contributor Author

@MathiasVP MathiasVP Dec 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before the introduction of final aliases this class couldn't be made abstract (since it would expose a abstract class from our libraries to queries). So instead of having this class be abstract we followed the pattern of:

  1. Specifying each subclass of this "pseudo-abstract" class in the charpred
  2. Provide a subclass that extended this "pseudo-abstract" class and repeated the required charpred to define the subclass.

That is, we've had to do:

class Base {
  Base() {
     myImpl1(this) or myImpl2(this)
  }
}

class MyImpl1 extends Base {
  MyImpl1() {
    myImpl1(this)
  }
}

class MyImpl2 extends Base {
  MyImpl2() {
    myImpl1(this)
  }
}

Now that we have final classes we can get rid of this pattern (as we've done many other places) and actually use abstract classes. And abstract classes don't need this charpred since they're defined as the union of all the subclasses.

That is, we can rewrite the above example to:

private abstract class BaseImpl { } // <--- Note: We can now remove the charpred and only have it in the subclasses

final class Base = BaseImpl;

class MyImpl1 extends BaseImpl {
  MyImpl1() {
    myImpl1(this)
  }
}

class MyImpl2 extends BaseImpl {
  MyImpl2() {
    myImpl1(this)
  }
}

Does that make sense?

Copy link
Contributor

@jketema jketema Dec 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes total sense.


abstract private class GuardConditionImpl extends Expr {
/**
* Holds if this condition controls `controlled`, meaning that `controlled` is only
* entered if the value of this condition is `v`.
*
* For details on what "controls" mean, see the QLDoc for `controls`.
*/
cached
predicate valueControls(BasicBlock controlled, AbstractValue v) { none() }
abstract predicate valueControls(BasicBlock controlled, AbstractValue v);

/**
* Holds if this condition controls `controlled`, meaning that `controlled` is only
Expand Down Expand Up @@ -197,61 +184,58 @@ class GuardCondition extends Expr {
}

/** Holds if (determined by this guard) `left < right + k` evaluates to `isLessThan` if this expression evaluates to `testIsTrue`. */
cached
predicate comparesLt(Expr left, Expr right, int k, boolean isLessThan, boolean testIsTrue) {
none()
}
abstract predicate comparesLt(Expr left, Expr right, int k, boolean isLessThan, boolean testIsTrue);

/**
* Holds if (determined by this guard) `left < right + k` must be `isLessThan` in `block`.
* If `isLessThan = false` then this implies `left >= right + k`.
*/
cached
predicate ensuresLt(Expr left, Expr right, int k, BasicBlock block, boolean isLessThan) { none() }
abstract predicate ensuresLt(Expr left, Expr right, int k, BasicBlock block, boolean isLessThan);

/**
* Holds if (determined by this guard) `e < k` evaluates to `isLessThan` if
* this expression evaluates to `value`.
*/
cached
predicate comparesLt(Expr e, int k, boolean isLessThan, AbstractValue value) { none() }
abstract predicate comparesLt(Expr e, int k, boolean isLessThan, AbstractValue value);

/**
* Holds if (determined by this guard) `e < k` must be `isLessThan` in `block`.
* If `isLessThan = false` then this implies `e >= k`.
*/
cached
predicate ensuresLt(Expr e, int k, BasicBlock block, boolean isLessThan) { none() }
abstract predicate ensuresLt(Expr e, int k, BasicBlock block, boolean isLessThan);

/** Holds if (determined by this guard) `left == right + k` evaluates to `areEqual` if this expression evaluates to `testIsTrue`. */
cached
predicate comparesEq(Expr left, Expr right, int k, boolean areEqual, boolean testIsTrue) {
none()
}
abstract predicate comparesEq(Expr left, Expr right, int k, boolean areEqual, boolean testIsTrue);

/**
* Holds if (determined by this guard) `left == right + k` must be `areEqual` in `block`.
* If `areEqual = false` then this implies `left != right + k`.
*/
cached
predicate ensuresEq(Expr left, Expr right, int k, BasicBlock block, boolean areEqual) { none() }
abstract predicate ensuresEq(Expr left, Expr right, int k, BasicBlock block, boolean areEqual);

/** Holds if (determined by this guard) `e == k` evaluates to `areEqual` if this expression evaluates to `value`. */
cached
predicate comparesEq(Expr e, int k, boolean areEqual, AbstractValue value) { none() }
abstract predicate comparesEq(Expr e, int k, boolean areEqual, AbstractValue value);

/**
* Holds if (determined by this guard) `e == k` must be `areEqual` in `block`.
* If `areEqual = false` then this implies `e != k`.
*/
cached
predicate ensuresEq(Expr e, int k, BasicBlock block, boolean areEqual) { none() }
abstract predicate ensuresEq(Expr e, int k, BasicBlock block, boolean areEqual);
}

final class GuardCondition = GuardConditionImpl;

/**
* A binary logical operator in the AST that guards one or more basic blocks.
*/
private class GuardConditionFromBinaryLogicalOperator extends GuardCondition {
private class GuardConditionFromBinaryLogicalOperator extends GuardConditionImpl {
GuardConditionFromBinaryLogicalOperator() {
this.(BinaryLogicalOperation).getAnOperand() instanceof GuardCondition
}
Expand Down Expand Up @@ -329,7 +313,7 @@ private class GuardConditionFromBinaryLogicalOperator extends GuardCondition {
* A Boolean condition in the AST that guards one or more basic blocks and has a corresponding IR
* instruction.
*/
private class GuardConditionFromIR extends GuardCondition {
private class GuardConditionFromIR extends GuardConditionImpl {
IRGuardCondition ir;

GuardConditionFromIR() { this = ir.getUnconvertedResultExpression() }
Expand Down