Catch Errors when calling complex parsing code

core/src/main/java/io/grpc/internal/ScParser.java

@@ -69,8 +69,19 @@ public ConfigOrError parseServiceConfig(Map<String, ?> rawServiceConfig) {
               maxHedgedAttemptsLimit,
               loadBalancingPolicySelection));
     } catch (RuntimeException e) {
+      // TODO(ejona): We really don't want parsers throwing exceptions; they should return an error.
+      // However, right now ManagedChannelServiceConfig itself uses exceptions like
+      // ClassCastException. We should handle those with a graceful return within
+      // ManagedChannelServiceConfig and then get rid of this case. Then all exceptions are
+      // "unexpected" and the INTERNAL status code makes it clear a bug needs to be fixed.
       return ConfigOrError.fromError(
           Status.UNKNOWN.withDescription("failed to parse service config").withCause(e));
+    } catch (Throwable t) {
+      // Even catch Errors, since broken config parsing could trigger AssertionError,
+      // StackOverflowError, and other errors we can reasonably safely recover. Since the config
+      // could be untrusted, we want to error on the side of recovering.
+      return ConfigOrError.fromError(
+          Status.INTERNAL.withDescription("Unexpected error parsing service config").withCause(t));
     }
   }
 }

core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java

@@ -3942,6 +3942,28 @@ public void nameResolverHelper_badConfigFails() {
     assertThat(coe.getError().getCause()).isInstanceOf(ClassCastException.class);
   }
 
+  @Test
+  public void nameResolverHelper_badParser_failsGracefully() {
+    boolean retryEnabled = false;
+    int maxRetryAttemptsLimit = 2;
+    int maxHedgedAttemptsLimit = 3;
+
+    Throwable t = new Error("really poor config parser");
+    when(mockLoadBalancerProvider.parseLoadBalancingPolicyConfig(any())).thenThrow(t);
+    ScParser parser = new ScParser(
+        retryEnabled,
+        maxRetryAttemptsLimit,
+        maxHedgedAttemptsLimit,
+        mockLoadBalancerProvider);
+
+    ConfigOrError coe = parser.parseServiceConfig(ImmutableMap.of());
+
+    assertThat(coe.getError()).isNotNull();
+    assertThat(coe.getError().getCode()).isEqualTo(Code.INTERNAL);
+    assertThat(coe.getError().getDescription()).contains("Unexpected error parsing service config");
+    assertThat(coe.getError().getCause()).isSameInstanceAs(t);
+  }
+
   @Test
   public void nameResolverHelper_noConfigChosen() {
     boolean retryEnabled = false;

xds/src/main/java/io/grpc/xds/client/XdsResourceType.java

@@ -33,10 +33,14 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.logging.Level;
+import java.util.logging.Logger;
 import javax.annotation.Nullable;
 
 @ExperimentalApi("https://github.com/grpc/grpc-java/issues/10847")
 public abstract class XdsResourceType<T extends ResourceUpdate> {
+  private static final Logger log = Logger.getLogger(XdsResourceType.class.getName());
+
   static final String TYPE_URL_RESOURCE =
       "type.googleapis.com/envoy.service.discovery.v3.Resource";
   protected static final String TRANSPORT_SOCKET_NAME_TLS = "envoy.transport_sockets.tls";
@@ -176,6 +180,16 @@ ValidatedResourceUpdate<T> parse(Args args, List<Any> resources) {
                 typeName(), unpackedClassName().getSimpleName(), cname, e.getMessage()));
         invalidResources.add(cname);
         continue;
+      } catch (Throwable t) {
+        log.log(Level.FINE, "Unexpected error in doParse()", t);
+        String errorMessage = t.getClass().getSimpleName();
+        if (t.getMessage() != null) {
+          errorMessage = errorMessage + ": " + t.getMessage();
+        }
+        errors.add(String.format("%s response '%s' unexpected error: %s",
+                typeName(), cname, errorMessage));
+        invalidResources.add(cname);
+        continue;
       }
 
       // Resource parsed successfully.

xds/src/test/java/io/grpc/xds/GrpcXdsClientImplTestBase.java

@@ -18,6 +18,7 @@
 
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.common.truth.Truth.assertWithMessage;
+import static io.grpc.StatusMatcher.statusHasCode;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.argThat;
 import static org.mockito.ArgumentMatchers.eq;
@@ -42,6 +43,7 @@
 import com.google.protobuf.Duration;
 import com.google.protobuf.InvalidProtocolBufferException;
 import com.google.protobuf.Message;
+import com.google.protobuf.StringValue;
 import com.google.protobuf.UInt32Value;
 import com.google.protobuf.util.Durations;
 import io.envoyproxy.envoy.config.cluster.v3.OutlierDetection;
@@ -59,6 +61,7 @@
 import io.grpc.Status;
 import io.grpc.Status.Code;
 import io.grpc.StatusOr;
+import io.grpc.StatusOrMatcher;
 import io.grpc.inprocess.InProcessChannelBuilder;
 import io.grpc.inprocess.InProcessServerBuilder;
 import io.grpc.internal.BackoffPolicy;
@@ -111,6 +114,7 @@
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Queue;
 import java.util.concurrent.BlockingDeque;
 import java.util.concurrent.CountDownLatch;
@@ -278,6 +282,8 @@ public long currentTimeNanos() {
   @Mock
   private ResourceWatcher<EdsUpdate> edsResourceWatcher;
   @Mock
+  private ResourceWatcher<StringUpdate> stringResourceWatcher;
+  @Mock
   private XdsClientMetricReporter xdsClientMetricReporter;
   @Mock
   private ServerConnectionCallback serverConnectionCallback;
@@ -667,6 +673,58 @@ private void verifyServerConnection(int times, boolean isConnected, String xdsSe
         eq(xdsServer));
   }
 
+  @Test
+  public void doParse_returnsSuccessfully() {
+    XdsStringResource resourceType = new XdsStringResource();
+    xdsClient.watchXdsResource(
+        resourceType, "resource1", stringResourceWatcher, MoreExecutors.directExecutor());
+    DiscoveryRpcCall call = resourceDiscoveryCalls.poll();
+
+    Any resource = Any.pack(StringValue.newBuilder().setValue("resource1").build());
+    call.sendResponse(resourceType, resource, VERSION_1, "0000");
+    verify(stringResourceWatcher).onResourceChanged(argThat(StatusOrMatcher.hasValue(
+        (StringUpdate arg) -> new StringUpdate("resource1").equals(arg))));
+  }
+
+  @Test
+  public void doParse_throwsResourceInvalidException_resourceInvalid() {
+    XdsStringResource resourceType = new XdsStringResource() {
+      @Override
+      protected StringUpdate doParse(Args args, Message unpackedMessage)
+          throws ResourceInvalidException {
+        throw new ResourceInvalidException("some bad input");
+      }
+    };
+    xdsClient.watchXdsResource(
+        resourceType, "resource1", stringResourceWatcher, MoreExecutors.directExecutor());
+    DiscoveryRpcCall call = resourceDiscoveryCalls.poll();
+
+    Any resource = Any.pack(StringValue.newBuilder().setValue("resource1").build());
+    call.sendResponse(resourceType, resource, VERSION_1, "0000");
+    verify(stringResourceWatcher).onResourceChanged(argThat(StatusOrMatcher.hasStatus(
+        statusHasCode(Status.Code.UNAVAILABLE)
+          .andDescriptionContains("validation error: some bad input"))));
+  }
+
+  @Test
+  public void doParse_throwsError_resourceInvalid() throws Exception {
+    XdsStringResource resourceType = new XdsStringResource() {
+      @Override
+      protected StringUpdate doParse(Args args, Message unpackedMessage) {
+        throw new AssertionError("something bad happened");
+      }
+    };
+    xdsClient.watchXdsResource(
+        resourceType, "resource1", stringResourceWatcher, MoreExecutors.directExecutor());
+    DiscoveryRpcCall call = resourceDiscoveryCalls.poll();
+
+    Any resource = Any.pack(StringValue.newBuilder().setValue("resource1").build());
+    call.sendResponse(resourceType, resource, VERSION_1, "0000");
+    verify(stringResourceWatcher).onResourceChanged(argThat(StatusOrMatcher.hasStatus(
+        statusHasCode(Status.Code.UNAVAILABLE)
+          .andDescriptionContains("unexpected error: AssertionError: something bad happened"))));
+  }
+
   @Test
   public void ldsResourceNotFound() {
     DiscoveryRpcCall call = startResourceWatcher(XdsListenerResource.getInstance(), LDS_RESOURCE,
@@ -5318,4 +5376,70 @@ protected abstract Message buildHttpConnectionManagerFilter(
 
     protected abstract Message buildTerminalFilter();
   }
+
+  private static class XdsStringResource extends XdsResourceType<StringUpdate> {
+    @Override
+    @SuppressWarnings("unchecked")
+    protected Class<? extends com.google.protobuf.Message> unpackedClassName() {
+      return StringValue.class;
+    }
+
+    @Override
+    public String typeName() {
+      return "EMPTY";
+    }
+
+    @Override
+    public String typeUrl() {
+      return "type.googleapis.com/google.protobuf.StringValue";
+    }
+
+    @Override
+    public boolean shouldRetrieveResourceKeysForArgs() {
+      return false;
+    }
+
+    @Override
+    protected boolean isFullStateOfTheWorld() {
+      return false;
+    }
+
+    @Override
+    @Nullable
+    protected String extractResourceName(Message unpackedResource) {
+      if (!(unpackedResource instanceof StringValue)) {
+        return null;
+      }
+      return ((StringValue) unpackedResource).getValue();
+    }
+
+    @Override
+    protected StringUpdate doParse(Args args, Message unpackedMessage)
+        throws ResourceInvalidException {
+      return new StringUpdate(((StringValue) unpackedMessage).getValue());
+    }
+  }
+
+  private static final class StringUpdate implements ResourceUpdate {
+    @SuppressWarnings("UnusedVariable")
+    public final String value;
+
+    public StringUpdate(String value) {
+      this.value = value;
+    }
+
+    @Override
+    public boolean equals(Object o) {
+      if (!(o instanceof StringUpdate)) {
+        return false;
+      }
+      StringUpdate that = (StringUpdate) o;
+      return Objects.equals(this.value, that.value);
+    }
+
+    @Override
+    public int hashCode() {
+      return Objects.hash(value);
+    }
+  }
 }