Skip to content

Commit bb9c888

Browse files
MathiasVPMathiasVP
authored
Merge pull request github#3786 from geoffw0/bufferwritecleanup
C++: Clean up BufferWrite.qll
2 parents 66a6fe7 + 8d8e47d commit bb9c888

File tree

9 files changed

+176
-126
lines changed

9 files changed

+176
-126
lines changed

‎cpp/ql/src/Likely Bugs/Likely Typos/UsingStrcpyAsBoolean.ql

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,15 @@ import cpp
1515
import semmle.code.cpp.models.implementations.Strcpy
1616
import semmle.code.cpp.dataflow.DataFlow
1717

18+
/**
19+
* A string copy function that returns a string, rather than an error code (for
20+
* example, `strcpy` returns a string, whereas `strcpy_s` returns an error
21+
* code).
22+
*/
23+
class InterestingStrcpyFunction extends StrcpyFunction {
24+
InterestingStrcpyFunction() { getType().getUnspecifiedType() instanceof PointerType }
25+
}
26+
1827
predicate isBoolean(Expr e1) {
1928
exists(Type t1 |
2029
t1 = e1.getType() and
@@ -25,12 +34,12 @@ predicate isBoolean(Expr e1) {
2534
predicate isStringCopyCastedAsBoolean(FunctionCall func, Expr expr1, string msg) {
2635
DataFlow::localExprFlow(func, expr1) and
2736
isBoolean(expr1.getConversion*()) and
28-
func.getTarget() instanceof StrcpyFunction and
37+
func.getTarget() instanceof InterestingStrcpyFunction and
2938
msg = "Return value of " + func.getTarget().getName() + " used as a Boolean."
3039
}
3140

3241
predicate isStringCopyUsedInLogicalOperationOrCondition(FunctionCall func, Expr expr1, string msg) {
33-
func.getTarget() instanceof StrcpyFunction and
42+
func.getTarget() instanceof InterestingStrcpyFunction and
3443
(
3544
(
3645
// it is being used in an equality or logical operation

‎cpp/ql/src/semmle/code/cpp/models/implementations/Gets.qll

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,8 @@
1+
/**
2+
* Provides implementation classes modelling `gets` and various similar
3+
* functions. See `semmle.code.cpp.models.Models` for usage information.
4+
*/
5+
16
import semmle.code.cpp.models.interfaces.DataFlow
27
import semmle.code.cpp.models.interfaces.Taint
38
import semmle.code.cpp.models.interfaces.ArrayFunction

‎cpp/ql/src/semmle/code/cpp/models/implementations/Memcpy.qll

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,8 @@
1+
/**
2+
* Provides implementation classes modelling `memcpy` and various similar
3+
* functions. See `semmle.code.cpp.models.Models` for usage information.
4+
*/
5+
16
import semmle.code.cpp.Function
27
import semmle.code.cpp.models.interfaces.ArrayFunction
38
import semmle.code.cpp.models.interfaces.DataFlow

‎cpp/ql/src/semmle/code/cpp/models/implementations/Memset.qll

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,8 @@
1+
/**
2+
* Provides implementation classes modelling `memset` and various similar
3+
* functions. See `semmle.code.cpp.models.Models` for usage information.
4+
*/
5+
16
import semmle.code.cpp.Function
27
import semmle.code.cpp.models.interfaces.ArrayFunction
38
import semmle.code.cpp.models.interfaces.DataFlow

‎cpp/ql/src/semmle/code/cpp/models/implementations/Printf.qll

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -66,12 +66,25 @@ class Sprintf extends FormattingFunction {
6666
Sprintf() {
6767
this instanceof TopLevelFunction and
6868
(
69-
hasGlobalOrStdName("sprintf") or
70-
hasGlobalName("_sprintf_l") or
71-
hasGlobalName("__swprintf_l") or
72-
hasGlobalOrStdName("wsprintf") or
73-
hasGlobalName("g_strdup_printf") or
74-
hasGlobalName("g_sprintf") or
69+
// sprintf(dst, format, args...)
70+
hasGlobalOrStdName("sprintf")
71+
or
72+
// _sprintf_l(dst, format, locale, args...)
73+
hasGlobalName("_sprintf_l")
74+
or
75+
// __swprintf_l(dst, format, locale, args...)
76+
hasGlobalName("__swprintf_l")
77+
or
78+
// wsprintf(dst, format, args...)
79+
hasGlobalOrStdName("wsprintf")
80+
or
81+
// g_strdup_printf(format, ...)
82+
hasGlobalName("g_strdup_printf")
83+
or
84+
// g_sprintf(dst, format, ...)
85+
hasGlobalName("g_sprintf")
86+
or
87+
// __builtin___sprintf_chk(dst, flag, os, format, ...)
7588
hasGlobalName("__builtin___sprintf_chk")
7689
) and
7790
not exists(getDefinition().getFile().getRelativePath())

‎cpp/ql/src/semmle/code/cpp/models/implementations/Strcat.qll

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,21 @@ class StrcatFunction extends TaintFunction, DataFlowFunction, ArrayFunction, Sid
2424
)
2525
}
2626

27+
/**
28+
* Gets the index of the parameter that is the size of the copy (in characters).
29+
*/
30+
int getParamSize() { exists(getParameter(2)) and result = 2 }
31+
32+
/**
33+
* Gets the index of the parameter that is the source of the copy.
34+
*/
35+
int getParamSrc() { result = 1 }
36+
37+
/**
38+
* Gets the index of the parameter that is the destination to be appended to.
39+
*/
40+
int getParamDest() { result = 0 }
41+
2742
override predicate hasDataFlow(FunctionInput input, FunctionOutput output) {
2843
input.isParameter(0) and
2944
output.isReturnValue()

‎cpp/ql/src/semmle/code/cpp/models/implementations/Strcpy.qll

Lines changed: 93 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,8 @@
1+
/**
2+
* Provides implementation classes modelling `strcpy` and various similar
3+
* functions. See `semmle.code.cpp.models.Models` for usage information.
4+
*/
5+
16
import semmle.code.cpp.models.interfaces.ArrayFunction
27
import semmle.code.cpp.models.interfaces.DataFlow
38
import semmle.code.cpp.models.interfaces.Taint
@@ -8,70 +13,110 @@ import semmle.code.cpp.models.interfaces.SideEffect
813
*/
914
class StrcpyFunction extends ArrayFunction, DataFlowFunction, TaintFunction, SideEffectFunction {
1015
StrcpyFunction() {
11-
this.hasName("strcpy") or
12-
this.hasName("_mbscpy") or
13-
this.hasName("wcscpy") or
14-
this.hasName("strncpy") or
15-
this.hasName("_strncpy_l") or
16-
this.hasName("_mbsncpy") or
17-
this.hasName("_mbsncpy_l") or
18-
this.hasName("wcsncpy") or
19-
this.hasName("_wcsncpy_l")
16+
exists(string name | name = getName() |
17+
// strcpy(dst, src)
18+
name = "strcpy"
19+
or
20+
// wcscpy(dst, src)
21+
name = "wcscpy"
22+
or
23+
// _mbscpy(dst, src)
24+
name = "_mbscpy"
25+
or
26+
(
27+
name = "strcpy_s" or // strcpy_s(dst, max_amount, src)
28+
name = "wcscpy_s" or // wcscpy_s(dst, max_amount, src)
29+
name = "_mbscpy_s" // _mbscpy_s(dst, max_amount, src)
30+
) and
31+
// exclude the 2-parameter template versions
32+
// that find the size of a fixed size destination buffer.
33+
getNumberOfParameters() = 3
34+
or
35+
// strncpy(dst, src, max_amount)
36+
name = "strncpy"
37+
or
38+
// _strncpy_l(dst, src, max_amount, locale)
39+
name = "_strncpy_l"
40+
or
41+
// wcsncpy(dst, src, max_amount)
42+
name = "wcsncpy"
43+
or
44+
// _wcsncpy_l(dst, src, max_amount, locale)
45+
name = "_wcsncpy_l"
46+
or
47+
// _mbsncpy(dst, src, max_amount)
48+
name = "_mbsncpy"
49+
or
50+
// _mbsncpy_l(dst, src, max_amount, locale)
51+
name = "_mbsncpy_l"
52+
)
2053
}
2154

22-
override predicate hasArrayInput(int bufParam) { bufParam = 1 }
55+
/**
56+
* Holds if this is one of the `strcpy_s` variants.
57+
*/
58+
private predicate isSVariant() {
59+
exists(string name | name = getName() | name.suffix(name.length() - 2) = "_s")
60+
}
61+
62+
/**
63+
* Gets the index of the parameter that is the maximum size of the copy (in characters).
64+
*/
65+
int getParamSize() {
66+
if isSVariant()
67+
then result = 1
68+
else
69+
if exists(getName().indexOf("ncpy"))
70+
then result = 2
71+
else none()
72+
}
2373

24-
override predicate hasArrayOutput(int bufParam) { bufParam = 0 }
74+
/**
75+
* Gets the index of the parameter that is the source of the copy.
76+
*/
77+
int getParamSrc() { if isSVariant() then result = 2 else result = 1 }
2578

26-
override predicate hasArrayWithNullTerminator(int bufParam) { bufParam = 1 }
79+
/**
80+
* Gets the index of the parameter that is the destination of the copy.
81+
*/
82+
int getParamDest() { result = 0 }
83+
84+
override predicate hasArrayInput(int bufParam) { bufParam = getParamSrc() }
85+
86+
override predicate hasArrayOutput(int bufParam) { bufParam = getParamDest() }
87+
88+
override predicate hasArrayWithNullTerminator(int bufParam) { bufParam = getParamSrc() }
2789

2890
override predicate hasArrayWithVariableSize(int bufParam, int countParam) {
29-
(
30-
this.hasName("strncpy") or
31-
this.hasName("_strncpy_l") or
32-
this.hasName("_mbsncpy") or
33-
this.hasName("_mbsncpy_l") or
34-
this.hasName("wcsncpy") or
35-
this.hasName("_wcsncpy_l")
36-
) and
37-
bufParam = 0 and
38-
countParam = 2
91+
bufParam = getParamDest() and
92+
countParam = getParamSize()
3993
}
4094

4195
override predicate hasArrayWithUnknownSize(int bufParam) {
42-
(
43-
this.hasName("strcpy") or
44-
this.hasName("_mbscpy") or
45-
this.hasName("wcscpy")
46-
) and
47-
bufParam = 0
96+
not exists(getParamSize()) and
97+
bufParam = getParamDest()
4898
}
4999

50100
override predicate hasDataFlow(FunctionInput input, FunctionOutput output) {
51-
input.isParameterDeref(1) and
52-
output.isParameterDeref(0)
101+
not exists(getParamSize()) and
102+
input.isParameterDeref(getParamSrc()) and
103+
output.isParameterDeref(getParamDest())
53104
or
54-
input.isParameterDeref(1) and
105+
not exists(getParamSize()) and
106+
input.isParameterDeref(getParamSrc()) and
55107
output.isReturnValueDeref()
56108
or
57-
input.isParameter(0) and
109+
input.isParameter(getParamDest()) and
58110
output.isReturnValue()
59111
}
60112

61113
override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
114+
// these may do only a partial copy of the input buffer to the output
115+
// buffer
116+
exists(getParamSize()) and
117+
input.isParameter(getParamSrc()) and
62118
(
63-
// these may do only a partial copy of the input buffer to the output
64-
// buffer
65-
this.hasName("strncpy") or
66-
this.hasName("_strncpy_l") or
67-
this.hasName("_mbsncpy") or
68-
this.hasName("_mbsncpy_l") or
69-
this.hasName("wcsncpy") or
70-
this.hasName("_wcsncpy_l")
71-
) and
72-
input.isParameter(2) and
73-
(
74-
output.isParameterDeref(0) or
119+
output.isParameterDeref(getParamDest()) or
75120
output.isReturnValueDeref()
76121
)
77122
}
@@ -81,17 +126,18 @@ class StrcpyFunction extends ArrayFunction, DataFlowFunction, TaintFunction, Sid
81126
override predicate hasOnlySpecificWriteSideEffects() { any() }
82127

83128
override predicate hasSpecificWriteSideEffect(ParameterIndex i, boolean buffer, boolean mustWrite) {
84-
i = 0 and
129+
i = getParamDest() and
85130
buffer = true and
86131
mustWrite = false
87132
}
88133

89134
override predicate hasSpecificReadSideEffect(ParameterIndex i, boolean buffer) {
90-
i = 1 and
135+
i = getParamSrc() and
91136
buffer = true
92137
}
93138

94139
override ParameterIndex getParameterSizeIndex(ParameterIndex i) {
95-
hasArrayWithVariableSize(i, result)
140+
i = getParamDest() and
141+
result = getParamSize()
96142
}
97143
}

‎cpp/ql/src/semmle/code/cpp/models/implementations/Strdup.qll

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,8 @@
1+
/**
2+
* Provides implementation classes modelling `strdup` and various similar
3+
* functions. See `semmle.code.cpp.models.Models` for usage information.
4+
*/
5+
16
import semmle.code.cpp.models.interfaces.Allocation
27
import semmle.code.cpp.models.interfaces.ArrayFunction
38
import semmle.code.cpp.models.interfaces.DataFlow

0 commit comments

Comments
 (0)