From 98b708d5a2344bbfbd919f60c1023e7b5681a4e5 Mon Sep 17 00:00:00 2001
From: Nathan Harold <nharold@google.com>
Date: Fri, 26 Apr 2024 15:49:34 -0700
Subject: [PATCH] Fix a Rare Race In Clearing ResourceId

If the IpSecTransform is being manipulated in
multiple threads, there is a race between the
clearing of the resource id (setting it to
INVALID_RESOURCE_ID and making binder calls
with it. This wasn't a significant problem before
becasue the only side effect was that it could
occasionally cause an IAE instead of an ISE when
the caller was doing something weird. Unfortunately
the exception type now matters because it is being
relied upon to provide a callback rather than a
throw in the event of an error. Accordingly reads of
mResourceId have to be synchronized against the
the possibility of close() of that resourceId to
ensure that the right exception is thrown on the
service side of the binder.

Bug: 336897538
Test: atest CtsNetTestCases:IpSecManagerTest
Change-Id: I21c893034efa7f322ae2aa81d6c7e00dc9ef8d2f
---
 .../src/android/net/IpSecTransform.java       | 46 +++++++++++--------
 1 file changed, 27 insertions(+), 19 deletions(-)

diff --git a/framework-t/src/android/net/IpSecTransform.java b/framework-t/src/android/net/IpSecTransform.java
index 4e10a96384..70c9bc8a2d 100644
--- a/framework-t/src/android/net/IpSecTransform.java
+++ b/framework-t/src/android/net/IpSecTransform.java
@@ -124,7 +124,7 @@ public final class IpSecTransform implements AutoCloseable {
     private IpSecTransform activate()
             throws IOException, IpSecManager.ResourceUnavailableException,
                     IpSecManager.SpiUnavailableException {
-        synchronized (this) {
+        synchronized (mLock) {
             try {
                 IpSecTransformResponse result = getIpSecManager(mContext).createTransform(
                         mConfig, new Binder(), mContext.getOpPackageName());
@@ -164,20 +164,23 @@ public final class IpSecTransform implements AutoCloseable {
     public void close() {
         Log.d(TAG, "Removing Transform with Id " + mResourceId);
 
-        // Always safe to attempt cleanup
-        if (mResourceId == INVALID_RESOURCE_ID) {
-            mCloseGuard.close();
-            return;
-        }
-        try {
-            getIpSecManager(mContext).deleteTransform(mResourceId);
-        } catch (Exception e) {
-            // On close we swallow all random exceptions since failure to close is not
-            // actionable by the user.
-            Log.e(TAG, "Failed to close " + this + ", Exception=" + e);
-        } finally {
-            mResourceId = INVALID_RESOURCE_ID;
-            mCloseGuard.close();
+        synchronized(mLock) {
+            // Always safe to attempt cleanup
+            if (mResourceId == INVALID_RESOURCE_ID) {
+                mCloseGuard.close();
+                return;
+            }
+
+            try {
+                    getIpSecManager(mContext).deleteTransform(mResourceId);
+            } catch (Exception e) {
+                // On close we swallow all random exceptions since failure to close is not
+                // actionable by the user.
+                Log.e(TAG, "Failed to close " + this + ", Exception=" + e);
+            } finally {
+                mResourceId = INVALID_RESOURCE_ID;
+                mCloseGuard.close();
+            }
         }
     }
 
@@ -196,14 +199,17 @@ public final class IpSecTransform implements AutoCloseable {
     }
 
     private final IpSecConfig mConfig;
-    private int mResourceId;
+    private final Object mLock = new Object();
+    private int mResourceId; // Partly guarded by mLock to ensure basic safety, not correctness
     private final Context mContext;
     private final CloseGuard mCloseGuard = CloseGuard.get();
 
     /** @hide */
     @VisibleForTesting
     public int getResourceId() {
-        return mResourceId;
+        synchronized(mLock) {
+            return mResourceId;
+        }
     }
 
     /**
@@ -224,8 +230,10 @@ public final class IpSecTransform implements AutoCloseable {
         // TODO: Consider adding check to prevent DDoS attack.
 
         try {
-            final IpSecTransformState ipSecTransformState =
-                    getIpSecManager(mContext).getTransformState(mResourceId);
+            IpSecTransformState ipSecTransformState;
+            synchronized(mLock) {
+                ipSecTransformState = getIpSecManager(mContext).getTransformState(mResourceId);
+            }
             executor.execute(
                     () -> {
                         callback.onResult(ipSecTransformState);