Skip to content

Conversation

@martin-strecker-sonarsource
Copy link
Contributor

@martin-strecker-sonarsource martin-strecker-sonarsource commented Oct 11, 2023

The 5 compiled regex used in ClearTextProtocolsAreSensitive are not cached between instances and are adding a significant JIT compilation overhead. This PR fixes this:

Time spend on compiling

akka.net project

Before:
image

After:
image

Overall

Before: 18th most expensive operation with 14,5 seconds
image

After: 55 ms
image

Copy link
Contributor

@andrei-epure-sonarsource andrei-epure-sonarsource left a comment

Choose a reason for hiding this comment

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

LGTM.

Please address the SC failed QG before merging.

{
}

static ClearTextProtocolsAreSensitive()
Copy link
Contributor

Choose a reason for hiding this comment

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

regarding the https://rules.sonarsource.com/csharp/RSPEC-3963/

I've marked it as FP after a discussion with @martin-strecker-sonarsource :

It is an FP because you can not move the logic to field initializers as suggested. It requires SE to figure this out because you have to look at the code flow (one variable is used to initialize more than one field).

@denis-troller open question whether we want to turn this rule into a Symbolic Execution rule, to avoid False Positives and only raise if the fix is doable

Copy link
Contributor

@andrei-epure-sonarsource andrei-epure-sonarsource Nov 3, 2023

Choose a reason for hiding this comment

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

On the other hand, couldn't you replace with

        private static readonly Lazy<string> ValidServerPattern = new Lazy<string>(() =>
        {
            const string allSubdomainsPattern = @"([^/?#]+\.)?";

            var domainsList = LocalhostAddresses
                .Concat(CommonlyUsedXmlDomains)
                .Select(Regex.Escape)
                .Concat(CommonlyUsedExampleDomains.Select(x => allSubdomainsPattern + Regex.Escape(x)));

            return domainsList.JoinStr("|");
        });
        private static readonly Regex HttpRegex = CompileRegex(@$"^http:\/\/(?!{ValidServerPattern.Value}).");
        private static readonly Regex FtpRegex = CompileRegex(@$"^ftp:\/\/.*@(?!{ValidServerPattern.Value})");
        private static readonly Regex TelnetRegex = CompileRegex(@$"^telnet:\/\/.*@(?!{ValidServerPattern.Value})") ;
        private static readonly Regex TelnetRegexForIdentifier = CompileRegex(@"Telnet(?![a-z])", false);
        private static readonly Regex ValidServerRegex = CompileRegex($"^({ValidServerPattern.Value})$");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would work (even without the Lazy). I explicitly did not do it, as re-ordering the members would break the logic at runtime. Reordering in the static constructor breaks at compile time.

See also this comment for the same problem but for object initializers:

All that said, it really shouldn't matter. If you have a class where the order of assignments to two or more properties affects the final outcome, you've got a pretty hazardous class on your hands. Such design ought to be avoided at all costs, as it is way too easy to break in any case, even when the compiler rules for object initialization are reasonably clear.

https://stackoverflow.com/a/47959273

Or this one by John Skeet:

I would also be very wary of actually writing code which depends on this..

https://stackoverflow.com/a/495622

{
}

static ClearTextProtocolsAreSensitive()
Copy link
Contributor

@andrei-epure-sonarsource andrei-epure-sonarsource Nov 3, 2023

Choose a reason for hiding this comment

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

On the other hand, couldn't you replace with

        private static readonly Lazy<string> ValidServerPattern = new Lazy<string>(() =>
        {
            const string allSubdomainsPattern = @"([^/?#]+\.)?";

            var domainsList = LocalhostAddresses
                .Concat(CommonlyUsedXmlDomains)
                .Select(Regex.Escape)
                .Concat(CommonlyUsedExampleDomains.Select(x => allSubdomainsPattern + Regex.Escape(x)));

            return domainsList.JoinStr("|");
        });
        private static readonly Regex HttpRegex = CompileRegex(@$"^http:\/\/(?!{ValidServerPattern.Value}).");
        private static readonly Regex FtpRegex = CompileRegex(@$"^ftp:\/\/.*@(?!{ValidServerPattern.Value})");
        private static readonly Regex TelnetRegex = CompileRegex(@$"^telnet:\/\/.*@(?!{ValidServerPattern.Value})") ;
        private static readonly Regex TelnetRegexForIdentifier = CompileRegex(@"Telnet(?![a-z])", false);
        private static readonly Regex ValidServerRegex = CompileRegex($"^({ValidServerPattern.Value})$");

@martin-strecker-sonarsource martin-strecker-sonarsource force-pushed the Martin/ClearTextProtocolsAreSensitive_Static branch from bb8bee5 to 96d98fa Compare November 6, 2023 11:57
@sonarqubecloud
Copy link

sonarqubecloud bot commented Nov 6, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sonarqubecloud
Copy link

sonarqubecloud bot commented Nov 6, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

Copy link
Contributor

@andrei-epure-sonarsource andrei-epure-sonarsource left a comment

Choose a reason for hiding this comment

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

LGTM

@andrei-epure-sonarsource andrei-epure-sonarsource merged commit 7e596dc into master Nov 6, 2023
@andrei-epure-sonarsource andrei-epure-sonarsource deleted the Martin/ClearTextProtocolsAreSensitive_Static branch November 6, 2023 12:43
@antonioaversa antonioaversa changed the title ClearTextProtocolsAreSensitive: Reuse compiled regex Improve S5332 performance: Reuse compiled regex Nov 23, 2023
@antonioaversa antonioaversa added this to the 9.14 milestone Nov 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Performance It takes too long.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants