Skip to content
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

False positive OBL_UNSATIFIED_OBLIGATION with try with resources #79

Closed
jsotuyod opened this issue Dec 7, 2016 · 2 comments
Closed
Labels

Comments

@jsotuyod
Copy link
Member

jsotuyod commented Dec 7, 2016

Local issue to track findbugsproject/findbugs#99

@jsotuyod jsotuyod self-assigned this Dec 8, 2016
@jsotuyod
Copy link
Member Author

jsotuyod commented Dec 8, 2016

The issue can be reproduced and is confirmed. I'll be working on a fix.

@jsotuyod jsotuyod added the bug label Dec 8, 2016
@jsotuyod
Copy link
Member Author

jsotuyod commented Dec 8, 2016

This one is going to be harder to fix than expected. The detector seems to be deeply broken as it's failing on lot of scenarios were it shouldn't.

The database should recognize that closing a Statement closes the ResultSet, 'though the opposite is not currently true:

database.addEntry(new MatchMethodEntry(new SubtypeTypeMatcher(BCELUtil.getObjectTypeInstance("java.sql.Statement")),
        new ExactStringMatcher("close"), new ExactStringMatcher("()V"), false, ObligationPolicyDatabaseActionType.DEL,
        ObligationPolicyDatabaseEntryType.STRONG, statement, resultSet));
database.addEntry(new MatchMethodEntry(new SubtypeTypeMatcher(BCELUtil.getObjectTypeInstance("java.sql.ResultSet")),
         new ExactStringMatcher("close"), new ExactStringMatcher("()V"), false, ObligationPolicyDatabaseActionType.DEL,
         ObligationPolicyDatabaseEntryType.STRONG, resultSet));

However, this is failing even without the use-with-resources construct:

public class Issue79 {
  private static final String QUERY = "";

  public void f(Connection cnx) throws SQLException {
    PreparedStatement st = null;
    ResultSet rs = null;
    try {
      st = cnx.prepareStatement(QUERY);
      rs = st.executeQuery();
      while (rs.next()) {
        System.out.println(rs.getString("ID"));
      }
    } finally {
      /*
       * The statement closes the result set, and there is no scenario where st may be null
       * but not the resultset, however an unsatisfied obligation is reported on the resultset
      */
      if (st != null) {
    	st.close();
      }
    }
  }
}

Also of note, the current implementation can't track which result set is associated with which statement, so when using more than one it can produce lots of false positives and false negatives...

We should probably drop this one for release 4.0.0 and think of a deep refactor for this detector.

@jsotuyod jsotuyod removed their assignment Dec 10, 2016
@jsotuyod jsotuyod added this to the SpotBugs 4.0.0 milestone Dec 10, 2016
@KengoTODA KengoTODA removed this from the SpotBugs 4.0.0 milestone Jan 29, 2020
zhouyinn added a commit to zhouyinn/spotbugs that referenced this issue Oct 20, 2020
gamesh411 pushed a commit to gamesh411/spotbugs that referenced this issue Apr 15, 2021
…h try with resources (spotbugs#1479)

* Fix issue spotbugs#79

* modification according to reviews

* Modify based on requested changes

* Fix | Log.debug format, LoggerFactory usage
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants