From 144ce0a2485b061b4ae9090b2b9f558e6e3d0e04 Mon Sep 17 00:00:00 2001 From: Nathan Harold Date: Tue, 3 Apr 2018 16:13:19 -0700 Subject: [PATCH] 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 ddeb90aa9db108d4a2e5aadc778a726b65e5c921) --- core/java/android/net/IpSecManager.java | 147 +++++++++++++++++++-- core/java/android/net/IpSecTransform.java | 16 +++ .../core/java/com/android/server/IpSecService.java | 71 ++++------ 3 files changed, 173 insertions(+), 61 deletions(-) diff --git a/core/java/android/net/IpSecManager.java b/core/java/android/net/IpSecManager.java index 87323755a9c1..1145d5bd4d9a 100644 --- a/core/java/android/net/IpSecManager.java +++ b/core/java/android/net/IpSecManager.java @@ -27,6 +27,9 @@ import android.content.Context; 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 final class IpSecManager { 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 @@ public final class IpSecManager { throw new RuntimeException( "Invalid Resource ID returned by IpSecService: " + status); } - } catch (RemoteException e) { throw e.rethrowFromSystemServer(); } @@ -240,6 +247,17 @@ public final class IpSecManager { 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 @@ public final class IpSecManager { 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 @@ public final class IpSecManager { * * @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 @@ public final class IpSecManager { 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 @@ public final class IpSecManager { // 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 final class IpSecManager { 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 @@ public final class IpSecManager { 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 @@ public final class IpSecManager { 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 final class IpSecManager { 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 @@ public final class IpSecManager { 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 @@ public final class IpSecManager { @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 @@ public final class IpSecManager { 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 @@ public final class IpSecManager { 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 final class IpSecManager { 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 final class IpSecManager { 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 final class IpSecManager { 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 @@ public final class IpSecManager { 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 @@ public final class IpSecManager { 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 fb5f46c117bf..23c8aa368d87 100644 --- a/core/java/android/net/IpSecTransform.java +++ b/core/java/android/net/IpSecTransform.java @@ -29,6 +29,7 @@ import android.os.Handler; 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 @@ public final class IpSecTransform implements AutoCloseable { 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 @@ public final class IpSecTransform implements AutoCloseable { 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 @@ public final class IpSecTransform implements AutoCloseable { 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 97098858870c..60f1877d3739 100644 --- a/services/core/java/com/android/server/IpSecService.java +++ b/services/core/java/com/android/server/IpSecService.java @@ -1100,9 +1100,11 @@ public class IpSecService extends IIpSecService.Stub { new RefcountedResource( 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 @@ public class IpSecService extends IIpSecService.Stub { */ private void releaseResource(RefcountedResourceArray resArray, int resourceId) throws RemoteException { - resArray.getRefcountedResourceOrThrow(resourceId).userRelease(); } @@ -1314,15 +1315,12 @@ public class IpSecService extends IIpSecService.Stub { 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 @@ public class IpSecService extends IIpSecService.Stub { 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 @@ public class IpSecService extends IIpSecService.Stub { 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 @@ public class IpSecService extends IIpSecService.Stub { 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 @@ public class IpSecService extends IIpSecService.Stub { 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 @@ public class IpSecService extends IIpSecService.Stub { @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()); } /** -- 2.11.0