False positive CT_CONSTRUCTOR_THROW on canonical "Compliant Solution"

[garydgregory]

The bug description for CT_CONSTRUCTOR_THROW at https://spotbugs.readthedocs.io/en/stable/bugDescriptions.html#ct-be-wary-of-letting-constructors-throw-exceptions-ct-constructor-throw says:

CT: Be wary of letting constructors throw exceptions. (CT_CONSTRUCTOR_THROW)[](https://spotbugs.readthedocs.io/en/stable/bugDescriptions.html#ct-be-wary-of-letting-constructors-throw-exceptions-ct-constructor-throw)
Classes that throw exceptions in their constructors are vulnerable to Finalizer attacks

A finalizer attack can be prevented, by declaring the class final, using an empty finalizer declared as final, or by a clever use of a private constructor.

See [SEI CERT Rule OBJ-11](https://wiki.sei.cmu.edu/confluence/display/java/OBJ11-J.+Be+wary+of+letting+constructors+throw+exceptions) for more information.

The link https://wiki.sei.cmu.edu/confluence/display/java/OBJ11-J.+Be+wary+of+letting+constructors+throw+exceptions provides the compliant solution:

public class BankOperations {
    public BankOperations() {
      this(performSSNVerification());
    }
   
    private BankOperations(boolean secure) {
      // secure is always true
      // Constructor without any security checks
    }
   
    private static boolean performSSNVerification() {
      // Returns true if data entered is valid, else throws a SecurityException
      // Assume that the attacker just enters invalid SSN, so this method always throws the exception
      throw new SecurityException("Invalid SSN!");
    }
   
    // ...remainder of BankOperations class definition
  }

which blows up SpotBugs checks:

[ERROR] Medium: Exception thrown in class org.apache.commons.io.BankOperations at new org.apache.commons.io.BankOperations() will leave the constructor. The object under construction remains partially initialized and may be vulnerable to Finalizer attacks. [org.apache.commons.io.BankOperations, org.apache.commons.io.BankOperations] At BankOperations.java:[line 5]At BankOperations.java:[line 5] CT_CONSTRUCTOR_THROW

This example reflects what I see in Commons IO when I try to address 64 such issues with a compliant solution.

[garydgregory]

TY! I'll update Apache Commons Parent when the next release comes out.

[garydgregory]

Hello @iloveeclipse and all:
I'm not sure this is really fixed. Using the latest (4.8.2), if I change the exception in the example from unchecked to checked, it shows up as a CT_CONSTRUCTOR_THROW again:

import java.sql.SQLException;

public class BankOperations {
    public BankOperations() throws SQLException {
      this(performSSNVerification());
    }
   
    private BankOperations(boolean secure) {
      // secure is always true
      // Constructor without any security checks
    }
   
    private static boolean performSSNVerification() throws SQLException {
      // Returns true if data entered is valid, else throws a SecurityException
      // Assume that the attacker just enters invalid SSN, so this method always throws the exception
      throw new SQLException("Invalid SSN!");
    }
   
    // ...remainder of BankOperations class definition
  }

WDYT?

[JuditKnoll]

I think that you are absolutely right, I'm really sorry. Thank you for bringing it up.

[garydgregory]

Hello @JuditKnoll
TY for the update. Looking forward to 4.8.3 😄
Would you please manage expectations regarding a release date window?

[garydgregory]

Will 4.8.3 be published on https://repo1.maven.org/maven2/com/github/spotbugs/spotbugs/ ?

[hazendaz]

@garydgregory Its released now, however, waiting on @JuditKnoll to push it out of sonatype. Should be done probably tonight. Trying to make sure more than just one person now actively maintaining here can do that so we are not stuck waiting. If @JuditKnoll has any issues, I'll finish it off tomorrow evening.