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.

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

Invalid format specifier on DateOnly in string interpolation leads to infinite loop instead of FormatException #64292

Closed
Tracked by #64603
CyberBotX opened this issue Jan 25, 2022 · 6 comments · Fixed by #64398 or #64460
Assignees
Milestone

Comments

@CyberBotX
Copy link

Description

If you attempt to use an invalid format specifier on a DateOnly within string interpolation, instead of getting a FormatException like you would if you use an invalid format specifier on, for example, an int, instead it seems that an infinite loop happens along with massive memory usage and in some cases an OutOfMemoryException.

Reproduction Steps

Easiest way to see the problem would probably be in LINQPad but could also be done in any place that can run .NET 6. The following line is all that is needed to see the problem occur:

$"{new DateOnly(2022, 1, 25):u}"

But the ToString(string) method of DateOnly does correctly throw a FormatException.

Expected behavior

I expected to get a FormatException like I would if I tried to do $"{1:a}" for instance.

Actual behavior

Actual behavior is that the code never finishes and memory usage balloons out of control (on my system, it would hover around 15GB of memory in use but going up and down as well).

Regression?

Since DateOnly debuted with .NET 6, it could not be a regression.

Known Workarounds

I know of no workarounds other than just not using an invalid format specifier in the first place.

Configuration

.NET 6.0.1
Windows 10 64-bit 21H2 (build 19044.1469)
x64

I do not know if my issue is specific to this configuration, but from stepping through the code in a debugger, it does not seem like this configuration is the issue.

Other information

I don't know of a fix but the problem seems to be partially due to the TryFormat method of DateOnly along with the AppendFormatted method of DefaultInterpolatedStringHandler. As far as I can tell, because TryFormat returns false and sets charsWritten to 0, .NET keeps trying to grow the buffer indefinitely.

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Jan 25, 2022
@svick
Copy link
Contributor

svick commented Jan 25, 2022

I think that the problem is that DefaultInterpolatedStringHandler and DateOnly disagree on what the return value of false in ISpanFormattable.TryFormat means. DefaultInterpolatedStringHandler understands it as "the buffer was too small", while for DateOnly, it means "some error occurred".

I think the correct answer is "the buffer was too small", because otherwise it would be impossible to correctly implement DefaultInterpolatedStringHandler. But that's not what the documentation says.

Assuming I'm right, I think the following should be done:

  • DateOnly.TryFormat should be changed to throw on invalid format.
  • Existing ISpanFormattable types should be reviewed to make sure they only return false when the buffer was too small.
  • The documentation of ISpanFormattable.TryFormat should be changed to say that false only means that the buffer was too small. It could even suggest throwing FormatException on invalid format (which is what e.g. DateTime.TryFormat does).

@ghost
Copy link

ghost commented Jan 26, 2022

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

If you attempt to use an invalid format specifier on a DateOnly within string interpolation, instead of getting a FormatException like you would if you use an invalid format specifier on, for example, an int, instead it seems that an infinite loop happens along with massive memory usage and in some cases an OutOfMemoryException.

Reproduction Steps

Easiest way to see the problem would probably be in LINQPad but could also be done in any place that can run .NET 6. The following line is all that is needed to see the problem occur:

$"{new DateOnly(2022, 1, 25):u}"

But the ToString(string) method of DateOnly does correctly throw a FormatException.

Expected behavior

I expected to get a FormatException like I would if I tried to do $"{1:a}" for instance.

Actual behavior

Actual behavior is that the code never finishes and memory usage balloons out of control (on my system, it would hover around 15GB of memory in use but going up and down as well).

Regression?

Since DateOnly debuted with .NET 6, it could not be a regression.

Known Workarounds

I know of no workarounds other than just not using an invalid format specifier in the first place.

Configuration

.NET 6.0.1
Windows 10 64-bit 21H2 (build 19044.1469)
x64

I do not know if my issue is specific to this configuration, but from stepping through the code in a debugger, it does not seem like this configuration is the issue.

Other information

I don't know of a fix but the problem seems to be partially due to the TryFormat method of DateOnly along with the AppendFormatted method of DefaultInterpolatedStringHandler. As far as I can tell, because TryFormat returns false and sets charsWritten to 0, .NET keeps trying to grow the buffer indefinitely.

Author: CyberBotX
Assignees: -
Labels:

area-System.Runtime, untriaged

Milestone: -

@HobbsCode
Copy link

DefaultInterpolatedStringHandler could implement a heuristic upon realloc to detect this kind of problem generally. If the capacity has grown large but the utilization % is low, then a runaway allocation scenario is likely. It could throw a helpful exception before an OOM condition occurs.

@stephentoub
Copy link
Member

DateOnly.TryFormat should be changed to throw on invalid format.

Yes, either it should throw or it should format something, but it shouldn't return false. For example, DateTime.Now.TryFormat with the meaningless format specifier "123" formats "123", using it as a custom specifier.

The documentation of ISpanFormattable.TryFormat should be changed to say that false only means that the buffer was too small. It could even suggest throwing FormatException on invalid format (which is what e.g. DateTime.TryFormat does).

This is true in general: the Try* pattern special-cases one thing that gets to use the Boolean return value, and any other exceptional condition throws.

cc: @tarekgh

@stephentoub stephentoub added bug and removed untriaged New issue has not been triaged by the area owner labels Jan 26, 2022
@tarekgh
Copy link
Member

tarekgh commented Jan 26, 2022

I'll take a look.

@tarekgh tarekgh added this to the 7.0.0 milestone Jan 27, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jan 27, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 28, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Feb 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.