Skip to content

Commit

Permalink
Reviews
Browse files Browse the repository at this point in the history
  • Loading branch information
mary-georgiou-sonarsource committed Oct 30, 2024
1 parent 3d84193 commit 9e1c8f9
Show file tree
Hide file tree
Showing 4 changed files with 87 additions and 25 deletions.
2 changes: 1 addition & 1 deletion rules/S7131/csharp/metadata.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"title": "FIXME",
"title": "You should not release a write lock when a read lock has been acquired",
"type": "BUG",
"status": "ready",
"remediation": {
Expand Down
89 changes: 65 additions & 24 deletions rules/S7131/csharp/rule.adoc
Original file line number Diff line number Diff line change
@@ -1,59 +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 exit a read lock while holding a write lock otherwise you might have unexpected behavior.
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 unexpected behavior.
The locks should be always correctly paired so that the shared resource is accessed safely.

This rule raises if:

* you acquire 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] before releasing the writerlock
* or you acquire 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] before you release it.
* you acquire 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] before you release it
* you acquire 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] before releasing the reader lock
* or you acquire 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] before you release it.
== Why is this an issue?

When you acquire a write lock you indicate that you need exclusive access to the resource, meaning no other thread should be able to read or write to the resource. If you exit a read lock while holding a write lock, you might inadvertently allow other threads to acquire a read lock, leading to race conditions and data corruption.
Additionally, it can lead to https://learn.microsoft.com/en-us/dotnet/core/diagnostics/debug-deadlock[deadlocks] (a situation where two or more threads are waiting indefinitely for each other to release locks) and runtime exceptions (unexpected errors that occur during the execution of a program), as the lock state becomes inconsistent.
Finally, correctly matching lock acquisition and release calls makes the code more transparent and easier to maintain. It ensures that the locking logic is straightforward and less prone to errors.
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]
----
var lock = new ReaderWriterLock();
lock.EnterWriteLock();
try
{
// Write resources
}
finally
class Example
{
Lock.ExitReadLock(); // Noncompliant
private static ReaderWriterLock rwLock = new ReaderWriterLock();
private static int sharedResource = 0;
static void Writer()
{
try
{
rwLock.AcquireWriterLock(2000);
sharedResource++;
}
finally
{
rwLock.ReleaseReaderLock(); // Noncompliant, will throw runtime exception
rwLock.ReleaseWriterLock();
}
}
static void Reader()
{
try
{
rwLock.AcquireReaderLock(2000);
}
finally
{
rwLock.ReleaseWriterLock(); // Noncompliant, will throw runtime exception
rwLock.ReleaseReaderLock();
}
}
}
----

==== Compliant solution

[source,csharp,diff-id=1,diff-type=compliant]
----
var lock = new ReaderWriterLock();
lock.EnterWriteLock();
try
{
// Write resources
}
finally
class Example
{
lock.ExitWriterLock();
private static ReaderWriterLock rwLock = new ReaderWriterLock();
private static int sharedResource = 0;
static void Writer()
{
try
{
rwLock.AcquireWriterLock(2000);
sharedResource++;
}
finally
{
rwLock.ReleaseWriterLock();
}
}
static void Reader()
{
try
{
rwLock.AcquireReaderLock(2000);
}
finally
{
rwLock.ReleaseReaderLock();
}
}
}
----

== Resources

=== Documentation

* Microsoft Learn - https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/statements/lock[The lock statement - ensure exclusive access to a shared resource]
* 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]
4 changes: 4 additions & 0 deletions rules/S7131/message.adoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
=== Message

You should not release this xxx lock as a xxx lock has been acquired.

17 changes: 17 additions & 0 deletions rules/S7131/rspecator.adoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
ifdef::env-github,rspecator-view[]

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

include::message.adoc[]

include::highlighting.adoc[]

'''
== Comments And Links
(visible only on this page)

include::comments-and-links.adoc[]

endif::env-github,rspecator-view[]

0 comments on commit 9e1c8f9

Please sign in to comment.