Mutable array clones #1551
Mutable array clones #1551
Conversation
baloghadamsoftware
force-pushed
the
Mutable_Array_Clones
branch
2 times, most recently
from
0dc9c9d
to
cdb6a45
Compare
|
Sorry for force pushing but there was a git mess which I had to sort out. |
Currently, the detector `FindReturnRef` which is responisble for bugs `EI_EXPOSE_REP`, `EI_EXPOSE_REP2`, `MS_EXPOSE_REP` and `EI_EXPOSE_STATIC_REP2` tracks the data on the top of the stack. However, this is not necessary in the current version of SpotBugs because the infrastructure already does this job more accurately. This patch removes the redundant tracking the the top of the stack and relies on the already existing infrastructure instead. This way the detector is able to find more true positives where returning of the private mutable attribute happens indirectly, e.g. by first assigning it to a local variable and then returning that local.
Some programmers try to avoid returning mutable private members by returning thir clones. However, in case of array the clone() method does a shallow-copy instead of a deep one. If the array members are of a mutable type, then we still return mutable private data. Similarly, when passing an array to a method, the method should clone its members one-by-one, if they are of a mutable type, instead of cloning (thus shallowly copying) the array itself. This patch improves the `FindReturnRef` detector to also issue `EI`, `EI2` and `MS` warnings in these cases.
baloghadamsoftware
force-pushed
the
Mutable_Array_Clones
branch
from
cdb6a45
to
dfa0be6
Compare
|
I had to force push this again, but no it should be no problem because the review did not start yet. |
|
With tihs patch spotbugs found a new error on MATSim. |
I'm really not sure this is a bug. Let's assume we have not array clone but a Collection copy - there are gazillion methods returning copies of internal collections without cloning every single element - that is not worth a warning IMHO. Why should it be different for array clones? |
Actually, according to the philosophy behind the SEI CERT rule OBJ05-J we should warn for every Collection shallow copy if the Collection contains mutable elements, the member is private and the method is public. However, finding all of them is difficult so we decided to start with arrays which are explicitly mentioned in the rule description at the second non-compliant code example: "The method fails to make a defensive copy of the array before returning it. Because the array contains references to Date objects that are mutable, a shallow copy of the array is insufficient because an attacker can modify the Date objects in the array." |
I believe in that case this detector should be disabled by default, or be part of https://find-sec-bugs.github.io/ |
Do you mean the whole detector here? The detector indeed produces many reports but this particular patch only adds the detection of returned clone of private arrays of mutable elements in a public method. If you click the link, you can see yourself that we got only one single new detection when applying this patch in a single project and no detection in the rest. Sorry, I disagree with you that returning such a clone is 99% false positive. On the contrary, I strongly believe that returning a clone of an array of mutable elements is at least a code smell. There are very few reasons to protect the array itself by only returning its clone and not the array itself but not protecting the elements of the array. (I cannot think an example myself where this is the intended behavior.) In most of the cases the programmer intends to protects the elements as well but forgets that clone() does not make a deep copy (or forgets that the array stores references). This is a typical programming error, committed by novice Java developers. |
|
What would be the best way forward? Should I maybe introduce of a new error message (still with prefix "EI" and "MS") for these cases. This way we could better explain the user this particular bug. Furthermore, it also enables the user to distinguish between the different "EI" and "MS" messages, maybe filtering some kinds. If you think it is the appropriate way then I can make the new message with low priority so it is disabled by default. This patch was tested on the following projects: MATSim, SpotBugs itself, Jenkins, NanoHTTPD, Bt, Ttorrent. It did not give any new reports, only 1 new finding on the MATSim project. Thanks for your review. I am looking forward to your suggestions on how to proceed. |
and debug printouts removed
|
@iloveeclipse I changed the priority of the new error to low but I did not introduce a new bug type for now. Is this OK? |
SEI-CERT rule OBJ05-J warns that cloning a private array before returning may not be sufficient if the elements of the array are of a mutable type. The clone() method of arrays only does a shallow-copy thus exposing the elements of the private array to all other classes. This patch improves the FindReturnRef detector to find such issues. The improved detector also warns for methods taking an array parameter of a mutable type and cloning it to a private array: the elements of the array are not copied, thus the caller can later modify them which results in modification of the elements of the private array as well.
Make sure these boxes are checked before submitting your PR -- thank you!
CHANGELOG.mdif you have changed SpotBugs code