SEI CERT Oracle Coding Standard for Java

Space shortcuts

Page tree

According to The Java Language Specification, §12.5, "Creation of New Class Instances" [JLS 2015]:

Unlike C++, the Java programming language does not specify altered rules for method dispatch during the creation of a new class instance. If methods are invoked that are overridden in subclasses in the object being initialized, then these overriding methods are used, even before the new object is completely initialized.

Invocation of an overridable method during object construction may result in the use of uninitialized data, leading to runtime exceptions or to unanticipated outcomes. Calling overridable methods from constructors can also leak the this reference before object construction is complete, potentially exposing uninitialized or inconsistent data to other threads (see TSM01-J. Do not let the this reference escape during object construction for additional information). As a result, a class's constructor must invoke (directly or indirectly) only methods in that class that are static, final or private.

Noncompliant Code Example

This noncompliant code example results in the use of uninitialized data by the doLogic() method:

class SuperClass {
  public SuperClass () {
    doLogic();
  }

  public void doLogic() {
    System.out.println("This is superclass!");
  }
}

class SubClass extends SuperClass {
  private String color = "red";

  public void doLogic() {
    System.out.println("This is subclass! The color is :" + color);
    // ...
  }
}

public class Overridable {
  public static void main(String[] args) {
    SuperClass bc = new SuperClass();
    // Prints "This is superclass!"
    SuperClass sc = new SubClass();
    // Prints "This is subclass! The color is :null"
  }
}

The doLogic() method is invoked from the superclass's constructor. When the superclass is constructed directly, the doLogic() method in the superclass is invoked and executes successfully. However, when the subclass initiates the superclass's construction, the subclass's doLogic() method is invoked instead. In this case, the value of color is still null because the subclass's constructor has not yet concluded.

Compliant Solution

This compliant solution declares the doLogic() method as final so that it cannot be overridden:

class SuperClass {
  public SuperClass() {
    doLogic();
  }

  public final void doLogic() {
    System.out.println("This is superclass!");
  }
}

Risk Assessment

Allowing a constructor to call overridable methods can provide an attacker with access to the this reference before an object is fully initialized, which could lead to a vulnerability.

Rule

Severity

Likelihood

Remediation Cost

Priority

Level

MET05-J

Medium

Probable

Medium

P8

L2

Automated Detection

Automated detection of constructors that contain invocations of overridable methods is straightforward.

ToolVersionCheckerDescription
PVS-Studio

7.33

V6052
SonarQube
9.9
S1699Constructors should only call non-overridable methods
SpotBugs

4.6.0

MC_OVERRIDABLE_METHOD_CALL_IN_CONSTRUCTORImplemented (since 4.5.0)


Related Guidelines

ISO/IEC TR 24772:2010

Inheritance [RIP]

Secure Coding Guidelines for Java SE, Version 5.0

Guideline 7-4 / OBJECT-4: Prevent constructors from calling methods that can be overridden

Bibliography

[ESA 2005]

Rule 62, Do not call nonfinal methods from within a constructor

[JLS 2015]

Chapter 8, "Classes"
§12.5, "Creation of New Class Instances"

[Rogue 2000]

Rule 81, Do not call non-final methods from within a constructor


19 Comments

  1. Wellington Vita

    Hi,

    I´d test this compliant example adding the solution to the noncompliante code example, but, I got a error message

    "Cannot override the final method from BaseClass" when I declare doLogic() method as a final. So the value returned by both codes is the same:

    This is super-class!

    This is sub-class! The color is :null

    Please verify what is wrong with this example.

    Regards,

    1. Dhruv Mohindra

      The purpose of this rule is to ensure that overridable methods are not called from the constructor of the superclass. That's why the compliant solution prohibits this by declaring the doLogic method final in the superclass which throws up a compilation error when you try to override it in the subclass. The other solution can be to not call the method doLogic at all but that would be equivalent to chaining the computer and submerging it in the ocean.

      To follow the described problem in the noncompliant example think like this: main() initiates the subclass's constructor by polymorphism, which in turn initiates the superclass's constructor as the keyword super is used. But the superclass's constructor calls the suclass's doLogic method (instead of its own) since this method is overidden. But since initialization of the subclass has not concluded, color has not been set to red. Thus the subclass's method prints null.

  2. David Svoboda

    I believe this rule also applies to finalizers, right? Might be worthwhile to add a NCCE/CS pair with finalizers, too.

    1. Dhruv Mohindra

      Sorry, I am slightly unsure about what you meant. We have a recommendation that says 'avoid using finalizers'. I don't know if anyone would be confused enough to call the overrideable Object.finalize() or similarly say the protected clone() method (well actually, this could be used for defensively copying the arguments so might be an exception to this rule) or a public readObject() (serialization) from within a constructor...to violate this rule. But that could be a bad assumption. (smile)

      1. David Svoboda

        This rule exists because while a Base class constructor is being called for a Derived class, the object is still a Base rather than a Derived. So method calls that would normally be handled by Derived methods are handled by base Methods instead.

        I believe finalizers have the same problem...when a Derived object is finalized, the Derived finalizer is invoked, then the Base finalizer is invoked. During Base.finalize(), the object is a Base and not a Derived, consequently Base methods will be invoked rather than overriding Derived methods.

        I am aware of the rec to not use finalizers; it is good, but is only a recommendation. Which means you need to either make it into a rule, or teach people how to write finalizers securely. I recommend the latter (smile)
        But in doing so, do mention the 'don't use finalizers' rec.

        1. Dhruv Mohindra

          OK, you are stating the reverse problem. The current NCE/CS show that a subclass's method is invoked by mistake whereas a superclass wanted to invoke its own. As per your comment, if it is desirable to invoke the subclass's overridden methods, the superclass's methods will get called instead.

          Based on that deduction, a few questions -
          The overridden finalize() in the subclass has to manually call super.finalize() in a finally block.

          @Override protected void finalize() throws Throwable {
  try {
    ... // Finalize subclass state
  } finally {
    super.finalize();
  }
}

          Assuming that it does, when Base.finalize() is in execution, do you mean that this method will call some other overridable method but the Base class's version will get executed instead of the subclass'? If so, we need to ask whether we really want the subclass's overridden method to be called (which has already been finalized, though there is no guarantee on when it will be which brings up the case of whether according to the given NCE/CS pair, whether a base finalizer will in fact call the overridden version of a method in the subclass because the subclass is still live)

          I think we also need a few rules about good/proper uses of finalizers, as you suggest.

          1. David Svoboda

            In C++ a common error is calling virtual functions in a Base class constructor or destructor. The programmer usually assumes the Derived virtual function is the one that executes, since it would execute when invoked from other code. But in a base class ctor or dtor, the Base function gets called rather than the derived function. I believe the same is true in Java, n'est-ce pas?

            I'm not sure about finalizers though...are you telling me that a Derived class finalizer must explicitly call the Base class finalizer, lest it not get called at all? That would be significantly different than C++. That would mean the Base class finalizer can still invoked methods from the Derived class. What does the specification say on this issue?

            1. Dhruv Mohindra

              main() initiates the subclass's constructor by polymorphism, which in turn initiates the superclass's constructor as the keyword super is used. But the superclass's constructor calls the suclass's doLogic method (instead of its own) since this method is overidden. If main() was to call the superclass's constructor then the latter would invoke the superclass methods. So same is true for Java.

              Yes, the superclass's finalizer has to be explicitly called as I showed. It is best not to consider a finalizer to be a destructor and compare with c++.

              The point you raised gave me the idea that the subclass may still be live since there is no guarantee that a finalize will be called timely. But since you are calling a method super.finalize, which suppose contains a call to an overridable method doSomething(), it would invoke the subclass' doSomething() implementation. That means that this rule is not just constructor specific but is also applicable to all cases where a superclass method is invoked explicitly (super.doSomething()) by using polymorphism (BaseClass sc = new SubClass();) and contains a call to overridable methods.

              For finalizers, I wrote some sample code -

              class BaseClass {
  public BaseClass() {
    doLogic();   
  }
	
  public void doLogic() {
    System.out.println("This is super-class!");
    try {
	  this.finalize();
    } catch (Throwable e) { }
  }
}

class SubClass extends BaseClass {

  public SubClass() {
    super.doLogic();
  }

  protected void finalize() throws Throwable {
    System.out.println("subclass finalize!");
  }
	
  public void doLogic() {
    System.out.println("This is sub-class!"); 
  }
}

public class Overridable {
  public static void main(String[] args) {
    BaseClass sc = new SubClass();  		
  }
}

              This prints:

              This is sub-class!
This is super-class!
subclass finalize!

              and finally the proper idiom -

              class BaseClass {
  public BaseClass() {  }

  protected void finalize() throws Throwable {
    System.out.println("superclass finalize!");
    doLogic(); // oops, calls subclass' (supposedly non existent) doLogic()
  }

  public void doLogic() {
    System.out.println("This is super-class!");
  }
}

class SubClass extends BaseClass {

  public SubClass() throws Throwable {
    try {
      this.finalize(); 		
    } finally {
      super.finalize();
    }
  }

  protected void finalize() throws Throwable {
    System.out.println("subclass finalize!");
  }
	
  public void doLogic() {
    System.out.println("This is sub-class!"); 
  }
}

public class Overridable {
  public static void main(String[] args) {
    try {
      BaseClass sc = new SubClass();
    } catch (Throwable e) { }  		
  }
}

              prints, as I expected:

              subclass finalize!
superclass finalize!
This is sub-class! // boy, you should have been finalized by now

              If you move out the finalization calls from the constructor to the doLogic() method of the subclass and invoke sc.doLogic() from main(), this condition still holds until the subclass actually gets garbage collected. (prints subclass finalize!, superclass finalize! in succession several times)

              1. David Svoboda

                I took out the xref between this rule and the c++ rule about calling virtual methods in the ctor & dtor. C++ and Java are distinct in how ctors and dtors/finalizers work that this rule and the c++ rule are not really equivalent; although they be about similar subjects.

              2. David Svoboda

                BTW is it OK to call this.finalize within a class method?
                (it's usually not OK to call a class dtor explicitly in C++)

                1. Dhruv Mohindra

                  Calling this.finalize is usually a pretty bad idea, even misleading, (so is System.runFinalization() or System.runFinalizersOnExit(true) ) be it from a class method or any other place. The subclass' finalizer will be automatically called before the garbage collection. It will close whatever it needs to in its try block and then do a super.finalize() in the finally block. But since the superclass's finalize() contains a call to an overridable method, the subclass' version will get called if the latter was not garbage collected. This would also mean that the subclass is now live and the finalizer would not run on it a second time. The following code explains this better:

                  class BaseClass {
  protected void finalize() throws Throwable {
    System.out.println("superclass finalize!");
    doLogic();
  }

  public void doLogic() throws Throwable{
    System.out.println("This is super-class!");
  }
}

class SubClass extends BaseClass {
  
  protected void finalize() throws Throwable {
    System.out.println("subclass finalize!");
    try {
      //  pretend to cleanup here 				
    } finally {
      super.finalize();
    }
  }
	
  public void doLogic() throws Throwable{
    System.out.println("This is sub-class!");
    System.runFinalizersOnExit(true); // artificially simulate finalization (bad!)
  }
}

public class Overridable {
  public static void main(String[] args) {
    try {
      BaseClass bc = new SubClass();
      bc.doLogic();
    } catch (Throwable e) {	}  		
  }
}

                  OUTPUT:

                  This is sub-class!
subclass finalize!
superclass finalize!
This is sub-class! // subclass is live
// program terminates as the finalizer won't be called again

                  If that's convincing, I could add an NCE/CS to this rule.

                  1. David Svoboda

                    Your code sample is revealing about the hazards of finalization, Dhruv. It should be simplified to illustrate just one point...prob the point that BaseClass.finalize() actually invokes SubClass.doLogic() because the subclass still exists upon base class finalization. Suggest this code example:

                    class BaseClass {
  protected void finalize() throws Throwable {
    System.out.println("superclass finalize!");
    doLogic();
  }

  public void doLogic() throws Throwable{
    System.out.println("This is super-class!");
  }
}

class SubClass extends BaseClass {
  
  protected void finalize() throws Throwable {
    System.out.println("subclass finalize!");
    try {
      //  pretend to cleanup here 				
    } finally {
      super.finalize();
    }
  }
	
  public void doLogic() throws Throwable{
    System.out.println("This is sub-class!");
  }
}

public class Baz {
  public static void main(String[] args) {
    try {
      BaseClass bc = new SubClass();
      System.runFinalizersOnExit(true); // artificially simulate finalization (bad!)
    } catch (Throwable e) {/* handle error */}  		
  }
}

                    subclass finalize!
                    superclass finalize!
                    This is sub-class!

                    1. Dhruv Mohindra

                      Much clearer. Thanks.

  3. Robert Seacord (Manager)

    I deleted this paragraph:

    In addition to constructors, do not call overridable methods from the clone(), readObject() and readObjectNoData() methods as it would allow attackers to obtain partially initialized instances of classes. (See guidelines MET07-J. Do not invoke overridable methods on the clone under construction and SER11-J. Do not invoke overridable methods from the readObject method.) It is also insecure to call an overridden method from the finalize() method. This can prolong the subclass' life and in fact, render the finalization call useless (see the example in guideline MET18-J. Avoid using finalizers.) Additionally, if the subclass's finalizer has terminated key resources, invoking its methods from the superclass may lead one to observe the object in an inconsistent state and in the worst case result in a NullPointerException.

    I can see some value in references, but this is just repeating information that is better presented elsewhere.

  4. Axel Hauschulte

    From my point of view the advice

    As a result, constructors must invoke only methods that are final or private.

    is too strict. Static methods cannot be overriden in Java (no runtime polymorphism) as well as final or private methods. So static methods too can be safely called from constructors.

     

    It might be worth mentioning that this rule also implies that overridable methods must not be called from instance initializer blocks and from field initializers. Although this kind of initialization is performed during constructor execution, it might not be obvious it can induce the same potential risks.

    1. David Svoboda

      I think the title is both succinct and technically precise. But I did wordsmith that sentence to include static methods, and to include methods indirectly called from constructors.

  5. Michael

    As a result, constructors must invoke (directly or indirectly) only methods that are static, final or private.

    If invoking methods which are not member functions of the class itself in constructor, does it violate this rule?

    Example:


    class MyTest {
    public void printfTest(){
        System.out.println("This is MyTest.");
    }
};

final class MyTest2 {
    public void printfTest(){
        System.out.println("This is MyTest2.");
    }
};

class MyClass {
    MyTest test = new MyTest();
    MyTest2 test2 = new MyTest2();

    public MyClass() {
        test.printfTest();  //MyTest.printfTest() may be overridden in other classes, does it violate this rule in constructor of MyClass ?
        test2.printfTest(); //MyTest2.printfTest() can't be overridden in other classes, does it violate this rule in constructor of MyClass ? 
    }

    1. David Svoboda

      This rule does not apply to overridable methods of a different class...it is meant to apply only to methods in the same class as the constructor. So your code example complies with this rule. I tweaked the introductory paragraph to be more clear about this.

      1. Maria Urban

        You're rigth