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

Arithmetic overflow in code, generated by GeneratedRegexAttribute #78214

Closed
MaceWindu opened this issue Nov 11, 2022 · 9 comments
Closed

Arithmetic overflow in code, generated by GeneratedRegexAttribute #78214

MaceWindu opened this issue Nov 11, 2022 · 9 comments

Comments

@MaceWindu
Copy link

Description

Note that we use checked builds (which probably not used by runtime tests): <CheckForOverflowUnderflow>True</CheckForOverflowUnderflow>

Reproduction Steps

using System.Text.RegularExpressions;

internal partial class Program
{
    [GeneratedRegex(@"^\s*GO\s*$\n*", RegexOptions.IgnoreCase | RegexOptions.Multiline | RegexOptions.Compiled)]
    private static partial Regex BuildRegex();

    private static void Main(string[] args)
    {
        BuildRegex().Split("X\r\n XX");
    }
}

Expected behavior

No exceptions

Actual behavior

System.OverflowException
  HResult=0x80131516
  Message=Arithmetic operation resulted in an overflow.
  Source=ConsoleApp41
  StackTrace:
   at System.Text.RegularExpressions.Generated.<RegexGenerator_g>F74B1AE921BCEFE4BA601AA541C7A23B1CA9711EA81E8FE504B5B6446748E035A__BuildRegex_0.RunnerFactory.Runner.TryFindNextPossibleStartingPosition(ReadOnlySpan`1 inputSpan) in RegexGenerator.g.cs:line 95
   at System.Text.RegularExpressions.Generated.<RegexGenerator_g>F74B1AE921BCEFE4BA601AA541C7A23B1CA9711EA81E8FE504B5B6446748E035A__BuildRegex_0.RunnerFactory.Runner.Scan(ReadOnlySpan`1 inputSpan) in RegexGenerator.g.cs:line 68
   at System.Text.RegularExpressions.Regex.ScanInternal(RegexRunnerMode mode, Boolean reuseMatchObject, String input, Int32 beginning, RegexRunner runner, ReadOnlySpan`1 span, Boolean returnNullIfReuseMatchObject)
   at System.Text.RegularExpressions.Regex.RunAllMatchesWithCallback[TState](String inputString, ReadOnlySpan`1 inputSpan, Int32 startat, TState& state, MatchCallback`1 callback, RegexRunnerMode mode, Boolean reuseMatchObject)
   at System.Text.RegularExpressions.Regex.RunAllMatchesWithCallback[TState](String input, Int32 startat, TState& state, MatchCallback`1 callback, RegexRunnerMode mode, Boolean reuseMatchObject)
   at System.Text.RegularExpressions.Regex.Split(Regex regex, String input, Int32 count, Int32 startat)
   at Program.Main(String[] args) in 

Regression?

No response

Known Workarounds

No response

Configuration

tfm: net7.0
extra compilation options: <CheckForOverflowUnderflow>True</CheckForOverflowUnderflow>

Other information

image

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Nov 11, 2022
@ghost
Copy link

ghost commented Nov 11, 2022

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

Issue Details

Description

Note that we use checked builds (which probably not used by runtime tests): <CheckForOverflowUnderflow>True</CheckForOverflowUnderflow>

Reproduction Steps

using System.Text.RegularExpressions;

internal partial class Program
{
    [GeneratedRegex(@"^\s*GO\s*$\n*", RegexOptions.IgnoreCase | RegexOptions.Multiline | RegexOptions.Compiled)]
    private static partial Regex BuildRegex();

    private static void Main(string[] args)
    {
        BuildRegex().Split("X\r\n XX");
    }
}

Expected behavior

No exceptions

Actual behavior

System.OverflowException
  HResult=0x80131516
  Message=Arithmetic operation resulted in an overflow.
  Source=ConsoleApp41
  StackTrace:
   at System.Text.RegularExpressions.Generated.<RegexGenerator_g>F74B1AE921BCEFE4BA601AA541C7A23B1CA9711EA81E8FE504B5B6446748E035A__BuildRegex_0.RunnerFactory.Runner.TryFindNextPossibleStartingPosition(ReadOnlySpan`1 inputSpan) in RegexGenerator.g.cs:line 95
   at System.Text.RegularExpressions.Generated.<RegexGenerator_g>F74B1AE921BCEFE4BA601AA541C7A23B1CA9711EA81E8FE504B5B6446748E035A__BuildRegex_0.RunnerFactory.Runner.Scan(ReadOnlySpan`1 inputSpan) in RegexGenerator.g.cs:line 68
   at System.Text.RegularExpressions.Regex.ScanInternal(RegexRunnerMode mode, Boolean reuseMatchObject, String input, Int32 beginning, RegexRunner runner, ReadOnlySpan`1 span, Boolean returnNullIfReuseMatchObject)
   at System.Text.RegularExpressions.Regex.RunAllMatchesWithCallback[TState](String inputString, ReadOnlySpan`1 inputSpan, Int32 startat, TState& state, MatchCallback`1 callback, RegexRunnerMode mode, Boolean reuseMatchObject)
   at System.Text.RegularExpressions.Regex.RunAllMatchesWithCallback[TState](String input, Int32 startat, TState& state, MatchCallback`1 callback, RegexRunnerMode mode, Boolean reuseMatchObject)
   at System.Text.RegularExpressions.Regex.Split(Regex regex, String input, Int32 count, Int32 startat)
   at Program.Main(String[] args) in 

Regression?

No response

Known Workarounds

No response

Configuration

tfm: net7.0
extra compilation options: <CheckForOverflowUnderflow>True</CheckForOverflowUnderflow>

Other information

image

Author: MaceWindu
Assignees: -
Labels:

area-System.Text.RegularExpressions

Milestone: -

@MaceWindu
Copy link
Author

I'm currently deciding how to mitigate it in our code - should we rollback to runtime Regex for all regular expressions or this issue exists only for specific patterns, so any information on issue impact will be useful.

@stephentoub
Copy link
Member

Thanks. From a correctness perspective, this highlights one of the downsides to CheckForOverflowUnderflow; we use casts like this in many places for efficient bounds checks, and the failure is a false positive. Still, I agree the generated code should work in projects that set this flag, and we'll want to wrap the generated code in unchecked blocks if the project has set this option.

@stephentoub stephentoub self-assigned this Nov 11, 2022
@EgorBo
Copy link
Member

EgorBo commented Nov 11, 2022

I wonder if eventually we want to get rid of that pattern, e.g.: https://sharplab.io/#v2:C4LghgzgtgPgAgJgIwFgBQcDMACR2DC2A3utmdqeZWVrgCzYCySAFAMoAOYAdgDwCW3YAD5sELtwA02QcBkBKasSXkZAM2wsWAV1nz+2Xpt1D54ngDoAMgFNuAc2AALRWlXvy57gG1+AXWwAXmwABgBuJQBfJSVaOAZGBHYJASFRL2lZBSUSNw91TQNhYJDsADIymUMxCWs7RxcVfK9fAJKIvLJotEigA===

Here both don't emit bound checks, the 2nd variation should be improved with the work being done around branchless optimizations (if-conversion, etc).

@stephentoub
Copy link
Member

stephentoub commented Nov 11, 2022

the 2nd variation should be improved with the work being done around branchless optimizations (if-conversion, etc)

Won't that still be more expensive? Or are you saying the JIT will end up emitting identical codegen rather than conditionals?

@EgorBo
Copy link
Member

EgorBo commented Nov 11, 2022

the 2nd variation should be improved with the work being done around branchless optimizations (if-conversion, etc)

Won't that still be more expensive? Or are you saying the JIT will end up emitting identical codegen rather than conditionals?

Yes, we already landed a few branchless optimizations so I don't see why we can't handle this one as well in .NET 8 🙂

@danmoseley
Copy link
Member

Do our other source generators need a similar change?

@stephentoub
Copy link
Member

Do our other source generators need a similar change?

Not to my knowledge.

@ghost ghost added in-pr There is an active PR which will close this issue when it is merged and removed in-pr There is an active PR which will close this issue when it is merged labels Nov 12, 2022
@buyaa-n buyaa-n added this to the 7.0.x milestone Nov 16, 2022
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Nov 16, 2022
@buyaa-n
Copy link
Contributor

buyaa-n commented Nov 16, 2022

Fixed with #78228

@buyaa-n buyaa-n closed this as completed Nov 16, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Dec 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants