core: Do not leak server state when application callbacks throw exceptions #3064

zpencer · 2017-06-05T23:41:18Z

Today JumpToApplicationThreadServerStreamListener leaks server state by transmitting details about uncaught StatusRuntimeException throwables to the client. This is a security problem.

This PR ensures that uncaught exceptions always close the ServerCall without leaking any state information. Users running in a trusted environment who want to transmit error details can use ServerInterceptors.STATUSRUNTIMEXCEPTION_TRANSMITTING_INTERCEPTOR .

fixes #2189

zhangkun83 · 2017-06-08T00:26:23Z

core/src/main/java/io/grpc/ServerInterceptors.java

@@ -17,6 +17,8 @@
 package io.grpc;

 import com.google.common.base.Preconditions;
+import io.grpc.ForwardingServerCallListener.SimpleForwardingServerCallListener;
+import io.grpc.internal.SynchronizedServerCall;


io.grpc should not reference io.grpc.internal.

moved StatusRuntimeExceptionTransmitter to its own class under io.grpc.util

zhangkun83 · 2017-06-08T00:27:02Z

core/src/main/java/io/grpc/ServerInterceptors.java

+   * if all clients are trusted.
+   */
+  @ExperimentalApi("https://github.com/grpc/grpc-java/issues/2189")
+  public static final ServerInterceptor STATUSRUNTIMEXCEPTION_TRANSMITTING_INTERCEPTOR =


This is not core API, but rather something provided for convenience. io.grpc.util seems to be a better place for this class.

moved StatusRuntimeExceptionTransmitter to its own class

zhangkun83 · 2017-06-09T01:23:14Z

core/src/main/java/io/grpc/ServerInterceptors.java

+              try {
+                super.onReady();
+              } catch (StatusRuntimeException t) {
+                closeWithException(t);


I tried to analyze the locking order with in-process transport.

InProcessServerStream is synchronized on every method, therefore the lock order on the outbound path is
SynchronizedServerCall.this > InProcessServerStream.this.

On the inbound path, InProcessClientStream.serverStreamListener is called under synchronized (InProcessClientStream.this), and will potentially call closeWithException(t) that calls into InProcessServerStream.close(), forming this lock order:
InProcessClientStream.this > SynchronizedServerCall.this > InProcessServerStream.this.

So, this change doesn't impose a deadline scenario.

However, if we (or the user) did the same thing on the client side with a SynchronizedClientCall, it would form a reverse lock order, and a deadlock is possible:
InProcessServerStream.this > SynchronizedClientCall.this > InProcessClientStream.this.

@ejona86 is this a valid concern? Should we try to reduce locking, e.g., call callbacks outside of lock in in-process transport?

Per offline discussion, the deadlock issue with in-process transport is legit, while not the fault of this PR. I have filed #3084

However, even without #3084, using synchronized to make thread-safe calls to ServerCall and ClientCall is still prone to deadlock when in-process transport is used with direct executor:

Thread1

SynchronizedClientCall.sendMessage() (locks SynchronizedClientCall.this)

ServerCall.Listener calls into application code, which calls SynchronizedServerCall.close() (locks SynchronizedServerCall.this)

Thread2

SynchronizedServerCall.sendMessage() (locks SynchronizedServerCall.this)

ClientCall.Listener calls into application code, which calls SynchronizedClientCall.close() (locks SynchronizedClientCall.this)

This time, it's the two synchronized calls that are involved in the deadlock.

This can be considered application-level code, and if application does that and got deadlock, it's the application's fault. Application shouldn't do it, and it means we should not do it either.

Like the proposed solution for #3084, we could also use ChannelExecutor instead to provide the thread-safety. ChannelExecutor should probably be renamed.

Reimplemented the functionality via SerializingServerCall. Opted to make this a private nested class to minimize visibility for now.

ejona86

Oops. Forgot to send these out.

ejona86 · 2017-06-09T16:23:40Z

core/src/main/java/io/grpc/ServerInterceptors.java

+   * if all clients are trusted.
+   */
+  @ExperimentalApi("https://github.com/grpc/grpc-java/issues/2189")
+  public static final ServerInterceptor STATUSRUNTIMEXCEPTION_TRANSMITTING_INTERCEPTOR =


Expose this as a method, not as a static field. That allows us, in the future, to avoid creating it until first use or similar. Android in particular is hurt quite a bit by static initialization since apps start frequently and want really low memory usage. So while it's fine not to worry about that now, we shouldn't have the API dictate the lifetime.

ejona86 · 2017-06-09T16:25:50Z

core/src/main/java/io/grpc/ServerInterceptors.java

+              if (metadata == null) {
+                metadata = new Metadata();
+              }
+              syncServerCall.close(Status.fromThrowable(t), metadata);


t.getStatus()

ejona86 · 2017-06-09T16:27:34Z

core/src/main/java/io/grpc/ServerInterceptors.java

@@ -17,6 +17,8 @@
 package io.grpc;

 import com.google.common.base.Preconditions;
+import io.grpc.ForwardingServerCallListener.SimpleForwardingServerCallListener;
+import io.grpc.internal.SynchronizedServerCall;


We can't have references to internal from io.grpc. I guess move SynchronizedServerCall to io.grpc and make it package-private? Or maybe put it in ForwardingServerCall (and still package-private)?

Moved to io.grpc.util

ejona86 · 2017-06-09T16:30:44Z

core/src/test/java/io/grpc/ServerInterceptorsTest.java

+      getSoleMethod(intercepted).getServerCallHandler().startCall(call, headers).onHalfClose();
+      getSoleMethod(intercepted).getServerCallHandler().startCall(call, headers).onReady();
+    } catch (Throwable t) {
+      fail("The interceptor should have handled the error by directly closing the ServerCall, "


This throws away t. maybe just make this a comment and get rid of the try-catch?

…ic nested class

ejona86 · 2017-06-12T15:42:16Z

core/src/main/java/io/grpc/util/interceptor/server/StatusRuntimeExceptionTransmitter.java

+@ExperimentalApi("https://github.com/grpc/grpc-java/issues/2189")
+public final class StatusRuntimeExceptionTransmitter implements ServerInterceptor {
+  private StatusRuntimeExceptionTransmitter() {
+    // do not instantiate


This looks like the constructor normally in a static utility class that is never instantiated. However, this class is instantiated. Maybe just remove this comment?

ejona86 · 2017-06-12T15:42:57Z

core/src/main/java/io/grpc/util/interceptor/server/StatusRuntimeExceptionTransmitter.java

+
+/**
+ * A class that intercepts uncaught exceptions of type {@link StatusRuntimeException} and handles
+ * them by closing the {@link ServerCall}, and transmitting the exception's details to the client.


s/exception's details/exception's status and metadata/

Otherwise people will think the stack trace shows up on the other side.

ejona86 · 2017-06-12T15:47:44Z

core/src/main/java/io/grpc/util/interceptor/server/StatusRuntimeExceptionTransmitter.java

+      }
+
+      private void closeWithException(StatusRuntimeException t) {
+        Metadata metadata = Status.trailersFromThrowable(t);


t.getTrailers()

ejona86 · 2017-06-12T15:57:41Z

core/src/main/java/io/grpc/util/interceptor/server/StatusRuntimeExceptionTransmitter.java

+ * limitations under the License.
+ */
+
+package io.grpc.util.interceptor.server;


That's a pretty deep package. I'd rather not group things by their class type (interceptor). @zhangkun83, you have a preference for package name?

I would prefer just io.grpc.util.

io.grpc.util SGTM as well.

moved and renamed the class to be more obvious that it's an interceptor

ejona86 · 2017-06-12T16:00:18Z

core/src/test/java/io/grpc/util/interceptor/server/UtilServerInterceptorsTest.java

+  @Mock
+  private ServerCall.Listener<String> listener;
+
+  private MethodDescriptor<String, Integer> flowMethod;


Use TestMethodDescriptors.noopMethod?

This is handy, done

ejona86 · 2017-06-12T16:02:35Z

core/src/test/java/io/grpc/util/interceptor/server/UtilServerInterceptorsTest.java

+  private Marshaller<Integer> responseMarshaller;
+
+  @Mock
+  private ServerCallHandler<String, Integer> handler;


Make a concrete implementation instead of a mock?

Most of these fields can turn into concrete implementations without too much boiler plate, so changing those as well.

ejona86 · 2017-06-12T22:51:39Z

core/src/main/java/io/grpc/util/TransmitStatusRuntimeExceptionInterceptor.java

+      }
+
+      private void closeWithException(StatusRuntimeException t) {
+        serverCall.close(t.getStatus(), t.getTrailers());


getTrailers may return null, but close() requires non-null Metadata

ejona86 · 2017-06-12T22:52:57Z

core/src/test/java/io/grpc/util/interceptor/server/UtilServerInterceptorsTest.java

-        .thenReturn(listener);
-
-    serviceDefinition = ServerServiceDefinition.builder(new ServiceDescriptor("basic", flowMethod))
+    handler = new ServerCallHandler<String, Integer>() {


This initialization can be done at declaration time, and then serviceDefinition can also be initialized when declared. That would leave just initMocks in setup.

ejona86 · 2017-06-12T22:54:55Z

core/src/test/java/io/grpc/util/interceptor/server/UtilServerInterceptorsTest.java

@@ -17,16 +17,12 @@
 package io.grpc.util.interceptor.server;


This needs to move folder/package as well.

d2chau · 2017-08-23T17:06:11Z

Note: ServerInterceptors.STATUSRUNTIMEXCEPTION_TRANSMITTING_INTERCEPTOR has been moved to it's own class. It is now TransmitStatusRuntimeExceptionInterceptor. Instantiating it requires calling .instance().

I wanted to document it at least here, since it has changed from the initial post above.

Make error detail transmissions opt in

a1db4c6

zpencer requested review from ejona86 and zhangkun83 June 5, 2017 23:41

Rename syncListener -> listener

d0a2ea3

zhangkun83 reviewed Jun 9, 2017

View reviewed changes

zhangkun83 self-assigned this Jun 9, 2017

ejona86 reviewed Jun 9, 2017

View reviewed changes

zpencer added 3 commits June 9, 2017 14:35

Move interceptor to utils package, make SynchronizedServerCall a stat…

ab44d1f

…ic nested class

Move unit tests

027e994

update for review comments

65e4d61

ejona86 reviewed Jun 12, 2017

View reviewed changes

review comments

b65e09f

ejona86 approved these changes Jun 12, 2017

View reviewed changes

zpencer added 2 commits June 13, 2017 08:35

fix null trailers, do test field inits in declarations

0514d6a

move test to util package

7eb4eab

ejona86 approved these changes Jun 16, 2017

View reviewed changes

zpencer merged commit 2b1eee9 into grpc:master Jun 19, 2017

zpencer deleted the zpencer/2189/lostMetadata branch July 15, 2017 19:40

lock bot locked as resolved and limited conversation to collaborators Jan 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: Do not leak server state when application callbacks throw exceptions #3064

core: Do not leak server state when application callbacks throw exceptions #3064

zpencer commented Jun 5, 2017

zhangkun83 Jun 8, 2017

zpencer Jun 10, 2017

zhangkun83 Jun 8, 2017

zpencer Jun 10, 2017

zhangkun83 Jun 9, 2017 •

edited

zhangkun83 Jun 9, 2017

zpencer Jun 10, 2017

ejona86 left a comment

ejona86 Jun 9, 2017

zpencer Jun 10, 2017

ejona86 Jun 9, 2017

zpencer Jun 10, 2017

ejona86 Jun 9, 2017

zpencer Jun 10, 2017

ejona86 Jun 9, 2017

zpencer Jun 10, 2017

ejona86 Jun 12, 2017

zpencer Jun 12, 2017

ejona86 Jun 12, 2017

zpencer Jun 12, 2017

ejona86 Jun 12, 2017

zpencer Jun 12, 2017

ejona86 Jun 12, 2017

zhangkun83 Jun 12, 2017

ejona86 Jun 12, 2017

zpencer Jun 12, 2017 •

edited

ejona86 Jun 12, 2017

zpencer Jun 12, 2017

ejona86 Jun 12, 2017

zpencer Jun 12, 2017

ejona86 Jun 12, 2017

ejona86 Jun 12, 2017

ejona86 Jun 12, 2017

d2chau commented Aug 23, 2017

core: Do not leak server state when application callbacks throw exceptions #3064

core: Do not leak server state when application callbacks throw exceptions #3064

Conversation

zpencer commented Jun 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangkun83 Jun 9, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ejona86 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zpencer Jun 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d2chau commented Aug 23, 2017

zhangkun83 Jun 9, 2017 •

edited

zpencer Jun 12, 2017 •

edited