Skip to content

Improve S5332 performance: Reuse compiled regex #8181

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

Merged
andrei-epure-sonarsource merged 4 commits into from
Nov 6, 2023
Merged

Improve S5332 performance: Reuse compiled regex #8181

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fdead43
Make regex static
martin-strecker-sonarsource Oct 11, 2023
4a8ac35
Fix code smells
martin-strecker-sonarsource Oct 11, 2023
96d98fa
Make static
martin-strecker-sonarsource Oct 11, 2023
231bd03
Formatting
martin-strecker-sonarsource Nov 6, 2023
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
50 changes: 26 additions & 24 deletions analyzers/src/SonarAnalyzer.CSharp/Rules/Hotspots/ClearTextProtocolsAreSensitive.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,23 +81,25 @@ public sealed class ClearTextProtocolsAreSensitive : HotspotDiagnosticAnalyzer
KnownType.System_Windows_Markup_XmlnsCompatibleWithAttribute,
};

private readonly CSharpObjectInitializationTracker objectInitializationTracker =
private static readonly CSharpObjectInitializationTracker ObjectInitializationTracker =
new(constantValue => constantValue is bool value && value,
ImmutableArray.Create(KnownType.System_Net_Mail_SmtpClient, KnownType.System_Net_FtpWebRequest),
propertyName => propertyName == EnableSslName);

private readonly Regex httpRegex;
private readonly Regex ftpRegex;
private readonly Regex telnetRegex;
private readonly Regex telnetRegexForIdentifier;
private readonly Regex validServerRegex;
private static readonly Regex HttpRegex;
private static readonly Regex FtpRegex;
private static readonly Regex TelnetRegex;
private static readonly Regex TelnetRegexForIdentifier;
private static readonly Regex ValidServerRegex;

public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics =>
ImmutableArray.Create(DefaultRule, EnableSslRule);

public ClearTextProtocolsAreSensitive() : this(AnalyzerConfiguration.Hotspot) { }

public ClearTextProtocolsAreSensitive(IAnalyzerConfiguration analyzerConfiguration) : base(analyzerConfiguration)
public ClearTextProtocolsAreSensitive(IAnalyzerConfiguration analyzerConfiguration) : base(analyzerConfiguration) { }

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.

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 •
edited
Loading

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

@martin-strecker-sonarsource martin-strecker-sonarsource Nov 6, 2023

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

{
const string allSubdomainsPattern = @"([^/?#]+\.)?";

Expand All @@ -108,11 +110,11 @@ public ClearTextProtocolsAreSensitive(IAnalyzerConfiguration analyzerConfigurati

var validServerPattern = domainsList.JoinStr("|");

httpRegex = CompileRegex(@$"^http:\/\/(?!{validServerPattern}).");
ftpRegex = CompileRegex(@$"^ftp:\/\/.*@(?!{validServerPattern})");
telnetRegex = CompileRegex(@$"^telnet:\/\/.*@(?!{validServerPattern})");
telnetRegexForIdentifier = CompileRegex(@"Telnet(?![a-z])", false);
validServerRegex = CompileRegex($"^({validServerPattern})$");
HttpRegex = CompileRegex(@$"^http:\/\/(?!{validServerPattern}).");
FtpRegex = CompileRegex(@$"^ftp:\/\/.*@(?!{validServerPattern})");
TelnetRegex = CompileRegex(@$"^telnet:\/\/.*@(?!{validServerPattern})");
TelnetRegexForIdentifier = CompileRegex(@"Telnet(?![a-z])", false);
ValidServerRegex = CompileRegex($"^({validServerPattern})$");
}

protected override void Initialize(SonarAnalysisContext context) =>
Expand All @@ -134,24 +136,24 @@ protected override void Initialize(SonarAnalysisContext context) =>
context.RegisterNodeAction(VisitAssignments, SyntaxKind.SimpleAssignmentExpression);
});

private void VisitObjectCreation(SonarSyntaxNodeReportingContext context)
private static void VisitObjectCreation(SonarSyntaxNodeReportingContext context)
{
var objectCreation = ObjectCreationFactory.Create(context.Node);

if (!IsServerSafe(objectCreation, context.SemanticModel) && objectInitializationTracker.ShouldBeReported(objectCreation, context.SemanticModel, false))
if (!IsServerSafe(objectCreation, context.SemanticModel) && ObjectInitializationTracker.ShouldBeReported(objectCreation, context.SemanticModel, false))
{
context.ReportIssue(Diagnostic.Create(EnableSslRule, objectCreation.Expression.GetLocation()));
}
else if (objectCreation.TypeAsString(context.SemanticModel) is { } typeAsString && telnetRegexForIdentifier.IsMatch(typeAsString))
else if (objectCreation.TypeAsString(context.SemanticModel) is { } typeAsString && TelnetRegexForIdentifier.IsMatch(typeAsString))
{
context.ReportIssue(Diagnostic.Create(DefaultRule, objectCreation.Expression.GetLocation(), TelnetKey, RecommendedProtocols[TelnetKey]));
}
}

private void VisitInvocationExpression(SonarSyntaxNodeReportingContext context)
private static void VisitInvocationExpression(SonarSyntaxNodeReportingContext context)
{
var invocation = (InvocationExpressionSyntax)context.Node;
if (telnetRegexForIdentifier.IsMatch(invocation.Expression.ToString()))
if (TelnetRegexForIdentifier.IsMatch(invocation.Expression.ToString()))
{
context.ReportIssue(Diagnostic.Create(DefaultRule, invocation.GetLocation(), TelnetKey, RecommendedProtocols[TelnetKey]));
}
Expand All @@ -169,30 +171,30 @@ private static void VisitAssignments(SonarSyntaxNodeReportingContext context)
}
}

private void VisitStringExpressions(SonarSyntaxNodeReportingContext c)
private static void VisitStringExpressions(SonarSyntaxNodeReportingContext c)
{
if (GetUnsafeProtocol(c.Node, c.SemanticModel) is { } unsafeProtocol)
{
c.ReportIssue(Diagnostic.Create(DefaultRule, c.Node.GetLocation(), unsafeProtocol, RecommendedProtocols[unsafeProtocol]));
}
}

private bool IsServerSafe(IObjectCreation objectCreation, SemanticModel semanticModel) =>
private static bool IsServerSafe(IObjectCreation objectCreation, SemanticModel semanticModel) =>
objectCreation.ArgumentList?.Arguments.Count > 0
&& validServerRegex.IsMatch(GetText(objectCreation.ArgumentList.Arguments[0].Expression, semanticModel));
&& ValidServerRegex.IsMatch(GetText(objectCreation.ArgumentList.Arguments[0].Expression, semanticModel));

private string GetUnsafeProtocol(SyntaxNode node, SemanticModel semanticModel)
private static string GetUnsafeProtocol(SyntaxNode node, SemanticModel semanticModel)
{
var text = GetText(node, semanticModel);
if (httpRegex.IsMatch(text) && !IsNamespace(semanticModel, node.Parent))
if (HttpRegex.IsMatch(text) && !IsNamespace(semanticModel, node.Parent))
{
return "http";
}
else if (ftpRegex.IsMatch(text))
else if (FtpRegex.IsMatch(text))
{
return "ftp";
}
else if (telnetRegex.IsMatch(text))
else if (TelnetRegex.IsMatch(text))
{
return "telnet";
}
Expand Down