Suppress instances of cs/leap-year/unsafe-date-construction-from-two-elements #99086
Conversation
Co-authored-by: Tarek Mahmoud Sayed <tarekms@microsoft.com>
ghost
assigned 
|
Tagging subscribers to this area: @dotnet/area-system-datetime Issue Detailsnull
|
| case DateTimeTypeCode.GMonth: | ||
| case DateTimeTypeCode.GDay: | ||
| // codeql[cs/leap-year/unsafe-date-construction-from-two-elements] - The XML specification does not explicitly define this behavior for parsing in a non-leap year. We intentionally throw here. Altering this behavior to be more resilient, producing dates like 2/28 or 3/1, could introduce unintended consequences and may not be desirable for user. | ||
| result = new DateTime(DateTime.Now.Year, xdt.Month, xdt.Day); |
There was a problem hiding this comment.
Can this really throw? It looks like DateTimeTypeCode.GMonth always has xdt.Day == 1, and DateTimeTypeCode.GDay always has xdt.Month == 1, so xdt can never have the February 29 combination that would cause an exception when DateTime.Now.Year is not a leap year.
runtime/src/libraries/System.Private.Xml/src/System/Xml/Schema/XsdDateTime.cs
Lines 862 to 865 in 35ee506
runtime/src/libraries/System.Private.Xml/src/System/Xml/Schema/XsdDateTime.cs
Lines 881 to 884 in 35ee506
I mean the code seems to be ok but the "We intentionally throw here" comment looks misleading.
There was a problem hiding this comment.
It can throw, the parsing code is just use leap year during the parsing which will make 2/29 valid parsing.
result = new DateTime(DateTime.Now.Year, xdt.Month, xdt.Day);
Is using DateTime.Now.Year which is possible to be non-leap year. This will throw .
There was a problem hiding this comment.
By the way, we have the same behavior in the XmlConverter.
There was a problem hiding this comment.
If xdt.InternalTypeCode is DateTimeTypeCode.GMonthDay, then xdt can be February 29 (and xdt.Year is always 1904 for GMonthDay). But when I search for places where the parser sets DateTimeTypeCode.GMonth or DateTimeTypeCode.GDay, it seems either xdt.Month or xdt.Day is always 1, so February 29 is just not possible.
There was a problem hiding this comment.
But it's not passing xdt.Year it's passing DateTime.Now.Year
I see @KalleOlaviNiemitalo is mentioning that only Day or Month will be valid when it's using DateTime.Now.Year, but not both (and they default to 1). We have a separate code, GMonthDay that's used when both are valid and that hits the default case returning the internal representation that uses 1904 for year. I agree - we can remove the comment here about intentionally throwing.
There was a problem hiding this comment.
I think you are right @KalleOlaviNiemitalo. We should fix the comment. Are you interested to submit a PR?
No description provided.