LCK08-J. Ensure actively held locks are released on exceptional conditions #2055
Conversation
gonczmisi
changed the title
| /** | ||
| * The resource is closed (or unlocked, etc). | ||
| */ | ||
| public static final int CLOSED = 3; |
There was a problem hiding this comment.
better to keep public APIs compatible. Can we move new constants after the NONEXISTENT?
There was a problem hiding this comment.
Unfortunately no, because the mergeing of these states are based on a Math.min() call. This would mean, that a CLOSED would override a CLOSED_WITHOUT_OPENED state. Then the information that the lock was not even acquired in the first place would be lost.
Edit: @KengoTODA please resolve the conversation if you are satisfied with my answer, and approve the change this way.
There was a problem hiding this comment.
Hi @KengoTODA could you revisit this PR and check my comment above?
There was a problem hiding this comment.
Then it is a breaking change, and I think only should be merged for 5.0.0, together with #2069, which may cause some merge conflicts with this one. Also, it would be worth to mention the breaking change in the changelog as well.
Sure thing, I will look into them in the next week. Thanks for the feedback 👍 |
spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/FindUnreleasedLockTest.java
Outdated
Show resolved
Hide resolved
spotbugs/src/main/java/edu/umd/cs/findbugs/ba/ResourceValueAnalysis.java
Show resolved
Hide resolved
| @@ -5,6 +5,9 @@ This is the changelog for SpotBugs. This follows [Keep a Changelog v1.0.0](http: | |||
| Currently the versioning policy of this project follows [Semantic Versioning v2.0.0](http://semver.org/spec/v2.0.0.html). | |||
|
|
|||
| ## Unreleased - 2023-??-?? | |||
| ### Added | |||
| * New bug type `CWO_CLOSED_WITHOUT_OPENED` for locks that might be released without even being acquired. (See [SEI CERT rule LCK08-J](https://wiki.sei.cmu.edu/confluence/display/java/LCK08-J.+Ensure+actively+held+locks+are+released+on+exceptional+conditions))([#2055](https://github.com/spotbugs/spotbugs/pull/2055)) | |||
|
|
|||
There was a problem hiding this comment.
There is already an Added section in Unreleased. Can you please merge the two?
| <
This PR is about SEI CERT rule: LCK08
First of all, the first part of the rule is unreleased locks on exceptional conditions. This part is already solved by a detector:
FindUnreleasedLock. I wrote a unit test to demonstrate it.The second part of the rule, is to try to catch exceptional conditions, which could end up unlocking a resource, without even locking it in the first place.
My PR is about this issue.
As mentioned, there is already a detector for unreleased locks, so I extended that one:
COW_CLOSED_WITHOUT_OPENEDI ran the new build on the RxJava source code to test if the new feature in
FindUnreleasedLockwould provide a lot of false positives or not.No bug with type
CWO_CLOSED_WITHOUT_OPENEDwas reported.