Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suppress instances of cs/leap-year/unsafe-date-construction-from-two-elements #99086

Merged
merged 2 commits into from
Feb 29, 2024
Merged

Suppress instances of cs/leap-year/unsafe-date-construction-from-two-elements #99086

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1a5246d
Adding suppressions for cs/leap-year/unsafe-date-construction-from-tw…
ericstj Feb 28, 2024
35ee506
Apply suggestions from code review
ericstj Feb 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/libraries/System.Management/src/System/Management/ManagementDateTime.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ public static DateTime ToDateTime(string dmtfDate)
throw new ArgumentOutOfRangeException(nameof(dmtfDate));
}


// codeql[cs/leap-year/unsafe-date-construction-from-two-elements] - DateTime not constructed from multiple elements - it's parsed from a string with defaults that are stable DateTime.MinValue. It would be intentional to throw if an invalid combination occurred.
var datetime = new DateTime(year, month, day, hour, minute, second, 0, DateTimeKind.Local);
// Then add the ticks calculated from the microseconds
datetime = datetime.AddTicks(ticks);
Expand Down
1 change: 1 addition & 0 deletions src/libraries/System.Private.CoreLib/src/System/DateTime.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,7 @@ public DateTime(int year, int month, int day, int hour, int minute, int second)
else
{
// if we have a leap second, then we adjust it to 59 so that DateTime will consider it the last in the specified minute.
// codeql[cs/leap-year/unsafe-date-construction-from-two-elements] - DateTime is constructed using the user specified values, not a combination of different sources. It would be intentional to throw if an invalid combination occurred.
this = new DateTime(year, month, day, hour, minute, 59);
ValidateLeapSecond();
}
Expand Down
1 change: 1 addition & 0 deletions src/libraries/System.Private.CoreLib/src/System/Globalization/GregorianCalendarHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ internal EraInfo(int era, int startYear, int startMonth, int startDay, int yearO
this.yearOffset = yearOffset;
this.minEraYear = minEraYear;
this.maxEraYear = maxEraYear;
// codeql[cs/leap-year/unsafe-date-construction-from-two-elements] - A DateTime object is created using values obtained from the machine configuration.
this.ticks = new DateTime(startYear, startMonth, startDay).Ticks;
this.eraName = eraName;
this.abbrevEraName = abbrevEraName;
Expand Down
1 change: 1 addition & 0 deletions src/libraries/System.Private.Xml/src/System/Xml/Schema/XsdDateTime.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,6 +396,7 @@ public static implicit operator DateTime(XsdDateTime xdt)
{
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);
Comment on lines 397 to 400
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

year = leapYear;
day = firstDay;
typeCode = DateTimeTypeCode.GMonth;
return true;

runtime/src/libraries/System.Private.Xml/src/System/Xml/Schema/XsdDateTime.cs

Lines 881 to 884 in 35ee506

year = leapYear;
month = firstMonth;
typeCode = DateTimeTypeCode.GDay;
return true;

I mean the code seems to be ok but the "We intentionally throw here" comment looks misleading.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 .

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, we have the same behavior in the XmlConverter.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@ericstj ericstj Feb 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are right @KalleOlaviNiemitalo. We should fix the comment. Are you interested to submit a PR?

break;
case DateTimeTypeCode.Time:
Expand Down
Loading