Calling overridable methods from the clone()
method is dangerous for two reasons. First, a malicious subclass may override the method and affect the behavior of the clone()
method. Second, a trusted subclass may observe the object in an uninitialized state before its construction has concluded. It is possible to leave the clone as well as the object being cloned, in an inconsistent state.
Noncompliant Code Example
This noncompliant code example shows two classes, BadClone
and Sub
. BadClone
calls an overridable method doSomething()
. The overridden method sets the value of the cookies whereas the overriding method sets the values of the domain names. At runtime, because of polymorphism, the doSomething()
method of the subclass Sub
gets executed erroneously. Not only does the subclass see the clone in an inconsistent state, its doSomething()
method modifies it in a way that creates inconsistent copies. This is because the deepCopy()
method occurs after the call to the doSomething()
method and the overriding doSomething()
implementation modifies the object undesirably.
class BadClone implements Cloneable { HttpCookie[] cookies; BadClone(HttpCookie[] c) { cookies = c; } public Object clone() throws CloneNotSupportedException { final BadClone clone = (BadClone) 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 BadClone { Sub(HttpCookie[] c) { super(c); } public Object clone() throws CloneNotSupportedException { final Sub clone = (Sub) super.clone(); clone.doSomething(); return clone; } void doSomething() { // erroneously gets 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); } BadClone bc = new Sub(hc); bc.clone(); } }
If an overridable method is invoked on a shallow copy of the object, the original object is also modified.
Compliant Solution
This compliant solution declares the doSomething()
and deepCopy()
methods final
.
final void doSomething() { // ... } final HttpCookie[] deepCopy() { // ... }
Alternatively, it is permissible to declare the methods private
or to declare the class final
. Eliminating the method calls by congregating the code together is also permissible.
Risk Assessment
Calling overridable methods on the clone under construction, can leave its state or that of the object being cloned, inconsistent.
Rule |
Severity |
Likelihood |
Remediation Cost |
Priority |
Level |
---|---|---|---|---|---|
MET38- J |
medium |
probable |
low |
P12 |
L1 |
Automated Detection
TODO
Related Vulnerabilities
Search for vulnerabilities resulting from the violation of this rule on the CERT website.
Other Languages
This rule appears in the C++ Secure Coding Standard as ARR40-CPP. Use a Valid Ordering Rule.
References
[[Bloch 08]] Item 11: Override clone
judiciously
[[Gong 03]]
MET37-J. Do not call overridable methods from a privileged block 10. Methods (MET) MET35-J. Ensure that the clone method calls super.clone