Rework Exception Handling for IpSecManager
In order to properly support EOPNOTSUPP this CL
applies a consistent approach to handling Exceptions.
Hereafter, all exceptions that aren't of a special
method-specific type (such as SpiUnavailableException)
will all be returned to the calling process unchanged.
At the API call site, the ServiceSpecificException,
which is really an Errno, will be inspected and either
converted to an unchecked exception for types we know,
or it will be converted to an IOException in cases where
that method can return a checked exception. In cases
where we do not expect an errno, we will simply throw
a generic RuntimeException. This means all API calls
will now properly throw UnsupportedOperationException
and may be CTS tested accordingly.
Bug: 72420898
Test: runtest frameworks-net
Merged-In: I4a00e221618896223fcdb4b4279fb14cd14e34d8
Change-Id: I4a00e221618896223fcdb4b4279fb14cd14e34d8
(cherry picked from commit 21c4188a979c55f336728397e18273978d11f7f4)
diff --git a/core/java/android/net/IpSecManager.java b/core/java/android/net/IpSecManager.java
index 8732375..1145d5b 100644
--- a/core/java/android/net/IpSecManager.java
+++ b/core/java/android/net/IpSecManager.java
@@ -27,6 +27,9 @@
import android.os.Binder;
import android.os.ParcelFileDescriptor;
import android.os.RemoteException;
+import android.os.ServiceSpecificException;
+import android.system.ErrnoException;
+import android.system.OsConstants;
import android.util.AndroidException;
import android.util.Log;
@@ -173,11 +176,16 @@
public void close() {
try {
mService.releaseSecurityParameterIndex(mResourceId);
- mResourceId = INVALID_RESOURCE_ID;
} catch (RemoteException e) {
throw e.rethrowFromSystemServer();
+ } 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();
}
- mCloseGuard.close();
}
/** Check that the SPI was closed properly. */
@@ -228,7 +236,6 @@
throw new RuntimeException(
"Invalid Resource ID returned by IpSecService: " + status);
}
-
} catch (RemoteException e) {
throw e.rethrowFromSystemServer();
}
@@ -240,6 +247,17 @@
public int getResourceId() {
return mResourceId;
}
+
+ @Override
+ public String toString() {
+ return new StringBuilder()
+ .append("SecurityParameterIndex{spi=")
+ .append(mSpi)
+ .append(",resourceId=")
+ .append(mResourceId)
+ .append("}")
+ .toString();
+ }
}
/**
@@ -262,7 +280,11 @@
mService,
destinationAddress,
IpSecManager.INVALID_SECURITY_PARAMETER_INDEX);
+ } catch (ServiceSpecificException e) {
+ throw rethrowUncheckedExceptionFromServiceSpecificException(e);
} catch (SpiUnavailableException unlikely) {
+ // Because this function allocates a totally random SPI, it really shouldn't ever
+ // fail to allocate an SPI; we simply need this because the exception is checked.
throw new ResourceUnavailableException("No SPIs available");
}
}
@@ -275,8 +297,8 @@
*
* @param destinationAddress the destination address for traffic bearing the requested SPI.
* For inbound traffic, the destination should be an address currently assigned on-device.
- * @param requestedSpi the requested SPI, or '0' to allocate a random SPI. The range 1-255 is
- * reserved and may not be used. See RFC 4303 Section 2.1.
+ * @param requestedSpi the requested SPI. The range 1-255 is reserved and may not be used. See
+ * RFC 4303 Section 2.1.
* @return the reserved SecurityParameterIndex
* @throws {@link #ResourceUnavailableException} indicating that too many SPIs are
* currently allocated for this user
@@ -290,7 +312,11 @@
if (requestedSpi == IpSecManager.INVALID_SECURITY_PARAMETER_INDEX) {
throw new IllegalArgumentException("Requested SPI must be a valid (non-zero) SPI");
}
- return new SecurityParameterIndex(mService, destinationAddress, requestedSpi);
+ try {
+ return new SecurityParameterIndex(mService, destinationAddress, requestedSpi);
+ } catch (ServiceSpecificException e) {
+ throw rethrowUncheckedExceptionFromServiceSpecificException(e);
+ }
}
/**
@@ -425,6 +451,8 @@
// constructor takes control and closes the user's FD when we exit the method.
try (ParcelFileDescriptor pfd = ParcelFileDescriptor.dup(socket)) {
mService.applyTransportModeTransform(pfd, direction, transform.getResourceId());
+ } catch (ServiceSpecificException e) {
+ throw rethrowCheckedExceptionFromServiceSpecificException(e);
} catch (RemoteException e) {
throw e.rethrowFromSystemServer();
}
@@ -483,6 +511,8 @@
public void removeTransportModeTransforms(@NonNull FileDescriptor socket) throws IOException {
try (ParcelFileDescriptor pfd = ParcelFileDescriptor.dup(socket)) {
mService.removeTransportModeTransforms(pfd);
+ } catch (ServiceSpecificException e) {
+ throw rethrowCheckedExceptionFromServiceSpecificException(e);
} catch (RemoteException e) {
throw e.rethrowFromSystemServer();
}
@@ -576,6 +606,13 @@
mResourceId = INVALID_RESOURCE_ID;
} catch (RemoteException e) {
throw e.rethrowFromSystemServer();
+ } 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();
}
try {
@@ -584,7 +621,6 @@
Log.e(TAG, "Failed to close UDP Encapsulation Socket with Port= " + mPort);
throw e;
}
- mCloseGuard.close();
}
/** Check that the socket was closed properly. */
@@ -601,6 +637,17 @@
public int getResourceId() {
return mResourceId;
}
+
+ @Override
+ public String toString() {
+ return new StringBuilder()
+ .append("UdpEncapsulationSocket{port=")
+ .append(mPort)
+ .append(",resourceId=")
+ .append(mResourceId)
+ .append("}")
+ .toString();
+ }
};
/**
@@ -628,7 +675,11 @@
if (port == 0) {
throw new IllegalArgumentException("Specified port must be a valid port number!");
}
- return new UdpEncapsulationSocket(mService, port);
+ try {
+ return new UdpEncapsulationSocket(mService, port);
+ } catch (ServiceSpecificException e) {
+ throw rethrowCheckedExceptionFromServiceSpecificException(e);
+ }
}
/**
@@ -651,7 +702,11 @@
@NonNull
public UdpEncapsulationSocket openUdpEncapsulationSocket()
throws IOException, ResourceUnavailableException {
- return new UdpEncapsulationSocket(mService, 0);
+ try {
+ return new UdpEncapsulationSocket(mService, 0);
+ } catch (ServiceSpecificException e) {
+ throw rethrowCheckedExceptionFromServiceSpecificException(e);
+ }
}
/**
@@ -699,6 +754,8 @@
try {
mService.addAddressToTunnelInterface(
mResourceId, new LinkAddress(address, prefixLen), mOpPackageName);
+ } catch (ServiceSpecificException e) {
+ throw rethrowCheckedExceptionFromServiceSpecificException(e);
} catch (RemoteException e) {
throw e.rethrowFromSystemServer();
}
@@ -719,6 +776,8 @@
try {
mService.removeAddressFromTunnelInterface(
mResourceId, new LinkAddress(address, prefixLen), mOpPackageName);
+ } catch (ServiceSpecificException e) {
+ throw rethrowCheckedExceptionFromServiceSpecificException(e);
} catch (RemoteException e) {
throw e.rethrowFromSystemServer();
}
@@ -771,11 +830,16 @@
public void close() {
try {
mService.deleteTunnelInterface(mResourceId, mOpPackageName);
- mResourceId = INVALID_RESOURCE_ID;
} catch (RemoteException e) {
throw e.rethrowFromSystemServer();
+ } 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();
}
- mCloseGuard.close();
}
/** Check that the Interface was closed properly. */
@@ -792,6 +856,17 @@
public int getResourceId() {
return mResourceId;
}
+
+ @Override
+ public String toString() {
+ return new StringBuilder()
+ .append("IpSecTunnelInterface{ifname=")
+ .append(mInterfaceName)
+ .append(",resourceId=")
+ .append(mResourceId)
+ .append("}")
+ .toString();
+ }
}
/**
@@ -815,8 +890,12 @@
public IpSecTunnelInterface createIpSecTunnelInterface(@NonNull InetAddress localAddress,
@NonNull InetAddress remoteAddress, @NonNull Network underlyingNetwork)
throws ResourceUnavailableException, IOException {
- return new IpSecTunnelInterface(
- mContext, mService, localAddress, remoteAddress, underlyingNetwork);
+ try {
+ return new IpSecTunnelInterface(
+ mContext, mService, localAddress, remoteAddress, underlyingNetwork);
+ } catch (ServiceSpecificException e) {
+ throw rethrowCheckedExceptionFromServiceSpecificException(e);
+ }
}
/**
@@ -844,6 +923,8 @@
mService.applyTunnelModeTransform(
tunnel.getResourceId(), direction,
transform.getResourceId(), mContext.getOpPackageName());
+ } catch (ServiceSpecificException e) {
+ throw rethrowCheckedExceptionFromServiceSpecificException(e);
} catch (RemoteException e) {
throw e.rethrowFromSystemServer();
}
@@ -859,4 +940,44 @@
mContext = ctx;
mService = checkNotNull(service, "missing service");
}
+
+ private static void maybeHandleServiceSpecificException(ServiceSpecificException sse) {
+ // OsConstants are late binding, so switch statements can't be used.
+ if (sse.errorCode == OsConstants.EINVAL) {
+ throw new IllegalArgumentException(sse);
+ } else if (sse.errorCode == OsConstants.EAGAIN) {
+ throw new IllegalStateException(sse);
+ } else if (sse.errorCode == OsConstants.EOPNOTSUPP) {
+ throw new UnsupportedOperationException(sse);
+ }
+ }
+
+ /**
+ * Convert an Errno SSE to the correct Unchecked exception type.
+ *
+ * This method never actually returns.
+ */
+ // package
+ static RuntimeException
+ rethrowUncheckedExceptionFromServiceSpecificException(ServiceSpecificException sse) {
+ maybeHandleServiceSpecificException(sse);
+ throw new RuntimeException(sse);
+ }
+
+ /**
+ * Convert an Errno SSE to the correct Checked or Unchecked exception type.
+ *
+ * This method may throw IOException, or it may throw an unchecked exception; it will never
+ * actually return.
+ */
+ // package
+ static IOException rethrowCheckedExceptionFromServiceSpecificException(
+ ServiceSpecificException sse) throws IOException {
+ // First see if this is an unchecked exception of a type we know.
+ // If so, then we prefer the unchecked (specific) type of exception.
+ maybeHandleServiceSpecificException(sse);
+ // If not, then all we can do is provide the SSE in the form of an IOException.
+ throw new ErrnoException(
+ "IpSec encountered errno=" + sse.errorCode, sse.errorCode).rethrowAsIOException();
+ }
}
diff --git a/core/java/android/net/IpSecTransform.java b/core/java/android/net/IpSecTransform.java
index fb5f46c..23c8aa3 100644
--- a/core/java/android/net/IpSecTransform.java
+++ b/core/java/android/net/IpSecTransform.java
@@ -29,6 +29,7 @@
import android.os.IBinder;
import android.os.RemoteException;
import android.os.ServiceManager;
+import android.os.ServiceSpecificException;
import android.util.Log;
import com.android.internal.annotations.VisibleForTesting;
@@ -137,6 +138,8 @@
mResourceId = result.resourceId;
Log.d(TAG, "Added Transform with Id " + mResourceId);
mCloseGuard.open("build");
+ } catch (ServiceSpecificException e) {
+ throw IpSecManager.rethrowUncheckedExceptionFromServiceSpecificException(e);
} catch (RemoteException e) {
throw e.rethrowAsRuntimeException();
}
@@ -181,6 +184,10 @@
stopNattKeepalive();
} catch (RemoteException e) {
throw e.rethrowAsRuntimeException();
+ } 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();
@@ -507,4 +514,13 @@
mConfig = new IpSecConfig();
}
}
+
+ @Override
+ public String toString() {
+ return new StringBuilder()
+ .append("IpSecTransform{resourceId=")
+ .append(mResourceId)
+ .append("}")
+ .toString();
+ }
}
diff --git a/services/core/java/com/android/server/IpSecService.java b/services/core/java/com/android/server/IpSecService.java
index 9709885..60f1877 100644
--- a/services/core/java/com/android/server/IpSecService.java
+++ b/services/core/java/com/android/server/IpSecService.java
@@ -1100,9 +1100,11 @@
new RefcountedResource<SpiRecord>(
new SpiRecord(resourceId, "", destinationAddress, spi), binder));
} catch (ServiceSpecificException e) {
- // TODO: Add appropriate checks when other ServiceSpecificException types are supported
- return new IpSecSpiResponse(
- IpSecManager.Status.SPI_UNAVAILABLE, INVALID_RESOURCE_ID, spi);
+ if (e.errorCode == OsConstants.ENOENT) {
+ return new IpSecSpiResponse(
+ IpSecManager.Status.SPI_UNAVAILABLE, INVALID_RESOURCE_ID, spi);
+ }
+ throw e;
} catch (RemoteException e) {
throw e.rethrowFromSystemServer();
}
@@ -1114,7 +1116,6 @@
*/
private void releaseResource(RefcountedResourceArray resArray, int resourceId)
throws RemoteException {
-
resArray.getRefcountedResourceOrThrow(resourceId).userRelease();
}
@@ -1314,15 +1315,12 @@
releaseNetId(ikey);
releaseNetId(okey);
throw e.rethrowFromSystemServer();
- } catch (ServiceSpecificException e) {
- // FIXME: get the error code and throw is at an IOException from Errno Exception
+ } catch (Throwable t) {
+ // Release keys if we got an error.
+ releaseNetId(ikey);
+ releaseNetId(okey);
+ throw t;
}
-
- // If we make it to here, then something has gone wrong and we couldn't create a VTI.
- // Release the keys that we reserved, and return an error status.
- releaseNetId(ikey);
- releaseNetId(okey);
- return new IpSecTunnelInterfaceResponse(IpSecManager.Status.RESOURCE_UNAVAILABLE);
}
/**
@@ -1351,9 +1349,6 @@
localAddr.getPrefixLength());
} catch (RemoteException e) {
throw e.rethrowFromSystemServer();
- } catch (ServiceSpecificException e) {
- // If we get here, one of the arguments provided was invalid. Wrap the SSE, and throw.
- throw new IllegalArgumentException(e);
}
}
@@ -1383,9 +1378,6 @@
localAddr.getPrefixLength());
} catch (RemoteException e) {
throw e.rethrowFromSystemServer();
- } catch (ServiceSpecificException e) {
- // If we get here, one of the arguments provided was invalid. Wrap the SSE, and throw.
- throw new IllegalArgumentException(e);
}
}
@@ -1589,12 +1581,7 @@
dependencies.add(refcountedSpiRecord);
SpiRecord spiRecord = refcountedSpiRecord.getResource();
- try {
- createOrUpdateTransform(c, resourceId, spiRecord, socketRecord);
- } catch (ServiceSpecificException e) {
- // FIXME: get the error code and throw is at an IOException from Errno Exception
- return new IpSecTransformResponse(IpSecManager.Status.RESOURCE_UNAVAILABLE);
- }
+ createOrUpdateTransform(c, resourceId, spiRecord, socketRecord);
// SA was created successfully, time to construct a record and lock it away
userRecord.mTransformRecords.put(
@@ -1641,23 +1628,15 @@
c.getMode() == IpSecTransform.MODE_TRANSPORT,
"Transform mode was not Transport mode; cannot be applied to a socket");
- try {
- mSrvConfig
- .getNetdInstance()
- .ipSecApplyTransportModeTransform(
- socket.getFileDescriptor(),
- resourceId,
- direction,
- c.getSourceAddress(),
- c.getDestinationAddress(),
- info.getSpiRecord().getSpi());
- } catch (ServiceSpecificException e) {
- if (e.errorCode == EINVAL) {
- throw new IllegalArgumentException(e.toString());
- } else {
- throw e;
- }
- }
+ mSrvConfig
+ .getNetdInstance()
+ .ipSecApplyTransportModeTransform(
+ socket.getFileDescriptor(),
+ resourceId,
+ direction,
+ c.getSourceAddress(),
+ c.getDestinationAddress(),
+ info.getSpiRecord().getSpi());
}
/**
@@ -1669,13 +1648,9 @@
@Override
public synchronized void removeTransportModeTransforms(ParcelFileDescriptor socket)
throws RemoteException {
- try {
- mSrvConfig
- .getNetdInstance()
- .ipSecRemoveTransportModeTransform(socket.getFileDescriptor());
- } catch (ServiceSpecificException e) {
- // FIXME: get the error code and throw is at an IOException from Errno Exception
- }
+ mSrvConfig
+ .getNetdInstance()
+ .ipSecRemoveTransportModeTransform(socket.getFileDescriptor());
}
/**