joy/android_packages_modules_Connectivity
Watch
1
Fork
You've already forked android_packages_modules_Connectivity
 0
Code Issues Pull requests Projects Releases Wiki Activity

Fix a Rare Race In Clearing ResourceId

Browse source 
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
This commit is contained in:
Nathan Harold 2024-04-26 15:49:34 -07:00
commit 98b708d5a2
1 changed files with 27 additions and 19 deletions
Download patch file Download diff file Expand all files Collapse all files

46
framework-t/src/android/net/IpSecTransform.java
View file

@ -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);