From 9d9c64dc4fb45d15847ed1081256f8f0f975e15c Mon Sep 17 00:00:00 2001
From: jborgers <jborgers@jpinpoint.com>
Date: Thu, 16 Apr 2020 17:10:38 +0200
Subject: [PATCH] Extend
 AvoidUnguardedAssignmentToNonFinalFieldsInSharedObjects rule for inherited
 fields #18

---
 rulesets/java/jpinpoint-rules.xml             | 26 ++++++---
 .../resources/category/java/concurrent.xml    | 10 +++-
 ...ignmentToNonFinalFieldsInSharedObjects.xml | 54 +++++++++++++++----
 3 files changed, 70 insertions(+), 20 deletions(-)

diff --git a/rulesets/java/jpinpoint-rules.xml b/rulesets/java/jpinpoint-rules.xml
index a8fd2a05..e878cf44 100644
--- a/rulesets/java/jpinpoint-rules.xml
+++ b/rulesets/java/jpinpoint-rules.xml
@@ -701,23 +701,25 @@ ClassOrInterfaceBodyDeclaration//MethodDeclaration/Block//PrimaryExpression/Prim
         <example>
             <![CDATA[
 class Bad {
+    PortletSession session; // same for HttpSession
     public void setAttribute(String name, Object obj) {
-	    getPortletSession().setAttribute(name, obj);                 // bad
+	    session.setAttribute(name, obj);                 // bad
 	}
-	public Object getAttribute(String sessionAttributeName) {
-		return getPortletSession().getAttribute(sessionAttributeName);
+	public Object getAttribute(String name) {
+		return session.getAttribute(name);
 	}
 }
 
 class Good {
+    PortletSession session; // same for HttpSession
 	public void setAttribute(String name, final Object obj) {
-		getPortletSession().setAttribute(name, obj);
+		session.setAttribute(name, obj);
 	}
-	public Object getAttribute(String sessionAttributeName) {
-		return getPortletSession().getAttribute(sessionAttributeName);
+	public Object getAttribute(String name) {
+		return session.getAttribute(name);
 	}
 	public void removeAttribute(String name) {
-		getPortletSession().removeAttribute(name);
+		session.removeAttribute(name);
 	}
 }
             ]]>
@@ -1314,6 +1316,7 @@ or (ancestor::ClassOrInterfaceBodyDeclaration/Annotation//Name[@Image='VisibleFo
             1. Autowiring/injection is thread safe, yet make sure no other thread-unsafe assignment is made to that field.&#13;
             2. In case you are sure the Component is used in single threaded context only (e.g. a Tasklet), annotate the class with @NotThreadSafe to make this explicit. &#13;
             3. Use package-private and @VisibleForTesting for methods (e.g. setters) used for JUnit only.
+            4. Use synchronized for accessors to inherited fields, or better: make field private and use proper accessors on base class level using @GuardedBy.
             (jpinpoint-rules)</description>
         <priority>2</priority>
         <properties>
@@ -1331,9 +1334,16 @@ and not (ancestor::TypeDeclaration/Annotation/MarkerAnnotation/Name[@Image='NotT
 and not ((ancestor::TypeDeclaration/Annotation//Name[@Image='ConfigurationProperties'])
          and not (ancestor::TypeDeclaration/Annotation/MarkerAnnotation/Name[@Image='Setter']))]
 (: assignment to a field :)
-/../../..//ClassOrInterfaceDeclaration[@Static=false()]/ClassOrInterfaceBody/ClassOrInterfaceBodyDeclaration/MethodDeclaration/Block/BlockStatement/Statement//StatementExpression/AssignmentOperator/../PrimaryExpression//*[@Image=
+/../../..//ClassOrInterfaceDeclaration[@Static=false()]/ClassOrInterfaceBody/ClassOrInterfaceBodyDeclaration/MethodDeclaration/Block/BlockStatement/Statement//StatementExpression/AssignmentOperator/../PrimaryExpression//*[(@Image=
 (: non-final, non-volatile, non-GuardedBy fields :)
 ancestor::ClassOrInterfaceBody/ClassOrInterfaceBodyDeclaration/FieldDeclaration[@Final=false() and @Volatile=false() and not (../Annotation//Name[@Image='GuardedBy'])]/VariableDeclarator/VariableDeclaratorId/@Image
+or
+(: not 'this' and extends a base class :)
+((@Image != '') and ancestor::ClassOrInterfaceDeclaration/ExtendsList and
+(: not a field of this class, param or local :)
+not (exists(index-of((ancestor::ClassOrInterfaceBody//VariableDeclaratorId/@Image), @Image))))
+and not (ancestor::SynchronizedStatement or ancestor::MethodDeclaration[@Synchronized = true()])
+)
 (: field not on accessor method with assignment, annotated with framework annotation :)
 and not (ancestor::ClassOrInterfaceBodyDeclaration/Annotation//Name[@Image='Autowired' or @Image='PostConstruct' or @Image='BeforeStep' or @Image='Value' or @Image='Inject']
 (: field not assigned in non-public accessor method annotated with VisibleForTesting :)
diff --git a/src/main/resources/category/java/concurrent.xml b/src/main/resources/category/java/concurrent.xml
index ddbf23bf..063fda1c 100644
--- a/src/main/resources/category/java/concurrent.xml
+++ b/src/main/resources/category/java/concurrent.xml
@@ -182,6 +182,7 @@ ancestor::StatementExpression/PrimaryExpression/PrimaryPrefix/Name/@Image = ance
             1. Autowiring/injection is thread safe, yet make sure no other thread-unsafe assignment is made to that field.&#13;
             2. In case you are sure the Component is used in single threaded context only (e.g. a Tasklet), annotate the class with @NotThreadSafe to make this explicit. &#13;
             3. Use package-private and @VisibleForTesting for methods (e.g. setters) used for JUnit only.
+            4. Use synchronized for accessors to inherited fields, or better: make field private and use proper accessors on base class level using @GuardedBy.
             (jpinpoint-rules)</description>
         <priority>2</priority>
         <properties>
@@ -199,9 +200,16 @@ and not (ancestor::TypeDeclaration/Annotation/MarkerAnnotation/Name[@Image='NotT
 and not ((ancestor::TypeDeclaration/Annotation//Name[@Image='ConfigurationProperties'])
          and not (ancestor::TypeDeclaration/Annotation/MarkerAnnotation/Name[@Image='Setter']))]
 (: assignment to a field :)
-/../../..//ClassOrInterfaceDeclaration[@Static=false()]/ClassOrInterfaceBody/ClassOrInterfaceBodyDeclaration/MethodDeclaration/Block/BlockStatement/Statement//StatementExpression/AssignmentOperator/../PrimaryExpression//*[@Image=
+/../../..//ClassOrInterfaceDeclaration[@Static=false()]/ClassOrInterfaceBody/ClassOrInterfaceBodyDeclaration/MethodDeclaration/Block/BlockStatement/Statement//StatementExpression/AssignmentOperator/../PrimaryExpression//*[(@Image=
 (: non-final, non-volatile, non-GuardedBy fields :)
 ancestor::ClassOrInterfaceBody/ClassOrInterfaceBodyDeclaration/FieldDeclaration[@Final=false() and @Volatile=false() and not (../Annotation//Name[@Image='GuardedBy'])]/VariableDeclarator/VariableDeclaratorId/@Image
+or
+(: not 'this' and extends a base class :)
+((@Image != '') and ancestor::ClassOrInterfaceDeclaration/ExtendsList and
+(: not a field of this class, param or local :)
+not (exists(index-of((ancestor::ClassOrInterfaceBody//VariableDeclaratorId/@Image), @Image))))
+and not (ancestor::SynchronizedStatement or ancestor::MethodDeclaration[@Synchronized = true()])
+)
 (: field not on accessor method with assignment, annotated with framework annotation :)
 and not (ancestor::ClassOrInterfaceBodyDeclaration/Annotation//Name[@Image='Autowired' or @Image='PostConstruct' or @Image='BeforeStep' or @Image='Value' or @Image='Inject']
 (: field not assigned in non-public accessor method annotated with VisibleForTesting :)
diff --git a/src/test/resources/com/jpinpoint/perf/lang/java/ruleset/concurrent/xml/AvoidUnguardedAssignmentToNonFinalFieldsInSharedObjects.xml b/src/test/resources/com/jpinpoint/perf/lang/java/ruleset/concurrent/xml/AvoidUnguardedAssignmentToNonFinalFieldsInSharedObjects.xml
index a4e1e005..f1f615e1 100644
--- a/src/test/resources/com/jpinpoint/perf/lang/java/ruleset/concurrent/xml/AvoidUnguardedAssignmentToNonFinalFieldsInSharedObjects.xml
+++ b/src/test/resources/com/jpinpoint/perf/lang/java/ruleset/concurrent/xml/AvoidUnguardedAssignmentToNonFinalFieldsInSharedObjects.xml
@@ -5,7 +5,8 @@
         xsi:schemaLocation="http://pmd.sourceforge.net/rule-tests http://pmd.sourceforge.net/rule-tests_1_0_0.xsd">
     <test-code>
         <description>Avoid unguarded assignment to non-final fields in shared objects</description>
-        <expected-problems>10</expected-problems>
+        <expected-problems>11</expected-problems>
+        <expected-linenumbers>55, 87, 96, 137, 138, 157, 198, 223, 328, 338, 395</expected-linenumbers>
         <code><![CDATA[
 import com.google.common.annotations.VisibleForTesting;
 import lombok.Data;
@@ -61,7 +62,7 @@ class AService1 {
     private String url; // violation on next line, of additional unguarded accessor method
 
     public void setUrlViolate(final String url) {
-        this.url = url;
+        this.url = url;                                         // bad
     } // violation: unguarded accessor method
 }
 
@@ -93,7 +94,7 @@ class ASingletonBean {
     private String url;
 
     public void setUrlViolate(final String url) {
-        this.url = url;
+        this.url = url;                                       // bad
     } // violation: unguarded accessor method
 }
 
@@ -102,7 +103,7 @@ class ASingletonBean {
 class AComponent2 {
     private String _url;
     public void setUrlViolate(final String url) {
-        _url = url;
+        _url = url;                                                // bad
     } // violation: unguarded accessor method
 }
 
@@ -143,8 +144,8 @@ class AnotherComponent {
     private String wiredOk;
 
     public void setUrlViolate(String url) {
-        url += "/";
-        this.url = url;
+        url += "/";                                          // bad
+        this.url = url;                                      // bad
     } // violation: unguarded accessor method
 
     public void setVolatileOk(String vol) {
@@ -163,7 +164,7 @@ class AnotherComponent {
 class ASessionScopedComponent {
     private String _url;
     public void setUrlViolate(final String url) {
-        _url = url;
+        _url = url;                                           // bad
     } // violation: unguarded accessor method
 }
 
@@ -204,7 +205,7 @@ class ASyncedSingletonConcurrencyBean {
 
     @VisibleForTesting
     public void setUrlForTestingOnlyViolate(String urlForTestingOnly) {
-        this.url = urlForTestingOnly;
+        this.url = urlForTestingOnly;                                             // bad
     }
 }
 
@@ -229,7 +230,7 @@ class AComponentMaoValidation {
     }
 
     public void doRestTemplateViolate() {
-        restTemplateViolate = null;
+        restTemplateViolate = null;                                 // bad
     }
 
     @Data
@@ -334,7 +335,7 @@ class APayRequestOutSender {
     }
     private void setMax() {
         try {
-            databaseQueryMaxResultsViolation = 5000;
+            databaseQueryMaxResultsViolation = 5000;                           // bad
         } catch (RuntimeException e) {}
     }
 }
@@ -344,7 +345,7 @@ abstract class AInitializeTaskletViolate implements Tasklet, InitializingBean {
     // Work directory for this instance only.
     protected String workDirectoryViolate;
     public void setWorkDirectory(final String workDirectory) {
-        this.workDirectoryViolate = workDirectory;
+        this.workDirectoryViolate = workDirectory;                              // bad
     }
 
 }
@@ -392,6 +393,37 @@ class Foo {
     String CONTEXT_ATTRIBUTES_BEAN_NAME = "contextAttributes";
 */
 
+// inherited field used
+@Component
+class SubClassBad extends Base {
+
+    SubClassBad() {
+        baseField = ""; // good, could be final
+    }
+
+    public void setBaseField(final String val) {
+        baseField = val;                                        // bad
+    } // violation: unguarded accessor method
+}
+
+// inherited field used
+@Component
+class SubClassGood extends Base {
+    private final Object LOCK = new Object();
+    SubClassGood() {
+        baseField = ""; // good, could be final
+    }
+
+    public synchronized void setBaseField1(final String val) {
+        baseField = val; // assumed good
+    }
+    public  void setBaseField2(final String val) {
+        synchronized(LOCK) {
+            baseField = val; // assumed good
+        }
+    }
+}
+
         ]]></code>
     </test-code>
 </test-data>