Skip to content

Commit a8d5357

Browse files
bdrodesbdrodes
committed
Added an additional test case and fixed a false positive. Also noted a new false positive scenario currently unaccounted for in a new test case.
1 parent 2e613ac commit a8d5357

3 files changed

Lines changed: 64 additions & 11 deletions

File tree

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

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -715,7 +715,12 @@ where
715715
not exists(DataFlow::Node dayOrMonthValSrc, DataFlow::Node dayOrMonthValSink, Assignment a |
716716
CandidateConstantToDayOrMonthAssignmentFlow::flow(dayOrMonthValSrc, dayOrMonthValSink) and
717717
a.getRValue() = dayOrMonthValSink.asExpr() and
718-
dayOrMonthValSrc.getBasicBlock() = src.getNode().getBasicBlock() and
718+
(
719+
// The source of the day is set in the same block as the source for the year
720+
// or the source for the day is set in the same block as the sink for the year
721+
dayOrMonthValSrc.getBasicBlock() = src.getNode().getBasicBlock() or
722+
dayOrMonthValSrc.getBasicBlock() = sink.getNode().getBasicBlock()
723+
) and
719724
dayOrMonthValSink.getBasicBlock() = sink.getNode().getBasicBlock() and
720725
(
721726
a.getLValue() instanceof MonthFieldAccess and

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

Lines changed: 26 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,11 @@
1616
| test.cpp:1245:2:1245:28 | ... = ... | test.cpp:1245:16:1245:28 | ... + ... | test.cpp:1245:2:1245:28 | ... = ... | Year field has been modified, but no appropriate check for LeapYear was found. |
1717
| test.cpp:1259:2:1259:28 | ... = ... | test.cpp:1259:16:1259:28 | ... + ... | test.cpp:1259:2:1259:28 | ... = ... | Year field has been modified, but no appropriate check for LeapYear was found. |
1818
| test.cpp:1293:2:1293:17 | ... = ... | test.cpp:1384:12:1384:17 | ... + ... | test.cpp:1293:2:1293:17 | ... = ... | Year field has been modified, but no appropriate check for LeapYear was found. |
19-
| test.cpp:1435:2:1435:15 | ... = ... | test.cpp:1432:2:1432:10 | ... += ... | test.cpp:1435:2:1435:15 | ... = ... | Year field has been modified, but no appropriate check for LeapYear was found. |
20-
| test.cpp:1465:2:1465:22 | ... += ... | test.cpp:1465:2:1465:22 | ... += ... | test.cpp:1465:2:1465:22 | ... += ... | Year field has been modified, but no appropriate check for LeapYear was found. |
21-
| test.cpp:1473:2:1473:22 | ... += ... | test.cpp:1473:2:1473:22 | ... += ... | test.cpp:1473:2:1473:22 | ... += ... | Year field has been modified, but no appropriate check for LeapYear was found. |
19+
| test.cpp:1293:2:1293:17 | ... = ... | test.cpp:1398:9:1398:16 | ... + ... | test.cpp:1293:2:1293:17 | ... = ... | Year field has been modified, but no appropriate check for LeapYear was found. |
20+
| test.cpp:1293:2:1293:17 | ... = ... | test.cpp:1410:9:1410:16 | ... + ... | test.cpp:1293:2:1293:17 | ... = ... | Year field has been modified, but no appropriate check for LeapYear was found. |
21+
| test.cpp:1467:2:1467:15 | ... = ... | test.cpp:1464:2:1464:10 | ... += ... | test.cpp:1467:2:1467:15 | ... = ... | Year field has been modified, but no appropriate check for LeapYear was found. |
22+
| test.cpp:1497:2:1497:22 | ... += ... | test.cpp:1497:2:1497:22 | ... += ... | test.cpp:1497:2:1497:22 | ... += ... | Year field has been modified, but no appropriate check for LeapYear was found. |
23+
| test.cpp:1505:2:1505:22 | ... += ... | test.cpp:1505:2:1505:22 | ... += ... | test.cpp:1505:2:1505:22 | ... += ... | Year field has been modified, but no appropriate check for LeapYear was found. |
2224
edges
2325
| test.cpp:813:21:813:40 | ... + ... | test.cpp:813:2:813:40 | ... = ... | provenance | |
2426
| test.cpp:818:13:818:24 | ... + ... | test.cpp:818:2:818:24 | ... = ... | provenance | |
@@ -53,7 +55,14 @@ edges
5355
| test.cpp:1372:15:1372:22 | ... + ... | test.cpp:1372:3:1372:22 | ... = ... | provenance | |
5456
| test.cpp:1377:12:1377:17 | ... + ... | test.cpp:1290:20:1290:23 | year | provenance | |
5557
| test.cpp:1384:12:1384:17 | ... + ... | test.cpp:1290:20:1290:23 | year | provenance | |
56-
| test.cpp:1432:2:1432:10 | ... += ... | test.cpp:1435:2:1435:15 | ... = ... | provenance | |
58+
| test.cpp:1398:2:1398:16 | ... = ... | test.cpp:1402:3:1402:18 | ... = ... | provenance | |
59+
| test.cpp:1398:2:1398:16 | ... = ... | test.cpp:1407:12:1407:15 | year | provenance | |
60+
| test.cpp:1398:9:1398:16 | ... + ... | test.cpp:1398:2:1398:16 | ... = ... | provenance | |
61+
| test.cpp:1407:12:1407:15 | year | test.cpp:1290:20:1290:23 | year | provenance | |
62+
| test.cpp:1410:2:1410:16 | ... = ... | test.cpp:1416:12:1416:15 | year | provenance | |
63+
| test.cpp:1410:9:1410:16 | ... + ... | test.cpp:1410:2:1410:16 | ... = ... | provenance | |
64+
| test.cpp:1416:12:1416:15 | year | test.cpp:1290:20:1290:23 | year | provenance | |
65+
| test.cpp:1464:2:1464:10 | ... += ... | test.cpp:1467:2:1467:15 | ... = ... | provenance | |
5766
nodes
5867
| test.cpp:422:2:422:14 | ... += ... | semmle.label | ... += ... |
5968
| test.cpp:440:2:440:11 | ... ++ | semmle.label | ... ++ |
@@ -130,10 +139,17 @@ nodes
130139
| test.cpp:1372:15:1372:22 | ... + ... | semmle.label | ... + ... |
131140
| test.cpp:1377:12:1377:17 | ... + ... | semmle.label | ... + ... |
132141
| test.cpp:1384:12:1384:17 | ... + ... | semmle.label | ... + ... |
133-
| test.cpp:1432:2:1432:10 | ... += ... | semmle.label | ... += ... |
134-
| test.cpp:1435:2:1435:15 | ... = ... | semmle.label | ... = ... |
135-
| test.cpp:1465:2:1465:22 | ... += ... | semmle.label | ... += ... |
136-
| test.cpp:1473:2:1473:22 | ... += ... | semmle.label | ... += ... |
137-
| test.cpp:1519:2:1519:22 | ... += ... | semmle.label | ... += ... |
138-
| test.cpp:1530:2:1530:22 | ... += ... | semmle.label | ... += ... |
142+
| test.cpp:1398:2:1398:16 | ... = ... | semmle.label | ... = ... |
143+
| test.cpp:1398:9:1398:16 | ... + ... | semmle.label | ... + ... |
144+
| test.cpp:1402:3:1402:18 | ... = ... | semmle.label | ... = ... |
145+
| test.cpp:1407:12:1407:15 | year | semmle.label | year |
146+
| test.cpp:1410:2:1410:16 | ... = ... | semmle.label | ... = ... |
147+
| test.cpp:1410:9:1410:16 | ... + ... | semmle.label | ... + ... |
148+
| test.cpp:1416:12:1416:15 | year | semmle.label | year |
149+
| test.cpp:1464:2:1464:10 | ... += ... | semmle.label | ... += ... |
150+
| test.cpp:1467:2:1467:15 | ... = ... | semmle.label | ... = ... |
151+
| test.cpp:1497:2:1497:22 | ... += ... | semmle.label | ... += ... |
152+
| test.cpp:1505:2:1505:22 | ... += ... | semmle.label | ... += ... |
153+
| test.cpp:1551:2:1551:22 | ... += ... | semmle.label | ... += ... |
154+
| test.cpp:1562:2:1562:22 | ... += ... | semmle.label | ... += ... |
139155
subpaths

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

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1385,6 +1385,38 @@ void constant_day_on_year_modification1(WORD year, WORD offset, WORD month){
13851385
}
13861386
}
13871387

1388+
void constant_day_on_year_modification2(WORD year, WORD month){
1389+
SYSTEMTIME tmp;
1390+
1391+
// FLASE POSITIVE SOURCE:
1392+
// flowing into set_time, the set time does pass a constant day
1393+
// but the source here and the source of that constant month don't align
1394+
// Current heuristics require the source of the constant day align with the
1395+
// source and/or the sink of the year modification.
1396+
// We could potentially improve this by checking the paths of both the year and day
1397+
// flows, but this may be more complex than is warranted for now.
1398+
year = year + 1; // $ SPURIOUS: Source
1399+
1400+
if(month++ > 12){
1401+
tmp.wDay = 1;
1402+
tmp.wYear = year;// OK since the year is incremented with a known non-leap year day
1403+
}
1404+
1405+
if(month++ > 12){
1406+
1407+
set_time(year, month, 1);// OK since the year is incremented with a known non-leap year day
1408+
}
1409+
1410+
year = year + 1; // $ Source
1411+
1412+
if(month++ > 12){
1413+
1414+
// BAD, year incremented, month unknown in block, and date is set to 31
1415+
// which is dangerous.
1416+
set_time(year, month, 31);
1417+
}
1418+
}
1419+
13881420

13891421
void modification_after_conversion1(tm timeinfo){
13901422
// convert a tm year into a civil year, then modify after conversion

0 commit comments

Comments
 (0)