Rule for Request #707: AvoidRecreatingMessageDigests - MessageDigest.getInstance is expensive because of classloading

docs/JavaCodePerformance.md

@@ -2237,7 +2237,7 @@ Inefficient use of security features
 
 **Observation: A security provider is re-created, that is, created in a method called more than once.**   
 **Problem:** Creating a security provider is expensive because of loading of algorithms and other classes. 
-Additionally, it uses synchronized which leads to lock contention when used with multiple threads.  
+Additionally, it uses synchronized which leads to lock contention when used with multiple threads, resulting in response time spikes.   
 **Solution:** This only needs to happen once in the JVM lifetime, because once loaded, the provider is available from the Security class. 
 Create the security provider only once: only in case it is not available from the Security class, yet.   
 **Rule name:** AvoidRecreatingSecurityProviders.    
@@ -2259,7 +2259,48 @@ class Foo {
   }
 }
 ```
-**Note:** An addProvider call inside a static main method or inside a @PostConstruct annotated method is not reported as a violation since it is assumed to be called only once. 
+**Note:** An addProvider call inside a static main method or inside a @PostConstruct annotated method is *not* reported as a violation since it is assumed to be called only once. 
+
+#### IUOSF02
+
+**Observation: A MessageDigest object is re-created, that is, created in a method called more than once.**   
+**Problem:** Creating a MessageDigest object is expensive because of provider lookup, loading of algorithms and other classes.
+Additionally, it uses synchronized which leads to lock contention when used with multiple threads, resulting in response time spikes.  
+**Solution:** Since MessageDigest is not thread-safe, an instance cannot simply be shared among threads. Create the MessageDigest only once and clone it for each use. 
+MessageDigest.clone() is efficient and typically supported by the provider, yet note that support is not guaranteed.   
+**Note:** Apache Commons codec DigestUtils does not do such optimization. Only methods which create a MessageDigest under the hood are, therefore, to avoid.   
+**Rule name:** AvoidRecreatingMessageDigests.    
+**Example:**
+```java
+import java.security.MessageDigest;
+import org.apache.commons.codec.digest.DigestUtils;
+import static org.apache.commons.codec.digest.MessageDigestAlgorithms.SHA_256;
+
+class Foo {
+    MessageDigest mdField = MessageDigest.getInstance("SHA-256");
+    byte[] dataToDigest = "Hello World!".getBytes("UTF-8");
+
+    byte[] bad() {
+        MessageDigest mdLocal = MessageDigest.getInstance("SHA-256"); // bad
+        return mdLocal.digest(dataToDigest);
+    }
+
+    byte[] good() {
+        MessageDigest mdLocal = mdField.clone();
+        return mdLocal.digest(dataToDigest);
+    }
+
+    byte[] badDigestUtils() {
+        return new DigestUtils(SHA_256).digest(dataToDigest); // bad
+    }
+
+    byte[] goodDigestUtils() {
+        MessageDigest mdLocal = mdField.clone();
+        return DigestUtils.digest(mdLocal, dataToDigest);
+    }
+}
+```
+**Note:** Creating a MessageDigest inside a static main method or inside a @PostConstruct annotated method is *not* reported as a violation since it is assumed to be called only once.
 
 Extensive use of classpath scanning
 -----------------------------------

rulesets/java/jpinpoint-java-rules.xml

@@ -888,6 +888,59 @@ class Good {
         </properties>
     </rule>
 
+    <rule name="AvoidRecreatingMessageDigests"
+          language="java"
+          message="Avoid re-creating MessageDigest objects, this is expensive."
+          class="net.sourceforge.pmd.lang.rule.xpath.XPathRule"
+
+          externalInfoUrl="https://github.com/jborgers/PMD-jPinpoint-rules/tree/pmd7/docs/JavaCodePerformance.md#iuosf02">
+        <description>Problem: Creating a MessageDigest object is expensive because of loading of algorithms and other classes. Additionally, it uses synchronized which leads to lock contention when used with multiple threads.
+            Solution: Since MessageDigest is not thread-safe, an instance cannot simply be shared among threads. Create the MessageDigest only once and clone it for each use. MessageDigest.clone() is efficient and typically supported by the provider, yet note that support is not guaranteed.
+        (jpinpoint-rules)</description>
+        <priority>2</priority>
+        <properties>
+            <property name="tags" value="cpu,io,jpinpoint-rule,performance,sustainability-high" type="String" description="classification"/>
+            <property name="xpath">
+                <value><![CDATA[
+//MethodDeclaration[not((@Name='main' and @Static=true()) or ModifierList/Annotation/@SimpleName='PostConstruct')]//(
+    LocalVariableDeclaration[ClassType[pmd-java:typeIs('java.security.MessageDigest')]]/VariableDeclarator/MethodCall[@MethodName='getInstance']
+  | MethodCall[starts-with(@MethodName,'digest')]/ConstructorCall[pmd-java:typeIs('org.apache.commons.codec.digest.DigestUtils')]
+  | LocalVariableDeclaration[ClassType[pmd-java:typeIs('java.security.MessageDigest')]]//MethodCall/TypeExpression[pmd-java:typeIs('org.apache.commons.codec.digest.DigestUtils')]
+  | MethodCall[starts-with(@MethodName,'sha') or starts-with(@MethodName,'md')]/TypeExpression[pmd-java:typeIs('org.apache.commons.codec.digest.DigestUtils')]
+)
+                ]]></value>
+            </property>
+        </properties>
+        <example>
+            <![CDATA[
+import java.security.MessageDigest;
+import org.apache.commons.codec.digest.DigestUtils;
+import static org.apache.commons.codec.digest.MessageDigestAlgorithms.SHA_256;
+
+class MessageDigestIssue {
+    MessageDigest mdField = MessageDigest.getInstance("SHA-256");
+    byte[] dataToDigest = "helloworld".getBytes("UTF-8");
+
+    byte[] bad() {
+        MessageDigest mdLocal = MessageDigest.getInstance("SHA-256"); // bad
+        return mdLocal.digest(dataToDigest);
+    }
+    byte[] good() {
+        MessageDigest mdLocal = mdField.clone();
+        return mdLocal.digest(dataToDigest);
+    }
+    byte[] badDigestUtils() {
+        return new DigestUtils(SHA_256).digest(dataToDigest); // bad
+    }
+    byte[] goodDigestUtils() {
+        MessageDigest mdLocal = mdField.clone();
+        return DigestUtils.digest(mdLocal, dataToDigest);
+    }
+}
+            ]]>
+        </example>
+    </rule>
+
     <rule name="AvoidRecreatingSecurityProviders"
           language="java"
           message="Avoid re-creating security providers, this is expensive."
@@ -903,7 +956,7 @@ class Good {
             <property name="xpath">
                 <value><![CDATA[
 //MethodDeclaration
-   [not((@Name='main' and @Static=true())or ModifierList/Annotation/@SimpleName='PostConstruct'
+   [not((@Name='main' and @Static=true()) or ModifierList/Annotation/@SimpleName='PostConstruct'
       or .//IfStatement//InfixExpression
          [@Operator='=='][VariableAccess[pmd-java:typeIs('java.security.Provider')] and NullLiteral]
    )]

rulesets/java/jpinpoint-rules.xml

@@ -888,6 +888,59 @@ class Good {
         </properties>
     </rule>
 
+    <rule name="AvoidRecreatingMessageDigests"
+          language="java"
+          message="Avoid re-creating MessageDigest objects, this is expensive."
+          class="net.sourceforge.pmd.lang.rule.xpath.XPathRule"
+
+          externalInfoUrl="https://github.com/jborgers/PMD-jPinpoint-rules/tree/pmd7/docs/JavaCodePerformance.md#iuosf02">
+        <description>Problem: Creating a MessageDigest object is expensive because of loading of algorithms and other classes. Additionally, it uses synchronized which leads to lock contention when used with multiple threads.
+            Solution: Since MessageDigest is not thread-safe, an instance cannot simply be shared among threads. Create the MessageDigest only once and clone it for each use. MessageDigest.clone() is efficient and typically supported by the provider, yet note that support is not guaranteed.
+        (jpinpoint-rules)</description>
+        <priority>2</priority>
+        <properties>
+            <property name="tags" value="cpu,io,jpinpoint-rule,performance,sustainability-high" type="String" description="classification"/>
+            <property name="xpath">
+                <value><![CDATA[
+//MethodDeclaration[not((@Name='main' and @Static=true()) or ModifierList/Annotation/@SimpleName='PostConstruct')]//(
+    LocalVariableDeclaration[ClassType[pmd-java:typeIs('java.security.MessageDigest')]]/VariableDeclarator/MethodCall[@MethodName='getInstance']
+  | MethodCall[starts-with(@MethodName,'digest')]/ConstructorCall[pmd-java:typeIs('org.apache.commons.codec.digest.DigestUtils')]
+  | LocalVariableDeclaration[ClassType[pmd-java:typeIs('java.security.MessageDigest')]]//MethodCall/TypeExpression[pmd-java:typeIs('org.apache.commons.codec.digest.DigestUtils')]
+  | MethodCall[starts-with(@MethodName,'sha') or starts-with(@MethodName,'md')]/TypeExpression[pmd-java:typeIs('org.apache.commons.codec.digest.DigestUtils')]
+)
+                ]]></value>
+            </property>
+        </properties>
+        <example>
+            <![CDATA[
+import java.security.MessageDigest;
+import org.apache.commons.codec.digest.DigestUtils;
+import static org.apache.commons.codec.digest.MessageDigestAlgorithms.SHA_256;
+
+class MessageDigestIssue {
+    MessageDigest mdField = MessageDigest.getInstance("SHA-256");
+    byte[] dataToDigest = "helloworld".getBytes("UTF-8");
+
+    byte[] bad() {
+        MessageDigest mdLocal = MessageDigest.getInstance("SHA-256"); // bad
+        return mdLocal.digest(dataToDigest);
+    }
+    byte[] good() {
+        MessageDigest mdLocal = mdField.clone();
+        return mdLocal.digest(dataToDigest);
+    }
+    byte[] badDigestUtils() {
+        return new DigestUtils(SHA_256).digest(dataToDigest); // bad
+    }
+    byte[] goodDigestUtils() {
+        MessageDigest mdLocal = mdField.clone();
+        return DigestUtils.digest(mdLocal, dataToDigest);
+    }
+}
+            ]]>
+        </example>
+    </rule>
+
     <rule name="AvoidRecreatingSecurityProviders"
           language="java"
           message="Avoid re-creating security providers, this is expensive."
@@ -903,7 +956,7 @@ class Good {
             <property name="xpath">
                 <value><![CDATA[
 //MethodDeclaration
-   [not((@Name='main' and @Static=true())or ModifierList/Annotation/@SimpleName='PostConstruct'
+   [not((@Name='main' and @Static=true()) or ModifierList/Annotation/@SimpleName='PostConstruct'
       or .//IfStatement//InfixExpression
          [@Operator='=='][VariableAccess[pmd-java:typeIs('java.security.Provider')] and NullLiteral]
    )]

src/main/resources/category/java/common.xml

@@ -2270,7 +2270,7 @@ class Good {
             <property name="xpath">
                 <value><![CDATA[
 //MethodDeclaration
-   [not((@Name='main' and @Static=true())or ModifierList/Annotation/@SimpleName='PostConstruct'
+   [not((@Name='main' and @Static=true()) or ModifierList/Annotation/@SimpleName='PostConstruct'
       or .//IfStatement//InfixExpression
          [@Operator='=='][VariableAccess[pmd-java:typeIs('java.security.Provider')] and NullLiteral]
    )]
@@ -2585,4 +2585,57 @@ import java.time.LocalDataTime; // good
         ]]>
         </example>
     </rule>
+
+    <rule name="AvoidRecreatingMessageDigests"
+          language="java"
+          message="Avoid re-creating MessageDigest objects, this is expensive."
+          class="net.sourceforge.pmd.lang.rule.xpath.XPathRule"
+
+          externalInfoUrl="${doc_root}/JavaCodePerformance.md#iuosf02">
+        <description>Problem: Creating a MessageDigest object is expensive because of loading of algorithms and other classes. Additionally, it uses synchronized which leads to lock contention when used with multiple threads.
+            Solution: Since MessageDigest is not thread-safe, an instance cannot simply be shared among threads. Create the MessageDigest only once and clone it for each use. MessageDigest.clone() is efficient and typically supported by the provider, yet note that support is not guaranteed.
+        </description>
+        <priority>2</priority>
+        <properties>
+            <property name="tags" value="cpu,io,jpinpoint-rule,performance,sustainability-high" type="String" description="classification"/>
+            <property name="xpath">
+                <value><![CDATA[
+//MethodDeclaration[not((@Name='main' and @Static=true()) or ModifierList/Annotation/@SimpleName='PostConstruct')]//(
+    LocalVariableDeclaration[ClassType[pmd-java:typeIs('java.security.MessageDigest')]]/VariableDeclarator/MethodCall[@MethodName='getInstance']
+  | MethodCall[starts-with(@MethodName,'digest')]/ConstructorCall[pmd-java:typeIs('org.apache.commons.codec.digest.DigestUtils')]
+  | LocalVariableDeclaration[ClassType[pmd-java:typeIs('java.security.MessageDigest')]]//MethodCall/TypeExpression[pmd-java:typeIs('org.apache.commons.codec.digest.DigestUtils')]
+  | MethodCall[starts-with(@MethodName,'sha') or starts-with(@MethodName,'md')]/TypeExpression[pmd-java:typeIs('org.apache.commons.codec.digest.DigestUtils')]
+)
+                ]]></value>
+            </property>
+        </properties>
+        <example>
+            <![CDATA[
+import java.security.MessageDigest;
+import org.apache.commons.codec.digest.DigestUtils;
+import static org.apache.commons.codec.digest.MessageDigestAlgorithms.SHA_256;
+
+class MessageDigestIssue {
+    MessageDigest mdField = MessageDigest.getInstance("SHA-256");
+    byte[] dataToDigest = "helloworld".getBytes("UTF-8");
+
+    byte[] bad() {
+        MessageDigest mdLocal = MessageDigest.getInstance("SHA-256"); // bad
+        return mdLocal.digest(dataToDigest);
+    }
+    byte[] good() {
+        MessageDigest mdLocal = mdField.clone();
+        return mdLocal.digest(dataToDigest);
+    }
+    byte[] badDigestUtils() {
+        return new DigestUtils(SHA_256).digest(dataToDigest); // bad
+    }
+    byte[] goodDigestUtils() {
+        MessageDigest mdLocal = mdField.clone();
+        return DigestUtils.digest(mdLocal, dataToDigest);
+    }
+}
+            ]]>
+        </example>
+    </rule>
 </ruleset>

src/test/java/com/jpinpoint/perf/lang/java/ruleset/common/AvoidRecreatingMessageDigestsTest.java

@@ -0,0 +1,6 @@
+package com.jpinpoint.perf.lang.java.ruleset.common;
+
+import net.sourceforge.pmd.test.PmdRuleTst;
+
+public class AvoidRecreatingMessageDigestsTest extends PmdRuleTst {
+}

src/test/resources/com/jpinpoint/perf/lang/java/ruleset/common/xml/AvoidRecreatingMessageDigests.xml

@@ -0,0 +1,94 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<test-data
+        xmlns="http://pmd.sourceforge.net/rule-tests"
+        xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+        xsi:schemaLocation="http://pmd.sourceforge.net/rule-tests http://pmd.sourceforge.net/rule-tests_1_0_0.xsd">
+    <test-code>
+        <description>violation: avoid recreating message digests</description>
+        <expected-problems>5</expected-problems>
+        <expected-linenumbers>10,19,23,31,36</expected-linenumbers>
+        <code><![CDATA[
+import java.security.MessageDigest;
+import org.apache.commons.codec.digest.DigestUtils;
+import static org.apache.commons.codec.digest.MessageDigestAlgorithms.SHA_256;
+
+class MessageDigestIssue {
+    MessageDigest mdField = MessageDigest.getInstance("SHA-256");
+    byte[] dataToDigest = "helloworld".getBytes("UTF-8");
+
+    byte[] bad() {
+        MessageDigest mdLocal = MessageDigest.getInstance("SHA-256"); // bad1
+        return mdLocal.digest(dataToDigest);
+    }
+    byte[] good() {
+        MessageDigest mdLocal = mdField.clone();
+        return mdLocal.digest(dataToDigest);
+    }
+
+    byte[] bad1DigestUtils() {
+        return new DigestUtils(SHA_256).digest(dataToDigest); // bad2
+    }
+
+    String bad2DigestUtils() {
+        return new DigestUtils(SHA_256).digestAsHex(dataToDigest); //bad3
+    }
+
+    MessageDigest good1DigestUtils() {
+        return DigestUtils.getMd5Digest(); // okay, can be used to initialize once
+    }
+
+    byte[] bad3DigestUtils() {
+        MessageDigest mdLocal = DigestUtils.getMd5Digest(); // bad4, as local var
+        return mdLocal.digest(dataToDigest);
+    }
+
+     byte[] bad4DigestUtils() {
+        return DigestUtils.sha256(dataToDigest); // bad5
+    }
+
+    // for the simple good cases I see no advantage of using DigestUtils.
+    // But for others like streaming it seems useful, to add
+    byte[] good2DigestUtils() {
+        MessageDigest mdLocal = mdField.clone();
+        return DigestUtils.digest(mdLocal, dataToDigest);
+    }
+
+    MessageDigest good3DigestUtils() {
+        MessageDigest mdLocal = mdField.clone();
+        return DigestUtils.updateDigest(mdLocal, dataToDigest);
+    }
+}
+     ]]></code>
+    </test-code>
+    <test-code>
+        <description>no violation: creating a MessageDigest in main is allowed</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+import java.security.MessageDigest;
+
+class MessageDigestInMain {
+    public static void main(String[] args) throws Exception {
+        MessageDigest mdLocal = MessageDigest.getInstance("SHA-256"); // okay, in main
+        mdLocal.digest("helloworld".getBytes("UTF-8"));
+    }
+}
+     ]]></code>
+    </test-code>
+    <test-code>
+        <description>no violation: creating a MessageDigest in a @PostConstruct method is allowed</description>
+        <expected-problems>0</expected-problems>
+        <code><![CDATA[
+import java.security.MessageDigest;
+import javax.annotation.PostConstruct;
+
+class MessageDigestInPostConstruct {
+    private MessageDigest mdField;
+
+    @PostConstruct
+    void init() throws Exception {
+        mdField = MessageDigest.getInstance("SHA-256"); // okay, in @PostConstruct
+    }
+}
+     ]]></code>
+    </test-code>
+</test-data>