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
diff --git a/framework-t/src/android/net/IpSecTransform.java b/framework-t/src/android/net/IpSecTransform.java
index 4e10a96..70c9bc8 100644
--- a/framework-t/src/android/net/IpSecTransform.java
+++ b/framework-t/src/android/net/IpSecTransform.java
@@ -124,7 +124,7 @@
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 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 @@
}
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 @@
// 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);