CODEWITHSUNDEEP
javasonarqubedefensive-copy
nurdan karaman
nurdan karaman

Reputation: 213

Sonar Violation:Security-Array is stored directly

I m trying to change my codes in my application with using Sonar. How to fix it and why? Thanks.

public class BeanResultSetHandler<T> extends BasicResultSetHandler<T> {
T instance;
Class<T> clas;
Object[] selectFields;

/**
 * Constructor
 */
**
public BeanResultSetHandler(Class<T> type, Object[] selectedFields) {
    this.clas = type;
    this.selectFields = selectedFields;
    if (selectedFields == null)
        this.selectFields = this.clas.getFields();
}

Upvotes: 0

Views: 135

Answers (2)

secolive
secolive

Reputation: 589

Sonar tells you that you are not copying the selectedFields array, but merely storing a reference to this array. As a consequence, if the caller is to later modify the array, it will modify the "content" of the BeanResultSetHandler object as well, e.g. with the following code:

h = new BeanResultSetHandler(MyClass.class, myFieldsArray);
myFieldsArray[0] = null;       // now t.selectFields[0] also is null
myFieldsArray[0] = someObject; // now t.selectFields[0] also references someObject

Whether this is a real problem depends IMHO if the caller is likely to use a custom array that she would be tempted to modify after the call to the constructor. If in all real cases the argument is null or the result of some Class.getFields() I would not bother with it too much.

The "defensive copy" idiom is to never store a reference to a mutable object or array, but to always clone in case of doubt, in your case cloning the array:

public BeanResultSetHandler(Class<T> type, Object[] selectedFields) {
    this.clas = type;
    if (selectedFields == null)
        this.selectFields = this.clas.getFields();
    else
        this.selectFields = selectedFields.clone();
}

The problem is that cloning can incur performance overhead, and when this approach is generalized (e.g. also when returning something from the objects) then an awful lot of useless clones will be performed. So I wouldn't start trying to correct all such issues reported by Sonar.

Personally, I tend to apply such idioms only over module boundaries, e.g. in the methods used to expose services to other modules. Then inside modules I do not use defensive copy and rely on unit tests instead.

Upvotes: 0

Fran Montero
Fran Montero

Reputation: 1680

You have to clone the array before storing it:

this.selectFields = Arrays.copyOf(selectFields, selectedFields.length)

Sonar complains because from BeanResultSetHandler array which owner is caller could be modified.

Upvotes: 1

Related Questions