C++: Refactor of UncheckedLeapYearAfterModification #21292

bdrodes · 2026-02-06T21:23:18Z

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.

…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.

Copilot

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.ql into a path-problem with new taint/dataflow-based modeling and multiple false-positive suppression heuristics.
Centralized/expanded time conversion API modeling in LeapYear.qll and 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.

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

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

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

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

geoffw0

Initial review - lots of small comments, though little in particular I have strong feelings about. I'm happy to see this query get an overhaul, I'm very happy to see lots of test cases, though there is more query code than I'd like from a readability perspective. It would be nice to have some kind of diagram of how all the flows to fit together to aid understanding.

If you make whichever improvements you feel are most valuable, then I expect to approve this. I do also want to briefly check:

performance
real world (MRVA) result changes
CI

geoffw0 · 2026-02-11T15:37:27Z

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

-	// BUG - UncheckedLeapYearAfterYearModification
-	st.wYear++;
+	// Safe, checked interprocedurally through Correct_filetime_conversion_check
+	st.wYear++; 


How does Correct_filetime_conversion_check correct st?

geoffw0 · 2026-02-11T15:39:58Z

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

+	timeinfo.tm_year++; 

-	// Usage of potentially invalid date
+	// mkgmtime result checked in nested call here, assume leap year conversion is potentially handled


Same question here, how does CorrectUsageOf_mkgmtime correct the problem?

geoffw0 · 2026-02-11T15:45:05Z

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

+			pst->wMonth = 1;
+			pst->wYear++;
+		}
+	}


This function looks bad to me, though I suppose the issue is incrementing the month without a days-in-the-month check - not incrementing the year without a leap-year check. I guess that's the responsibility of a different query (that I think doesn't exist yet).

geoffw0 · 2026-02-11T17:22:15Z

cpp/ql/src/Likely Bugs/Leap Year/LeapYear.qll

  TaintTracking::Global<PossibleYearArithmeticOperationCheckConfig>;
+
+/**
+ * This list of APIs should check for the return value to detect problems during the conversion.


Suggested change

* This list of APIs should check for the return value to detect problems during the conversion.

* Time conversion functions with a return value that should be checked to detect problems during the conversion.

geoffw0 · 2026-02-11T17:38:05Z

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

+ */
+class IgnorableFunction extends Function {
+  IgnorableFunction() {
+    this instanceof TimeConversionFunction


I think I'd like a comment explaining the presence of TimeConversionFunction here. I think the idea is that these functions will return an error which, assuming its properly handled, allows the leap year problem to be detected and corrected (or else cause the whole operation to fail without doing something unexpected with the invalid date).

geoffw0 · 2026-02-11T18:45:43Z

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

+    // in cases of x.year = x and the x is checked, but the year x.year isn't directly
+    // flow from a year assignment node to an RHS if it is an assignment


I didn't follow this comment, can you rephrase?

geoffw0 · 2026-02-11T18:51:03Z

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

+ * auto convert to a sanity check guard of the result for error conditions.
+ */
+module YearAssignmentToCheckedTimeConversionConfig implements DataFlow::StateConfigSig {
+  class FlowState = boolean;


It would help readability if we briefly explain what FlowState is tracking here. My understanding is its used to ensure flow goes through the steps in order from source -> TimeConversionFunction -> check condition, unless we reach a TimeConversionFunction with .isAutoLeapYearCorrecting() in which case that is a simple unconditional sink.

geoffw0 · 2026-02-11T18:55:35Z

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

+    or
+    // flow from a year access qualifier to a year field
+    exists(YearFieldAccess yfa | node2.asExpr() = yfa and node1.asExpr() = yfa.getQualifier())
+  }


Some of these cases look very similar to cases in YearAssignmentToLeapYearCheckConfig. We could perhaps take the common code out and name it.

geoffw0 · 2026-02-11T19:33:33Z

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

+ *        ... values eventually used in the same time struct
+ * If this is even more challenging if the struct the values end up in are not
+ * local (set inter-procedurally).
+ * This flow flows constants 1-31 to a month or day assignment.


Suggested change

* This flow flows constants 1-31 to a month or day assignment.

* This configuration looks for constants 1-31 flowing to a month or day assignment.

geoffw0 · 2026-02-11T19:41:50Z

cpp/ql/lib/change-notes/2026-02-06-UncheckedLeapYearAfterModification_Refactor.md

+---
+category: minorAnalysis
+---
+* Refactor of UncheckedLeapYearAfterYearModification.ql to address large numbers of false positives. Reduced alerts from 40k to 2k.


Suggested change

* Refactor of UncheckedLeapYearAfterYearModification.ql to address large numbers of false positives. Reduced alerts from 40k to 2k.

* Refactored the "Year field changed using an arithmetic operation without checking for leap year" query (`cpp/leap-year/unchecked-after-arithmetic-year-modification`) to address large numbers of false positive results.

The number of alerts is nice, but will be meaningless to a general audience.

bdrodes added 8 commits February 6, 2026 16:03

C++: Add more tests for modified years with and without leap year che…

6c171c8

…cks (UncheckedLeapYearAfterYearModification). Switch to using 'postprocess' for unit tests.

C++. Accept test changes. One false positive introduced, and one fals…

d9feadc

…e negative remains.

C++: Correct false positive. Only TimeConversionFunction that do not …

ca18179

…auto correct for leap year should be considered.

C++: Accept test changes.

a534d26

C++: Add missing DateTime models for PTIME_FIELDS and TIME_FIELDS

2b806ad

C++: Accept test changes (removing false negative)

36cc209

C++: Add change note.

1796bc0

bdrodes requested a review from a team as a code owner February 6, 2026 21:23

Copilot AI review requested due to automatic review settings February 6, 2026 21:23

github-actions bot added the C++ label Feb 6, 2026

Copilot AI reviewed Feb 6, 2026

View reviewed changes

github-advanced-security bot found potential problems Feb 6, 2026

View reviewed changes

bdrodes added 2 commits February 9, 2026 10:42

C++: Addressing Copilot PR suggestions.

8e36316

C++: Change log missing .md extension.

98b5114

github-actions bot added the documentation label Feb 9, 2026

geoffw0 reviewed Feb 11, 2026

View reviewed changes

	* This list of APIs should check for the return value to detect problems during the conversion.
	* Time conversion functions with a return value that should be checked to detect problems during the conversion.

		// in cases of x.year = x and the x is checked, but the year x.year isn't directly
		// flow from a year assignment node to an RHS if it is an assignment

	* This flow flows constants 1-31 to a month or day assignment.
	* This configuration looks for constants 1-31 flowing to a month or day assignment.

	* Refactor of UncheckedLeapYearAfterYearModification.ql to address large numbers of false positives. Reduced alerts from 40k to 2k.
	* Refactored the "Year field changed using an arithmetic operation without checking for leap year" query (`cpp/leap-year/unchecked-after-arithmetic-year-modification`) to address large numbers of false positive results.

C++: Refactor of UncheckedLeapYearAfterModification #21292

Are you sure you want to change the base?

C++: Refactor of UncheckedLeapYearAfterModification #21292

Uh oh!

Conversation

bdrodes commented Feb 6, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

geoffw0 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants