-
Notifications
You must be signed in to change notification settings - Fork 578
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.
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
LCK08-J. Ensure actively held locks are released on exceptional conditions #2055
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution, could you check my comments?
/** | ||
* The resource is closed (or unlocked, etc). | ||
*/ | ||
public static final int CLOSED = 3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better to keep public APIs compatible. Can we move new constants after the NONEXISTENT
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @KengoTODA could you revisit this PR and check my comment above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is already an Added
section in Unreleased. Can you please merge the two?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add more test cases? What happens if the release of the locks is after the try-catch block, outside of it?
<![CDATA[ | ||
<p> This method releases a JSR-166 (<code>java.util.concurrent</code>) lock, | ||
but it is possible that the lock is not even acquired at this point, | ||
due to an exception thrower method before the locking. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add a statement about what the programmer should do, when this bug occurs?
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_OPENED
I ran the new build on the RxJava source code to test if the new feature in
FindUnreleasedLock
would provide a lot of false positives or not.No bug with type
CWO_CLOSED_WITHOUT_OPENED
was reported.