Sonar Violation: Security - Array is stored directly

Question

There is a Sonar Violation:

Sonar Violation: Security - Array is stored directly

public void setMyArray(String[] myArray) { 
  this.myArray = myArray; 
}

Solution:

public void setMyArray(String[] newMyArray) { 
  if(newMyArray == null) { 
    this.myArray = new String[0]; 
  } else { 
   this.myArray = Arrays.copyOf(newMyArray, newMyArray.length); 
  } 
}

But I wonder why ?

Umm...that solution didn't work for me, actually. Sonar still picks it up and complains about it, despite making a copy. — ndtreviv, Oct 07 '13 at 15:30
@ndtreviv: I was having this issue as well, and this error will not go away if the name of the local variable supplied to the method is the same as the instance variable you are storing. Make sure they are different, and the above solution should work. I found this out through the following [link](http://sonarqube.15.x6.nabble.com/Security-Array-is-stored-directly-weirness-td3632714.html) — Matt, Jun 03 '14 at 21:00
Java definitely should invent `const` to avoid copying const data forced by this d*** rule. — bebbo, Jul 26 '18 at 11:32

Brian Agnew · Accepted Answer · 2012-07-20T14:30:43.180

56

It's complaining that the array you're storing is the same array that is held by the caller. That is, if the caller subsequently modifies this array, the array stored in the object (and hence the object itself) will change.

The solution is to make a copy within the object when it gets passed. This is called defensive copying. A subsequent modification of the collection won't affect the array stored within the object.

It's also good practice to normally do this when returning a collection (e.g. in a corresponding getMyArray() call). Otherwise the receiver could perform a modification and affect the stored instance.

Note that this obviously applies to all mutable collections (and in fact all mutable objects) - not just arrays. Note also that this has a performance impact which needs to be assessed alongside other concerns.

edited Jul 20 '12 at 14:30

answered Jul 20 '12 at 14:08

Brian Agnew

268,207
37
334
440

I can see the reason.... what if I deliberately want the caller and its target holding the same copy? – Junchen Liu Jul 20 '12 at 15:23
2

That's a design decision. But I think it's important to understand *who* owns this data, and how (if necessary) you inform objects holding it that it's changed. It's quite reasonable in set of components closely-related to pass collections around without defensive copying. But at some point there'll be a boundary at which you need to protect yourself (e.g. plugging into a 3rd pary or clients' code) – Brian Agnew Jul 20 '12 at 15:30
1

surely the OP *is* making a defensive copy with `this.myArray = Arrays.copyOf(newMyArray, newMyArray.length);`? – Qwerky Oct 04 '12 at 14:25
2

Also i dont understand why it is just showing arrays. Because, in a bean we will have custom objects and same problem (caller will hold the same copy and if he changes, it will affect internally) can occur, but it is not complaining. – Manoj Jun 04 '14 at 14:37
1

@Manoj probably because sonar doesn't know if the object is mutable or not. With an immutable object this is no concern that some other class can unexpectedly change the object's state. Arrays cannot be immutable so sonar knows to flag them all the time – icyitscold Aug 06 '14 at 02:56

ewernli · Answer 2 · 2012-07-20T14:09:46.567

22

It's called defensive copying. A nice article on the topic is "Whose object is it, anyway?" by Brian Goetz, which discusses difference between value and reference semantics for getters and setters.

Basically, the risk with reference semantics (without a copy) is that you erronously think you own the array, and when you modify it, you also modify other structures that have aliases to the array. You can find many information about defensive copying and problems related to object aliasing online.

edited Jul 20 '12 at 14:09

answered Jul 20 '12 at 14:04

ewernli

38,045
5
92
123

thanks mate, def a up vote. can you make a small example, or compress the reason in 50 words? – Junchen Liu Jul 20 '12 at 14:08
1

@ewernli : The link was broken, Please fix it. Thanks! – Abimaran Kugathasan Nov 18 '13 at 05:36

Jorgesys · Answer 3 · 2014-07-10T23:34:59.727

I had the same issue:

Security - Array is stored directly The user-supplied array 'palomitas' is stored directly.

my original method:

public void setCheck(boolean[] palomitas) {
        this.check=palomitas;
    }

fixed turned to:

public void setCheck(boolean[] palomitas) { 
      if(palomitas == null) { 
        this.check = new boolean[0]; 
      } else { 
       this.check = Arrays.copyOf(palomitas, palomitas.length); 
      } 
}

Other Example:

Security - Array is stored directly The user-supplied array

private String[] arrString;

    public ListaJorgeAdapter(String[] stringArg) {      
        arrString = stringArg;
    }

Fixed:

public ListaJorgeAdapter(String[] stringArg) {  
    if(stringArg == null) { 
      this.arrString = new String[0]; 
    } else { 
      this.arrString = Arrays.copyOf(stringArg, stringArg.length); 
    } 
}

score 4 · Answer 4 · answered Jul 16 '14 at 09:50

4

To eliminate them you have to clone the Array before storing / returning it as shown in the following class implementation, so noone can modify or get the original data of your class but only a copy of them.

public byte[] getarrString() {
    return arrString.clone();
}
/**
 * @param arrStringthe arrString to set
 */
public void arrString(byte[] arrString) {
    this.arrString= arrString.clone();
}

I used it like this and Now I am not getting any SONAR violation...

answered Jul 16 '14 at 09:50

Wolverine789

407
2
6
10

for using clone() in user defined array you will need to implement cloneable interface than only it will work else it will throw error – Amit Singh Bhardwaj Jan 11 '23 at 13:01

score 2 · Answer 5 · answered Jan 12 '16 at 09:48

It's more ease than all of this. You only need to rename the method parameter to anything else to avoid Sonar violations.

http://osdir.com/ml/java-sonar-general/2012-01/msg00223.html

public void setInventoryClassId(String[] newInventoryClassId)
    {                
            if(newInventoryClassId == null)
            {
                    this.inventoryClassId = new String[0];
            }
            else
            {
                    this.inventoryClassId = Arrays.copyOf(newInventoryClassId, newInventoryClassId.length);
            }

    }

score 0 · Answer 6 · answered Nov 28 '12 at 11:46

0

To go the defensive-implementation-way can save you a lot of time. In Guava you get another nice solution to reach the goal: ImmutableCollections

http://code.google.com/p/guava-libraries/wiki/ImmutableCollectionsExplained

answered Nov 28 '12 at 11:46

Jordi Laforge

670
3
19

score 0 · Answer 7 · answered Oct 14 '15 at 11:15

0

There are certain cases where it is a design decision and not missed out. In these cases, you need to modify the Sonar rules to exclude it so that it doesn't show such issues in report.

answered Oct 14 '15 at 11:15

Tushar Patel

365
3
16

Sonar Violation: Security - Array is stored directly

7 Answers7

Linked

Related