Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
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
- Loading branch information
Ádám Balogh
committed
Apr 8, 2021
1 parent
ff6582e
commit 4f3879c
Showing
6 changed files
with
295 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
194 changes: 194 additions & 0 deletions
194
spotbugs/src/main/java/edu/umd/cs/findbugs/detect/PublicAttributes.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<XField> writtenFields = new HashSet<XField>(); | ||
|
|
||
| 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"); | ||
| } | ||
| } |
23 changes: 23 additions & 0 deletions
23
spotbugsTestCases/src/java/PublicAttributesNegativeTest.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<Integer, String> hm = new HashMap<Integer, String>(); | ||
|
|
||
| @NoWarning("PA") | ||
| public static final String[] items = { "foo", "bar", "baz" }; | ||
|
|
||
| PublicAttributesNegativeTest(int a1) { | ||
| attr1 = a1; | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<Integer, String> hm = new HashMap<Integer, String>(); | ||
| // This field is not written, but only read. | ||
| @NoWarning("PA") | ||
| public static final Set<String> hs = new HashSet<String>(); | ||
|
|
||
| @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"; | ||
| } | ||
| } |