Merge "Rework Exception Handling for IpSecManager" into pi-dev

am: 16c671dc9a

Change-Id: I9c1acdc79053c70a8484935f61a537379b9be927

core/java/android/net/IpSecManager.java

@@ -26,6 +26,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;
 
@@ -172,12 +175,17 @@ 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();
             }
+        }
 
         /** Check that the SPI was closed properly. */
         @Override
@@ -227,7 +235,6 @@ public final class IpSecManager {
                     throw new RuntimeException(
                             "Invalid Resource ID returned by IpSecService: " + status);
                 }
-
             } catch (RemoteException e) {
                 throw e.rethrowFromSystemServer();
             }
@@ -239,6 +246,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();
+        }
     }
 
     /**
@@ -261,7 +279,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");
         }
     }
@@ -274,8 +296,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
+     * @param requestedSpi the requested SPI. The range 1-255 is reserved and may not be used. See
-     *     reserved and may not be used. See RFC 4303 Section 2.1.
+     *     RFC 4303 Section 2.1.
      * @return the reserved SecurityParameterIndex
      * @throws {@link #ResourceUnavailableException} indicating that too many SPIs are
      *     currently allocated for this user
@@ -289,7 +311,11 @@ public final class IpSecManager {
         if (requestedSpi == IpSecManager.INVALID_SECURITY_PARAMETER_INDEX) {
             throw new IllegalArgumentException("Requested SPI must be a valid (non-zero) SPI");
         }
+        try {
             return new SecurityParameterIndex(mService, destinationAddress, requestedSpi);
+        } catch (ServiceSpecificException e) {
+            throw rethrowUncheckedExceptionFromServiceSpecificException(e);
+        }
     }
 
     /**
@@ -424,6 +450,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();
         }
@@ -482,6 +510,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();
         }
@@ -575,6 +605,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 {
@@ -583,7 +620,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. */
@@ -600,6 +636,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();
+        }
     };
 
     /**
@@ -627,7 +674,11 @@ public final class IpSecManager {
         if (port == 0) {
             throw new IllegalArgumentException("Specified port must be a valid port number!");
         }
+        try {
             return new UdpEncapsulationSocket(mService, port);
+        } catch (ServiceSpecificException e) {
+            throw rethrowCheckedExceptionFromServiceSpecificException(e);
+        }
     }
 
     /**
@@ -650,7 +701,11 @@ public final class IpSecManager {
     @NonNull
     public UdpEncapsulationSocket openUdpEncapsulationSocket()
             throws IOException, ResourceUnavailableException {
+        try {
             return new UdpEncapsulationSocket(mService, 0);
+        } catch (ServiceSpecificException e) {
+            throw rethrowCheckedExceptionFromServiceSpecificException(e);
+        }
     }
 
     /**
@@ -696,6 +751,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();
             }
@@ -715,6 +772,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();
             }
@@ -767,12 +826,17 @@ 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();
             }
+        }
 
         /** Check that the Interface was closed properly. */
         @Override
@@ -788,6 +852,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();
+        }
     }
 
     /**
@@ -810,8 +885,12 @@ public final class IpSecManager {
     public IpSecTunnelInterface createIpSecTunnelInterface(@NonNull InetAddress localAddress,
             @NonNull InetAddress remoteAddress, @NonNull Network underlyingNetwork)
             throws ResourceUnavailableException, IOException {
+        try {
             return new IpSecTunnelInterface(
                     mContext, mService, localAddress, remoteAddress, underlyingNetwork);
+        } catch (ServiceSpecificException e) {
+            throw rethrowCheckedExceptionFromServiceSpecificException(e);
+        }
     }
 
     /**
@@ -838,6 +917,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();
         }
@@ -853,4 +934,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();
+    }
 }

core/java/android/net/IpSecTransform.java

@@ -28,6 +28,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;
@@ -136,6 +137,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();
             }
@@ -180,6 +183,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();
@@ -502,4 +509,13 @@ public final class IpSecTransform implements AutoCloseable {
             mConfig = new IpSecConfig();
         }
     }
+
+    @Override
+    public String toString() {
+        return new StringBuilder()
+            .append("IpSecTransform{resourceId=")
+            .append(mResourceId)
+            .append("}")
+            .toString();
+    }
 }

services/core/java/com/android/server/IpSecService.java

@@ -1101,9 +1101,11 @@ public class IpSecService extends IIpSecService.Stub {
                     new RefcountedResource<SpiRecord>(
                             new SpiRecord(resourceId, "", destinationAddress, spi), binder));
         } catch (ServiceSpecificException e) {
-            // TODO: Add appropriate checks when other ServiceSpecificException types are supported
+            if (e.errorCode == OsConstants.ENOENT) {
                 return new IpSecSpiResponse(
                         IpSecManager.Status.SPI_UNAVAILABLE, INVALID_RESOURCE_ID, spi);
+            }
+            throw e;
         } catch (RemoteException e) {
             throw e.rethrowFromSystemServer();
         }
@@ -1115,7 +1117,6 @@ public class IpSecService extends IIpSecService.Stub {
      */
     private void releaseResource(RefcountedResourceArray resArray, int resourceId)
             throws RemoteException {
-
         resArray.getRefcountedResourceOrThrow(resourceId).userRelease();
     }
 
@@ -1315,15 +1316,12 @@ public class IpSecService extends IIpSecService.Stub {
             releaseNetId(ikey);
             releaseNetId(okey);
             throw e.rethrowFromSystemServer();
-        } catch (ServiceSpecificException e) {
+        } catch (Throwable t) {
-            // FIXME: get the error code and throw is at an IOException from Errno Exception
+            // Release keys if we got an error.
-        }
-
-        // 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);
+            throw t;
+        }
     }
 
     /**
@@ -1352,9 +1350,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);
         }
     }
 
@@ -1384,9 +1379,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);
         }
     }
 
@@ -1590,12 +1582,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);
-        }
 
         // SA was created successfully, time to construct a record and lock it away
         userRecord.mTransformRecords.put(
@@ -1642,7 +1629,6 @@ 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(
@@ -1652,13 +1638,6 @@ public class IpSecService extends IIpSecService.Stub {
                         c.getSourceAddress(),
                         c.getDestinationAddress(),
                         info.getSpiRecord().getSpi());
-        } catch (ServiceSpecificException e) {
-            if (e.errorCode == EINVAL) {
-                throw new IllegalArgumentException(e.toString());
-            } else {
-                throw e;
-            }
-        }
     }
 
     /**
@@ -1670,13 +1649,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
-        }
     }
 
     /**