Fix potential NPE for switch statement when null case is missing (#20044)

gaobinlong · web-flow · commit c4b12cf1c06a · 2025-11-21T11:05:24.000+08:00
* Fix potential NPE for switch case

Signed-off-by: Binlong Gao &lt;gbinlong@amazon.com&gt;

* Remove null case if checked null before switch

Signed-off-by: Binlong Gao &lt;gbinlong@amazon.com&gt;

* Add more unit tests

Signed-off-by: Binlong Gao &lt;gbinlong@amazon.com&gt;

---------

Signed-off-by: Binlong Gao &lt;gbinlong@amazon.com&gt;
diff --git a/modules/lang-painless/src/main/java/org/opensearch/painless/ir/ConstantNode.java b/modules/lang-painless/src/main/java/org/opensearch/painless/ir/ConstantNode.java
@@ -82,7 +82,7 @@ protected void write(ClassWriter classWriter, MethodWriter methodWriter, WriteSc
             case Short shortValue -> methodWriter.push(shortValue);
             case Byte byteValue -> methodWriter.push(byteValue);
             case Boolean boolValue -> methodWriter.push(boolValue);
-            default -> throw new IllegalStateException("unexpected constant [" + constant + "]");
+            case null, default -> throw new IllegalStateException("unexpected constant [" + constant + "]");
         }
     }
 }
diff --git a/modules/percolator/src/main/java/org/opensearch/percolator/QueryAnalyzer.java b/modules/percolator/src/main/java/org/opensearch/percolator/QueryAnalyzer.java
@@ -216,7 +216,7 @@ public void visitLeaf(Query query) {
                 case MatchAllDocsQuery ignored -> terms.add(new Result(true, true));
                 case MatchNoDocsQuery ignored -> terms.add(Result.MATCH_NONE);
                 case PointRangeQuery pointRangeQuery -> terms.add(pointRangeQuery(pointRangeQuery));
-                default -> terms.add(Result.UNKNOWN);
+                case null, default -> terms.add(Result.UNKNOWN);
             }
         }
 
diff --git a/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/opensearchexception/OpenSearchExceptionProtoUtils.java b/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/opensearchexception/OpenSearchExceptionProtoUtils.java
@@ -247,7 +247,7 @@ public static Map<String, ObjectMap.Value> metadataToProto(OpenSearchException e
             case SearchParseException spe -> SearchParseExceptionProtoUtils.metadataToProto(spe);
             case SearchPhaseExecutionException spee -> SearchPhaseExecutionExceptionProtoUtils.metadataToProto(spee);
             case MultiBucketConsumerService.TooManyBucketsException tmbe -> TooManyBucketsExceptionProtoUtils.metadataToProto(tmbe);
-            default -> new HashMap<>();
+            case null, default -> new HashMap<>();
         };
     }
 }
diff --git a/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/DefaultShardOperationFailedExceptionProtoUtils.java b/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/DefaultShardOperationFailedExceptionProtoUtils.java
@@ -45,6 +45,8 @@ public static ShardFailure toProto(DefaultShardOperationFailedException exceptio
                 indicesShardStoresFailure
             );
             case CloseIndexResponse.ShardResult.Failure closeIndexFailure -> innerToProto(shardFailureBuilder, closeIndexFailure);
+            case null -> {
+            }
             default -> parentInnerToProto(shardFailureBuilder, exception);
         }
         return shardFailureBuilder.build();
diff --git a/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardOperationFailedExceptionProtoUtils.java b/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardOperationFailedExceptionProtoUtils.java
@@ -40,6 +40,9 @@ public static ShardFailure toProto(ShardOperationFailedException exception) thro
             case SnapshotShardFailure ssf -> SnapshotShardFailureProtoUtils.toProto(ssf);
             case DefaultShardOperationFailedException dsofe -> DefaultShardOperationFailedExceptionProtoUtils.toProto(dsofe);
             case ReplicationResponse.ShardInfo.Failure sf -> ReplicationResponseShardInfoFailureProtoUtils.toProto(sf);
+            case null -> throw new UnsupportedOperationException(
+                "Unsupported ShardOperationFailedException [null] cannot be converted to proto."
+            );
             default -> throw new UnsupportedOperationException(
                 "Unsupported ShardOperationFailedException " + exception.getClass().getName() + "cannot be converted to proto."
             );
diff --git a/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/util/GrpcErrorHandler.java b/modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/util/GrpcErrorHandler.java
@@ -97,7 +97,10 @@ public static StatusRuntimeException convertToGrpcError(Exception e) {
             case IOException ioException -> {
                 return Status.INTERNAL.withDescription(ExceptionsHelper.stackTrace(ioException)).asRuntimeException();
             }
-
+            case null -> {
+                logger.warn("Unexpected null exception type, treating as INTERNAL error");
+                return Status.INTERNAL.withDescription("Unexpected null exception").asRuntimeException();
+            }
             // ========== 5. Unknown/Unmapped Exceptions ==========
             // Safety fallback for any unexpected exception to {@code Status.INTERNAL} with full debugging info
             default -> {
diff --git a/modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/DefaultShardOperationFailedExceptionProtoUtilsTests.java b/modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/DefaultShardOperationFailedExceptionProtoUtilsTests.java
@@ -122,4 +122,13 @@ public void testToProtoWithNullNodeId() throws IOException {
         assertFalse("Node should not be set", shardFailure.hasNode());
         assertNotNull("Reason should not be null", shardFailure.getReason());
     }
+
+    public void testToProtoWithNull() throws IOException {
+        ShardFailure shardFailure = DefaultShardOperationFailedExceptionProtoUtils.toProto(null);
+        assertNotNull("ShardFailure should not be null", shardFailure);
+        assertFalse("Index should not be set", shardFailure.hasIndex());
+        assertFalse("Status should not be set", shardFailure.hasStatus());
+        assertFalse("Node should not be set", shardFailure.hasNode());
+        assertFalse("Reason should not be set", shardFailure.hasReason());
+    }
 }
diff --git a/modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardOperationFailedExceptionProtoUtilsTests.java b/modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/exceptions/shardoperationfailedexception/ShardOperationFailedExceptionProtoUtilsTests.java
@@ -119,4 +119,14 @@ public void testToProtoWithUnsupportedShardOperationFailedException() {
             exception.getMessage().contains("Unsupported ShardOperationFailedException")
         );
     }
+
+    public void testToProtoWithNullShardOperationFailedException() {
+        // Call the method under test with null, should throw UnsupportedOperationException
+        UnsupportedOperationException exception = expectThrows(
+            UnsupportedOperationException.class,
+            () -> ShardOperationFailedExceptionProtoUtils.toProto(null)
+        );
+
+        assertEquals("Unsupported ShardOperationFailedException [null] cannot be converted to proto.", exception.getMessage());
+    }
 }
diff --git a/modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/util/GrpcErrorHandlerTests.java b/modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/util/GrpcErrorHandlerTests.java
@@ -279,4 +279,12 @@ public RestStatus status() {
         assertTrue(result.getMessage().contains("Root cause"));
         assertTrue(result.getMessage().contains("Wrapper exception"));
     }
+
+    public void testNullExceptionConversion() {
+        StatusRuntimeException result = GrpcErrorHandler.convertToGrpcError(null);
+
+        // null -> INTERNAL via case null handling
+        assertEquals(Status.INTERNAL.getCode(), result.getStatus().getCode());
+        assertEquals("INTERNAL: Unexpected null exception", result.getMessage());
+    }
 }
diff --git a/server/src/main/java/org/opensearch/common/inject/internal/Errors.java b/server/src/main/java/org/opensearch/common/inject/internal/Errors.java
@@ -356,7 +356,7 @@ public static Collection<Message> getMessagesFromThrowable(Throwable throwable)
             case ProvisionException provisionException -> provisionException.getErrorMessages();
             case ConfigurationException configurationException -> configurationException.getErrorMessages();
             case CreationException creationException -> creationException.getErrorMessages();
-            default -> emptySet();
+            case null, default -> emptySet();
         };
     }
 
diff --git a/server/src/main/java/org/opensearch/common/inject/internal/MoreTypes.java b/server/src/main/java/org/opensearch/common/inject/internal/MoreTypes.java
@@ -143,32 +143,37 @@ public static Type canonicalize(Type type) {
     }
 
     public static Class<?> getRawType(Type type) {
-        if (type instanceof Class<?>) {
-            // type is a normal class.
-            return (Class<?>) type;
-
-        } else if (type instanceof ParameterizedType parameterizedType) {
-
-            // I'm not exactly sure why getRawType() returns Type instead of Class.
-            // Neal isn't either but suspects some pathological case related
-            // to nested classes exists.
-            Type rawType = parameterizedType.getRawType();
-            if (!(rawType instanceof Class)) {
-                throw new IllegalArgumentException("Expected a Class, but <" + type + "> is of type " + type.getClass().getName());
+        switch (type) {
+            case Class<?> aClass -> {
+                // type is a normal class.
+                return aClass;
+                // type is a normal class.
             }
-            return (Class<?>) rawType;
-
-        } else if (type instanceof GenericArrayType) {
-            // TODO: Is this sufficient?
-            return Object[].class;
-
-        } else if (type instanceof TypeVariable) {
-            // we could use the variable's bounds, but that'll won't work if there are multiple.
-            // having a raw type that's more general than necessary is okay
-            return Object.class;
+            case ParameterizedType parameterizedType -> {
 
-        } else {
-            throw new IllegalArgumentException(
+                // I'm not exactly sure why getRawType() returns Type instead of Class.
+                // Neal isn't either but suspects some pathological case related
+                // to nested classes exists.
+                Type rawType = parameterizedType.getRawType();
+                if (!(rawType instanceof Class)) {
+                    throw new IllegalArgumentException("Expected a Class, but <" + type + "> is of type " + type.getClass().getName());
+                }
+                return (Class<?>) rawType;
+            }
+            case GenericArrayType ignored -> {
+                // TODO: Is this sufficient?
+                return Object[].class;
+                // TODO: Is this sufficient?
+            }
+            case TypeVariable<?> ignored -> {
+                // we could use the variable's bounds, but that'll won't work if there are multiple.
+                // having a raw type that's more general than necessary is okay
+                return Object.class;
+                // we could use the variable's bounds, but that'll won't work if there are multiple.
+                // having a raw type that's more general than necessary is okay
+            }
+            case null -> throw new IllegalArgumentException("Unsupported type [null]");
+            default -> throw new IllegalArgumentException(
                 "Expected a Class, ParameterizedType, or " + "GenericArrayType, but <" + type + "> is of type " + type.getClass().getName()
             );
         }
@@ -227,15 +232,15 @@ public static boolean equals(Type a, Type b) {
      */
     public static int hashCode(Type type) {
         return switch (type) {
-            case Class ignored ->
+            case Class<?> ignored ->
                 // Class specifies hashCode().
                 type.hashCode();
             case ParameterizedType p -> Arrays.hashCode(p.getActualTypeArguments()) ^ p.getRawType().hashCode() ^ hashCodeOrZero(
                 p.getOwnerType()
             );
             case GenericArrayType genericArrayType -> hashCode(genericArrayType.getGenericComponentType());
             case WildcardType w -> Arrays.hashCode(w.getLowerBounds()) ^ Arrays.hashCode(w.getUpperBounds());
-            default ->
+            case null, default ->
                 // This isn't a type we support. Probably a type variable
                 hashCodeOrZero(type);
         };
@@ -288,6 +293,7 @@ public static String toString(Type type) {
                     return "? extends " + toString(upperBounds[0]);
                 }
             }
+            case null -> throw new UnsupportedOperationException("Unsupported wildcard type [null]");
             default -> {
                 return type.toString();
             }
@@ -304,7 +310,7 @@ public static Class<? extends Member> memberType(Member member) {
             case MemberImpl memberImpl -> memberImpl.memberType;
             case Field ignored -> Field.class;
             case Method ignored -> Method.class;
-            case Constructor ignored -> Constructor.class;
+            case Constructor<?> ignored -> Constructor.class;
             default -> throw new IllegalArgumentException("Unsupported implementation class for Member, " + member.getClass());
         };
     }
diff --git a/server/src/test/java/org/opensearch/common/inject/internal/MoreTypesTests.java b/server/src/test/java/org/opensearch/common/inject/internal/MoreTypesTests.java
@@ -0,0 +1,86 @@
+/*
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * The OpenSearch Contributors require contributions made to
+ * this file be licensed under the Apache-2.0 license or a
+ * compatible open source license.
+ */
+
+package org.opensearch.common.inject.internal;
+
+import org.opensearch.test.OpenSearchTestCase;
+
+import java.lang.reflect.Type;
+
+public class MoreTypesTests extends OpenSearchTestCase {
+
+    public void testHashCode() {
+        assertEquals(0, MoreTypes.hashCode(null));
+        assertEquals(String.class.hashCode(), MoreTypes.hashCode(String.class));
+
+        MoreTypes.ParameterizedTypeImpl paramType = new MoreTypes.ParameterizedTypeImpl(null, java.util.List.class, String.class);
+        int expectedParamHash = java.util.Arrays.hashCode(new Type[] { String.class }) ^ java.util.List.class.hashCode();
+        assertEquals(expectedParamHash, MoreTypes.hashCode(paramType));
+
+        MoreTypes.GenericArrayTypeImpl arrayType = new MoreTypes.GenericArrayTypeImpl(String.class);
+        assertEquals(String.class.hashCode(), MoreTypes.hashCode(arrayType));
+
+        MoreTypes.WildcardTypeImpl wildcardType = new MoreTypes.WildcardTypeImpl(new Type[] { String.class }, new Type[] {});
+        int expectedWildcardHash = java.util.Arrays.hashCode(new Type[] {}) ^ java.util.Arrays.hashCode(new Type[] { String.class });
+        assertEquals(expectedWildcardHash, MoreTypes.hashCode(wildcardType));
+    }
+
+    public void testToString() {
+        assertEquals("java.lang.String", MoreTypes.toString(String.class));
+
+        MoreTypes.ParameterizedTypeImpl paramType = new MoreTypes.ParameterizedTypeImpl(null, java.util.List.class, String.class);
+        assertEquals("java.util.List<java.lang.String>", MoreTypes.toString(paramType));
+
+        MoreTypes.GenericArrayTypeImpl arrayType = new MoreTypes.GenericArrayTypeImpl(String.class);
+        assertEquals("java.lang.String[]", MoreTypes.toString(arrayType));
+
+        MoreTypes.WildcardTypeImpl unboundedWildcard = new MoreTypes.WildcardTypeImpl(
+            new java.lang.reflect.Type[] { Object.class },
+            new java.lang.reflect.Type[] {}
+        );
+        assertEquals("?", MoreTypes.toString(unboundedWildcard));
+
+        MoreTypes.WildcardTypeImpl upperBoundedWildcard = new MoreTypes.WildcardTypeImpl(
+            new java.lang.reflect.Type[] { String.class },
+            new java.lang.reflect.Type[] {}
+        );
+        assertEquals("? extends java.lang.String", MoreTypes.toString(upperBoundedWildcard));
+
+        MoreTypes.WildcardTypeImpl lowerBoundedWildcard = new MoreTypes.WildcardTypeImpl(
+            new java.lang.reflect.Type[] { Object.class },
+            new java.lang.reflect.Type[] { String.class }
+        );
+        assertEquals("? super java.lang.String", MoreTypes.toString(lowerBoundedWildcard));
+
+        UnsupportedOperationException exception = expectThrows(
+            UnsupportedOperationException.class,
+            () -> { MoreTypes.toString((Type) null); }
+        );
+        assertEquals("Unsupported wildcard type [null]", exception.getMessage());
+    }
+
+    public void testGetRawType() {
+        assertEquals(String.class, MoreTypes.getRawType(String.class));
+
+        MoreTypes.ParameterizedTypeImpl paramType = new MoreTypes.ParameterizedTypeImpl(null, java.util.List.class, String.class);
+        assertEquals(java.util.List.class, MoreTypes.getRawType(paramType));
+
+        MoreTypes.GenericArrayTypeImpl arrayType = new MoreTypes.GenericArrayTypeImpl(String.class);
+        assertEquals(Object[].class, MoreTypes.getRawType(arrayType));
+
+        java.lang.reflect.TypeVariable<?> typeVar = java.util.List.class.getTypeParameters()[0];
+        assertEquals(Object.class, MoreTypes.getRawType(typeVar));
+
+        MoreTypes.WildcardTypeImpl wildcardType = new MoreTypes.WildcardTypeImpl(new Type[] { String.class }, new Type[] {});
+        IllegalArgumentException wildcardException = expectThrows(IllegalArgumentException.class, () -> MoreTypes.getRawType(wildcardType));
+        assertTrue(wildcardException.getMessage().contains("Expected a Class, ParameterizedType, or GenericArrayType"));
+
+        IllegalArgumentException nullException = expectThrows(IllegalArgumentException.class, () -> MoreTypes.getRawType(null));
+        assertTrue(nullException.getMessage().contains("Unsupported type [null]"));
+    }
+}

Original file line number	Diff line number	Diff line change
`@@ -82,7 +82,7 @@ protected void write(ClassWriter classWriter, MethodWriter methodWriter, WriteSc`
`82`	`82`	`case Short shortValue -> methodWriter.push(shortValue);`
`83`	`83`	`case Byte byteValue -> methodWriter.push(byteValue);`
`84`	`84`	`case Boolean boolValue -> methodWriter.push(boolValue);`
`85`		`- default -> throw new IllegalStateException("unexpected constant [" + constant + "]");`
	`85`	`+ case null, default -> throw new IllegalStateException("unexpected constant [" + constant + "]");`
`86`	`86`	`}`
`87`	`87`	`}`
`88`	`88`	`}`
Original file line number	Diff line number	Diff line change
`@@ -216,7 +216,7 @@ public void visitLeaf(Query query) {`
`216`	`216`	`case MatchAllDocsQuery ignored -> terms.add(new Result(true, true));`
`217`	`217`	`case MatchNoDocsQuery ignored -> terms.add(Result.MATCH_NONE);`
`218`	`218`	`case PointRangeQuery pointRangeQuery -> terms.add(pointRangeQuery(pointRangeQuery));`
`219`		`- default -> terms.add(Result.UNKNOWN);`
	`219`	`+ case null, default -> terms.add(Result.UNKNOWN);`
`220`	`220`	`}`
`221`	`221`	`}`
`222`	`222`
Original file line number	Diff line number	Diff line change
`@@ -247,7 +247,7 @@ public static Map<String, ObjectMap.Value> metadataToProto(OpenSearchException e`
`247`	`247`	`case SearchParseException spe -> SearchParseExceptionProtoUtils.metadataToProto(spe);`
`248`	`248`	`case SearchPhaseExecutionException spee -> SearchPhaseExecutionExceptionProtoUtils.metadataToProto(spee);`
`249`	`249`	`case MultiBucketConsumerService.TooManyBucketsException tmbe -> TooManyBucketsExceptionProtoUtils.metadataToProto(tmbe);`
`250`		`- default -> new HashMap<>();`
	`250`	`+ case null, default -> new HashMap<>();`
`251`	`251`	`};`
`252`	`252`	`}`
`253`	`253`	`}`
Original file line number	Diff line number	Diff line change
`@@ -45,6 +45,8 @@ public static ShardFailure toProto(DefaultShardOperationFailedException exceptio`
`45`	`45`	`indicesShardStoresFailure`
`46`	`46`	`);`
`47`	`47`	`case CloseIndexResponse.ShardResult.Failure closeIndexFailure -> innerToProto(shardFailureBuilder, closeIndexFailure);`
	`48`	`+ case null -> {`
	`49`	`+ }`
`48`	`50`	`default -> parentInnerToProto(shardFailureBuilder, exception);`
`49`	`51`	`}`
`50`	`52`	`return shardFailureBuilder.build();`
Original file line number	Diff line number	Diff line change
`@@ -122,4 +122,13 @@ public void testToProtoWithNullNodeId() throws IOException {`
`122`	`122`	`assertFalse("Node should not be set", shardFailure.hasNode());`
`123`	`123`	`assertNotNull("Reason should not be null", shardFailure.getReason());`
`124`	`124`	`}`
	`125`	`+`
	`126`	`+ public void testToProtoWithNull() throws IOException {`
	`127`	`+ ShardFailure shardFailure = DefaultShardOperationFailedExceptionProtoUtils.toProto(null);`
	`128`	`+ assertNotNull("ShardFailure should not be null", shardFailure);`
	`129`	`+ assertFalse("Index should not be set", shardFailure.hasIndex());`
	`130`	`+ assertFalse("Status should not be set", shardFailure.hasStatus());`
	`131`	`+ assertFalse("Node should not be set", shardFailure.hasNode());`
	`132`	`+ assertFalse("Reason should not be set", shardFailure.hasReason());`
	`133`	`+ }`
`125`	`134`	`}`
Original file line number	Diff line number	Diff line change
`@@ -279,4 +279,12 @@ public RestStatus status() {`
`279`	`279`	`assertTrue(result.getMessage().contains("Root cause"));`
`280`	`280`	`assertTrue(result.getMessage().contains("Wrapper exception"));`
`281`	`281`	`}`
	`282`	`+`
	`283`	`+ public void testNullExceptionConversion() {`
	`284`	`+ StatusRuntimeException result = GrpcErrorHandler.convertToGrpcError(null);`
	`285`	`+`
	`286`	`+ // null -> INTERNAL via case null handling`
	`287`	`+ assertEquals(Status.INTERNAL.getCode(), result.getStatus().getCode());`
	`288`	`+ assertEquals("INTERNAL: Unexpected null exception", result.getMessage());`
	`289`	`+ }`
`282`	`290`	`}`
Original file line number	Diff line number	Diff line change
`@@ -356,7 +356,7 @@ public static Collection<Message> getMessagesFromThrowable(Throwable throwable)`
`356`	`356`	`case ProvisionException provisionException -> provisionException.getErrorMessages();`
`357`	`357`	`case ConfigurationException configurationException -> configurationException.getErrorMessages();`
`358`	`358`	`case CreationException creationException -> creationException.getErrorMessages();`
`359`		`- default -> emptySet();`
	`359`	`+ case null, default -> emptySet();`
`360`	`360`	`};`
`361`	`361`	`}`
`362`	`362`