From ccfaa3c06d0d23a888e9f006d4157c91998af6a9 Mon Sep 17 00:00:00 2001
From: Benedict Wong <benedictwong@google.com>
Date: Thu, 28 Feb 2019 20:28:48 -0800
Subject: [PATCH] Fix remove-before-add for IpSecService RefcountedResource

This patch fixes a bug where if a binder dies before the linkToDeath
call, the cleanup will be performed before the entry is added to the
array. While it is safe in that quotas and tracking performs as per
normal, the RefcountedRecord may not be cleaned up.

Rethrowing this exception is safe, since the only paths that would hit
this are all on binder threads coming from applications. Further, it
seems there is only one real way of this getting hit - if the app that
called the creation died during the binder call.

Bug: 126802451
Test: Compiled, CTS tests passing
Change-Id: I6db75853da9f29e1573512e26351623f22770c5d
---
 .../core/java/com/android/server/IpSecService.java    |  1 +
 .../server/IpSecServiceRefcountedResourceTest.java    | 11 ++++++-----
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/services/core/java/com/android/server/IpSecService.java b/services/core/java/com/android/server/IpSecService.java
index 126bf6556538e..fc7fe5ea56525 100644
--- a/services/core/java/com/android/server/IpSecService.java
+++ b/services/core/java/com/android/server/IpSecService.java
@@ -207,6 +207,7 @@ public class IpSecService extends IIpSecService.Stub {
                     mBinder.linkToDeath(this, 0);
                 } catch (RemoteException e) {
                     binderDied();
+                    e.rethrowFromSystemServer();
                 }
             }
         }
diff --git a/tests/net/java/com/android/server/IpSecServiceRefcountedResourceTest.java b/tests/net/java/com/android/server/IpSecServiceRefcountedResourceTest.java
index 68ff777a0160d..22a2c94fc194e 100644
--- a/tests/net/java/com/android/server/IpSecServiceRefcountedResourceTest.java
+++ b/tests/net/java/com/android/server/IpSecServiceRefcountedResourceTest.java
@@ -18,6 +18,7 @@ package com.android.server;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNull;
+import static org.junit.Assert.fail;
 import static org.mockito.Matchers.anyInt;
 import static org.mockito.Matchers.anyObject;
 import static org.mockito.Matchers.eq;
@@ -134,11 +135,11 @@ public class IpSecServiceRefcountedResourceTest {
         IBinder binderMock = mock(IBinder.class);
         doThrow(new RemoteException()).when(binderMock).linkToDeath(anyObject(), anyInt());
 
-        RefcountedResource<IResource> refcountedResource = getTestRefcountedResource(binderMock);
-
-        // Verify that cleanup is performed (Spy limitations prevent verification of method calls
-        // for binder death scenario; check refcount to determine if cleanup was performed.)
-        assertEquals(-1, refcountedResource.mRefCount);
+        try {
+            getTestRefcountedResource(binderMock);
+            fail("Expected exception to propogate when binder fails to link to death");
+        } catch (RuntimeException expected) {
+        }
     }
 
     @Test