Skip to end of metadata
Go to start of metadata

Calling overridable methods from the clone() method is insecure. First, a malicious subclass could override the method and affect the behavior of the clone() method. Second, a trusted subclass could observe (and potentially modify) the cloned object in a partially initialized state before its construction has concluded. In either case, the subclass could leave the clone, the object being cloned, or both in an inconsistent state. Consequently, clone() methods may invoke only methods that are final or private. 

This rule is closely related to MET05-J. Ensure that constructors do not call overridable methods.

Noncompliant Code Example

This noncompliant code example shows two classes, CloneExample and Sub. The class CloneExample calls an overridable method doSomething(). The overridden method sets the value of the cookies; the overriding method sets the values of the domain names. The doSomething() method of the subclass Sub is erroneously executed twice at runtime because of polymorphism. The first invocation comes from CloneExample.clone(), and the other comes from Sub.clone(). Consequently, the values of the cookies are never initialized, whereas the domains are initialized twice.

Furthermore, the subclass not only sees the clone in an inconsistent state but also modifies the clone in a manner that creates inconsistent copies. This is because the deepCopy() method occurs after the call to the doSomething() method, and the overriding doSomething() implementation erroneously modifies the object.

class CloneExample implements Cloneable {
  HttpCookie[] cookies;

  CloneExample(HttpCookie[] c) {
    cookies = c;
  }

  public Object clone() throws CloneNotSupportedException {
    final CloneExample clone = (CloneExample) super.clone();
    clone.doSomething(); // Invokes overridable method
    clone.cookies = clone.deepCopy();
    return clone;
  }

  void doSomething() { // Overridable
    for (int i = 0; i < cookies.length; i++) {
      cookies[i].setValue("" + i);
    }
  }

  HttpCookie[] deepCopy() {
    if (cookies == null) {
      throw new NullPointerException();
    }

    // Deep copy
    HttpCookie[] cookiesCopy = new HttpCookie[cookies.length];

    for (int i = 0; i < cookies.length; i++) {
      // Manually create a copy of each element in array
      cookiesCopy[i] = (HttpCookie) cookies[i].clone();
    }
    return cookiesCopy;
  }
}

class Sub extends CloneExample {
  Sub(HttpCookie[] c) {
    super(c);
  }

  public Object clone() throws CloneNotSupportedException {
    final Sub clone = (Sub) super.clone();
    clone.doSomething();
    return clone;
  }

  void doSomething() { // Erroneously executed
    for (int i = 0;i < cookies.length; i++) {
      cookies[i].setDomain(i + ".foo.com");
    }
  }

  public static void main(String[] args)
      throws CloneNotSupportedException {
    HttpCookie[] hc = new HttpCookie[20];
    for (int i = 0 ; i < hc.length; i++){
      hc[i] = new HttpCookie("cookie" + i,"" + i);
    }
    CloneExample bc = new Sub(hc);
    bc.clone();
  }
}

When an overridable method is invoked on a shallow copy of the object, the original object is also modified.

Compliant Solution

This compliant solution declares both the doSomething() and the deepCopy() methods final, preventing overriding of these methods:

class CloneExample implements Cloneable {
  final void doSomething() {
    // ...
  }
  final HttpCookie[] deepCopy() {
    // ...
  }

  // ...
}

Alternative solutions that prevent invocation of overridden methods include declaring these methods private or final or declaring the class containing these methods final.

Exceptions

MET06-J-EX0: It is permitted to call a superclass's method via super.method(...), since such calls will not be dynamically dispatched to methods defined by a subclass. In fact, calling super.clone() is expected behavior.

Risk Assessment

Calling overridable methods on the clone under construction can expose class internals to malicious code or violate class invariants by exposing the clone to trusted code in a partially initialized state, affording the opportunity to corrupt the state of the clone, the object being cloned, or both.

Rule

Severity

Likelihood

Remediation Cost

Priority

Level

MET06-J

Medium

Probable

Low

P12

L1

Automated Detection

Automated detection is straightforward.

Bibliography

[Bloch 2008]

Item 11, "Override clone Judiciously"

[Gong 2003]

 

 


2 Comments

  1. This guidelines is very similar to MET04-J. Ensure that constructors do not call overridable methods

    It could be combined, but minimally should be collocated.

    rCs

  2. There are two problems stated (1) a malicious subclass and (2) a trusted subclass.

    For (1) the solution must be that methods invoked by the clone() method must be final.

    For (2) the solution can just be that these classes are not overridden, or even possibly that these overridden classes don't break any invariants. Disallowing them to be overridden is more analyzable / enforceable if perhaps overly strict.