Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Add new rule set PA_PUBLIC_PRIMITIVE_ATTRIBUTE, PA_PUBLIC_ARRAY_ATTRI…
…BUTE and PA_PUBLIC_MUTABLE_OBJECT_ATTRIBUTE (#1500) * 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. Besides final fields, 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. * Updated according to the comments of @KengoTODA * URL to SEI CERT rule changed to a HTML link in messages.xml * Warnings separated to 3 different messages * Changed detection of mutable classes and refactored two utility functions * Constant containers renamed to all-capitals, moved to the front & made static * Static mutables are detected by MS, duplicate error reports removed * Missing space inserted into the error message description. * Grammar fix * Changelog adjusted * Detector renamed, bug messages and descriptions changed * add license header, fix typos * refactoring tests to the new type * add sourceline for non-static fields at the first PUT (init) * making PA_PUBLIC_ATTRIBUTES messages more concrete * fix tests, whitespaces * fix CHANGELOG.md to master merge * Fix Error marker location to the first assignment * Remove unnecessary line * CHANGELOG updated * Small refactor * Reverting changes to firstFile.xml and secondFile.xml * spotless apply --------- Co-authored-by: Ádám Balogh <adam.balogh@ericsson.com> Co-authored-by: Judit Knoll <judit.knoll@sigmatechnology.com> Co-authored-by: Judit Knoll <123470644+JuditKnoll@users.noreply.github.com>
- Loading branch information
4 people
committed
Jun 9, 2023
1 parent
1ce8b39
commit 64e8c9c
Showing
7 changed files
with
415 additions
and
1 deletion.
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
58 changes: 58 additions & 0 deletions
58
spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/FindPublicAttributesTest.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,58 @@ | ||
| package edu.umd.cs.findbugs.detect; | ||
|
|
||
| import edu.umd.cs.findbugs.*; | ||
| import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcher; | ||
| import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcherBuilder; | ||
| import org.junit.Test; | ||
|
|
||
| import static edu.umd.cs.findbugs.test.CountMatcher.containsExactly; | ||
| import static org.hamcrest.MatcherAssert.assertThat; | ||
| import static org.hamcrest.Matchers.hasItem; | ||
|
|
||
| public class FindPublicAttributesTest extends AbstractIntegrationTest { | ||
| private static final String PRIMITIVE_PUBLIC = "PA_PUBLIC_PRIMITIVE_ATTRIBUTE"; | ||
| private static final String MUTABLE_PUBLIC = "PA_PUBLIC_MUTABLE_OBJECT_ATTRIBUTE"; | ||
| private static final String ARRAY_PUBLIC = "PA_PUBLIC_ARRAY_ATTRIBUTE"; | ||
|
|
||
| @Test | ||
| public void testPublicAttributesChecks() { | ||
| performAnalysis("PublicAttributesTest.class"); | ||
|
|
||
| assertNumOfBugs(PRIMITIVE_PUBLIC, 3); | ||
| assertNumOfBugs(ARRAY_PUBLIC, 4); | ||
| assertNumOfBugs(MUTABLE_PUBLIC, 1); | ||
|
|
||
| assertBugTypeAtField(PRIMITIVE_PUBLIC, "attr1", 11); | ||
| assertBugTypeAtField(PRIMITIVE_PUBLIC, "attr2", 42); | ||
| assertBugTypeAtField(PRIMITIVE_PUBLIC, "sattr1", 15); | ||
| assertBugTypeAtField(MUTABLE_PUBLIC, "hm", 20); | ||
| assertBugTypeAtField(ARRAY_PUBLIC, "items", 28); | ||
| assertBugTypeAtField(ARRAY_PUBLIC, "nitems", 48); | ||
| assertBugTypeAtField(ARRAY_PUBLIC, "sitems", 32); | ||
| assertBugTypeAtField(ARRAY_PUBLIC, "SFITEMS", 34); | ||
| } | ||
|
|
||
| @Test | ||
| public void testGoodPublicAttributesChecks() { | ||
| performAnalysis("PublicAttributesNegativeTest.class"); | ||
| assertNumOfBugs(PRIMITIVE_PUBLIC, 0); | ||
| assertNumOfBugs(ARRAY_PUBLIC, 0); | ||
| assertNumOfBugs(MUTABLE_PUBLIC, 0); | ||
| } | ||
|
|
||
| private void assertNumOfBugs(String bugtype, int num) { | ||
| final BugInstanceMatcher bugTypeMatcher = new BugInstanceMatcherBuilder() | ||
| .bugType(bugtype).build(); | ||
| assertThat(getBugCollection(), containsExactly(num, bugTypeMatcher)); | ||
| } | ||
|
|
||
| private void assertBugTypeAtField(String bugtype, String field, int line) { | ||
| final BugInstanceMatcher bugInstanceMatcher = new BugInstanceMatcherBuilder() | ||
| .bugType(bugtype) | ||
| .inClass("PublicAttributesTest") | ||
| .atField(field) | ||
| .atLine(line) | ||
| .build(); | ||
| assertThat(getBugCollection(), hasItem(bugInstanceMatcher)); | ||
| } | ||
| } |
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
208 changes: 208 additions & 0 deletions
208
spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindPublicAttributes.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,208 @@ | ||
| /* | ||
| * SpotBugs - Find bugs in Java programs | ||
| * | ||
| * This library is free software; you can redistribute it and/or | ||
| * modify it under the terms of the GNU Lesser General Public | ||
| * License as published by the Free Software Foundation; either | ||
| * version 2.1 of the License, or (at your option) any later version. | ||
| * | ||
| * This library is distributed in the hope that it will be useful, | ||
| * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU | ||
| * Lesser General Public License for more details. | ||
| * | ||
| * You should have received a copy of the GNU Lesser General Public | ||
| * License along with this library; if not, write to the Free Software | ||
| * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA | ||
| */ | ||
|
|
||
| package edu.umd.cs.findbugs.detect; | ||
|
|
||
| import java.util.Arrays; | ||
| import java.util.HashMap; | ||
| import java.util.HashSet; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.Set; | ||
|
|
||
| import edu.umd.cs.findbugs.SourceLineAnnotation; | ||
| import org.apache.bcel.Const; | ||
|
|
||
| import edu.umd.cs.findbugs.BugInstance; | ||
| import edu.umd.cs.findbugs.BugReporter; | ||
| import edu.umd.cs.findbugs.ba.XField; | ||
| import edu.umd.cs.findbugs.ba.XMethod; | ||
| import edu.umd.cs.findbugs.bcel.OpcodeStackDetector; | ||
| import edu.umd.cs.findbugs.util.MutableClasses; | ||
| import org.apache.bcel.classfile.JavaClass; | ||
| import org.apache.bcel.classfile.Method; | ||
|
|
||
| public class FindPublicAttributes extends OpcodeStackDetector { | ||
|
|
||
| private static final Set<String> CONSTRUCTOR_LIKE_NAMES = new HashSet<String>(Arrays.asList( | ||
| Const.CONSTRUCTOR_NAME, Const.STATIC_INITIALIZER_NAME, | ||
| "clone", "init", "initialize", "dispose", "finalize", "this", | ||
| "_jspInit", "jspDestroy")); | ||
|
|
||
| private static final List<String> SETTER_LIKE_NAMES = Arrays.asList( | ||
| "set", "put", "add", "insert", "delete", "remove", "erase", "clear", | ||
| "push", "pop", "enqueue", "dequeue", "write", "append"); | ||
|
|
||
| private final BugReporter bugReporter; | ||
|
|
||
| private final Set<XField> writtenFields = new HashSet<XField>(); | ||
|
|
||
| private final Map<XField, SourceLineAnnotation> fieldDefLineMap = new HashMap<XField, SourceLineAnnotation>(); | ||
|
|
||
| public FindPublicAttributes(BugReporter bugReporter) { | ||
| this.bugReporter = bugReporter; | ||
| } | ||
|
|
||
| @Override | ||
| public void sawField() { | ||
| XField field = getXFieldOperand(); | ||
| if (field != null && !fieldDefLineMap.containsKey(field)) { | ||
| SourceLineAnnotation sla = SourceLineAnnotation.fromVisitedInstruction(this); | ||
| fieldDefLineMap.put(field, sla); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public void visit(JavaClass obj) { | ||
| for (Method m : obj.getMethods()) { | ||
| String methodName = m.getName(); | ||
| // First visit the Constructor and the static initializer to collect the field initialization lines | ||
| if (Const.CONSTRUCTOR_NAME.equals(methodName) || Const.STATIC_INITIALIZER_NAME.equals(methodName)) { | ||
| doVisitMethod(m); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Check for each statement which writes 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 (isConstructorLikeMethod(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; | ||
| } | ||
|
|
||
| SourceLineAnnotation sla = fieldDefLineMap.containsKey(field) | ||
| ? fieldDefLineMap.get(field) | ||
| : SourceLineAnnotation.fromVisitedInstruction(this); | ||
|
|
||
| bugReporter.reportBug(new BugInstance(this, | ||
| "PA_PUBLIC_PRIMITIVE_ATTRIBUTE", | ||
| NORMAL_PRIORITY) | ||
| .addClass(this).addField(field).addSourceLine(sla)); | ||
| 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; | ||
| } | ||
|
|
||
| SourceLineAnnotation sla = fieldDefLineMap.containsKey(field) | ||
| ? fieldDefLineMap.get(field) | ||
| : SourceLineAnnotation.fromVisitedInstruction(this); | ||
|
|
||
| bugReporter.reportBug(new BugInstance(this, | ||
| "PA_PUBLIC_ARRAY_ATTRIBUTE", | ||
| NORMAL_PRIORITY) | ||
| .addClass(this).addField(field).addSourceLine(sla)); | ||
| 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; | ||
| } | ||
|
|
||
| // Mutable static fields are already detected by MS | ||
| if (field.isStatic()) { | ||
| return; | ||
| } | ||
|
|
||
| // Only consider attributes of self. | ||
| if (!field.getClassDescriptor().equals(getClassDescriptor())) { | ||
| return; | ||
| } | ||
|
|
||
| if (!field.isPublic() || !field.isReferenceType()) { | ||
| return; | ||
| } | ||
|
|
||
| if (!MutableClasses.mutableSignature(field.getSignature())) { | ||
| return; | ||
| } | ||
|
|
||
| SourceLineAnnotation sla = fieldDefLineMap.containsKey(field) | ||
| ? fieldDefLineMap.get(field) | ||
| : SourceLineAnnotation.fromVisitedInstruction(this); | ||
|
|
||
| bugReporter.reportBug(new BugInstance(this, | ||
| "PA_PUBLIC_MUTABLE_OBJECT_ATTRIBUTE", | ||
| NORMAL_PRIORITY) | ||
| .addClass(this).addField(field).addSourceLine(sla)); | ||
| writtenFields.add(field); | ||
| } | ||
| } | ||
|
|
||
| private static boolean isConstructorLikeMethod(String methodName) { | ||
| return CONSTRUCTOR_LIKE_NAMES.contains(methodName); | ||
| } | ||
|
|
||
| private static boolean looksLikeASetter(String methodName) { | ||
| for (String name : SETTER_LIKE_NAMES) { | ||
| if (methodName.startsWith(name)) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
| } |
Oops, something went wrong.