Skip to content

Commit aba87c5

Browse files
ropwareJBropwareJB
committed
Massaging cpp leap year AP1
1 parent 9d5a797 commit aba87c5

2 files changed

Lines changed: 136 additions & 56 deletions

File tree

‎cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql‎

Lines changed: 85 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -12,55 +12,6 @@
1212
import cpp
1313
import LeapYear
1414

15-
/**
16-
* Holds if there is no known leap-year verification for the given `YearWriteOp`.
17-
* Binds the `var` argument to the qualifier of the `ywo` argument.
18-
*/
19-
bindingset[ywo]
20-
predicate isYearModifedWithoutExplicitLeapYearCheck(Variable var, YearWriteOp ywo) {
21-
exists(VariableAccess va, YearFieldAccess yfa |
22-
yfa = ywo.getYearAccess() and
23-
yfa.getQualifier() = va and
24-
var.getAnAccess() = va and
25-
// Avoid false positives
26-
not (
27-
// If there is a local check for leap year after the modification
28-
exists(LeapYearFieldAccess yfacheck |
29-
yfacheck.getQualifier() = var.getAnAccess() and
30-
yfacheck.isUsedInCorrectLeapYearCheck() and
31-
yfacheck.getBasicBlock() = yfa.getBasicBlock().getASuccessor*()
32-
)
33-
or
34-
// If there is a data flow from the variable that was modified to a function that seems to check for leap year
35-
exists(VariableAccess source, ChecksForLeapYearFunctionCall fc |
36-
source = var.getAnAccess() and
37-
LeapYearCheckFlow::flow(DataFlow::exprNode(source), DataFlow::exprNode(fc.getAnArgument()))
38-
)
39-
or
40-
// If there is a data flow from the field that was modified to a function that seems to check for leap year
41-
exists(VariableAccess vacheck, YearFieldAccess yfacheck, ChecksForLeapYearFunctionCall fc |
42-
vacheck = var.getAnAccess() and
43-
yfacheck.getQualifier() = vacheck and
44-
LeapYearCheckFlow::flow(DataFlow::exprNode(yfacheck), DataFlow::exprNode(fc.getAnArgument()))
45-
)
46-
or
47-
// If there is a successor or predecessor that sets the month or day to a fixed value
48-
exists(FieldAccess mfa, AssignExpr ae, int val |
49-
mfa instanceof MonthFieldAccess or mfa instanceof DayFieldAccess
50-
|
51-
mfa.getQualifier() = var.getAnAccess() and
52-
mfa.isModified() and
53-
(
54-
mfa.getBasicBlock() = yfa.getBasicBlock().getASuccessor*() or
55-
yfa.getBasicBlock() = mfa.getBasicBlock().getASuccessor+()
56-
) and
57-
ae = mfa.getEnclosingElement() and
58-
ae.getAnOperand().getValue().toInt() = val
59-
)
60-
)
61-
)
62-
}
63-
6415
/**
6516
* The set of all write operations to the Year field of a date struct.
6617
*/
@@ -70,6 +21,61 @@ abstract class YearWriteOp extends Operation {
7021

7122
/** Get the expression which represents the new value. */
7223
abstract Expr getMutationExpr();
24+
25+
/**
26+
* Checks if this Operation is a simple normalization, converting between formats.
27+
*/
28+
predicate isNormalizationOperation(){
29+
isNormalizationOperation(this.getMutationExpr())
30+
}
31+
32+
/**
33+
* Holds if there is no known leap-year verification for the `YearWriteOp`.
34+
* Binds the `var` argument to the qualifier of the `ywo` argument.
35+
*/
36+
predicate isYearModifedWithoutExplicitLeapYearCheck(Variable var) {
37+
exists(VariableAccess va, YearFieldAccess yfa |
38+
yfa = this.getYearAccess() and
39+
yfa.getQualifier() = va and
40+
var.getAnAccess() = va and
41+
// Avoid false positives
42+
not (
43+
// If there is a local check for leap year after the modification
44+
exists(LeapYearFieldAccess yfacheck |
45+
yfacheck.getQualifier() = var.getAnAccess() and
46+
yfacheck.isUsedInCorrectLeapYearCheck() and
47+
yfacheck.getBasicBlock() = yfa.getBasicBlock().getASuccessor*()
48+
)
49+
or
50+
// If there is a data flow from the variable that was modified to a function that seems to check for leap year
51+
exists(VariableAccess source, ChecksForLeapYearFunctionCall fc |
52+
source = var.getAnAccess() and
53+
LeapYearCheckFlow::flow(DataFlow::exprNode(source), DataFlow::exprNode(fc.getAnArgument()))
54+
)
55+
or
56+
// If there is a data flow from the field that was modified to a function that seems to check for leap year
57+
exists(VariableAccess vacheck, YearFieldAccess yfacheck, ChecksForLeapYearFunctionCall fc |
58+
vacheck = var.getAnAccess() and
59+
yfacheck.getQualifier() = vacheck and
60+
LeapYearCheckFlow::flow(DataFlow::exprNode(yfacheck), DataFlow::exprNode(fc.getAnArgument()))
61+
)
62+
or
63+
// If there is a successor or predecessor that sets the month or day to a fixed value
64+
exists(FieldAccess mfa, AssignExpr ae, int val |
65+
mfa instanceof MonthFieldAccess or mfa instanceof DayFieldAccess
66+
|
67+
mfa.getQualifier() = var.getAnAccess() and
68+
mfa.isModified() and
69+
(
70+
mfa.getBasicBlock() = yfa.getBasicBlock().getASuccessor*() or
71+
yfa.getBasicBlock() = mfa.getBasicBlock().getASuccessor+()
72+
) and
73+
ae = mfa.getEnclosingElement() and
74+
ae.getAnOperand().getValue().toInt() = val
75+
)
76+
)
77+
)
78+
}
7379
}
7480

7581
/**
@@ -125,14 +131,37 @@ module OperationToYearAssignmentConfig implements DataFlow::ConfigSig {
125131

126132
module OperationToYearAssignmentFlow = DataFlow::Global<OperationToYearAssignmentConfig>;
127133

128-
from Variable var, YearWriteOp ywo, Expr mutationExpr
134+
/**
135+
* A Call to some known Time conversion function, which maps between time structs or scalars.
136+
*/
137+
class TimeConversionCall extends Call{
138+
TimeConversionCall(){
139+
this = any(TimeConversionFunction f).getACallToThisFunction()
140+
}
141+
142+
bindingset[var]
143+
predicate converts(Variable var){
144+
this.getAnArgument().getAChild*() = var.getAnAccess()
145+
}
146+
}
147+
148+
/**
149+
* A YearWriteOp that is non-mutating (AddressOfExpr *could* mutate but doesnt)
150+
*/
151+
class NonMutatingYearWriteOp extends Operation instanceof YearWriteOp{
152+
NonMutatingYearWriteOp(){
153+
this instanceof AddressOfExpr
154+
}
155+
}
156+
157+
from Variable var, YearWriteOp ywo
129158
where
130-
mutationExpr = ywo.getMutationExpr() and
131-
isYearModifedWithoutExplicitLeapYearCheck(var, ywo) and
132-
not isNormalizationOperation(mutationExpr) and
133-
not ywo instanceof AddressOfExpr and
134-
not exists(Call c, TimeConversionFunction f | f.getACallToThisFunction() = c |
135-
c.getAnArgument().getAChild*() = var.getAnAccess() and
159+
not ywo.isNormalizationOperation() and
160+
not ywo instanceof NonMutatingYearWriteOp and
161+
ywo.isYearModifedWithoutExplicitLeapYearCheck(var) and
162+
// If there is a Time conversion after, which utilizes the variable - could lead to false negatives maybe?
163+
not exists(TimeConversionCall c |
164+
c.converts(var) and
136165
ywo.getASuccessor*() = c
137166
)
138167
select ywo,

‎cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/test.cpp‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,8 @@ typedef struct _TIME_DYNAMIC_ZONE_INFORMATION {
4646
BOOLEAN DynamicDaylightTimeDisabled;
4747
} DYNAMIC_TIME_ZONE_INFORMATION, *PDYNAMIC_TIME_ZONE_INFORMATION;
4848

49+
typedef const wchar_t* LPCWSTR;
50+
4951
struct tm
5052
{
5153
int tm_sec; // seconds after the minute - [0, 60] including leap second
@@ -111,6 +113,10 @@ TzSpecificLocalTimeToSystemTimeEx(
111113
LPSYSTEMTIME lpUniversalTime
112114
);
113115

116+
int _wtoi(
117+
const wchar_t *str
118+
);
119+
114120
void GetSystemTime(
115121
LPSYSTEMTIME lpSystemTime
116122
);
@@ -1004,3 +1010,48 @@ bool tp_intermediaryVar(struct timespec now, struct logtime &timestamp_remote)
10041010
tm.tm_mday = tm.tm_mon == 2 && tm.tm_mday == 29 && !isLeapYear ? 28 : tm.tm_mday;
10051011
return tm;
10061012
}
1013+
1014+
/**
1015+
* Negative Case - Anti-pattern 1: [year ±n, month, day]
1016+
* Modification of SYSTEMTIME struct by copying from another struct, but no arithmetic is performed.
1017+
*/
1018+
bool
1019+
FMAPITimeToSysTimeW(LPCWSTR wszTime, SYSTEMTIME *psystime)
1020+
{
1021+
// if (!wszTime || SafeIsBadReadPtr(wszTime, 1) || lstrlenW(wszTime) != cchMAPITime)
1022+
// return false;
1023+
// AssertTag(!SafeIsBadWritePtr(psystime, sizeof(SYSTEMTIME)), 0x0004289a /* tag_abc80 */);
1024+
// memset(psystime, 0, sizeof(SYSTEMTIME));
1025+
1026+
psystime->wYear = (WORD)_wtoi(wszTime);
1027+
psystime->wMonth = (WORD)_wtoi(wszTime+5);
1028+
psystime->wDay = (WORD)_wtoi(wszTime+8);
1029+
psystime->wHour = (WORD)_wtoi(wszTime+11);
1030+
psystime->wMinute = (WORD)_wtoi(wszTime+14);
1031+
return true;
1032+
}
1033+
1034+
/**
1035+
* Negative Case - Anti-pattern 1: [year ±n, month, day]
1036+
* Modification of SYSTEMTIME struct by copying from another struct, but no arithmetic is performed.
1037+
*/
1038+
bool
1039+
ATime::HrGetSysTime(
1040+
SYSTEMTIME *pst
1041+
) const
1042+
{
1043+
// if (!FValidSysTime())
1044+
// {
1045+
// TrapSzTag("ATime cannot be converted to SYSTEMTIME", 0x1e14f5c3 /* tag_4fpxd */);
1046+
// CORgTag(E_FAIL, 0x6c373230 /* tag_l720 */);
1047+
// }
1048+
1049+
// pst->wYear = static_cast<WORD>(m_lYear);
1050+
// pst->wMonth = static_cast<WORD>(m_lMonth);
1051+
// //pst->wDayOfWeek = ???;
1052+
// pst->wDay = static_cast<WORD>(m_lDay);
1053+
// pst->wHour = static_cast<WORD>(m_lHour);
1054+
// pst->wMinute = static_cast<WORD>(m_lMinute);
1055+
// pst->wSecond = static_cast<WORD>(m_lSecond);
1056+
// pst->wMilliseconds = 0;
1057+
}

0 commit comments

Comments
 (0)