Reasoning behind ArrayIsStoredDirectly rule of PMD

Question

PMD has a rule called ArrayIsStoredDirectly in the Sun Security ruleset:

Constructors and methods receiving arrays should clone objects and store the copy. This prevents that future changes from the user affect the internal functionality.

Here is their example:

public class Foo {
 private String [] x;
  public void foo (String [] param) {
      // Don't do this, make a copy of the array at least
      this.x=param;
  }
}

I don't think I completely understand the reasoning behind this rule. Is it because the values in the array passed can be altered somewhere else? Is there a difference between passing a Collection vs passing an array in regards to this?

Answer 1

There is no differnce between passing a collection or an array: in both cases sender and receiver can modify the content of the datastructure. Here's an example:

// ... in some method
Foo myfoo = new Foo();
String[] array = {"One", "Two", "Three"};
myfoo.foo(array);     // now the Foo instance gets {"One", "Two", "Three"}

array[1] = "Changed"; // now the internal field x in myfoo is {"One", "Changed", "Three"}

If you do not want this behaviour, you have to, following this PMD rule, clone the array in Foo and store a reference to the clone. This way you make sure, that no other class holds a reference to your internal array (unless we forget about reflection for a moment and unless we don't return this internal array in another method...)

Answer 2

The problem is that the caller may keep a copy of the array argument that it passed, and can then change its contents. If the object is security critical and the call is made from untrusted code, you've got a security hole.

In this context, passing a collection and saving it without copying it would also be a potential security risk. (I don't know if there's a PMD rule to tell you this.)

In both cases, the way to address the risk (if it is real) is to set the attribute to a copy of the argument array or collection. On the other hand, if you know that the caller is always going to be trusted code, the copy is a waste of time, and a better solution would be to tell PMD to be quiet about that particular method.