C++: Refactor of UncheckedLeapYearAfterModification#21292
C++: Refactor of UncheckedLeapYearAfterModification#21292bdrodes wants to merge 8 commits intogithub:mainfrom
Conversation
…cks (UncheckedLeapYearAfterYearModification). Switch to using 'postprocess' for unit tests.
…ion. Includes new logic for detecting leap year checks, new forms of leap year checks detected, and various heuristics to remove false postives. Move TimeConversionFunction into LeapYear.qll and refactored to separate conversion functions that are expected to be checked for failure from those that auto correct leap year dates if feb 29 is provided on a non-leap year. Increas the set of known TimeConversionFunctions.
…e negative remains.
…auto correct for leap year should be considered.
There was a problem hiding this comment.
Pull request overview
Refactors the UncheckedLeapYearAfterYearModification CodeQL query to significantly reduce false positives by introducing more precise flow/guard modeling and expanded heuristics around conversions and “ignorable” operations.
Changes:
- Reworked
UncheckedLeapYearAfterYearModification.qlinto a path-problem with new taint/dataflow-based modeling and multiple false-positive suppression heuristics. - Centralized/expanded time conversion API modeling in
LeapYear.qlland updated related query/tests/expected outputs accordingly. - Extended date/time type support (e.g.,
TIME_FIELDS) and added release change notes.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/test.cpp | Adds many new positive/negative/edge test scenarios for the refactored query. |
| cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/UncheckedReturnValueForTimeFunctions.expected | Updates expected results to match new line numbers/coverage. |
| cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/UncheckedLeapYearAfterYearModification.qlref | Switches to inline-expectations postprocessing. |
| cpp/ql/test/query-tests/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification/UncheckedLeapYearAfterYearModification.expected | Updates expected results to reflect path-problem output and new test suite. |
| cpp/ql/src/Likely Bugs/Leap Year/UncheckedReturnValueForTimeFunctions.ql | Refines conversion-return checking to exclude auto-leap-year-correcting APIs. |
| cpp/ql/src/Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql | Full refactor: new flow configurations, guard recognition, and suppression heuristics. |
| cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll | Moves/expands TimeConversionFunction modeling and adds auto-correcting classification. |
| cpp/ql/lib/semmle/code/cpp/commons/DateTime.qll | Expands recognized unpacked time/date types and adds field-access classes for TIME_FIELDS. |
| cpp/ql/lib/change-notes/2026-02-06-UncheckedLeapYearAfterModification_Refactor | Adds changelog entry documenting the refactor and alert reduction. |
| timestamp_remote.tm.tm_year = year; | ||
| if (mktime(×tamp_remote.tm) < t_now + 7 * 24 * 60 * 60) | ||
| break; | ||
| } |
There was a problem hiding this comment.
The function returns bool but has no return statement, which is undefined behavior if execution reaches the end. Return an appropriate value (for example return timestamp_found;) and ensure all paths return consistently.
| } | |
| } | |
| return timestamp_found; |
| // pst->wHour = static_cast<WORD>(m_lHour); | ||
| // pst->wMinute = static_cast<WORD>(m_lMinute); | ||
| // pst->wSecond = static_cast<WORD>(m_lSecond); | ||
| // pst->wMilliseconds = 0; |
There was a problem hiding this comment.
ATime_HrGetSysTime is declared to return bool but never returns a value. Either change the return type to void (if appropriate for the test) or return true/false to match the intended semantics.
| // pst->wMilliseconds = 0; | |
| // pst->wMilliseconds = 0; | |
| return true; |
| tm.tm_year=(st.wYear>=1900?st.wYear-1900:0); | ||
|
|
||
| // Check for leap year, and adjust the date accordingly | ||
| isLeapYear = tm.tm_year % 4 == 0 && (tm.tm_year % 100 != 0 || tm.tm_year % 400 == 0); | ||
| tm.tm_mday = tm.tm_mon == 2 && tm.tm_mday == 29 && !isLeapYear ? 28 : tm.tm_mday; |
There was a problem hiding this comment.
The leap-year calculation is incorrect because tm.tm_year is “years since 1900”, but the % 100 and % 400 rules must be applied to the civil year (tm.tm_year + 1900). This will misclassify years like 2000 (tm_year=100) as non-leap. Compute the check using the civil year.
| exists(MonthEqualityCheckGuard monthGuard | | ||
| monthGuard.controls(e.getBasicBlock(), true) and | ||
| not monthGuard.getExprCompared().getValueText() = "2" |
There was a problem hiding this comment.
Using getValueText() = "2" is brittle for numeric comparisons (formatting like 02, macro-expanded literals, or different textual representations can cause misclassification). Prefer comparing the numeric value (for example via getValue().toInt() = 2) and restrict it to integer literals/constants where appropriate.
| exists(MonthEqualityCheckGuard monthGuard | | |
| monthGuard.controls(e.getBasicBlock(), true) and | |
| not monthGuard.getExprCompared().getValueText() = "2" | |
| exists(MonthEqualityCheckGuard monthGuard, IntegerLiteral monthLit | | |
| monthGuard.controls(e.getBasicBlock(), true) and | |
| monthLit = monthGuard.getExprCompared() and | |
| not monthLit.getValue().toInt() = 2 |
| class IgnorableExpr10MulipleComponent extends IgnorableOperation { | ||
| IgnorableExpr10MulipleComponent() { | ||
| this.(Operation).getAnOperand().getValue().toInt() in [10, 100, 1000, 10000] | ||
| or | ||
| exists(AssignOperation a | a.getRValue() = this | | ||
| a.getRValue().getValue().toInt() in [10, 100, 1000, 10000] | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
The class name IgnorableExpr10MulipleComponent appears to have a typo (“Muliple”). Renaming it to IgnorableExpr10MultipleComponent would improve readability and reduce confusion when referenced elsewhere.
|
|
||
|
|
||
| if(month++ > 12){ | ||
| // some hueristics to detect a false positive here rely on variable names |
There was a problem hiding this comment.
Correct typo: “hueristics” → “heuristics”.
| // some hueristics to detect a false positive here rely on variable names | |
| // some heuristics to detect a false positive here rely on variable names |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| import cpp | ||
| import LeapYear | ||
| import semmle.code.cpp.controlflow.IRGuards | ||
| import semmle.code.cpp.ir.IR |
Check warning
Code scanning / CodeQL
Redundant import Warning
| import LeapYear | ||
| import semmle.code.cpp.controlflow.IRGuards | ||
| import semmle.code.cpp.ir.IR | ||
| import semmle.code.cpp.commons.DateTime |
Check warning
Code scanning / CodeQL
Redundant import Warning
| /** | ||
| * Anything involving an operation with 10, 100, 1000, 10000 is often a sign of conversion | ||
| * or atoi. | ||
| */ |
Check warning
Code scanning / CodeQL
Class QLDoc style Warning
| /** | ||
| * Anything involving a sub expression with char literal 48, ignore as a likely string conversion | ||
| * e.g., X - '0' | ||
| */ |
Check warning
Code scanning / CodeQL
Class QLDoc style Warning
| /** | ||
| * Some constants indicate conversion that are ignorable, e.g., | ||
| * julian to gregorian conversion or conversions from linux time structs | ||
| * that start at 1900, etc. | ||
| */ |
Check warning
Code scanning / CodeQL
Class QLDoc style Warning
| /** | ||
| * An expression that is a candidate source for an dataflow configuration for an Operation that could flow to a Year field. | ||
| */ |
Check warning
Code scanning / CodeQL
Predicate QLDoc style Warning
| // This is assuming a user would have done this all on one line though. | ||
| // setting a variable for the conversion and passing that separately would be more difficult to track | ||
| // considering this approach good enough for current observed false positives | ||
| exists(Call c, Expr arg | |
Check warning
Code scanning / CodeQL
Omittable 'exists' variable Warning
| LogicalAndExpr andExpr, LogicalOrExpr orExpr, GuardCondition div4Check, | ||
| GuardCondition div100Check, GuardCondition div400Check, GuardValue gv | ||
| | | ||
| // Cannonical case: |
Check warning
Code scanning / CodeQL
Misspelling Warning
| /** | ||
| * The variable access that is used in all 3 components of the leap year check | ||
| * e.g., see getYearSinkDiv4/100/400.. | ||
| * If a field access is used, the qualifier and the field access are both returned | ||
| * in checked condition. | ||
| * NOTE: if the year is not checked using the same access in all 3 components, no result is returned. | ||
| * The typical case observed is a consistent variable access is used. If not, this may indicate a bug. | ||
| * We could check more accurately with a dataflow analysis, but this is likely sufficient for now. | ||
| */ |
Check warning
Code scanning / CodeQL
Predicate QLDoc style Warning
| /** | ||
| * The value that the assignment resolves to doesn't represent February, | ||
| * and/or if it represents a day, is a 'safe' day (meaning the 27th or prior). | ||
| */ |
Check warning
Code scanning / CodeQL
Predicate QLDoc style Warning
1 participant
This PR addresses excessive false positive alerting from
Likely Bugs/Leap Year/UncheckedLeapYearAfterYearModification.ql. In separate in-depth auditing, the number of alerts drops from 40,000 down to 2,000 with these changes, with a much higher rate of true positives, though still too high to be considered more than medium precision (~25% false positive rate remaining).This PR is a complete refactor of the original query to address the false positives observed on production code. Some of the lessons learned here could be extrapolated into the LeapYear.qll library, but leaving changes like this for future PRs.