Skip to content

CPP: Autoformat some untidy files #744

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
jbj merged 5 commits into from
Jan 14, 2019
Merged

CPP: Autoformat some untidy files #744

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b7febb0
CPP: Autoformat some Power of 10 queries.
geoffw0 Jan 9, 2019
346bc1a
CPP: Autoformat some code from Critical.
geoffw0 Jan 9, 2019
c4b01d0
CPP: Autoformat some other untidy source files.
geoffw0 Jan 10, 2019
ba3bc15
CPP: Manual fixup.
geoffw0 Jan 9, 2019
87569d1
CPP: QLDoc comments.
geoffw0 Jan 10, 2019
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
63 changes: 38 additions & 25 deletions cpp/ql/src/Critical/DeadCodeCondition.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,51 +7,64 @@
* @tags reliability
* external/cwe/cwe-561
*/

import cpp

predicate testAndBranch(Expr e, Stmt branch)
{
exists(IfStmt ifstmt | ifstmt.getCondition() = e and
(ifstmt.getThen() = branch or ifstmt.getElse() = branch))
predicate testAndBranch(Expr e, Stmt branch) {
exists(IfStmt ifstmt |
ifstmt.getCondition() = e and
(ifstmt.getThen() = branch or ifstmt.getElse() = branch)
)
or
exists(WhileStmt while | while.getCondition() = e and
while.getStmt() = branch)
exists(WhileStmt while |
while.getCondition() = e and
while.getStmt() = branch
)
}

predicate choice(LocalScopeVariable v, Stmt branch, string value)
{
predicate choice(LocalScopeVariable v, Stmt branch, string value) {
exists(AnalysedExpr e |
testAndBranch(e, branch) and
(
(e.getNullSuccessor(v) = branch and value = "null")
or
(e.getNonNullSuccessor(v) = branch and value = "non-null")
))
)
)
}


predicate guarded(LocalScopeVariable v, Stmt loopstart, AnalysedExpr child)
{
predicate guarded(LocalScopeVariable v, Stmt loopstart, AnalysedExpr child) {
choice(v, loopstart, _) and
loopstart.getChildStmt*() = child.getEnclosingStmt() and
(definition(v, child) or exists(child.getNullSuccessor(v)))
}

predicate addressLeak(Variable v, Stmt leak)
{
predicate addressLeak(Variable v, Stmt leak) {
exists(VariableAccess access |
v.getAnAccess() = access and
access.getEnclosingStmt() = leak and
access.isAddressOfAccess())
access.isAddressOfAccess()
)
}

from LocalScopeVariable v, Stmt branch, AnalysedExpr cond, string context, string test, string testresult
where choice(v, branch, context)
and forall(ControlFlowNode def | definition(v, def) and definitionReaches(def, cond) | not guarded(v, branch, def))
and not cond.isDef(v)
and guarded(v, branch, cond)
and exists(cond.getNullSuccessor(v))
and not addressLeak(v, branch.getChildStmt*())
and ((cond.isNullCheck(v) and test = "null") or (cond.isValidCheck(v) and test = "non-null"))
and (if context = test then testresult = "succeed" else testresult = "fail")
select cond, "Variable '" + v.getName() + "' is always " + context + " here, this check will always " + testresult + "."
from
LocalScopeVariable v, Stmt branch, AnalysedExpr cond, string context, string test,
string testresult
where
choice(v, branch, context) and
forall(ControlFlowNode def | definition(v, def) and definitionReaches(def, cond) |
not guarded(v, branch, def)
) and
not cond.isDef(v) and
guarded(v, branch, cond) and
exists(cond.getNullSuccessor(v)) and
not addressLeak(v, branch.getChildStmt*()) and
(
(cond.isNullCheck(v) and test = "null")
or
(cond.isValidCheck(v) and test = "non-null")
) and
(if context = test then testresult = "succeed" else testresult = "fail")
select cond,
"Variable '" + v.getName() + "' is always " + context + " here, this check will always " +
testresult + "."
48 changes: 32 additions & 16 deletions cpp/ql/src/Critical/NotInitialised.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,34 +7,46 @@
* @tags reliability
* external/cwe/cwe-457
*/

import cpp

// See also InitialisationNotRun.ql and GlobalUseBeforeInit.ql
Copy link
Copy Markdown
Contributor

@aschackmull aschackmull Jan 10, 2019

Choose a reason for hiding this comment

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

If this was a multi-line /* .. */ comment it would get the extra newline after it. Generally the autoformatter expects toplevel comments to be of the form /* .. */ and in-predicate comments to be // ....

Copy link
Copy Markdown
Contributor Author

@geoffw0 geoffw0 Jan 10, 2019

Choose a reason for hiding this comment

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

I personally tend to use // for most comments, unless either (1) it's going to be many lines or (2) I want to finish it before the end of the line. Of course use of qldoc comments /** .. */ frequently overrides this preference.

Copy link
Copy Markdown
Contributor Author

@geoffw0 geoffw0 Jan 10, 2019

Choose a reason for hiding this comment

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

(in this case I believe the comments were originally written by someone else)


// Holds if s defines variable v (conservative)
/**
* Holds if `s` defines variable `v` (conservative).
*/
predicate defines(ControlFlowNode s, Variable lv) {
exists(VariableAccess va | va = s and va.getTarget() = lv and va.isUsedAsLValue())
}

// Holds if s uses variable v (conservative)
/**
* Holds if `s` uses variable `v` (conservative).
*/
predicate uses(ControlFlowNode s, Variable lv) {
exists(VariableAccess va | va = s and va.getTarget() = lv and va.isRValue()
and not va.getParent+() instanceof SizeofOperator)
exists(VariableAccess va |
va = s and
va.getTarget() = lv and
va.isRValue() and
not va.getParent+() instanceof SizeofOperator
)
}

// Holds if there is a path from the declaration of lv to n such that lv is
// definitely not defined before n
/**
* Holds if there is a path from the declaration of `lv` to `n` such that `lv` is
* definitely not defined before `n`.
*/
predicate noDefPath(LocalVariable lv, ControlFlowNode n) {
n.(DeclStmt).getADeclaration() = lv and not exists(lv.getInitializer())
or exists(ControlFlowNode p | noDefPath(lv, p) and n = p.getASuccessor() and not defines(p, lv))
n.(DeclStmt).getADeclaration() = lv and not exists(lv.getInitializer())
or
exists(ControlFlowNode p | noDefPath(lv, p) and n = p.getASuccessor() and not defines(p, lv))
}

predicate isAggregateType(Type t) {
t instanceof Class or t instanceof ArrayType
}
predicate isAggregateType(Type t) { t instanceof Class or t instanceof ArrayType }

// Holds if va is a use of a local variable that has not been previously
// defined
/**
* Holds if `va` is a use of a local variable that has not been previously
* defined.
*/
predicate undefinedLocalUse(VariableAccess va) {
exists(LocalVariable lv |
// it is hard to tell when a struct or array has been initialized, so we
Expand All @@ -43,17 +55,21 @@ predicate undefinedLocalUse(VariableAccess va) {
not lv.getType().hasName("va_list") and
va = lv.getAnAccess() and
noDefPath(lv, va) and
uses(va, lv))
uses(va, lv)
)
}

// Holds if gv is a potentially uninitialized global variable
/**
* Holds if `gv` is a potentially uninitialized global variable.
*/
predicate uninitialisedGlobal(GlobalVariable gv) {
exists(VariableAccess va |
not isAggregateType(gv.getUnderlyingType()) and
va = gv.getAnAccess() and
va.isRValue() and
not gv.hasInitializer() and
not gv.hasSpecifier("extern"))
not gv.hasSpecifier("extern")
)
}

from Element elt
Expand Down
53 changes: 29 additions & 24 deletions cpp/ql/src/Critical/SizeCheck.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,56 +11,61 @@
* external/cwe/cwe-131
* external/cwe/cwe-122
*/

import cpp

class Allocation extends FunctionCall
{
class Allocation extends FunctionCall {
Allocation() {
exists(string name |
this.getTarget().hasQualifiedName(name) and
(name = "malloc" or name = "calloc" or name = "realloc"))
(name = "malloc" or name = "calloc" or name = "realloc")
)
}

string getName() { result = this.getTarget().getQualifiedName() }

int getSize() {
(this.getName() = "malloc" and
this.getArgument(0).getValue().toInt() = result)
(
this.getName() = "malloc" and
this.getArgument(0).getValue().toInt() = result
)
or
(this.getName() = "realloc" and
this.getArgument(1).getValue().toInt() = result)
(
this.getName() = "realloc" and
this.getArgument(1).getValue().toInt() = result
)
or
(this.getName() = "calloc" and
result =
this.getArgument(0).getValue().toInt() *
this.getArgument(1).getValue().toInt())
(
this.getName() = "calloc" and
result = this.getArgument(0).getValue().toInt() * this.getArgument(1).getValue().toInt()
)
}
}

predicate baseType(Allocation alloc, Type base)
{
predicate baseType(Allocation alloc, Type base) {
exists(PointerType pointer |
pointer.getBaseType() = base and
(
exists(AssignExpr assign |
assign.getRValue() = alloc and assign.getLValue().getType() = pointer)
assign.getRValue() = alloc and assign.getLValue().getType() = pointer
)
or
exists(Variable v |
v.getInitializer().getExpr() = alloc and v.getType() = pointer)
exists(Variable v | v.getInitializer().getExpr() = alloc and v.getType() = pointer)
)
)
}

predicate decideOnSize(Type t, int size)
{
predicate decideOnSize(Type t, int size) {
// If the codebase has more than one type with the same name, it can have more than one size.
size = min(t.getSize())
}

from Allocation alloc, Type base, int basesize, int allocated
where baseType(alloc, base)
and allocated = alloc.getSize()
and decideOnSize(base, basesize)
and basesize > allocated
select alloc, "Type '" + base.getName() + "' is " + basesize.toString() +
" bytes, but only " + allocated.toString() + " bytes are allocated."
where
baseType(alloc, base) and
allocated = alloc.getSize() and
decideOnSize(base, basesize) and
basesize > allocated
select alloc,
"Type '" + base.getName() + "' is " + basesize.toString() + " bytes, but only " +
allocated.toString() + " bytes are allocated."
56 changes: 31 additions & 25 deletions cpp/ql/src/Critical/SizeCheck2.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,54 +11,60 @@
* external/cwe/cwe-131
* external/cwe/cwe-122
*/

import cpp

class Allocation extends FunctionCall
{
class Allocation extends FunctionCall {
Allocation() {
exists(string name |
this.getTarget().hasQualifiedName(name) and
(name = "malloc" or name = "calloc" or name = "realloc"))
(name = "malloc" or name = "calloc" or name = "realloc")
)
}

string getName() { result = this.getTarget().getQualifiedName() }

int getSize() {
(this.getName() = "malloc" and
this.getArgument(0).getValue().toInt() = result)
(
this.getName() = "malloc" and
this.getArgument(0).getValue().toInt() = result
)
or
(this.getName() = "realloc" and
this.getArgument(1).getValue().toInt() = result)
(
this.getName() = "realloc" and
this.getArgument(1).getValue().toInt() = result
)
or
(this.getName() = "calloc" and
result =
this.getArgument(0).getValue().toInt() *
this.getArgument(1).getValue().toInt())
(
this.getName() = "calloc" and
result = this.getArgument(0).getValue().toInt() * this.getArgument(1).getValue().toInt()
)
}
}

predicate baseType(Allocation alloc, Type base)
{
predicate baseType(Allocation alloc, Type base) {
exists(PointerType pointer |
pointer.getBaseType() = base and
(
exists(AssignExpr assign |
assign.getRValue() = alloc and assign.getLValue().getType() = pointer)
assign.getRValue() = alloc and assign.getLValue().getType() = pointer
)
or
exists(Variable v |
v.getInitializer().getExpr() = alloc and v.getType() = pointer)
exists(Variable v | v.getInitializer().getExpr() = alloc and v.getType() = pointer)
)
)
}

from Allocation alloc, Type base, int basesize, int allocated
where baseType(alloc, base)
and allocated = alloc.getSize()
where
baseType(alloc, base) and
allocated = alloc.getSize() and
// If the codebase has more than one type with the same name, check if any matches
and not exists(int size | base.getSize() = size |
size = 0
or (allocated / size) * size = allocated)
and basesize = min(base.getSize())
select alloc, "Allocated memory (" + allocated.toString() +
" bytes) is not a multiple of the size of '" +
base.getName() + "' (" + basesize.toString() + " bytes)."
not exists(int size | base.getSize() = size |
size = 0 or
(allocated / size) * size = allocated
) and
basesize = min(base.getSize())
select alloc,
"Allocated memory (" + allocated.toString() + " bytes) is not a multiple of the size of '" +
base.getName() + "' (" + basesize.toString() + " bytes)."
17 changes: 12 additions & 5 deletions cpp/ql/src/Header Cleanup/Cleanup-DuplicateIncludeGuard.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
* maintainability
* modularity
*/

import cpp
import semmle.code.cpp.headers.MultipleInclusion

Expand All @@ -20,9 +21,15 @@ import semmle.code.cpp.headers.MultipleInclusion
* However one case must be a correctIncludeGuard to prove that this macro really is intended
* to be an include guard.
*/

from HeaderFile hf, PreprocessorDirective ifndef, string macroName, int num
where hasIncludeGuard(hf, ifndef, _, macroName)
and exists(HeaderFile other | hasIncludeGuard(other, _, _, macroName) and hf.getShortName() != other.getShortName())
and num = strictcount(HeaderFile other | hasIncludeGuard(other, _, _, macroName))
and correctIncludeGuard(_, _, _, _, macroName)
select ifndef, "The macro name '" + macroName + "' of this include guard is used in " + num + " different header files."
where
hasIncludeGuard(hf, ifndef, _, macroName) and
exists(HeaderFile other |
hasIncludeGuard(other, _, _, macroName) and hf.getShortName() != other.getShortName()
) and
num = strictcount(HeaderFile other | hasIncludeGuard(other, _, _, macroName)) and
correctIncludeGuard(_, _, _, _, macroName)
select ifndef,
"The macro name '" + macroName + "' of this include guard is used in " + num +
" different header files."
Loading