#278 Add bug pattern for 'mutable enum'

3.0.1
closed-fixed
Tagir Valeev
5
2015-03-08
2012-12-12
Jens Bannmann
No

Implementing an enum that is stateful is widely considered to be bad practice and even outright dangerous (at the very least, it's a violation of the principle of least astonishment). I can't seem to find a FindBugs pattern that warns about this.

The easiest check would probably be to ensure that fields within enums are always declared final. An alternative approach would allow non-final fields (e.g. for implementing lazy instantiation), but complain about setters (which I found in one project - unused, but obviously added/generated accidently).

Discussion

  • Tagir Valeev
    Tagir Valeev
    2015-01-12

    Hello! We can easily check whether the field is not modified after constructor. I wrote a simple test and ran it against several open-source projects. Often it produces a false-positive on lazy initialization like KCode#getEncoding. There are however very suspicious places like CipherSuite#setUnknownValue. It's unconditional public setter to non-static non-final enum field which actually used in several places to store presumably different values.

    This case is quite similar to mutable public static fields which we report under malicious code vulnerability. I can write a detector which reports public non-final fields of public enums as well as public methods which unconditionally set enum field. Though probably for the completeness we will also need to detect public methods which unconditionally set the static fields.

    I will think more about it.

     
  • Tagir Valeev
    Tagir Valeev
    2015-01-15

    Implemented:
    https://code.google.com/p/findbugs/source/detail?r=749e468b2abd3e0b51f769e50b3a3d8915c89743

    Two new bug patterns created:
    ME_MUTABLE_ENUM_FIELD - triggered when public enum has public non-final non-static field which may be changed by anybody.
    ME_ENUM_FIELD_SETTER - triggered when public enum has public method which unconditionally sets its non-static field.

    Found once in Lucene project: Format#id field (probably it was made public by mistake), twice in JMol project (public enum fields as well) and a dozen of times in Hadoop project (both public fields and setters). Seems that it has its own access rights system based on the annotations, but ok.

    Both warnings are in "bad practice" category, though probably should be moved to malicious code vulnerability. Feel free to comment if you have additional suggestions.

     
  • Tagir Valeev
    Tagir Valeev
    2015-01-15

    • status: open --> closed-fixed
    • Group: --> 3.0.1
     
    • assigned_to: Tagir Valeev