From 5adea709d2d8ee69c0921b1b757152faa60f8daf Mon Sep 17 00:00:00 2001 From: SonarTech Date: Fri, 20 Dec 2024 02:42:54 +0000 Subject: [PATCH 01/21] update coverage information --- frontend/public/covered_rules.json | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/frontend/public/covered_rules.json b/frontend/public/covered_rules.json index 32993954313..3383bb8b491 100644 --- a/frontend/public/covered_rules.json +++ b/frontend/public/covered_rules.json @@ -321,7 +321,7 @@ "S2216": "sonar-cpp 5.1.0.10083", "S2234": "sonar-cpp 5.1.0.10083", "S2245": "sonar-cpp 6.15.0.25047", - "S2253": "sonar-cpp master", + "S2253": "sonar-cpp 6.62.0.78645", "S2259": "sonar-cpp 5.1.0.10083", "S2260": "sonar-cpp 5.1.0.10083", "S2275": "sonar-cpp 6.2.0.11201", @@ -412,7 +412,7 @@ "S5271": "sonar-cpp 6.2.0.11201", "S5273": "sonar-cpp 6.2.0.11201", "S5276": "sonar-cpp 6.10.0.18490", - "S5277": "sonar-cpp master", + "S5277": "sonar-cpp 6.62.0.78645", "S5278": "sonar-cpp 6.2.0.11201", "S5279": "sonar-cpp 6.2.0.11201", "S5280": "sonar-cpp 6.2.0.11201", @@ -536,7 +536,7 @@ "S959": "sonar-cpp 5.1.0.10083", "S960": "sonar-cpp 5.1.0.10083", "S961": "sonar-cpp 5.1.0.10083", - "S963": "sonar-cpp master", + "S963": "sonar-cpp 6.62.0.78645", "S966": "sonar-cpp 5.1.0.10083", "S967": "sonar-cpp 6.3.0.11371", "S968": "sonar-cpp 5.1.0.10083", @@ -950,7 +950,7 @@ "S2216": "sonar-cpp 5.1.0.10083", "S2234": "sonar-cpp 5.1.0.10083", "S2245": "sonar-cpp 6.15.0.25047", - "S2253": "sonar-cpp master", + "S2253": "sonar-cpp 6.62.0.78645", "S2259": "sonar-cpp 5.1.0.10083", "S2260": "sonar-cpp 5.1.0.10083", "S2275": "sonar-cpp 6.2.0.11201", @@ -4180,7 +4180,7 @@ "S2216": "sonar-cpp 5.1.0.10083", "S2234": "sonar-cpp 5.1.0.10083", "S2245": "sonar-cpp 6.15.0.25047", - "S2253": "sonar-cpp master", + "S2253": "sonar-cpp 6.62.0.78645", "S2259": "sonar-cpp 6.5.0.12506", "S2260": "sonar-cpp 5.1.0.10083", "S2275": "sonar-cpp 6.2.0.11201", @@ -4260,7 +4260,7 @@ "S5271": "sonar-cpp 6.2.0.11201", "S5273": "sonar-cpp 6.2.0.11201", "S5276": "sonar-cpp 6.10.0.18490", - "S5277": "sonar-cpp master", + "S5277": "sonar-cpp 6.62.0.78645", "S5278": "sonar-cpp 6.2.0.11201", "S5279": "sonar-cpp 6.2.0.11201", "S5280": "sonar-cpp 6.2.0.11201", @@ -4380,7 +4380,7 @@ "S959": "sonar-cpp 5.1.0.10083", "S960": "sonar-cpp 5.1.0.10083", "S961": "sonar-cpp 5.1.0.10083", - "S963": "sonar-cpp master", + "S963": "sonar-cpp 6.62.0.78645", "S966": "sonar-cpp 5.1.0.10083", "S967": "sonar-cpp 6.3.0.11371", "S968": "sonar-cpp 5.1.0.10083", @@ -6299,6 +6299,7 @@ "S1702": "sonar-vb 2.4.0.1305", "S1821": "sonar-vb 2.7.0.2492", "S2260": "sonar-vb 2.4.0.1305", + "S6146": "sonar-vb master", "S907": "sonar-vb 2.4.0.1305" }, "VBNET": { From 98e58e1e76aa3d2a44593a7bb1ed3df2ececa329 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri, 20 Dec 2024 09:11:58 +0000 Subject: [PATCH 02/21] Create rule S7173: "GoSub" statements should not be used (#4580) --- rules/S7173/metadata.json | 2 ++ rules/S7173/vb6/metadata.json | 24 +++++++++++++++++++ rules/S7173/vb6/rule.adoc | 43 +++++++++++++++++++++++++++++++++++ 3 files changed, 69 insertions(+) create mode 100644 rules/S7173/metadata.json create mode 100644 rules/S7173/vb6/metadata.json create mode 100644 rules/S7173/vb6/rule.adoc diff --git a/rules/S7173/metadata.json b/rules/S7173/metadata.json new file mode 100644 index 00000000000..2c63c085104 --- /dev/null +++ b/rules/S7173/metadata.json @@ -0,0 +1,2 @@ +{ +} diff --git a/rules/S7173/vb6/metadata.json b/rules/S7173/vb6/metadata.json new file mode 100644 index 00000000000..b5ce6dbd588 --- /dev/null +++ b/rules/S7173/vb6/metadata.json @@ -0,0 +1,24 @@ +{ + "title": "\"GoSub\" statements should not be used", + "type": "CODE_SMELL", + "status": "ready", + "remediation": { + "func": "Constant\/Issue", + "constantCost": "10min" + }, + "tags": [ + "brain-overload" + ], + "defaultSeverity": "Major", + "ruleSpecification": "RSPEC-7173", + "sqKey": "S7173", + "scope": "All", + "defaultQualityProfiles": ["Sonar way"], + "quickfix": "infeasible", + "code": { + "impacts": { + "MAINTAINABILITY": "MEDIUM" + }, + "attribute": "CLEAR" + } +} diff --git a/rules/S7173/vb6/rule.adoc b/rules/S7173/vb6/rule.adoc new file mode 100644 index 00000000000..bbfd41bc885 --- /dev/null +++ b/rules/S7173/vb6/rule.adoc @@ -0,0 +1,43 @@ +== Why is this an issue? + +The `GoSub` statement in VB6 is an unstructured control flow statement. It can lead to complex and difficult-to-maintain code, as well as potential stack overflow errors due to improper return handling. + +Modern programming practices recommend using proper subroutine or function calls instead, which provide better readability, maintainability, and error handling. + +== How to fix it + +Replace `GoSub` statements with proper subroutine or function calls. + +=== Code examples + +==== Noncompliant code example + +[source,vb6,diff-id=1,diff-type=noncompliant] +---- +Sub ExampleProcedure() + GoSub SubRoutine + Exit Sub + +SubRoutine: + ' ... + Return +End Sub +---- + +==== Compliant solution + +[source,vb6,diff-id=1,diff-type=compliant] +---- +Sub ExampleProcedure() + Call SubRoutine +End Sub + +Sub SubRoutine() + ' ... +End Sub +---- + +== Resources +=== Documentation + +* Microsoft Learn - https://learn.microsoft.com/en-us/office/vba/language/reference/user-interface-help/gosubreturn-statement[GoSub...Return statement] From 434c3bf4dfe4831d500c86c1a0dc1878a13088f5 Mon Sep 17 00:00:00 2001 From: Sebastien Marichal Date: Fri, 20 Dec 2024 14:12:24 +0100 Subject: [PATCH 03/21] Modify rule S1542: Prevent strong substitution (#4585) --- rules/S1542/vb6/rule.adoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rules/S1542/vb6/rule.adoc b/rules/S1542/vb6/rule.adoc index 113584b3aed..5d06d21bcb6 100644 --- a/rules/S1542/vb6/rule.adoc +++ b/rules/S1542/vb6/rule.adoc @@ -4,7 +4,7 @@ include::../description.adoc[] === Noncompliant code example -With default provided regular expression: ^([A-Z][a-zA-Z0-9_]*)|([a-z][a-zA-Z0-9]*_[A-Z][a-zA-Z]*)$ +With default provided regular expression: ^([A-Z][a-zA-Z0-9_]\*)|([a-z][a-zA-Z0-9]*_[A-Z][a-zA-Z]*)$ [source,vb6] From bb47c97c62459e5879a997b2e1ccbe1a4fb3b104 Mon Sep 17 00:00:00 2001 From: SonarTech Date: Sat, 21 Dec 2024 02:40:14 +0000 Subject: [PATCH 04/21] update coverage information --- frontend/public/covered_rules.json | 1 + 1 file changed, 1 insertion(+) diff --git a/frontend/public/covered_rules.json b/frontend/public/covered_rules.json index 3383bb8b491..710478428c3 100644 --- a/frontend/public/covered_rules.json +++ b/frontend/public/covered_rules.json @@ -6300,6 +6300,7 @@ "S1821": "sonar-vb 2.7.0.2492", "S2260": "sonar-vb 2.4.0.1305", "S6146": "sonar-vb master", + "S7173": "sonar-vb master", "S907": "sonar-vb 2.4.0.1305" }, "VBNET": { From 07d614dd5b0a8d7818a80f25c1489a6ea15233f3 Mon Sep 17 00:00:00 2001 From: Sebastien Marichal Date: Mon, 23 Dec 2024 14:57:36 +0100 Subject: [PATCH 05/21] Modify rule S4039: Improve description to match the implementation (#4586) --- rules/S4039/csharp/rule.adoc | 51 ++++++++++++++---------------------- 1 file changed, 19 insertions(+), 32 deletions(-) diff --git a/rules/S4039/csharp/rule.adoc b/rules/S4039/csharp/rule.adoc index 65399fb0755..b865c935f7f 100644 --- a/rules/S4039/csharp/rule.adoc +++ b/rules/S4039/csharp/rule.adoc @@ -1,14 +1,22 @@ == Why is this an issue? -When a base type explicitly implements a public interface method, that method is only accessible in derived types through a reference to the current instance (namely ``++this++``). If the derived type explicitly overrides that interface method, the base implementation becomes inaccessible. +When a base type explicitly implements a public interface method, property or event, that member is only accessible in derived types through a reference to the current instance (namely `this`). If the derived type explicitly overrides that interface member, the base implementation becomes inaccessible. +This rule raises an issue when an unsealed, externally visible type provides an explicit member implementation of an `interface` and does not provide an alternate, externally visible member with the same name. -This rule raises an issue when an unsealed, externally visible type provides an explicit method implementation of a ``++public interface++`` and does not provide an alternate, externally visible method with the same name. +=== Exceptions + +This rule does not report a violation for an explicit implementation of `IDisposable.Dispose` when an externally visible `Close()` or `System.IDisposable.Dispose(Boolean)` method is provided. + +== How to fix it +Make the class sealed, change the class member to a non-explicit declaration, or provide a new class member exposing the functionality of the explicit interface member. -=== Noncompliant code example +=== Code examples -[source,csharp] +==== Noncompliant code example + +[source,csharp,diff-id=1,diff-type=noncompliant] ---- public interface IMyInterface { @@ -21,27 +29,13 @@ public class Foo : IMyInterface { MyMethod(); } - - void MyMethod() - { - // Do something ... - } -} - -public class Bar : Foo, IMyInterface -{ - public void MyMethod() - { - // Can't access base.MyMethod() - // ((IMyInterface)this).MyMethod() would be a recursive call - } } ---- -=== Compliant solution +==== Compliant solution -[source,csharp] +[source,csharp,diff-id=1,diff-type=compliant] ---- public interface IMyInterface { @@ -55,26 +49,19 @@ public class Foo : IMyInterface MyMethod(); } - protected void MyMethod() // or public + // This method can be public or protected + protected void MyMethod() { // Do something ... } } - -public class Bar : Foo, IMyInterface -{ - public void MyMethod() - { - // Do something - base.MyMethod(); - } -} ---- +== Resources -=== Exceptions +=== Documentation -This rule does not report a violation for an explicit implementation of ``++IDisposable.Dispose++`` when an externally visible ``++Close()++`` or ``++System.IDisposable.Dispose(Boolean)++`` method is provided. +* Microsoft Learn - https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/interfaces/explicit-interface-implementation[Explicit Interface Implementation] ifdef::env-github,rspecator-view[] From f96f4c8de784ced5bdad76a7c1cb121a3697f8af Mon Sep 17 00:00:00 2001 From: Sebastien Marichal Date: Mon, 23 Dec 2024 15:37:47 +0100 Subject: [PATCH 06/21] NET-915 Modify rule S2930: Include tracked types in the description (#4588) --- rules/S2930/csharp/rule.adoc | 42 +++++++++++++++++++++++------------- 1 file changed, 27 insertions(+), 15 deletions(-) diff --git a/rules/S2930/csharp/rule.adoc b/rules/S2930/csharp/rule.adoc index 211e22ba7c8..4e33cbcbe1e 100644 --- a/rules/S2930/csharp/rule.adoc +++ b/rules/S2930/csharp/rule.adoc @@ -1,17 +1,29 @@ == Why is this an issue? -When writing https://learn.microsoft.com/en-us/dotnet/standard/managed-code[managed code], there is no need to worry about memory allocation or deallocation as it is taken care of by the https://learn.microsoft.com/en-us/dotnet/standard/garbage-collection[garbage collector]. However, certain objects, such as `Bitmap`, utilize unmanaged memory for specific purposes like https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/unsafe-code[pointer arithmetic]. These objects may have substantial unmanaged memory footprints while having minimal managed footprints. Unfortunately, the garbage collector only recognizes the small managed footprint and does not promptly reclaim the corresponding unmanaged memory (by invoking the finalizer method of `Bitmap`) for efficiency reasons. +When writing https://learn.microsoft.com/en-us/dotnet/standard/managed-code[managed code], there is no need to worry about memory allocation or deallocation as it is taken care of by the https://learn.microsoft.com/en-us/dotnet/standard/garbage-collection[garbage collector]. However, certain objects, such as `Bitmap`, utilize unmanaged memory for specific purposes like https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/unsafe-code[pointer arithmetic]. These objects may have substantial unmanaged memory footprints while having minimal managed footprints. Unfortunately, the garbage collector only recognizes the small managed footprint and does not promptly reclaim the corresponding unmanaged memory (by invoking the finalizer method of `Bitmap`) for efficiency reasons. In addition, it's essential to manage other system resources besides memory. The operating system has limits on the number of https://en.wikipedia.org/wiki/File_descriptor[file descriptors] (e.g., `FileStream`) or https://en.wikipedia.org/wiki/Network_socket[sockets] (e.g., `WebClient`) that can remain open simultaneously. Therefore, it's crucial to `Dispose` of these resources promptly when they are no longer required, instead of relying on the garbage collector to invoke the finalizers of these objects at an unpredictable time in the future. This rule keeps track of `private` fields and local variables of specific types that implement `IDisposable` or `IAsyncDisposable`. It identifies instances of these types that are not properly disposed, closed, aliased, returned, or passed to other methods. This applies to instances that are either directly created using the `new` operator or instantiated through a predefined list of factory methods. +Here is the list of the types tacked by this rule: + +* `FluentAssertions.Execution.AssertionScope` +* `System.Drawing.Bitmap` +* `System.Drawing.Image` +* `System.IO.FileStream` +* `System.IO.StreamReader` +* `System.IO.StreamWriter` +* `System.Net.Sockets.TcpClient` +* `System.Net.Sockets.UdpClient` +* `System.Net.WebClient` + Here is the list of predefined factory methods tracked by this rule: -* `System.IO.File.Create()` -* `System.IO.File.Open()` * `System.Drawing.Image.FromFile()` * `System.Drawing.Image.FromStream()` +* `System.IO.File.Create()` +* `System.IO.File.Open()` === Exceptions @@ -48,7 +60,7 @@ When creating the disposable resource for a one-time use (cases not covered by t [source,csharp,diff-id=1,diff-type=noncompliant] ---- -public class ResourceHolder +public class ResourceHolder { private FileStream fs; // Noncompliant: dispose or close are never called @@ -79,7 +91,7 @@ public class ResourceHolder : IDisposable, IAsyncDisposable this.fs = new FileStream(path, FileMode.Open); } - public void Dispose() + public void Dispose() { this.fs.Dispose(); } @@ -104,16 +116,16 @@ public class ResourceHolder : IDisposable, IAsyncDisposable === Documentation -* https://learn.microsoft.com/en-us/dotnet/standard/managed-code[What is "managed code"?] -* https://learn.microsoft.com/en-us/dotnet/standard/garbage-collection[Garbage collection] -* https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/finalizers[Finalizers] -* https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/unsafe-code[Unsafe code, pointer types, and function pointers] -* https://en.wikipedia.org/wiki/File_descriptor[File descriptor - Wiki] -* https://en.wikipedia.org/wiki/Network_socket[Network socket - Wiki] -* https://learn.microsoft.com/en-us/dotnet/standard/design-guidelines/dispose-pattern[Dispose pattern] -** https://learn.microsoft.com/en-us/dotnet/standard/garbage-collection/implementing-dispose[Implement a Dispose method] -** https://learn.microsoft.com/en-us/dotnet/standard/garbage-collection/implementing-disposeasync[Implement a DisposeAsync method] -* https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/statements/using[using statement and using declaration] +* Microsoft Learn - https://learn.microsoft.com/en-us/dotnet/standard/managed-code[What is "managed code"?] +* Microsoft Learn - https://learn.microsoft.com/en-us/dotnet/standard/garbage-collection[Garbage collection] +* Microsoft Learn - https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/finalizers[Finalizers] +* Microsoft Learn - https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/unsafe-code[Unsafe code, pointer types, and function pointers] +* Wikipedia - https://en.wikipedia.org/wiki/File_descriptor[File descriptor] +* Wikipedia - https://en.wikipedia.org/wiki/Network_socket[Network socket] +* Microsoft Learn - https://learn.microsoft.com/en-us/dotnet/standard/design-guidelines/dispose-pattern[Dispose pattern] +** Microsoft Learn - https://learn.microsoft.com/en-us/dotnet/standard/garbage-collection/implementing-dispose[Implement a Dispose method] +** Microsoft Learn - https://learn.microsoft.com/en-us/dotnet/standard/garbage-collection/implementing-disposeasync[Implement a DisposeAsync method] +* Microsoft Learn - https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/statements/using[using statement and using declaration] * CWE - https://cwe.mitre.org/data/definitions/459[CWE-459 - Incomplete Cleanup] ifdef::env-github,rspecator-view[] From deac0761b1cc7b20064855855040671ce869a01e Mon Sep 17 00:00:00 2001 From: SonarTech Date: Tue, 24 Dec 2024 02:42:28 +0000 Subject: [PATCH 07/21] update coverage information --- frontend/public/covered_rules.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frontend/public/covered_rules.json b/frontend/public/covered_rules.json index 710478428c3..c09a233e4a2 100644 --- a/frontend/public/covered_rules.json +++ b/frontend/public/covered_rules.json @@ -6299,8 +6299,8 @@ "S1702": "sonar-vb 2.4.0.1305", "S1821": "sonar-vb 2.7.0.2492", "S2260": "sonar-vb 2.4.0.1305", - "S6146": "sonar-vb master", - "S7173": "sonar-vb master", + "S6146": "sonar-vb 2.14.0.5475", + "S7173": "sonar-vb 2.14.0.5475", "S907": "sonar-vb 2.4.0.1305" }, "VBNET": { From 330156276c66c0f25eeecccc440c38c4e70ed656 Mon Sep 17 00:00:00 2001 From: Sebastien Marichal Date: Tue, 24 Dec 2024 11:26:00 +0100 Subject: [PATCH 08/21] NET-913 Modify rule S1264: Improve description to match the implementation (#4587) --- rules/S1264/csharp/rule.adoc | 64 +++++++++++++++++++++++++++++++++++- 1 file changed, 63 insertions(+), 1 deletion(-) diff --git a/rules/S1264/csharp/rule.adoc b/rules/S1264/csharp/rule.adoc index fe598e7f71a..b6c2654bcda 100644 --- a/rules/S1264/csharp/rule.adoc +++ b/rules/S1264/csharp/rule.adoc @@ -1,4 +1,66 @@ -include::../rule.adoc[] +== Why is this an issue? + +Using a `for` loop without its typical structure (initialization, condition, increment) can be confusing. In those cases, it is better to use a `while` loop as it is more readable. + +The initializer section should contain a variable declaration to be considered as a valid initialization. + +== How to fix it + +Replace the `for` loop with a `while` loop. + +=== Code example + +==== Noncompliant code example + +[source,csharp,diff-id=1,diff-type=noncompliant] +---- +for (;condition;) // Noncompliant; both the initializer and increment sections are missing +{ + // Do something +} +---- + +==== Compliant solution + +[source,csharp,diff-id=1,diff-type=compliant] +---- +while (condition) +{ + // Do something +} +---- + +==== Noncompliant code example + +[source,csharp,diff-id=2,diff-type=noncompliant] +---- +int i; + +for (i = 0; i < 10;) // Noncompliant; the initializer section should contain a variable declaration +{ + // Do something + i++; +} +---- + +==== Compliant solution + +[source,csharp,diff-id=2,diff-type=compliant] +---- +int i = 0; + +while (i < 10) +{ + // Do something + i++; +} +---- + +== Resources + +=== Documentation + +* Microsoft Learn - https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/statements/iteration-statements#the-for-statement[The `for` statement] ifdef::env-github,rspecator-view[] From e5a05283023b7230a9b1d55926658df37ece70a4 Mon Sep 17 00:00:00 2001 From: Sebastien Marichal Date: Tue, 24 Dec 2024 14:55:44 +0100 Subject: [PATCH 09/21] NET-920 Modify rule S125: Add dotnet example (#4589) --- rules/S125/csharp/rule.adoc | 38 ++++++++++++++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/rules/S125/csharp/rule.adoc b/rules/S125/csharp/rule.adoc index fe598e7f71a..96e544d8da2 100644 --- a/rules/S125/csharp/rule.adoc +++ b/rules/S125/csharp/rule.adoc @@ -1,4 +1,40 @@ -include::../rule.adoc[] +== Why is this an issue? + +Commented-out code distracts the focus from the actual executed code. It creates a noise that increases maintenance code. And because it is never executed, it quickly becomes out of date and invalid. + +Commented-out code should be deleted and can be retrieved from source control history if required. + +== How to fix it + +Delete the commented out code. + +=== Code examples + +==== Noncompliant code example + +[source,csharp,diff-id=1,diff-type=noncompliant] +---- +void Method(string s) +{ + // if (s.StartsWith('A')) + // { + // s = s.Substring(1); + // } + + // Do something... +} +---- + +==== Compliant solution + +[source,csharp,diff-id=1,diff-type=compliant] +---- +void Method(string s) +{ + // Do something... +} +---- + ifdef::env-github,rspecator-view[] From 8db2c956de4d64b8651b74e761f4e309ef0ea91b Mon Sep 17 00:00:00 2001 From: tomasz-kaminski-sonarsource <79814193+tomasz-kaminski-sonarsource@users.noreply.github.com> Date: Thu, 2 Jan 2025 11:37:02 +0100 Subject: [PATCH 10/21] S6018 Add example showing use of `inline` out of line (CPP-4342) --- rules/S6018/cfamily/rule.adoc | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/rules/S6018/cfamily/rule.adoc b/rules/S6018/cfamily/rule.adoc index 68cd2995ba5..8c11e009bb5 100644 --- a/rules/S6018/cfamily/rule.adoc +++ b/rules/S6018/cfamily/rule.adoc @@ -2,6 +2,7 @@ ``{cpp}17`` introduced inline variables. They provide a proper way to define global variables in header files. Before inline variables, it wasn’t possible to simply define global variables without compile or link errors: +[source,cpp] ---- struct A { static std::string s1 = "s1"; // doesn’t compile @@ -20,14 +21,19 @@ This rule will detect these workarounds and suggest using inline variables inste [source,cpp] ---- -struct A { +struct Clazz { static std::string& getS1() { // Noncompliant static std::string s1 = "s1"; return s1; } + + static Clazz const& getSelf() { // Noncompliant + static Clazz self; + return self; + } }; -inline std::string& gets2() { // Noncompliant +inline std::string& getS2() { // Noncompliant static std::string s2 = "s2"; return s2; } @@ -41,9 +47,12 @@ T s3 = "s3"; // Noncompliant. Available starting C++14 [source,cpp] ---- -struct A { - inline static std::string s1 = "s1"; // Compliant +struct Clazz { + inline static std::string s1 = "s1"; // Compliant, + static Clazz self; }; +inline Class Clazz::self; // Compliant +// Out of line definition is required for `Clazz` to be complete. inline std::string s2 = "s2"; // Compliant ---- From 56cf51e747eb14b39f51aa2c1a87fbd74db0278d Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Mon, 6 Jan 2025 15:54:54 +0100 Subject: [PATCH 11/21] Create rule S7174: Square API keys should not be disclosed (#4591) --- rules/S7174/metadata.json | 2 ++ rules/S7174/secrets/metadata.json | 56 +++++++++++++++++++++++++++++++ rules/S7174/secrets/rule.adoc | 35 +++++++++++++++++++ 3 files changed, 93 insertions(+) create mode 100644 rules/S7174/metadata.json create mode 100644 rules/S7174/secrets/metadata.json create mode 100644 rules/S7174/secrets/rule.adoc diff --git a/rules/S7174/metadata.json b/rules/S7174/metadata.json new file mode 100644 index 00000000000..2c63c085104 --- /dev/null +++ b/rules/S7174/metadata.json @@ -0,0 +1,2 @@ +{ +} diff --git a/rules/S7174/secrets/metadata.json b/rules/S7174/secrets/metadata.json new file mode 100644 index 00000000000..308ee367ea7 --- /dev/null +++ b/rules/S7174/secrets/metadata.json @@ -0,0 +1,56 @@ +{ + "title": "Square API keys should not be disclosed", + "type": "VULNERABILITY", + "code": { + "impacts": { + "SECURITY": "HIGH" + }, + "attribute": "TRUSTWORTHY" + }, + "status": "ready", + "remediation": { + "func": "Constant\/Issue", + "constantCost": "30min" + }, + "tags": [ + "cwe", + "cert" + ], + "defaultSeverity": "Blocker", + "ruleSpecification": "RSPEC-7174", + "sqKey": "S7174", + "scope": "All", + "securityStandards": { + "CWE": [ + 798, + 259 + ], + "OWASP": [ + "A3" + ], + "CERT": [ + "MSC03-J." + ], + "OWASP Top 10 2021": [ + "A7" + ], + "PCI DSS 3.2": [ + "6.5.10" + ], + "PCI DSS 4.0": [ + "6.2.4" + ], + "ASVS 4.0": [ + "2.10.4", + "3.5.2", + "6.4.1" + ], + "STIG ASD_V5R3": [ + "V-222642" + ] + }, + "defaultQualityProfiles": [ + "Sonar way" + ], + "quickfix": "unknown" +} diff --git a/rules/S7174/secrets/rule.adoc b/rules/S7174/secrets/rule.adoc new file mode 100644 index 00000000000..0efb9d982d4 --- /dev/null +++ b/rules/S7174/secrets/rule.adoc @@ -0,0 +1,35 @@ + +include::../../../shared_content/secrets/description.adoc[] + +== Why is this an issue? + +include::../../../shared_content/secrets/rationale.adoc[] + +=== What is the potential impact? + +Below are some real-world scenarios that illustrate some impacts of an attacker +exploiting the secret. + +include::../../../shared_content/secrets/impact/financial_loss.adoc[] + +include::../../../shared_content/secrets/impact/disclosure_of_financial_data.adoc[] + +include::../../../shared_content/secrets/impact/data_compromise.adoc[] + +== How to fix it + +include::../../../shared_content/secrets/fix/revoke.adoc[] + +include::../../../shared_content/secrets/fix/vault.adoc[] + +=== Code examples + +:example_secret: EAAAl5PRAg8E1ehV-PFqSBwSYZO8L0_w0oYp518u4wGl1zm4MA5KvGPtI_qkbQVb +:example_name: square-key +:example_env: SQUARE_KEY + +include::../../../shared_content/secrets/examples.adoc[] + +== Resources + +include::../../../shared_content/secrets/resources/standards.adoc[] From aa709674da52bda2b1493bae4e8b7e90d71d8b5c Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Tue, 7 Jan 2025 10:14:41 +0100 Subject: [PATCH 12/21] Create rule S7175: Linear API keys should not be disclosed (#4594) --- rules/S7175/metadata.json | 2 ++ rules/S7175/secrets/metadata.json | 56 +++++++++++++++++++++++++++++++ rules/S7175/secrets/rule.adoc | 33 ++++++++++++++++++ 3 files changed, 91 insertions(+) create mode 100644 rules/S7175/metadata.json create mode 100644 rules/S7175/secrets/metadata.json create mode 100644 rules/S7175/secrets/rule.adoc diff --git a/rules/S7175/metadata.json b/rules/S7175/metadata.json new file mode 100644 index 00000000000..2c63c085104 --- /dev/null +++ b/rules/S7175/metadata.json @@ -0,0 +1,2 @@ +{ +} diff --git a/rules/S7175/secrets/metadata.json b/rules/S7175/secrets/metadata.json new file mode 100644 index 00000000000..ea29ca430eb --- /dev/null +++ b/rules/S7175/secrets/metadata.json @@ -0,0 +1,56 @@ +{ + "title": "Linear API keys should not be disclosed", + "type": "VULNERABILITY", + "code": { + "impacts": { + "SECURITY": "HIGH" + }, + "attribute": "TRUSTWORTHY" + }, + "status": "ready", + "remediation": { + "func": "Constant\/Issue", + "constantCost": "30min" + }, + "tags": [ + "cwe", + "cert" + ], + "defaultSeverity": "Blocker", + "ruleSpecification": "RSPEC-7175", + "sqKey": "S7175", + "scope": "All", + "securityStandards": { + "CWE": [ + 798, + 259 + ], + "OWASP": [ + "A3" + ], + "CERT": [ + "MSC03-J." + ], + "OWASP Top 10 2021": [ + "A7" + ], + "PCI DSS 3.2": [ + "6.5.10" + ], + "PCI DSS 4.0": [ + "6.2.4" + ], + "ASVS 4.0": [ + "2.10.4", + "3.5.2", + "6.4.1" + ], + "STIG ASD_V5R3": [ + "V-222642" + ] + }, + "defaultQualityProfiles": [ + "Sonar way" + ], + "quickfix": "unknown" +} diff --git a/rules/S7175/secrets/rule.adoc b/rules/S7175/secrets/rule.adoc new file mode 100644 index 00000000000..4dedcca5827 --- /dev/null +++ b/rules/S7175/secrets/rule.adoc @@ -0,0 +1,33 @@ + +include::../../../shared_content/secrets/description.adoc[] + +== Why is this an issue? + +include::../../../shared_content/secrets/rationale.adoc[] + +=== What is the potential impact? + +Below are some real-world scenarios that illustrate some impacts of an attacker +exploiting the secret. + +include::../../../shared_content/secrets/impact/financial_loss.adoc[] + +include::../../../shared_content/secrets/impact/data_compromise.adoc[] + +== How to fix it + +include::../../../shared_content/secrets/fix/revoke.adoc[] + +include::../../../shared_content/secrets/fix/vault.adoc[] + +=== Code examples + +:example_secret: lin_oauth_1462cb9a9d462cab2d2992281cb8e94b972cdebe8a33e72067383fd1d4a6ea23 +:example_name: linear-key +:example_env: LINEAR_KEY + +include::../../../shared_content/secrets/examples.adoc[] + +== Resources + +include::../../../shared_content/secrets/resources/standards.adoc[] From dff46bdcfd77209aafbf7bd88afc6d4d0e1deed6 Mon Sep 17 00:00:00 2001 From: "Loris S." <91723853+loris-s-sonarsource@users.noreply.github.com> Date: Tue, 7 Jan 2025 14:53:28 +0100 Subject: [PATCH 13/21] Modify S3649(Python): Fix logic error (#4598) --- rules/S3649/python/how-to-fix-it/sqlalchemy.adoc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rules/S3649/python/how-to-fix-it/sqlalchemy.adoc b/rules/S3649/python/how-to-fix-it/sqlalchemy.adoc index f839c894393..8fa14bfd944 100644 --- a/rules/S3649/python/how-to-fix-it/sqlalchemy.adoc +++ b/rules/S3649/python/how-to-fix-it/sqlalchemy.adoc @@ -27,7 +27,7 @@ import sqlalchemy @app.route('/example') def get_users(): user = request.args["user"] - conn = sqlalchemy.create_engine(connection_string) + engine = sqlalchemy.create_engine(connection_string) conn = engine.connect() conn.execute("SELECT user FROM users WHERE user = '" + user + "'") # Noncompliant @@ -43,7 +43,7 @@ import sqlalchemy @app.route('/example') def get_users(): user = request.args["user"] - conn = sqlalchemy.create_engine(connection_string) + engine = sqlalchemy.create_engine(connection_string) metadata = sqlalchemy.MetaData(bind=conn, reflect=True) users = metadata.tables['users'] conn = engine.connect() From 383361a9e8fa864e6959c875b475d47b91752afb Mon Sep 17 00:00:00 2001 From: SonarTech Date: Wed, 8 Jan 2025 02:42:08 +0000 Subject: [PATCH 14/21] update coverage information --- frontend/public/covered_rules.json | 1 + 1 file changed, 1 insertion(+) diff --git a/frontend/public/covered_rules.json b/frontend/public/covered_rules.json index c09a233e4a2..99608fed544 100644 --- a/frontend/public/covered_rules.json +++ b/frontend/public/covered_rules.json @@ -1340,6 +1340,7 @@ "S7127": "sonar-cpp 6.61.0.77816", "S7129": "sonar-cpp 6.61.0.77816", "S7132": "sonar-cpp 6.61.0.77816", + "S7172": "sonar-cpp master", "S784": "sonar-cpp 5.1.0.10083", "S787": "sonar-cpp 5.1.0.10083", "S793": "sonar-cpp 5.1.0.10083", From a2aa4066134ae0333a5407bb0329b367c3af226e Mon Sep 17 00:00:00 2001 From: Sebastien Marichal Date: Wed, 8 Jan 2025 10:23:35 +0100 Subject: [PATCH 15/21] NET-934 Modify S1643: Add benchmarks (#4593) --- rules/S1643/csharp/rule.adoc | 83 +++++++++++++++++++++++++++++++++--- 1 file changed, 76 insertions(+), 7 deletions(-) diff --git a/rules/S1643/csharp/rule.adoc b/rules/S1643/csharp/rule.adoc index 03f030c0d08..1d104e58863 100644 --- a/rules/S1643/csharp/rule.adoc +++ b/rules/S1643/csharp/rule.adoc @@ -1,30 +1,99 @@ == Why is this an issue? -``++StringBuilder++`` is more efficient than string concatenation, especially when the operator is repeated over and over as in loops. +Concatenating multiple string literals or strings using the `+` operator creates a new string object for each concatenation. This can lead to a large number of intermediate string objects and can be inefficient. The `StringBuilder` class is more efficient than string concatenation, especially when the operator is repeated over and over as in loops. -=== Noncompliant code example +== How to fix it -[source,csharp] +Replace string concatenation with `StringBuilder`. + +=== Code examples + +==== Noncompliant code example + +[source,csharp,diff-id=1,diff-type=noncompliant] ---- string str = ""; -for (int i = 0; i < arrayOfStrings.Length ; ++i) +for (int i = 0; i < arrayOfStrings.Length ; ++i) { str = str + arrayOfStrings[i]; } ---- -=== Compliant solution +==== Compliant solution -[source,csharp] +[source,csharp,diff-id=1,diff-type=compliant] ---- StringBuilder bld = new StringBuilder(); -for (int i = 0; i < arrayOfStrings.Length; ++i) +for (int i = 0; i < arrayOfStrings.Length; ++i) { bld.Append(arrayOfStrings[i]); } string str = bld.ToString(); ---- +== Resources + +=== Documentation + +* Microsoft Learn - https://learn.microsoft.com/en-us/dotnet/api/system.text.stringbuilder[StringBuilder Class] + +=== Benchmarks + +[options="header"] +|=== +| Method | Runtime | Mean | Standard Deviation | Allocated +| StringConcatenation | .NET 9.0 | 45,723.79 us | 4,589.713 us | 586280.56 KB +| StringBuilder | .NET 9.0 | 77.73 us | 1.221 us | 243.79 KB +| StringConcatenation | .NET Framework 4.6.2 | 33,922.61 us | 302.498 us | 586450.35 KB +| StringBuilder | .NET Framework 4.6.2 | 178.14 us | 9.010 us | 244.15 KB +|=== + + +==== Glossary + +* https://en.wikipedia.org/wiki/Arithmetic_mean[Mean] +* https://en.wikipedia.org/wiki/Standard_deviation[Standard Deviation] + +The results were generated by running the following snippet with https://github.com/dotnet/BenchmarkDotNet[BenchmarkDotNet]: + +[source,csharp] +---- +[Params(10_000)] +public int Iterations; + +[Benchmark] +public void StringConcatenation() +{ + string str = ""; + for (int i = 0; i < Iterations; i++) + { + str = str + "append"; + } +} + +[Benchmark] +public void StringBuilder() +{ + StringBuilder builder = new StringBuilder(); + for (int i = 0; i < Iterations; i++) + { + builder.Append("append"); + } + _ = builder.ToString(); +} +---- + +Hardware Configuration: + +[source] +---- +BenchmarkDotNet v0.14.0, Windows 10 (10.0.19045.5247/22H2/2022Update) +12th Gen Intel Core i7-12800H, 1 CPU, 20 logical and 14 physical cores + [Host] : .NET Framework 4.8.1 (4.8.9282.0), X64 RyuJIT VectorSize=256 + .NET 9.0 : .NET 9.0.0 (9.0.24.52809), X64 RyuJIT AVX2 + .NET Framework 4.6.2 : .NET Framework 4.8.1 (4.8.9282.0), X64 RyuJIT VectorSize=256 +---- + ifdef::env-github,rspecator-view[] ''' From 43247cd487570ef6de261fb8298ab81a836a1e47 Mon Sep 17 00:00:00 2001 From: Sebastien Marichal Date: Wed, 8 Jan 2025 10:23:49 +0100 Subject: [PATCH 16/21] NET-933 Modify S1155: Add benchmarks (#4592) --- rules/S1155/csharp/rule.adoc | 65 ++++++++++++++++++++++++++++++++++-- 1 file changed, 63 insertions(+), 2 deletions(-) diff --git a/rules/S1155/csharp/rule.adoc b/rules/S1155/csharp/rule.adoc index 3a859b8ddf5..8bde8dac4d3 100644 --- a/rules/S1155/csharp/rule.adoc +++ b/rules/S1155/csharp/rule.adoc @@ -5,6 +5,14 @@ When you call `Any()`, it clearly communicates the code's intention, which is to * if the collection is an `EntityFramework` or other ORM query, calling `Count()` will cause executing a potentially massive SQL query and could put a large overhead on the application database. Calling `Any()` will also connect to the database, but will generate much more efficient SQL. * if the collection is part of a LINQ query that contains `Select()` statements that create objects, a large amount of memory could be unnecessarily allocated. Calling `Any()` will be much more efficient because it will execute fewer iterations of the enumerable. +== How to fix it + +Prefer using `Any()` to test for emptiness over `Count()`. + +=== Code examples + +==== Noncompliant code example + [source,csharp,diff-id=1,diff-type=noncompliant] ---- private static bool HasContent(IEnumerable strings) @@ -23,7 +31,7 @@ private static bool IsEmpty(IEnumerable strings) } ---- -Prefer using `Any()` to test for emptiness over `Count()`. +==== Compliant solution [source,csharp,diff-id=1,diff-type=compliant] ---- @@ -43,6 +51,59 @@ private static bool IsEmpty(IEnumerable strings) } ---- +== Resources + +=== Benchmarks + +[options="header"] +|=== +| Method | Runtime | Mean | Standard Deviation +| Count | .NET 9.0 | 2,841.003 ns | 266.0238 ns +| Any | .NET 9.0 | 1.749 ns | 0.1242 ns +| Count | .NET Framework 4.8.1 | 71,125.275 ns | 731.0382 ns +| Any | .NET Framework 4.8.1 | 31.774 ns | 0.3196 ns +|=== + +==== Glossary + +* https://en.wikipedia.org/wiki/Arithmetic_mean[Mean] +* https://en.wikipedia.org/wiki/Standard_deviation[Standard Deviation] + +The results were generated by running the following snippet with https://github.com/dotnet/BenchmarkDotNet[BenchmarkDotNet]: + +[source,csharp] +---- +private IEnumerable collection; + +public const int N = 10_000; + +[GlobalSetup] +public void GlobalSetup() +{ + collection = Enumerable.Range(0, N).Select(x => N - x); +} + +[Benchmark(Baseline = true)] +public bool Count() => + collection.Count() > 0; + +[Benchmark] +public bool Any() => + collection.Any(); +---- + +Hardware Configuration: + +[source] +---- +BenchmarkDotNet v0.14.0, Windows 10 (10.0.19045.5247/22H2/2022Update) +12th Gen Intel Core i7-12800H, 1 CPU, 20 logical and 14 physical cores + [Host] : .NET Framework 4.8.1 (4.8.9282.0), X64 RyuJIT VectorSize=256 + .NET 9.0 : .NET 9.0.0 (9.0.24.52809), X64 RyuJIT AVX2 + .NET Framework 4.8.1 : .NET Framework 4.8.1 (4.8.9282.0), X64 RyuJIT VectorSize=256 +---- + + ifdef::env-github,rspecator-view[] ''' @@ -101,7 +162,7 @@ Added the `Count() == 0` to the description and extended the code samples \[~ann.campbell.2] You are right, I removed the comparison to `0`. === on 27 May 2015, 14:04:31 Ann Campbell wrote: -Thanks [~tamas.vajk]. I've merged the code blocks into one block each for Compliant and Noncompliant +Thanks [~tamas.vajk]. I've merged the code blocks into one block each for Compliant and Noncompliant === on 1 Jun 2015, 14:30:42 Ann Campbell wrote: I've updated the examples with `List`. Please double-check me. From 4e15f3d6531ad9dd426cee16bb75744ca1a3603a Mon Sep 17 00:00:00 2001 From: Martin Strecker <103252490+martin-strecker-sonarsource@users.noreply.github.com> Date: Wed, 8 Jan 2025 12:15:50 +0100 Subject: [PATCH 17/21] Modify S3169: Add benchmarks (#4595) * Modify S3169: Add benchmarks * Update rules/S3169/csharp/rule.adoc Co-authored-by: Sebastien Marichal * Update benchmark * LAYC * diff-id --------- Co-authored-by: Sebastien Marichal --- rules/S3169/csharp/rule.adoc | 89 +++++++++++++++++++++++++++++--- rules/S6961/csharp/metadata.json | 3 +- 2 files changed, 83 insertions(+), 9 deletions(-) diff --git a/rules/S3169/csharp/rule.adoc b/rules/S3169/csharp/rule.adoc index b2e9860eb1d..df9d2c517f5 100644 --- a/rules/S3169/csharp/rule.adoc +++ b/rules/S3169/csharp/rule.adoc @@ -1,14 +1,16 @@ == Why is this an issue? -There's no point in chaining multiple ``++OrderBy++`` calls in a LINQ; only the last one will be reflected in the result because each subsequent call completely reorders the list. Thus, calling ``++OrderBy++`` multiple times is a performance issue as well, because all of the sorting will be executed, but only the result of the last sort will be kept. +There's no point in chaining multiple `OrderBy` calls in a LINQ; only the last one will be reflected in the result because each subsequent call completely reorders the list. Thus, calling `OrderBy` multiple times is a performance issue as well, because all of the sorting will be executed, but only the result of the last sort will be kept. +== How to fix it -Instead, use ``++ThenBy++`` for each call after the first. +Instead, use `ThenBy` for each call after the first. +=== Code examples -=== Noncompliant code example +==== Noncompliant code example -[source,csharp] +[source,csharp,diff-id=1,diff-type=noncompliant] ---- var x = personList .OrderBy(person => person.Age) @@ -17,9 +19,9 @@ var x = personList ---- -=== Compliant solution +==== Compliant solution -[source,csharp] +[source,csharp,diff-id=1,diff-type=compliant] ---- var x = personList .OrderBy(person => person.Age) @@ -27,6 +29,79 @@ var x = personList .ToList(); ---- +== Resources + +=== Benchmarks + +[options="header"] +|=== +| Method | Runtime | Mean | StdDev | Allocated +| OrderByAge | .NET 9.0 | 12.84 ms | 0.804 ms | 1.53 MB +| OrderByAgeOrderBySize | .NET 9.0 | 24.08 ms | 0.267 ms | 3.05 MB +| OrderByAgeThenBySize | .NET 9.0 | 18.58 ms | 0.747 ms | 1.91 MB +| | | | | +| OrderByAge | .NET Framework 4.8.1 | 22.99 ms | 0.228 ms | 1.53 MB +| OrderByAgeOrderBySize | .NET Framework 4.8.1 | 44.90 ms | 0.581 ms | 4.3 MB +| OrderByAgeThenBySize | .NET Framework 4.8.1 | 31.72 ms | 0.402 ms | 1.91 MB +|=== + +==== Glossary + +* https://en.wikipedia.org/wiki/Arithmetic_mean[Mean] +* https://en.wikipedia.org/wiki/Standard_deviation[Standard Deviation] + +The results were generated by running the following snippet with https://github.com/dotnet/BenchmarkDotNet[BenchmarkDotNet]: + +[source,csharp] +---- +public class Person +{ + public string Name { get; set; } + public int Age { get; set; } + public int Size { get; set; } +} + +private Random random = new Random(1); +private Consumer consumer = new Consumer(); +private Person[] array; + +[Params(100_000)] +public int N { get; set; } + +[GlobalSetup] +public void GlobalSetup() +{ + array = Enumerable.Range(0, N).Select(x => new Person + { + Name = Path.GetRandomFileName(), + Age = random.Next(0, 100), + Size = random.Next(0, 200) + }).ToArray(); +} + +[Benchmark(Baseline = true)] +public void OrderByAge() => + array.OrderBy(x => x.Age).Consume(consumer); + +[Benchmark] +public void OrderByAgeOrderBySize() => + array.OrderBy(x => x.Age).OrderBy(x => x.Size).Consume(consumer); + +[Benchmark] +public void OrderByAgeThenBySize() => + array.OrderBy(x => x.Age).ThenBy(x => x.Size).Consume(consumer); +---- + +Hardware configuration: + +[source] +---- +BenchmarkDotNet v0.14.0, Windows 10 (10.0.19045.5247/22H2/2022Update) +Intel Core Ultra 7 165H, 1 CPU, 22 logical and 16 physical cores + [Host] : .NET Framework 4.8.1 (4.8.9282.0), X64 RyuJIT VectorSize=256 + .NET 9.0 : .NET 9.0.0 (9.0.24.52809), X64 RyuJIT AVX2 + .NET Framework 4.8.1 : .NET Framework 4.8.1 (4.8.9282.0), X64 RyuJIT VectorSize=256 +---- ifdef::env-github,rspecator-view[] @@ -56,7 +131,7 @@ I shuffled the text some, [~tamas.vajk] \[~ann.campbell.2] Shouldn't this issue have some performance related label as well? -I simplified the message as the ordering might not happen by some property, but by some complex logic, and in this case we can't display the whole expression and ``++Comparer++`` in the message. +I simplified the message as the ordering might not happen by some property, but by some complex logic, and in this case we can't display the whole expression and `Comparer` in the message. === on 1 Jul 2015, 11:26:48 Ann Campbell wrote: added [~tamas.vajk] diff --git a/rules/S6961/csharp/metadata.json b/rules/S6961/csharp/metadata.json index 7116988462a..f3d4d9be3d7 100644 --- a/rules/S6961/csharp/metadata.json +++ b/rules/S6961/csharp/metadata.json @@ -7,8 +7,7 @@ "constantCost": "5min" }, "tags": [ - "asp.net", - "performance" + "asp.net" ], "defaultSeverity": "Major", "ruleSpecification": "RSPEC-6961", From efd18e59bf950676447a31e4942a865673e2af8d Mon Sep 17 00:00:00 2001 From: Sebastien Marichal Date: Wed, 8 Jan 2025 13:54:10 +0100 Subject: [PATCH 18/21] NET-934 Modify S1643: Use NetFx 4.8.1 in Brenchmark (#4600) --- rules/S1643/csharp/rule.adoc | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/rules/S1643/csharp/rule.adoc b/rules/S1643/csharp/rule.adoc index 1d104e58863..123cdfb61bf 100644 --- a/rules/S1643/csharp/rule.adoc +++ b/rules/S1643/csharp/rule.adoc @@ -42,13 +42,12 @@ string str = bld.ToString(); [options="header"] |=== | Method | Runtime | Mean | Standard Deviation | Allocated -| StringConcatenation | .NET 9.0 | 45,723.79 us | 4,589.713 us | 586280.56 KB -| StringBuilder | .NET 9.0 | 77.73 us | 1.221 us | 243.79 KB -| StringConcatenation | .NET Framework 4.6.2 | 33,922.61 us | 302.498 us | 586450.35 KB -| StringBuilder | .NET Framework 4.6.2 | 178.14 us | 9.010 us | 244.15 KB +| StringConcatenation | .NET 9.0 | 50,530.75 us | 2,699.189 us | 586280.70 KB +| StringBuilder | .NET 9.0 | 82.31 us | 3.387 us | 243.79 KB +| StringConcatenation | .NET Framework 4.8.1 | 37,453.72 us | 1,543.051 us | 586450.38 KB +| StringBuilder | .NET Framework 4.8.1 | 178.32 us | 6.148 us | 244.15 KB |=== - ==== Glossary * https://en.wikipedia.org/wiki/Arithmetic_mean[Mean] @@ -91,7 +90,7 @@ BenchmarkDotNet v0.14.0, Windows 10 (10.0.19045.5247/22H2/2022Update) 12th Gen Intel Core i7-12800H, 1 CPU, 20 logical and 14 physical cores [Host] : .NET Framework 4.8.1 (4.8.9282.0), X64 RyuJIT VectorSize=256 .NET 9.0 : .NET 9.0.0 (9.0.24.52809), X64 RyuJIT AVX2 - .NET Framework 4.6.2 : .NET Framework 4.8.1 (4.8.9282.0), X64 RyuJIT VectorSize=256 + .NET Framework 4.8.1 : .NET Framework 4.8.1 (4.8.9282.0), X64 RyuJIT VectorSize=256 ---- ifdef::env-github,rspecator-view[] From 444c23805989cba3ebdd79d578388f8f6626f596 Mon Sep 17 00:00:00 2001 From: Rudy Regazzoni <110470341+rudy-regazzoni-sonarsource@users.noreply.github.com> Date: Wed, 8 Jan 2025 15:19:38 +0100 Subject: [PATCH 19/21] SONARIAC-1856 Modify S7019: add EXEC alternatives and exceptions (#4597) * SONARIAC-1856 Update S7019 content * Remove script example * Fix id * Update rules/S7019/docker/rule.adoc Co-authored-by: Jonas Wielage * Address review comment --------- Co-authored-by: Jonas Wielage --- rules/S7019/docker/rule.adoc | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/rules/S7019/docker/rule.adoc b/rules/S7019/docker/rule.adoc index b58061fac80..5afb753b2b5 100644 --- a/rules/S7019/docker/rule.adoc +++ b/rules/S7019/docker/rule.adoc @@ -10,6 +10,17 @@ This can cause problems when trying to gracefully stop containers because the ma Moreover, the exec form provides more control and predictability over the execution of the command. It does not invoke a command shell, which means it does not have the potential side effects of shell processing. + +=== Exceptions + +The exec form does not allow shell features such as variable expansion, piping (`|`) and command chaining (`&&`, `||`, `;`). +In case you need to use these features, there are a few alternatives: +* Creation of a wrapper script +* Explicitly specify the shell to use with the `SHELL` instruction before `CMD` or `ENTRYPOINT` + + +This rule will not raise an issue if the `SHELL` instruction is used before the `CMD` or `ENTRYPOINT` instruction, as we consider this a conscious decision. + == How to fix it === Code examples @@ -22,6 +33,14 @@ FROM scratch ENTRYPOINT echo "Welcome!" ---- +[source,docker,diff-id=2,diff-type=noncompliant] +---- +FROM scratch +ENTRYPOINT echo "Long script with chaining commands" \ + && echo "Welcome!" \ + && echo "Goodbye" +---- + ==== Compliant solution [source,docker,diff-id=1,diff-type=compliant] @@ -30,6 +49,21 @@ FROM scratch ENTRYPOINT ["echo", "Welcome!"] ---- +[source,docker,diff-id=2,diff-type=compliant] +---- +FROM scratch +SHELL ["/bin/bash", "-c"] +ENTRYPOINT echo "Long script with chaining commands" \ + && echo "Welcome!" \ + && echo "Goodbye" +---- + +[source,docker,diff-id=2,diff-type=compliant] +---- +FROM scratch +ENTRYPOINT ["/entrypoint.sh"] +---- + == Resources === Documentation From 4be8383d8918080032885d43738143ed857a222e Mon Sep 17 00:00:00 2001 From: Martin Strecker <103252490+martin-strecker-sonarsource@users.noreply.github.com> Date: Wed, 8 Jan 2025 17:25:48 +0100 Subject: [PATCH 20/21] NET-943 Modify rule S1215: Add benchmark (#4601) * Add benchmark * Update rules/S1215/csharp/rule.adoc Co-authored-by: Sebastien Marichal * Review --------- Co-authored-by: Sebastien Marichal --- rules/S1215/csharp/rule.adoc | 78 ++++++++++++++++++++++++++++++++++++ 1 file changed, 78 insertions(+) diff --git a/rules/S1215/csharp/rule.adoc b/rules/S1215/csharp/rule.adoc index ff4480ba20a..4e749a942db 100644 --- a/rules/S1215/csharp/rule.adoc +++ b/rules/S1215/csharp/rule.adoc @@ -29,4 +29,82 @@ There may be exceptions to this rule: for example, you've just triggered some ev * https://learn.microsoft.com/en-us/dotnet/standard/garbage-collection/latency[Garbage collection latency modes] * https://learn.microsoft.com/en-us/dotnet/standard/garbage-collection/performance#troubleshoot-performance-issues[Garbage collection troubleshoot performance issues] +=== Benchmarks + +Each .NET runtime features distinct implementations, modes, and configurations for its garbage collector. +The benchmark below illustrates how invoking `GC.Collect()` can have opposite effects across different runtimes. + +[options="header"] +|=== +| Runtime | Collect | Mean | Standard Deviation | Allocated +| .NET 9.0 | False | 659.2 ms | 15.69 ms | 205.95 MB +| .NET 9.0 | True | 888.8 ms | 15.34 ms | 205.95 MB +| | | | | +| .NET Framework 4.8.1 | False | 545.7 ms | 19.49 ms | 228.8 MB +| .NET Framework 4.8.1 | True | 484.8 ms | 11.79 ms | 228.8 MB +|=== + +==== Glossary + +* Collect - if `True`, `GC.Collect()` is called in the middle of the allocation heavy `Benchmark()` method +* https://en.wikipedia.org/wiki/Arithmetic_mean[Mean] +* https://en.wikipedia.org/wiki/Standard_deviation[Standard Deviation] +* https://github.com/dotnet/BenchmarkDotNet/blob/master/docs/articles/configs/diagnosers.md[Allocated] + +The results were generated by running the following snippet with https://github.com/dotnet/BenchmarkDotNet[BenchmarkDotNet]: + +[source,csharp] +---- +class Tree +{ + public List Children = new(); +} + +private void AppendToTree(Tree tree, int childsPerTree, int depth) +{ + if (depth == 0) + { + return; + } + for (int i = 0; i < childsPerTree; i++) + { + var child = new Tree(); + tree.Children.Add(child); + AppendToTree(child, childsPerTree, depth - 1); + } +} + +[Benchmark] +[Arguments(true)] +[Arguments(false)] +public void Benchmark(bool collect) +{ + var tree = new Tree(); + AppendToTree(tree, 8, 7); // Create 8^7 Tree objects (2.097.152 objects) linked via List Children + GC.Collect(); + GC.Collect(); // Move the objects to generation 2 + AppendToTree(new Tree(), 8, 6); // Add some more memory preasure (8^6 262.144 objects) which can be collected right after this call + tree = null; // Remove all references to the tree and its content. This freees up 8^7 Tree objects (2.097.152 objects) + if (collect) + { + GC.Collect(); // Force GC to run and block until it finishes + } + AppendToTree(new Tree(), 3, 10); // Do some more allocations (3^10 = 59.049) + AppendToTree(new Tree(), 4, 7); // 4^10 = 1.048.576 + AppendToTree(new Tree(), 5, 7); // 5^7 = 78.125 + GC.Collect(); // Collect all the memory allocated in this method +} +---- + +Hardware configuration: + +[source] +---- +BenchmarkDotNet v0.14.0, Windows 10 (10.0.19045.5247/22H2/2022Update) +Intel Core Ultra 7 165H, 1 CPU, 22 logical and 16 physical cores + [Host] : .NET Framework 4.8.1 (4.8.9282.0), X64 RyuJIT VectorSize=256 + .NET 9.0 : .NET 9.0.0 (9.0.24.52809), X64 RyuJIT AVX2 + .NET Framework 4.8.1 : .NET Framework 4.8.1 (4.8.9282.0), X64 RyuJIT VectorSize=256 +---- + include::rspecator.adoc[] \ No newline at end of file From e39e8bb76deb9bc6d774cddacd7e1a539e28fe42 Mon Sep 17 00:00:00 2001 From: Sebastien Marichal Date: Wed, 8 Jan 2025 17:31:17 +0100 Subject: [PATCH 21/21] NET-938 Modify S2629: Add benchmarks (#4602) --- rules/S2629/csharp/rule.adoc | 179 ++++++++++++++++++++++++++++++++++- 1 file changed, 178 insertions(+), 1 deletion(-) diff --git a/rules/S2629/csharp/rule.adoc b/rules/S2629/csharp/rule.adoc index 7f4a371d9f9..fa281a8f3e8 100644 --- a/rules/S2629/csharp/rule.adoc +++ b/rules/S2629/csharp/rule.adoc @@ -38,4 +38,181 @@ logger.DebugFormat("The value of the parameter is: {Parameter}.", parameter); === Documentation -* Microsoft Learn - https://learn.microsoft.com/en-us/dotnet/api/system.runtime.compilerservices.interpolatedstringhandlerattribute[InterpolatedStringHandlerArgumentAttribute] \ No newline at end of file +* Microsoft Learn - https://learn.microsoft.com/en-us/dotnet/api/system.runtime.compilerservices.interpolatedstringhandlerattribute[InterpolatedStringHandlerArgumentAttribute] + +=== Benchmarks + +[options="header"] +|=== +| Method | Runtime | Mean | Standard Deviation | Allocated +| | | | | +| CastleCoreLoggingTemplateNotConstant | .NET 9.0 | 230.306 us | 2.7116 us | 479200 B +| CastleCoreLoggingTemplateConstant | .NET 9.0 | 46.827 us | 1.4743 us | 560000 B +| CastleCoreLoggingTemplateNotConstant | .NET Framework 4.8.1 | 1,060.413 us | 32.3559 us | 1115276 B +| CastleCoreLoggingTemplateConstant | .NET Framework 4.8.1 | 93.697 us | 1.8201 us | 561650 B +| | | | | +| MSLoggingTemplateNotConstant | .NET 9.0 | 333.246 us | 12.9214 us | 479200 B +| MSLoggingTemplateConstant | .NET 9.0 | 441.118 us | 68.7999 us | 560000 B +| MSLoggingTemplateNotConstant | .NET Framework 4.8.1 | 1,542.076 us | 99.3423 us | 1115276 B +| MSLoggingTemplateConstant | .NET Framework 4.8.1 | 698.071 us | 18.6319 us | 561653 B +| | | | | +| NLogLoggingTemplateNotConstant | .NET 9.0 | 178.789 us | 9.2528 us | 479200 B +| NLogLoggingTemplateConstant | .NET 9.0 | 6.009 us | 1.3303 us | - +| NLogLoggingTemplateNotConstant | .NET Framework 4.8.1 | 1,086.260 us | 44.1670 us | 1115276 B +| NLogLoggingTemplateConstant | .NET Framework 4.8.1 | 25.132 us | 0.5666 us | - +| | | | | +| SerilogLoggingTemplateNotConstant | .NET 9.0 | 234.460 us | 7.4831 us | 479200 B +| SerilogLoggingTemplateConstant | .NET 9.0 | 49.854 us | 1.8232 us | - +| SerilogLoggingTemplateNotConstant | .NET Framework 4.8.1 | 1,103.939 us | 47.0203 us | 1115276 B +| SerilogLoggingTemplateConstant | .NET Framework 4.8.1 | 35.752 us | 0.6022 us | - +| | | | | +| Log4NetLoggingTemplateNotConstant | .NET 9.0 | 255.754 us | 5.6046 us | 479200 B +| Log4NetLoggingTemplateConstant | .NET 9.0 | 46.425 us | 1.7087 us | 240000 B +| Log4NetLoggingTemplateNotConstant | .NET Framework 4.8.1 | 1,109.874 us | 23.4388 us | 1115276 B +| Log4NetLoggingTemplateConstant | .NET Framework 4.8.1 | 92.305 us | 2.4161 us | 240707 B +|=== + + + +==== Glossary + +* https://en.wikipedia.org/wiki/Arithmetic_mean[Mean] +* https://en.wikipedia.org/wiki/Standard_deviation[Standard Deviation] + +The results were generated by running the following snippet with https://github.com/dotnet/BenchmarkDotNet[BenchmarkDotNet]: + +[source,csharp] +---- +using Microsoft.Extensions.Logging; +using ILogger = Microsoft.Extensions.Logging.ILogger; + +[Params(10_000)] +public int Iterations; + +private ILogger ms_logger; +private Castle.Core.Logging.ILogger cc_logger; +private log4net.ILog l4n_logger; +private Serilog.ILogger se_logger; +private NLog.ILogger nl_logger; + +[GlobalSetup] +public void GlobalSetup() +{ + ms_logger = new LoggerFactory().CreateLogger(); + cc_logger = new Castle.Core.Logging.NullLogFactory().Create("Castle.Core.Logging"); + l4n_logger = log4net.LogManager.GetLogger(typeof(LoggingTemplates)); + se_logger = Serilog.Log.Logger; + nl_logger = NLog.LogManager.GetLogger("NLog"); +} + +[BenchmarkCategory("Microsoft.Extensions.Logging")] +[Benchmark] +public void MSLoggingTemplateNotConstant() +{ + for (int i = 0; i < Iterations; i++) + { + ms_logger.LogInformation($"Param: {i}"); + } +} + +[BenchmarkCategory("Microsoft.Extensions.Logging")] +[Benchmark] +public void MSLoggingTemplateConstant() +{ + for (int i = 0; i < Iterations; i++) + { + ms_logger.LogInformation("Param: {Parameter}", i); + } +} + +[BenchmarkCategory("Castle.Core.Logging")] +[Benchmark] +public void CastleCoreLoggingTemplateNotConstant() +{ + for (int i = 0; i < Iterations; i++) + { + cc_logger.Info($"Param: {i}"); + } +} + +[BenchmarkCategory("Castle.Core.Logging")] +[Benchmark] +public void CastleCoreLoggingTemplateConstant() +{ + for (int i = 0; i < Iterations; i++) + { + cc_logger.InfoFormat("Param: {Parameter}", i); + } +} + +[BenchmarkCategory("log4net")] +[Benchmark] +public void Log4NetLoggingTemplateNotConstant() +{ + for (int i = 0; i < Iterations; i++) + { + l4n_logger.Info($"Param: {i}"); + } +} + +[BenchmarkCategory("log4net")] +[Benchmark] +public void Log4NetLoggingTemplateConstant() +{ + for (int i = 0; i < Iterations; i++) + { + l4n_logger.InfoFormat("Param: {Parameter}", i); + } +} + +[BenchmarkCategory("Serilog")] +[Benchmark] +public void SerilogLoggingTemplateNotConstant() +{ + for (int i = 0; i < Iterations; i++) + { + se_logger.Information($"Param: {i}"); + } +} + +[BenchmarkCategory("Serilog")] +[Benchmark] +public void SerilogLoggingTemplateConstant() +{ + for (int i = 0; i < Iterations; i++) + { + se_logger.Information("Param: {Parameter}", i); + } +} + +[BenchmarkCategory("NLog")] +[Benchmark] +public void NLogLoggingTemplateNotConstant() +{ + for (int i = 0; i < Iterations; i++) + { + nl_logger.Info($"Param: {i}"); + } +} + +[BenchmarkCategory("NLog")] +[Benchmark] +public void NLogLoggingTemplateConstant() +{ + for (int i = 0; i < Iterations; i++) + { + nl_logger.Info("Param: {Parameter}", i); + } +} +---- + +Hardware Configuration: + +[source] +---- +BenchmarkDotNet v0.14.0, Windows 10 (10.0.19045.5247/22H2/2022Update) +12th Gen Intel Core i7-12800H, 1 CPU, 20 logical and 14 physical cores + [Host] : .NET Framework 4.8.1 (4.8.9282.0), X64 RyuJIT VectorSize=256 + .NET 9.0 : .NET 9.0.0 (9.0.24.52809), X64 RyuJIT AVX2 + .NET Framework 4.8.1 : .NET Framework 4.8.1 (4.8.9282.0), X64 RyuJIT VectorSize=256 +----