Skip to content

Commit

Permalink
Merge branch 'master' into rule/add-RSPEC-S7165
Browse files Browse the repository at this point in the history
  • Loading branch information
sebastien-andrivet-sonarsource authored Nov 15, 2024
2 parents 2789728 + 9592b44 commit d664aa1
Show file tree
Hide file tree
Showing 37 changed files with 869 additions and 61 deletions.
1 change: 1 addition & 0 deletions frontend/public/covered_rules.json
Original file line number Diff line number Diff line change
Expand Up @@ -3392,6 +3392,7 @@
"S7027": "sonar-architecture 1.0.0.1901",
"S7044": "sonar-security 10.7.0.32997",
"S7091": "sonar-architecture 1.0.0.1901",
"S7134": "sonar-architecture master",
"S818": "sonar-java 4.15.0.12310",
"S864": "sonar-java 4.15.0.12310",
"S881": "sonar-java 4.15.0.12310",
Expand Down
11 changes: 1 addition & 10 deletions rules/S5408/cfamily/rule.adoc
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
== Why is this an issue?

Declaring a function or a static member variable ``++constexpr++`` makes it implicitly inline.

Declaring a function ``++constexpr++`` makes it implicitly inline.

In that situation, explicitly using the ``++inline++`` keyword would be redundant, and might lead to confusion if it's used in some cases but not others. It's better to simply omit it.

Expand All @@ -11,22 +10,14 @@ In that situation, explicitly using the ``++inline++`` keyword would be redundan
[source,cpp]
----
inline constexpr int addOne(int n) { return n+1; } // Noncompliant
struct A {
inline constexpr static int secretNumber = 0; // Noncompliant
};
----


=== Compliant solution

[source,cpp]
----
constexpr int addOne(int n) { return n+1; }
struct A {
constexpr static int secretNumber = 0;
};
----



ifdef::env-github,rspecator-view[]
Expand Down
16 changes: 10 additions & 6 deletions rules/S6249/terraform/rule.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ include::../recommended.adoc[]

No secure policy is attached to this bucket:

[source,terraform]
----
resource "aws_s3_bucket" "mynoncompliantbucket" { # Sensitive
bucket = "mynoncompliantbucketname"
Expand All @@ -16,6 +17,7 @@ resource "aws_s3_bucket" "mynoncompliantbucket" { # Sensitive

A policy is defined but forces only HTTPs communication for some users:

[source,terraform]
----
resource "aws_s3_bucket" "mynoncompliantbucket" { # Sensitive
bucket = "mynoncompliantbucketname"
Expand All @@ -31,13 +33,13 @@ resource "aws_s3_bucket_policy" "mynoncompliantbucketpolicy" {
{
Sid = "HTTPSOnly"
Effect = "Deny"
Principal = [
"arn:aws:iam::123456789123:root"
] # secondary location: only one principal is forced to use https
Principal = {
"AWS": "arn:aws:iam::123456789123:root"
} # secondary location: only one principal is forced to use https
Action = "s3:*"
Resource = [
aws_s3_bucket.mynoncompliantbucketpolicy.arn,
"${aws_s3_bucket.mynoncompliantbucketpolicy.arn}/*",
aws_s3_bucket.mynoncompliantbucket.arn,
"${aws_s3_bucket.mynoncompliantbucket.arn}/*",
]
Condition = {
Bool = {
Expand Down Expand Up @@ -70,7 +72,9 @@ resource "aws_s3_bucket_policy" "mycompliantpolicy" {
{
Sid = "HTTPSOnly"
Effect = "Deny"
Principal = "*"
Principal = {
"AWS": "*"
}
Action = "s3:*"
Resource = [
aws_s3_bucket.mycompliantbucket.arn,
Expand Down
2 changes: 1 addition & 1 deletion rules/S6418/java/rule.adoc
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
:detectson: variables/fields
:detections: variables/fields
:defaultsensibility: 5

include::../description.adoc[]
Expand Down
21 changes: 0 additions & 21 deletions rules/S7130/csharp/metadata.json
Original file line number Diff line number Diff line change
@@ -1,23 +1,2 @@
{
"title": "First/Single should be used instead of FirstOrDefault/SingleOrDefault on collections that are known to be non-empty",
"type": "CODE_SMELL",
"status": "ready",
"remediation": {
"func": "Constant\/Issue",
"constantCost": "1min"
},
"tags": [
],
"defaultSeverity": "Major",
"ruleSpecification": "RSPEC-7130",
"sqKey": "S7130",
"scope": "All",
"defaultQualityProfiles": ["Sonar way"],
"quickfix": "targeted",
"code": {
"impacts": {
"MAINTAINABILITY": "MEDIUM"
},
"attribute": "CLEAR"
}
}
25 changes: 2 additions & 23 deletions rules/S7130/csharp/rule.adoc
Original file line number Diff line number Diff line change
@@ -1,14 +1,4 @@
When working with collections that are known to be non-empty, using https://learn.microsoft.com/en-us/dotnet/api/system.linq.enumerable.first[First] or https://learn.microsoft.com/en-us/dotnet/api/system.linq.enumerable.single[Single] is generally preferred over https://learn.microsoft.com/en-us/dotnet/api/system.linq.enumerable.firstordefault[FirstOrDefault] or https://learn.microsoft.com/en-us/dotnet/api/system.linq.enumerable.singleordefault[SingleOrDefault].

== Why is this an issue?

Using `FirstOrDefault` or `SingleOrDefault` on collections that are known to be non-empty is an issue due to:

* Code Clarity and intent: When you use `FirstOrDefault` or `SingleOrDefault`, it implies that the collection might be empty, which can be misleading if you know it is not. It can be confusing for other developers who read your code, making it harder for them to understand the actual constraints and behavior of the collection. This leads to confusion and harder-to-maintain code.

* Error handling: If the developer's intend is for the collection not to be empty, using `FirstOrDefault` and `SingleOrDefault` can lead to subtle bugs. These methods return a default value (`null` for reference types and `default` for value types) when the collection is empty, potentially causing issues like `NullReferenceException` later in the code. In contrast, `First` or `Single` will throw an `InvalidOperationException` immediately if the collection is empty, making it easier to detect and address issues early in the development process.

* Code coverage: Potentially, having to check if the result is `null`, you introduces a condition that cannot be fully tested, impacting the code coverage.
include::../description-dotnet.adoc[]

=== Code examples

Expand All @@ -30,17 +20,6 @@ var items = new List<int> { 1, 2, 3 };
int firstItem = items.First(); // Compliant
----

== Resources

=== Documentation

* Microsoft Learn - https://learn.microsoft.com/en-us/dotnet/api/system.linq.enumerable.single[`Single`]
* Microsoft Learn - https://learn.microsoft.com/en-us/dotnet/api/system.linq.enumerable.first[`First`]
* Microsoft Learn - https://learn.microsoft.com/en-us/dotnet/api/system.linq.enumerable.singleordefault[`SingleOrDefault`]
* Microsoft Learn - https://learn.microsoft.com/en-us/dotnet/api/system.linq.enumerable.firstordefault[`FirstOrDefault`]

=== Articles & blog posts

* https://medium.com/@anyanwuraphaelc/first-vs-firstordefault-single-vs-singleordefault-a-high-level-look-d24db17a2bc3[First vs FirstOrDefault, Single vs SingleOrDefault: A High-level Look]
include::../resources-dotnet.adoc[]

include::../rspecator.adoc[]
12 changes: 12 additions & 0 deletions rules/S7130/description-dotnet.adoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
When working with collections that are known to be non-empty, using https://learn.microsoft.com/en-us/dotnet/api/system.linq.enumerable.first[First] or https://learn.microsoft.com/en-us/dotnet/api/system.linq.enumerable.single[Single] is generally preferred over https://learn.microsoft.com/en-us/dotnet/api/system.linq.enumerable.firstordefault[FirstOrDefault] or https://learn.microsoft.com/en-us/dotnet/api/system.linq.enumerable.singleordefault[SingleOrDefault].

== Why is this an issue?

Using `FirstOrDefault` or `SingleOrDefault` on collections that are known to be non-empty is an issue due to:

* Code Clarity and intent: When you use `FirstOrDefault` or `SingleOrDefault`, it implies that the collection might be empty, which can be misleading if you know it is not. It can be confusing for other developers who read your code, making it harder for them to understand the actual constraints and behavior of the collection. This leads to confusion and harder-to-maintain code.

* Error handling: If the developer's intend is for the collection not to be empty, using `FirstOrDefault` and `SingleOrDefault` can lead to subtle bugs. These methods return a default value (`null` for reference types and `default` for value types) when the collection is empty, potentially causing issues like `NullReferenceException` later in the code. In contrast, `First` or `Single` will throw an `InvalidOperationException` immediately if the collection is empty, making it easier to detect and address issues early in the development process.

* Code coverage: Potentially, having to check if the result is `null`, you introduces a condition that cannot be fully tested, impacting the code coverage.

21 changes: 21 additions & 0 deletions rules/S7130/metadata.json
Original file line number Diff line number Diff line change
@@ -1,2 +1,23 @@
{
"title": "First/Single should be used instead of FirstOrDefault/SingleOrDefault on collections that are known to be non-empty",
"type": "CODE_SMELL",
"status": "ready",
"remediation": {
"func": "Constant\/Issue",
"constantCost": "1min"
},
"tags": [
],
"defaultSeverity": "Major",
"ruleSpecification": "RSPEC-7130",
"sqKey": "S7130",
"scope": "All",
"defaultQualityProfiles": [ "Sonar way" ],
"quickfix": "targeted",
"code": {
"impacts": {
"MAINTAINABILITY": "MEDIUM"
},
"attribute": "CLEAR"
}
}
13 changes: 13 additions & 0 deletions rules/S7130/resources-dotnet.adoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
== Resources

=== Documentation

* Microsoft Learn - https://learn.microsoft.com/en-us/dotnet/api/system.linq.enumerable.single[`Single`]
* Microsoft Learn - https://learn.microsoft.com/en-us/dotnet/api/system.linq.enumerable.first[`First`]
* Microsoft Learn - https://learn.microsoft.com/en-us/dotnet/api/system.linq.enumerable.singleordefault[`SingleOrDefault`]
* Microsoft Learn - https://learn.microsoft.com/en-us/dotnet/api/system.linq.enumerable.firstordefault[`FirstOrDefault`]

=== Articles & blog posts

* https://medium.com/@anyanwuraphaelc/first-vs-firstordefault-single-vs-singleordefault-a-high-level-look-d24db17a2bc3[First vs FirstOrDefault, Single vs SingleOrDefault: A High-level Look]

2 changes: 2 additions & 0 deletions rules/S7130/vbnet/metadata.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
{
}
25 changes: 25 additions & 0 deletions rules/S7130/vbnet/rule.adoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
include::../description-dotnet.adoc[]

=== Code examples

==== Noncompliant code example

[source,csharp,diff-id=1,diff-type=noncompliant]
----
Dim Items As New list(Of Integer) From {1, 2, 3}
Dim FirstItem As Integer = Items.FirstOrDefault() ' Noncompliant, this implies the collection might be empty, when we know it is not
----

==== Compliant solution

[source,csharp,diff-id=1,diff-type=compliant]
----
Dim Items As New list(Of Integer) From {1, 2, 3}
Dim FirstItem As Integer = Items.First() ' Compliant
----

include::../resources-dotnet.adoc[]

include::../rspecator.adoc[]
23 changes: 23 additions & 0 deletions rules/S7131/csharp/metadata.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
{
"title": "A write lock should not be released when a read lock has been acquired and vice versa",
"type": "BUG",
"status": "ready",
"remediation": {
"func": "Constant\/Issue",
"constantCost": "30min"
},
"tags": [
],
"defaultSeverity": "Major",
"ruleSpecification": "RSPEC-7131",
"sqKey": "S7131",
"scope": "All",
"defaultQualityProfiles": ["Sonar way"],
"quickfix": "infeasible",
"code": {
"impacts": {
"RELIABILITY": "HIGH"
},
"attribute": "LOGICAL"
}
}
100 changes: 100 additions & 0 deletions rules/S7131/csharp/rule.adoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@

When using https://learn.microsoft.com/en-us/dotnet/api/system.threading.readerwriterlock[ReaderWriterLock] and https://learn.microsoft.com/en-us/dotnet/api/system.threading.readerwriterlockslim[ReaderWriterLockSlim] for managing read and write locks, you should not release a read lock while holding a write lock and vice versa, otherwise you might have runtime exceptions.
The locks should be always correctly paired so that the shared resource is accessed safely.

This rule raises if:

* you call https://learn.microsoft.com/en-us/dotnet/api/system.threading.readerwriterlock.acquirewriterlock[ReaderWriterLock.AcquireWriterLock] or https://learn.microsoft.com/en-us/dotnet/api/system.threading.readerwriterlock.upgradetowriterlock[ReaderWriterLock.UpgradeToWriterLock] and then use https://learn.microsoft.com/en-us/dotnet/api/system.threading.readerwriterlock.releasereaderlock[ReaderWriterLock.ReleaseReaderLock]
* you call https://learn.microsoft.com/en-us/dotnet/api/system.threading.readerwriterlockslim.enterwritelock[ReaderWriterLockSlim.EnterWriteLock] or https://learn.microsoft.com/en-us/dotnet/api/system.threading.readerwriterlockslim.tryenterwritelock[ReaderWriterLockSlim.TryEnterWriteLock] and then use https://learn.microsoft.com/en-us/dotnet/api/system.threading.readerwriterlockslim.exitreadlock[ReaderWriterLockSlim.ExitReadLock]
* you call https://learn.microsoft.com/en-us/dotnet/api/system.threading.readerwriterlock.acquirereaderlock[ReaderWriterLock.AcquireReaderLock] or https://learn.microsoft.com/en-us/dotnet/api/system.threading.readerwriterlock.downgradefromwriterlock[ReaderWriterLock.DowngradeFromWriterLock] and then use https://learn.microsoft.com/en-us/dotnet/api/system.threading.readerwriterlock.releasewriterlock[ReaderWriterLock.ReleaseWriterLock]
* or you call https://learn.microsoft.com/en-us/dotnet/api/system.threading.readerwriterlockslim.enterreadlock[ReaderWriterLockSlim.EnterReadLock], https://learn.microsoft.com/en-us/dotnet/api/system.threading.readerwriterlockslim.tryenterreadlock[ReaderWriterLockSlim.TryEnterReadLock], https://learn.microsoft.com/en-us/dotnet/api/system.threading.readerwriterlockslim.enterupgradeablereadlock[ReaderWriterLockSlim.EnterUpgradeableReadLock] or https://learn.microsoft.com/en-us/dotnet/api/system.threading.readerwriterlockslim.tryenterupgradeablereadlock[ReaderWriterLockSlim.TryEnterUpgradeableReadLock] and then use https://learn.microsoft.com/en-us/dotnet/api/system.threading.readerwriterlockslim.exitwritelock[ReaderWriterLockSlim.ExitWriteLock]
== Why is this an issue?

If you use the `ReaderWriterLockSlim` class, you will get a https://learn.microsoft.com/en-us/dotnet/api/system.threading.lockrecursionexception[LockRecursionException].
In the case of `ReaderWriterLock`, you'll get a runtime exception for trying to release a lock that is not owned by the calling thread.


=== Code examples

==== Noncompliant code example

[source,csharp,diff-id=1,diff-type=noncompliant]
----
public class Example
{
private static ReaderWriterLock rwLock = new();
public void Writer()
{
rwLock.AcquireWriterLock(2000);
try
{
// ...
}
finally
{
rwLock.ReleaseReaderLock(); // Noncompliant, will throw runtime exception
}
}
public void Reader()
{
rwLock.AcquireReaderLock(2000);
try
{
// ...
}
finally
{
rwLock.ReleaseWriterLock(); // Noncompliant, will throw runtime exception
}
}
}
----

==== Compliant solution

[source,csharp,diff-id=1,diff-type=compliant]
----
public class Example
{
private static ReaderWriterLock rwLock = new();
public static void Writer()
{
rwLock.AcquireWriterLock(2000);
try
{
// ...
}
finally
{
rwLock.ReleaseWriterLock();
}
}
public static void Reader()
{
rwLock.AcquireReaderLock(2000);
try
{
// ...
}
finally
{
rwLock.ReleaseReaderLock();
}
}
}
----

== Resources

=== Documentation

* Microsoft Learn - https://learn.microsoft.com/en-us/dotnet/api/system.threading.readerwriterlock[ReaderWriterLock Class]
* Microsoft Learn - https://learn.microsoft.com/en-us/dotnet/api/system.threading.readerwriterlockslim[ReaderWriterLockSlim]

include::../rspecator.adoc[]
3 changes: 3 additions & 0 deletions rules/S7131/message.adoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
=== Message

You should not release this [reader/writer] lock when [reader/writer] lock was acquired
2 changes: 2 additions & 0 deletions rules/S7131/metadata.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
{
}
9 changes: 9 additions & 0 deletions rules/S7131/rspecator.adoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
ifdef::env-github,rspecator-view[]

'''
== Implementation Specification
(visible only on this page)

include::message.adoc[]

endif::env-github,rspecator-view[]
2 changes: 2 additions & 0 deletions rules/S7153/metadata.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
{
}
Loading

0 comments on commit d664aa1

Please sign in to comment.