GEODE-7864: Fix some LGTM alerts and suppress some false positives (a…

…pache#5473) - Fix dereferenced variable may be null alerts in Oplog, PartitionedRegion, InitialImageOperation, GMSMembership, ParallelGatewaySenderQueue, DiskStoreImpl, LocalRegion and HARegionQueue - Remove redundant check in Oplog.java - Suppress false positive alerts in SessionCachingFilter, NullToken, Undefined, ClusterDistributionManager and LocalRegion - Add explanatory comments for LGTM suppressions Authored-by: Donal Evans <[email protected]>
IronOnet · Aug 27, 2020 · 93efb80 · 93efb80
1 parent 0c2a8a3
commit 93efb80
Show file tree

Hide file tree

Showing 15 changed files with 195 additions and 184 deletions.
diff --git a/...s-session/src/main/java/org/apache/geode/modules/session/filter/SessionCachingFilter.java b/...s-session/src/main/java/org/apache/geode/modules/session/filter/SessionCachingFilter.java
@@ -209,7 +209,9 @@ private void addSessionCookie(HttpServletResponse response) {
       cookie.setPath("".equals(getContextPath()) ? "/" : getContextPath());
       cookie.setHttpOnly(cookieConfig.isHttpOnly());
       cookie.setSecure(cookieConfig.isSecure());
-      response.addCookie(cookie);
+      response.addCookie(cookie); // lgtm [java/insecure-cookie]
+      // The lgtm alert for the above line is suppressed as a false positive, since we are simply
+      // using the security setting from the context's cookie config
     }
 
     /**

diff --git a/geode-core/src/main/java/org/apache/geode/cache/query/internal/NullToken.java b/geode-core/src/main/java/org/apache/geode/cache/query/internal/NullToken.java
@@ -33,6 +33,9 @@
  */
 public class NullToken implements DataSerializableFixedID, Comparable {
 
+  @SuppressWarnings("lgtm[java/useless-null-check]")
+  // The "useless null check" alert is suppressed here as the first time the constructor is called,
+  // QueryService.NULL is actually null, but subsequent times it is not
   public NullToken() {
     Support.assertState(IndexManager.NULL == null, "NULL constant already instantiated");
   }

diff --git a/geode-core/src/main/java/org/apache/geode/cache/query/internal/Undefined.java b/geode-core/src/main/java/org/apache/geode/cache/query/internal/Undefined.java
@@ -40,12 +40,11 @@ public class Undefined implements DataSerializableFixedID, Comparable, Serializa
 
   private static final long serialVersionUID = 6643107525908324141L;
 
+  @SuppressWarnings("lgtm[java/useless-null-check]")
+  // The "useless null check" alert is suppressed here as the first time the constructor is called,
+  // QueryService.UNDEFINED is actually null, but subsequent times it is not
   public Undefined() {
     Support.assertState(QueryService.UNDEFINED == null, "UNDEFINED constant already instantiated");
-
-    // org.apache.persistence.CanonicalizationHelper
-    // .putCanonicalObj("com/gemstone/persistence/query/QueryService.UNDEFINED",
-    // this);
   }
 
   @Override

diff --git a/...-core/src/main/java/org/apache/geode/distributed/internal/ClusterDistributionManager.java b/...-core/src/main/java/org/apache/geode/distributed/internal/ClusterDistributionManager.java
@@ -1380,13 +1380,16 @@ private void handleViewInstalledEvent(ViewInstalledEvent ev) {
 
   /**
    * This stalls waiting for the current membership view (as seen by the membership manager) to be
-   * acknowledged by all membership listeners
+   * acknowledged by all membership listeners.
    */
+  @SuppressWarnings("lgtm[java/constant-comparison]")
   void waitForViewInstallation(long id) throws InterruptedException {
     if (id <= membershipViewIdAcknowledged) {
       return;
     }
     synchronized (membershipViewIdGuard) {
+      // The LGTM alert for this line is suppressed as it fails to take into account the value of
+      // membershipViewIdAcknowledged being modified by another thread
       while (membershipViewIdAcknowledged < id && !stopper.isCancelInProgress()) {
         if (logger.isDebugEnabled()) {
           logger.debug("waiting for view {}.  Current DM view processed by all listeners is {}", id,

diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/DiskStoreImpl.java b/geode-core/src/main/java/org/apache/geode/internal/cache/DiskStoreImpl.java
@@ -837,7 +837,7 @@ Object get(DiskRegion dr, DiskId id) {
           if (logger.isDebugEnabled()) {
             logger.debug(
                 "DiskRegion: Tried {}, getBytesAndBitsWithoutLock returns wrong byte array: {}",
-                count, Arrays.toString(bb.getBytes()));
+                count, Arrays.toString(bb != null ? bb.getBytes() : null));
           }
           ex = e;
         }

diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/InitialImageOperation.java b/geode-core/src/main/java/org/apache/geode/internal/cache/InitialImageOperation.java
@@ -378,7 +378,7 @@ GIIStatus getFromOne(Set recipientSet, boolean targetReinitialized,
       }
 
       Boolean inhibitFlush = (Boolean) inhibitStateFlush.get();
-      if (!inhibitFlush.booleanValue() && !this.region.doesNotDistribute()) {
+      if (!inhibitFlush && !this.region.doesNotDistribute()) {
         if (region instanceof BucketRegionQueue) {
           // get the corresponding userPRs and do state flush on all of them
           // TODO we should be able to do this state flush with a single
@@ -457,7 +457,8 @@ GIIStatus getFromOne(Set recipientSet, boolean targetReinitialized,
                   this.region.getFullPath());
             }
             m.versionVector = null;
-          } else if (keysOfUnfinishedOps.size() > MAXIMUM_UNFINISHED_OPERATIONS) {
+          } else if (keysOfUnfinishedOps != null
+              && keysOfUnfinishedOps.size() > MAXIMUM_UNFINISHED_OPERATIONS) {
             if (isDebugEnabled) {
               logger.debug(
                   "Region {} has {} unfinished operations, which exceeded threshold {}, do full GII instead",
@@ -843,11 +844,8 @@ boolean processChunk(List entries, InternalDistributedMember sender)
       if (entryCount <= 1000 && isDebugEnabled) {
         keys = new HashSet();
       }
-      final ByteArrayDataInput in = new ByteArrayDataInput();
-      List<Entry> entriesToSynchronize = null;
-      if (this.isSynchronizing) {
-        entriesToSynchronize = new ArrayList<>();
-      }
+      List<Entry> entriesToSynchronize = new ArrayList<>();
+
       for (int i = 0; i < entryCount; i++) {
         // stream is null-terminated
         if (internalDuringApplyDelta != null && !internalDuringApplyDelta.isRunning
@@ -891,13 +889,6 @@ boolean processChunk(List entries, InternalDistributedMember sender)
         final long lastModified = entry.getLastModified(this.region.getDistributionManager());
 
         Object tmpValue = entry.value;
-        byte[] tmpBytes = null;
-
-        {
-          if (tmpValue instanceof byte[]) {
-            tmpBytes = (byte[]) tmpValue;
-          }
-        }
 
         boolean didIIP = false;
         boolean wasRecovered = false;
@@ -939,7 +930,7 @@ boolean processChunk(List entries, InternalDistributedMember sender)
                     tag.replaceNullIDs(sender);
                   }
                   boolean record;
-                  if (this.region.getVersionVector() != null) {
+                  if (this.region.getVersionVector() != null && tag != null) {
                     this.region.getVersionVector().recordVersion(tag.getMemberID(), tag);
                     record = true;
                   } else {
@@ -953,9 +944,7 @@ record = (tmpValue != Token.TOMBSTONE);
                       entriesToSynchronize.add(entry);
                     }
                   }
-                } catch (RegionDestroyedException e) {
-                  return false;
-                } catch (CancelException e) {
+                } catch (RegionDestroyedException | CancelException e) {
                   return false;
                 }
                 didIIP = true;
@@ -990,20 +979,17 @@ record = (tmpValue != Token.TOMBSTONE);
                   "processChunk:initialImagePut:key={},lastModified={},tmpValue={},wasRecovered={},tag={}",
                   entry.key, lastModified, tmpValue, wasRecovered, tag);
             }
-            if (this.region.getVersionVector() != null) {
+            if (this.region.getVersionVector() != null && tag != null) {
               this.region.getVersionVector().recordVersion(tag.getMemberID(), tag);
             }
             this.entries.initialImagePut(entry.key, lastModified, tmpValue, wasRecovered, false,
                 tag, sender, this.isSynchronizing);
             if (this.isSynchronizing) {
               entriesToSynchronize.add(entry);
             }
-          } catch (RegionDestroyedException e) {
-            return false;
-          } catch (CancelException e) {
+          } catch (RegionDestroyedException | CancelException e) {
             return false;
           }
-          didIIP = true;
         }
       }
       if (this.isSynchronizing && !entriesToSynchronize.isEmpty()) {
@@ -1724,7 +1710,8 @@ protected void process(final ClusterDistributionManager dm) {
               if (isGiiDebugEnabled) {
                 RegionVersionHolder holderOfRequest =
                     this.versionVector.getHolderForMember(this.lostMemberVersionID);
-                if (holderToSync.isNewerThanOrCanFillExceptionsFor(holderOfRequest)) {
+                if (holderToSync != null
+                    && holderToSync.isNewerThanOrCanFillExceptionsFor(holderOfRequest)) {
                   logger.trace(LogMarker.INITIAL_IMAGE_VERBOSE,
                       "synchronizeWith detected mismatch region version holder for lost member {}. Old is {}, new is {}",
                       lostMemberVersionID, holderOfRequest, holderToSync);
@@ -2445,9 +2432,11 @@ public void process(DistributionMessage msg) {
       } finally {
         if (received_rvv == null) {
           if (isGiiDebugEnabled) {
-            logger.trace(LogMarker.INITIAL_IMAGE_VERBOSE,
-                "{} did not send back rvv. Maybe it's non-persistent proxy region or remote region {} not found or not initialized. Nothing to do.",
-                reply.getSender(), region.getFullPath());
+            if (reply != null) {
+              logger.trace(LogMarker.INITIAL_IMAGE_VERBOSE,
+                  "{} did not send back rvv. Maybe it's non-persistent proxy region or remote region {} not found or not initialized. Nothing to do.",
+                  reply.getSender(), region.getFullPath());
+            }
           }
         }
         super.process(msg);

diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java b/geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java
@@ -3049,11 +3049,9 @@ void serverPut(EntryEventImpl event, boolean requireOldValue, Object expectedOld
             if (requireOldValue && result == null) {
               throw new EntryNotFoundException("entry not found for replace");
             }
-            if (!requireOldValue) {
-              if (!(Boolean) result) {
-                // customers don't see this exception
-                throw new EntryNotFoundException("entry found with wrong value");
-              }
+            if (!requireOldValue && result != null && !((Boolean) result)) {
+              // customers don't see this exception
+              throw new EntryNotFoundException("entry found with wrong value");
             }
           }
         }
@@ -3369,11 +3367,10 @@ void notifyClientsOfTombstoneGC(Map<VersionSource, Long> regionGCVersions,
         RegionEventImpl regionEvent =
             new RegionEventImpl(this, Operation.REGION_DESTROY, null, true, getMyId());
         regionEvent.setEventID(eventID);
-        FilterInfo clientRouting = routing;
-        if (clientRouting == null) {
-          clientRouting = fp.getLocalFilterRouting(regionEvent);
+        if (routing == null) {
+          routing = fp.getLocalFilterRouting(regionEvent);
         }
-        regionEvent.setLocalFilterInfo(clientRouting);
+        regionEvent.setLocalFilterInfo(routing);
         ClientUpdateMessage clientMessage =
             ClientTombstoneMessage.gc(this, regionGCVersions, eventID);
         CacheClientNotifier.notifyClients(regionEvent, clientMessage);
@@ -3450,7 +3447,9 @@ Object nonTXbasicGetValueInVM(KeyInfo keyInfo) {
       checkEntryNotFound(keyInfo.getKey());
     }
     // OFFHEAP returned to callers
-    Object value = regionEntry.getValueInVM(this);
+    // The warning for regionEntry possibly being null is incorrect, as checkEntryNotFound() always
+    // throws, making the following line unreachable if regionEntry is null
+    Object value = regionEntry.getValueInVM(this); // lgtm [java/dereferenced-value-may-be-null]
     if (Token.isRemoved(value)) {
       checkEntryNotFound(keyInfo.getKey());
     }