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

Check that the iterator is not end() after calling findKey #1758

Merged

Conversation

kevinbackhouse
Copy link
Collaborator

Fixes GHSA-h9x9-4f77-336w

The bug is that this call to findKey might not find anything, but the iterator still gets dereferenced. Every call to findKey should always be followed by a check that the iterator doesn't equal end().

I used a CodeQL query to find other instances of the same bug and fixed them too.

@kevinbackhouse
Copy link
Collaborator Author

CodeQL query that I used:

import cpp
import semmle.code.cpp.controlflow.Guards
import semmle.code.cpp.dataflow.DataFlow

// Holds if `cond` is a condition like `use == table.end()`.
// `eq_is_true` is `true` for `==`, `false` for '!=`.
// Note: the `==` is actually an overloaded `operator==`.
predicate end_condition(GuardCondition cond, Expr use, FunctionCall endCall, boolean eq_is_true) {
  exists(FunctionCall eq |
    exists(string opName | eq.getTarget().getName() = opName |
      opName = "operator==" and eq_is_true = true
      or
      opName = "operator!=" and eq_is_true = false
    ) and
    DataFlow::localExprFlow(use, eq.getAnArgument()) and
    DataFlow::localExprFlow(endCall, eq.getAnArgument()) and
    endCall.getTarget().getName() = "end" and
    DataFlow::localExprFlow(eq, cond)
  )
}

from FunctionCall call, Expr use
where
  call.getTarget().getName() = "findKey" and
  DataFlow::localExprFlow(call, use) and
  use != call and
  not use.(AssignExpr).getRValue() = call and
  not end_condition(_, use, _, _) and
  not exists(
    Expr cond_use, FunctionCall endCall, GuardCondition cond, BasicBlock block, boolean branch
  |
    end_condition(cond, cond_use, endCall, branch) and
    DataFlow::localExprFlow(call, cond_use) and
    cond.controls(block, branch.booleanNot()) and
    block.contains(use)
  )
select call, use

@kevinbackhouse
Copy link
Collaborator Author

Turns out that this also fixes GHSA-54f7-vvj7-545w

src/convert.cpp Show resolved Hide resolved
@hassec hassec added the bug label Jul 5, 2021
@hassec hassec added this to the v0.27.5 milestone Jul 5, 2021
@kevinbackhouse kevinbackhouse added the forward-to-main Forward changes in a 0.28.x PR to main with Mergify label Jul 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug forward-to-main Forward changes in a 0.28.x PR to main with Mergify
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants