From 4f3879c4465027dd321e347dd24a46ed4a89ccea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81d=C3=A1m=20Balogh?= Date: Thu, 3 Dec 2020 17:55:25 +0100 Subject: [PATCH] Add new rule PA_PUBLIC_ATTRIBUTES SEI CERT rule OBJ01-J requires that accessibility of fields must be limited. In general, requiring that no fields must be public is overkill and unrealistic. Even the rule mentions that final fields may be public, except if the type of the field is a mutable reference type which can be modified despite the reference itself being final. Beside final field, there may be other usages for public fields: some public fields may serve as "flags" which affect the behavior of the class. These fields are expected to be read by the containing class itself and written by other classes. If a field is both written by the methods of the containing class itself and from outside is suspicious. This new rule PA_PUBLIC_ATTRIBUTES warns for such field. Change-Id: I25ae6ce2ab42a0ac4a3f383cf04d15382bca0e8f --- CHANGELOG.md | 3 + spotbugs/etc/findbugs.xml | 3 + spotbugs/etc/messages.xml | 32 +++ .../cs/findbugs/detect/PublicAttributes.java | 194 ++++++++++++++++++ .../java/PublicAttributesNegativeTest.java | 23 +++ .../src/java/PublicAttributesTest.java | 40 ++++ 6 files changed, 295 insertions(+) create mode 100644 spotbugs/src/main/java/edu/umd/cs/findbugs/detect/PublicAttributes.java create mode 100644 spotbugsTestCases/src/java/PublicAttributesNegativeTest.java create mode 100644 spotbugsTestCases/src/java/PublicAttributesTest.java diff --git a/CHANGELOG.md b/CHANGELOG.md index e2120bd0fde..fc9f7ac0fab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,9 @@ Currently the versioning policy of this project follows [Semantic Versioning v2. - Should issue warning for SecureRandom object created and used only once ([#1464](https://github.com/spotbugs/spotbugs/issues/1464)) - False positive OBL_UNSATIFIED_OBLIGATION with try with resources ([#79](https://github.com/spotbugs/spotbugs/issues/79)) +### Added +- New rule `PA_PUBLIC_ATTRIBUTES` to warn for public attributes which are written by the methods of the class. This rule is losely based on SEI CERT rule `OBJ01-J Limit accessibility of fields`. ([#OBJ01-J](https://wiki.sei.cmu.edu/confluence/display/java/OBJ01-J.+Limit+accessibility+of+fields)) + ## 4.2.2 - 2021-03-03 ### Fixed diff --git a/spotbugs/etc/findbugs.xml b/spotbugs/etc/findbugs.xml index 0d7653be424..cf5c541b35a 100644 --- a/spotbugs/etc/findbugs.xml +++ b/spotbugs/etc/findbugs.xml @@ -561,6 +561,8 @@ reports="RI_REDUNDANT_INTERFACES"/> + + diff --git a/spotbugs/etc/messages.xml b/spotbugs/etc/messages.xml index bdde9a3b818..5f67bf00c23 100644 --- a/spotbugs/etc/messages.xml +++ b/spotbugs/etc/messages.xml @@ -1321,6 +1321,14 @@ the subclass.

This detector looks for potential problems in implementing the Struts framework.

+]]> + + + +
+This detector looks for public attributes that are also written by the metgods of the class. +

]]>
@@ -7025,6 +7033,29 @@ just use the constant. Methods detected are: ]]> + + Field is both public and written by a method + Field {1} is both public and written by a method +
+ + SEI CERT rule OBJ01-J requires that accessibility of fields must be limited: + https://wiki.sei.cmu.edu/confluence/display/java/OBJ01-J.+Limit+accessibility+of+fields + In general, requiring that no fields must be public is overkill and unrealistic. Even the rule + mentions that final fields may be public, except if the type of the field is a mutable reference + type which can be modified despite the reference itself being final. Beside final field, there + may be other usages for public fields: some public fields may serve as "flags" which affect the + behavior of the class. These fields are expected to be read by the containing class itself and + written by other classes. If a field is both written by the methods of the containing class + itself and from outside is suspicious. Consider to make these fields private and provide setters + if necessary. Please note that constructors, initializers and finalizers are exceptions, if only + they write the field inside the class, the field is not considered as written by the class + itself. Also note that for reference type fields "writing" means calling methods whose name + suggests modification. +

+ ]]> +
+ Class exposes synchronization and semaphores in its public interface Class {0} exposes synchronization and semaphores in its public interface @@ -8513,6 +8544,7 @@ after the call to initLogging, the logger configuration is lost Circular Dependencies Redundant Interfaces Multithreaded Instance Access + Public Attributes Public Semaphores Bad shift Casting from integer values diff --git a/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/PublicAttributes.java b/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/PublicAttributes.java new file mode 100644 index 00000000000..84fb2a0787e --- /dev/null +++ b/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/PublicAttributes.java @@ -0,0 +1,194 @@ +package edu.umd.cs.findbugs.detect; + +import java.util.Set; +import java.util.HashSet; + +import org.apache.bcel.Const; +import org.apache.bcel.Repository; +import org.apache.bcel.classfile.JavaClass; +import org.apache.bcel.classfile.Field; +import org.apache.bcel.generic.ObjectType; +import org.apache.bcel.generic.Type; + +import edu.umd.cs.findbugs.BugInstance; +import edu.umd.cs.findbugs.BugReporter; +import edu.umd.cs.findbugs.Detector; +import edu.umd.cs.findbugs.ba.XField; +import edu.umd.cs.findbugs.ba.XMethod; +import edu.umd.cs.findbugs.bcel.OpcodeStackDetector; + +public class PublicAttributes + extends OpcodeStackDetector + implements Detector { + + private final BugReporter bugReporter; + + private Set writtenFields = new HashSet(); + + public PublicAttributes(BugReporter bugReporter) { + this.bugReporter = bugReporter; + } + + // Check for each statements which write an own attribute of the class + // instance. If the attribute written is public then report a bug. + @Override + public void sawOpcode(int seen) { + // It is normal that classes used as simple data types have a + // constructor to make initialization easy. + if (isConstructor(getMethodName())) + return; + + // We do not care for enums and nested classes. + if (getThisClass().isEnum() || getClassName().indexOf('$') >= 0) + return; + + if (seen == Const.PUTFIELD || seen == Const.PUTSTATIC) { + XField field = getXFieldOperand(); + // Do not report the same public field twice. + if (field == null || writtenFields.contains(field)) + return; + + // Only consider attributes of self. + if (!field.getClassDescriptor().equals(getClassDescriptor())) + return; + + if (!field.isPublic()) + return; + + bugReporter.reportBug(new BugInstance(this, "PA_PUBLIC_ATTRIBUTES", + NORMAL_PRIORITY) + .addClass(this).addField(field)); + writtenFields.add(field); + } else if (seen == Const.AASTORE) { + XField field = stack.getStackItem(2).getXField(); + // Do not report the same public field twice. + if (field == null || writtenFields.contains(field)) + return; + + // Only consider attributes of self. + if (!field.getClassDescriptor().equals(getClassDescriptor())) + return; + + if (!field.isPublic()) + return; + + bugReporter.reportBug(new BugInstance(this, "PA_PUBLIC_ATTRIBUTES", + NORMAL_PRIORITY) + .addClass(this).addField(field)); + writtenFields.add(field); + } else if (seen == Const.INVOKEINTERFACE || + seen == Const.INVOKEVIRTUAL) { + XMethod xmo = getXMethodOperand(); + if (xmo == null) + return; + + // Heuristics: suppose that that model has a proper name in English + // which roughly describes its behavior, beginning with the verb. + // If the verb does not hint that the method writes the object + // then we skip it. + if (!looksLikeASetter(xmo.getName())) + return; + + XField field = stack.getStackItem(xmo.getNumParams()).getXField(); + // Do not report the same public field twice. + if (field == null || writtenFields.contains(field)) + return; + + // Only consider attributes of self. + if (!field.getClassDescriptor().equals(getClassDescriptor())) + return; + + if (!field.isPublic() || !field.isReferenceType()) + return; + + Field clsField = getClassField(field, getClassName()); + Type type = clsField.getType(); + // Instances of an immutable classes cannot be modified, thus + // the method is not a modifier, despite its name. + if (type instanceof ObjectType && + isImmutable(((ObjectType) type).getClassName())) + return; + + bugReporter.reportBug(new BugInstance(this, "PA_PUBLIC_ATTRIBUTES", + NORMAL_PRIORITY) + .addClass(this).addField(field)); + writtenFields.add(field); + } + } + + private static boolean isConstructor(String methodName) { + return Const.CONSTRUCTOR_NAME.equals(methodName) || + Const.STATIC_INITIALIZER_NAME.equals(methodName) || + "clone".equals(methodName) || + "init".equals(methodName) || + "initialize".equals(methodName) || + "dispose".equals(methodName) || + "finalize".equals(methodName) || + "this".equals(methodName) || + "_jspInit".equals(methodName) || + "_jspDestroy".equals(methodName); + } + + private static Field getClassField(XField field, String className) { + try { + JavaClass cls = Repository.lookupClass(className); + for (Field clsField : cls.getFields()) { + if (field.getName().equals(clsField.getName())) + return clsField; + } + } catch (ClassNotFoundException cnfe) { + assert false; + } + return null; + } + + private static boolean isImmutable(String className) { + if ("java.lang.String".equals(className) || + "java.lang.Character".equals(className) || + "java.lang.Byte".equals(className) || + "java.lang.Short".equals(className) || + "java.lang.Integer".equals(className) || + "java.lang.Long".equals(className) || + "java.lang.Float".equals(className) || + "java.lang.Double".equals(className) || + "java.lang.Boolean".equals(className)) { + return true; + } + + try { + JavaClass cls = Repository.lookupClass(className); + + if (!cls.isFinal()) { + return false; + } + + for (Field field : cls.getFields()) { + if (!field.isStatic() && + (!field.isFinal() || !field.isPrivate())) { + return false; + } + } + + return true; + } catch (ClassNotFoundException cnfe) { + assert false; + } + return false; + } + + private static boolean looksLikeASetter(String methodName) { + return methodName.startsWith("set") || + methodName.startsWith("put") || + methodName.startsWith("add") || + methodName.startsWith("insert") || + methodName.startsWith("delete") || + methodName.startsWith("remove") || + methodName.startsWith("erase") || + methodName.startsWith("push") || + methodName.startsWith("pop") || + methodName.startsWith("enqueue") || + methodName.startsWith("dequeue") || + methodName.startsWith("write") || + methodName.startsWith("append"); + } +} diff --git a/spotbugsTestCases/src/java/PublicAttributesNegativeTest.java b/spotbugsTestCases/src/java/PublicAttributesNegativeTest.java new file mode 100644 index 00000000000..ba74f8082b1 --- /dev/null +++ b/spotbugsTestCases/src/java/PublicAttributesNegativeTest.java @@ -0,0 +1,23 @@ +// This class is just a structure to store data. It does not have any methods, +// except a constructor which initializes some of the fields. Therefore public +// attributes can be allowed here. + +import edu.umd.cs.findbugs.annotations.NoWarning; + +import java.util.Map; +import java.util.HashMap; + +class PublicAttributesNegativeTest { + @NoWarning("PA") + public int attr1 = 0; + + @NoWarning("PA") + public static final Map hm = new HashMap(); + + @NoWarning("PA") + public static final String[] items = { "foo", "bar", "baz" }; + + PublicAttributesNegativeTest(int a1) { + attr1 = a1; + } +} diff --git a/spotbugsTestCases/src/java/PublicAttributesTest.java b/spotbugsTestCases/src/java/PublicAttributesTest.java new file mode 100644 index 00000000000..b95bf974130 --- /dev/null +++ b/spotbugsTestCases/src/java/PublicAttributesTest.java @@ -0,0 +1,40 @@ +// This class is more than a plain structure which stores data. It also has a +// public method which means that it has some kind of "behavior". Its data +// fields should not be visible from the outside. + +import edu.umd.cs.findbugs.annotations.ExpectWarning; +import edu.umd.cs.findbugs.annotations.NoWarning; + +import java.util.Map; +import java.util.HashMap; +import java.util.Set; +import java.util.HashSet; + +public class PublicAttributesTest { + @ExpectWarning("PA") + public int attr1 = 0; + + // A final field. It cannot be written in methods. + @NoWarning("PA") + public final int Const1 = 10; + + @ExpectWarning("PA") + public static final Map hm = new HashMap(); + // This field is not written, but only read. + @NoWarning("PA") + public static final Set hs = new HashSet(); + + @ExpectWarning("PA") + public static final String[] items = { "foo", "bar", "baz" }; + + public PublicAttributesTest(int a1) { + attr1 = a1; + } + + public void doSomething() { + attr1 = 1; + hm.put(1, "test"); + boolean b = hs.contains("x"); + items[0] = "test"; + } +}