clone() non final classes

Question

As per Josh Bloch's Effective java :-

do not use the clone method to make a defensive copy of a parameter whose type is subclassable by untrusted parties.

Now taking a expert from his book only:-

public final class Period {
private final Date start;
private final Date end;

/**
 * @param  start the beginning of the period
 * @param  end the end of the period; must not precede start
 * @throws IllegalArgumentException if start is after end
 * @throws NullPointerException if start or end is null
 */
public Period(Date start, Date end) {
    if (start.compareTo(end) > 0)
        throw new IllegalArgumentException(
            start + " after " + end);
    this.start = start;
    this.end   = end;
}

public Date start() {
    return start;
}
public Date end() {
    return end;
}

...  // Remainder omitted

}

I do not get what wrong would happen if I modify a accesor method to return a copy of date object using clone function instead of copy using constructor like this :-

public Date start() {
    return start.clone();
}

instead of

public Date start() {
       return new Date(start.getTime());
}

How possibly can a instance of malicious subclass be returned?

Duplicate of http://stackoverflow.com/questions/7082553/java-util-date-clone-or-copy-to-not-expose-internal-reference — Ankit Tripathi, Nov 29 '16 at 10:02
As Date is not Final class so clone() is not guaranteed to return defensive copy of the object. — Ankit Tripathi, Nov 29 '16 at 10:09

davidxxx · Answer 1 · 2016-11-29T10:32:50.867

3

Take the java.util.Date class which provides a defensive copy in its clone() method but which is also not final.
Suppose I subclass Date and I override theclone() method.
Now, at runtime, if I receive an instance of this subclass, clone() of Date is not used anylonger. So, I am not sure that the implementation of the child class makes still a defensive copy as the original.

This :

public Date start() {
    return start.clone();
}

will not be a defensive copy if a subclass overrides clone() like that :

@Override 
public Object clone() {
    return this; 
}

edited Nov 29 '16 at 10:32

answered Nov 29 '16 at 10:05

davidxxx

125,838
23
214
215

You mean to say :- Period p = new Period(new MaliciousDate(), new MaliciousDate()); would potentially cause harm? MaliciousDate extends Date and override clone(). – prvn Nov 29 '16 at 10:15
Yes if you call the clone() method on these parameters and the MaliciousDate implementation of clone() is not a defensive copy. In this case, you will provide the same instance to your client. And that, even if the clone() method of Date makes a defensive copy. Because at the runtime, it is the method of the effective instance which is called. – davidxxx Nov 29 '16 at 10:30

score 0 · Answer 2 · answered May 04 '19 at 12:54

Instance of malicious subclass be returned for the accessor method. Quoting from the same chapter -

In the accessors, unlike the constructor, it would be permissible to use the clone method to make the defensive copies. This is so because we know that class of Period's internal Date objects is java.util.Date, and not some potentially untrusted subclass.

Expanding a bit on

do not use the clone method to make a defensive copy of a parameter whose type is subclassable by untrusted parties.

Since Date is not final, it can be subclassed to change its behavior so the clone method of Date could very well return an instance of a malicious subclass.

Example

public class TestNonFinalCloneIssue {
    public static void main(String[] args) {

        NonFinalDate start = new NonFinalDate();
        NonFinalDate end = new NonFinalDate();
        Period p = new Period(start, end);

        // We are completely obvious to the fact that copies of our data
        // exists in another malicious class
        System.out.println("Secretly copied data - " + NonFinalDate.MaliciousDate.getListOfInstances());
    }
}

final class Period {
    private final NonFinalDate start;
    private final NonFinalDate end;

    public Period(NonFinalDate start, NonFinalDate end){
        NonFinalDate s = start.clone();
        NonFinalDate e = end.clone();
        if (s.compareTo(e) > 0){
            throw new IllegalStateException(start + " after " + end);
        }
        this.start = s;
        this.end = e;
    }
}

class NonFinalDate extends Date{
    @Override
    public NonFinalDate clone(){
        // NonFinalDate returning a malicious subclass (subclassing possible only because NonFinalDate is not final)
        return new MaliciousDate(this);
    }
    static class MaliciousDate extends NonFinalDate {
        private static List<NonFinalDate> listOfInstances = new ArrayList<>();

        public MaliciousDate(NonFinalDate date){
            // Secretly making copies of the data
            listOfInstances.add(date);
        }

        public static List<NonFinalDate> getListOfInstances() {
            return listOfInstances;
        }
    }
}

clone() non final classes

2 Answers2