lishunli / projectlombok

Automatically exported from code.google.com/p/projectlombok
0 stars 0 forks source link

@Data with arrays generates FindBugs violation #77

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
What steps will reproduce the problem?
1. Create a @Data POJO with a String[] field (e.g. private String[] 
services;)
2. Run the FindBugs maven plugin against the code
3. Findbugs generates a warning on the generated code:
"may expose internal representation by returning" your array
MALICIOUS_CODE  EI_EXPOSE_REP

The FindBugs glossary describes this as:

EI: May expose internal representation by returning reference to mutable 
object (EI_EXPOSE_REP)

Returning a reference to a mutable object value stored in one of the 
object's fields exposes the internal representation of the object.  If 
instances are accessed by untrusted code, and unchecked changes to the 
mutable object would compromise security or other important properties, you 
will need to do something different. Returning a new copy of the object is 
better approach in many situations.

What is the expected output? What do you see instead?

No FindBugs warnings

What version of the product are you using? On what operating system?

0.9.0, on Ubuntu 9.04 with JDK 6

Please provide any additional information below.

Found this after adding the Findbugs maven plugin to our pom.xml

Original issue reported on code.google.com by jace...@gmail.com on 4 Dec 2009 at 4:54

GoogleCodeExporter commented 9 years ago
The only way to fix this is to duplicate the array.

Which is not a good solution - it's somewhat surprising that this will in fact 
happen, and for consistency we 
should then duplicate ALL mutable fields. But we can't. For two reasons:

(1) we have no idea which types are mutable and which types are immutable; 
mutability is not part of java's 
type system.

(2) Even if we did know, there's no general way to duplicate an object. clone() 
usually doesn't work.

So, good point, but we can't fix it, at least not without adding a heap of 
complexity into how lombok works.

Original comment by reini...@gmail.com on 4 Dec 2009 at 8:12

GoogleCodeExporter commented 9 years ago
This problem also adds for other situations, e.g. Date types.
What about a specific annotation to get lombok provide a clone-based 
implementation? That might be generic enough...

Original comment by mclaus...@gmail.com on 1 Jul 2013 at 1:30

GoogleCodeExporter commented 9 years ago
This will remain WontFix - this academicy model of thinking just doesn't fly in 
java. Array data is usually very large, so just duplicating it is crazy, 
especially as, in the vast majority of circumstances, that duplication is 
completely pointless as nobody _actually_ messes with the data. More generally, 
if this is an issue you should not just duplicate the array*, you should change 
the API of the object so that it is no longer possible to just ask for the 
array data. For example, change to a list implementation, and wrap things in 
guava's ImmutableList. Telling lombok to clone isn't good enough. When should 
lombok clone?

* When the object is provided during object creation / calling of the set 
method.
* When the object is returned via a getter.

Cloning for both produces a huge amount of garbage collection cruft and in 
general results in ridiculous performance penalties. Again, this is working on 
the general basis that your mutable objects are large, but they tend to be.

For java.util.Date objects, the right solution is to not use date and instead 
roll with jodatime's date representations, or 'long', because those are both 
immutable. The solution is not to clone the incoming j.u.Date object on both 
edges of the set/get process. That's crazy. The solution for arraydata is to 
make it ImmutableList, and to actually _USE_ the ImmutableList type in your 
data, or, alternatively, to accept in constructor/setter any list, but to have 
the field type be immutablelist. This makes it clear for those doing a get() 
call that the returned list is immutable.

*) You can't just create a single public copy or even cache this copy, because 
if you did, then 2 different codebases that both call getX() can see each 
other's modifications.

It's hugely impractical to try and automate this with lombok. Even if it was 
possible (and note that the feature page for this would be many tens of pages 
long to cover all the hairy details!), it's clearly no longer boilerplate.

Original comment by reini...@gmail.com on 1 Jul 2013 at 4:08

GoogleCodeExporter commented 9 years ago
We stand by the advice that you should just in general tell findbugs to not 
warn on this, but a feature was just added to tell findbugs not to warn on 
anything that lombok generates. See:

https://code.google.com/p/projectlombok/issues/detail?id=702

Original comment by reini...@gmail.com on 1 Feb 2015 at 11:22