LineageOS/android_packages_modules_Connectivity
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Merge "ClatCoordinator: improve file descriptor clean up in error handling"

Browse Source
This commit is contained in:
Treehugger Robot
2022-06-08 07:19:31 +00:00
committed by Gerrit Code Review
parent d3a3af5c34 b7bbdbebc6
commit a58028e31a
2 changed files with 87 additions and 53 deletions
Download Patch File Download Diff File Expand all files Collapse all files

75
service/src/com/android/server/connectivity/ClatCoordinator.java
View File

@@ -498,6 +498,31 @@ public class ClatCoordinator {
}
}
private void maybeCleanUp(ParcelFileDescriptor tunFd, ParcelFileDescriptor readSock6,
ParcelFileDescriptor writeSock6) {
if (tunFd != null) {
try {
tunFd.close();
} catch (IOException e) {
Log.e(TAG, "Fail to close tun file descriptor " + e);
}
}
if (readSock6 != null) {
try {
readSock6.close();
} catch (IOException e) {
Log.e(TAG, "Fail to close read socket " + e);
}
}
if (writeSock6 != null) {
try {
writeSock6.close();
} catch (IOException e) {
Log.e(TAG, "Fail to close write socket " + e);
}
}
}
/**
* Start clatd for a given interface and NAT64 prefix.
*/
@@ -546,8 +571,15 @@ public class ClatCoordinator {
// [3] Open, configure and bring up the tun interface.
// Create the v4-... tun interface.
// Initialize all required file descriptors with null pointer. This makes the following
// error handling easier. Simply always call #maybeCleanUp for closing file descriptors,
// if any valid ones, in error handling.
ParcelFileDescriptor tunFd = null;
ParcelFileDescriptor readSock6 = null;
ParcelFileDescriptor writeSock6 = null;
final String tunIface = CLAT_PREFIX + iface;
final ParcelFileDescriptor tunFd;
try {
tunFd = mDeps.adoptFd(mDeps.createTunInterface(tunIface));
} catch (IOException e) {
@@ -556,7 +588,7 @@ public class ClatCoordinator {
final int tunIfIndex = mDeps.getInterfaceIndex(tunIface);
if (tunIfIndex == INVALID_IFINDEX) {
tunFd.close();
maybeCleanUp(tunFd, readSock6, writeSock6);
throw new IOException("Fail to get interface index for interface " + tunIface);
}
@@ -564,7 +596,7 @@ public class ClatCoordinator {
try {
mNetd.interfaceSetEnableIPv6(tunIface, false /* enabled */);
} catch (RemoteException | ServiceSpecificException e) {
tunFd.close();
maybeCleanUp(tunFd, readSock6, writeSock6);
Log.e(TAG, "Disable IPv6 on " + tunIface + " failed: " + e);
}
@@ -575,19 +607,17 @@ public class ClatCoordinator {
detectedMtu = mDeps.detectMtu(pfx96Str,
ByteBuffer.wrap(GOOGLE_DNS_4.getAddress()).getInt(), fwmark);
} catch (IOException e) {
tunFd.close();
maybeCleanUp(tunFd, readSock6, writeSock6);
throw new IOException("Detect MTU on " + tunIface + " failed: " + e);
}
final int mtu = adjustMtu(detectedMtu);
Log.i(TAG, "ipv4 mtu is " + mtu);
// TODO: add setIptablesDropRule
// Config tun interface mtu, address and bring up.
try {
mNetd.interfaceSetMtu(tunIface, mtu);
} catch (RemoteException | ServiceSpecificException e) {
tunFd.close();
maybeCleanUp(tunFd, readSock6, writeSock6);
throw new IOException("Set MTU " + mtu + " on " + tunIface + " failed: " + e);
}
final InterfaceConfigurationParcel ifConfig = new InterfaceConfigurationParcel();
@@ -599,14 +629,13 @@ public class ClatCoordinator {
try {
mNetd.interfaceSetCfg(ifConfig);
} catch (RemoteException | ServiceSpecificException e) {
tunFd.close();
maybeCleanUp(tunFd, readSock6, writeSock6);
throw new IOException("Setting IPv4 address to " + ifConfig.ipv4Addr + "/"
+ ifConfig.prefixLength + " failed on " + ifConfig.ifName + ": " + e);
}
// [4] Open and configure local 464xlat read/write sockets.
// Opens a packet socket to receive IPv6 packets in clatd.
final ParcelFileDescriptor readSock6;
try {
// Use a JNI call to get native file descriptor instead of Os.socket() because we would
// like to use ParcelFileDescriptor to manage file descriptor. But ctor
@@ -614,27 +643,23 @@ public class ClatCoordinator {
// descriptor to initialize ParcelFileDescriptor object instead.
readSock6 = mDeps.adoptFd(mDeps.openPacketSocket());
} catch (IOException e) {
tunFd.close();
maybeCleanUp(tunFd, readSock6, writeSock6);
throw new IOException("Open packet socket failed: " + e);
}
// Opens a raw socket with a given fwmark to send IPv6 packets in clatd.
final ParcelFileDescriptor writeSock6;
try {
// Use a JNI call to get native file descriptor instead of Os.socket(). See above
// reason why we use jniOpenPacketSocket6().
writeSock6 = mDeps.adoptFd(mDeps.openRawSocket6(fwmark));
} catch (IOException e) {
tunFd.close();
readSock6.close();
maybeCleanUp(tunFd, readSock6, writeSock6);
throw new IOException("Open raw socket failed: " + e);
}
final int ifIndex = mDeps.getInterfaceIndex(iface);
if (ifIndex == INVALID_IFINDEX) {
tunFd.close();
readSock6.close();
writeSock6.close();
maybeCleanUp(tunFd, readSock6, writeSock6);
throw new IOException("Fail to get interface index for interface " + iface);
}
@@ -642,9 +667,7 @@ public class ClatCoordinator {
try {
mDeps.addAnycastSetsockopt(writeSock6.getFileDescriptor(), v6Str, ifIndex);
} catch (IOException e) {
tunFd.close();
readSock6.close();
writeSock6.close();
maybeCleanUp(tunFd, readSock6, writeSock6);
throw new IOException("add anycast sockopt failed: " + e);
}
@@ -653,9 +676,7 @@ public class ClatCoordinator {
try {
cookie = mDeps.tagSocketAsClat(writeSock6.getFileDescriptor());
} catch (IOException e) {
tunFd.close();
readSock6.close();
writeSock6.close();
maybeCleanUp(tunFd, readSock6, writeSock6);
throw new IOException("tag raw socket failed: " + e);
}
@@ -663,9 +684,7 @@ public class ClatCoordinator {
try {
mDeps.configurePacketSocket(readSock6.getFileDescriptor(), v6Str, ifIndex);
} catch (IOException e) {
tunFd.close();
readSock6.close();
writeSock6.close();
maybeCleanUp(tunFd, readSock6, writeSock6);
throw new IOException("configure packet socket failed: " + e);
}
@@ -679,9 +698,9 @@ public class ClatCoordinator {
mDeps.untagSocket(cookie);
throw new IOException("Error start clatd on " + iface + ": " + e);
} finally {
tunFd.close();
readSock6.close();
writeSock6.close();
// The file descriptors have been duplicated (dup2) to clatd in native_startClatd().
// Close these file descriptor stubs which are unused anymore.
maybeCleanUp(tunFd, readSock6, writeSock6);
}
// [6] Initialize and store clatd tracker object.

65
tests/unit/java/com/android/server/connectivity/ClatCoordinatorTest.java
View File

@@ -37,6 +37,7 @@ import static org.mockito.Mockito.argThat;
import static org.mockito.Mockito.clearInvocations;
import static org.mockito.Mockito.eq;
import static org.mockito.Mockito.inOrder;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.verify;
@@ -556,14 +557,28 @@ public class ClatCoordinatorTest {
() -> coordinator.clatStart(BASE_IFACE, NETID, NAT64_IP_PREFIX));
}
private void checkNotStartClat(final TestDependencies deps, final boolean verifyTunFd,
final boolean verifyPacketSockFd, final boolean verifyRawSockFd) throws Exception {
private void checkNotStartClat(final TestDependencies deps, final boolean needToCloseTunFd,
final boolean needToClosePacketSockFd, final boolean needToCloseRawSockFd)
throws Exception {
// [1] Expect that modified TestDependencies can't start clatd.
// Use precise check to make sure that there is no unexpected file descriptor closing.
clearInvocations(TUN_PFD, RAW_SOCK_PFD, PACKET_SOCK_PFD);
assertNotStartClat(deps);
if (verifyTunFd) verify(TUN_PFD).close();
if (verifyPacketSockFd) verify(PACKET_SOCK_PFD).close();
if (verifyRawSockFd) verify(RAW_SOCK_PFD).close();
if (needToCloseTunFd) {
verify(TUN_PFD).close();
} else {
verify(TUN_PFD, never()).close();
}
if (needToClosePacketSockFd) {
verify(PACKET_SOCK_PFD).close();
} else {
verify(PACKET_SOCK_PFD, never()).close();
}
if (needToCloseRawSockFd) {
verify(RAW_SOCK_PFD).close();
} else {
verify(RAW_SOCK_PFD, never()).close();
}
// [2] Expect that unmodified TestDependencies can start clatd.
// Used to make sure that the above modified TestDependencies has really broken the
@@ -582,8 +597,8 @@ public class ClatCoordinatorTest {
throw new IOException();
}
}
checkNotStartClat(new FailureDependencies(), false /* verifyTunFd */,
false /* verifyPacketSockFd */, false /* verifyRawSockFd */);
checkNotStartClat(new FailureDependencies(), false /* needToCloseTunFd */,
false /* needToClosePacketSockFd */, false /* needToCloseRawSockFd */);
}
@Test
@@ -595,8 +610,8 @@ public class ClatCoordinatorTest {
throw new IOException();
}
}
checkNotStartClat(new FailureDependencies(), false /* verifyTunFd */,
false /* verifyPacketSockFd */, false /* verifyRawSockFd */);
checkNotStartClat(new FailureDependencies(), false /* needToCloseTunFd */,
false /* needToClosePacketSockFd */, false /* needToCloseRawSockFd */);
}
@Test
@@ -607,8 +622,8 @@ public class ClatCoordinatorTest {
throw new IOException();
}
}
checkNotStartClat(new FailureDependencies(), false /* verifyTunFd */,
false /* verifyPacketSockFd */, false /* verifyRawSockFd */);
checkNotStartClat(new FailureDependencies(), false /* needToCloseTunFd */,
false /* needToClosePacketSockFd */, false /* needToCloseRawSockFd */);
}
@Test
@@ -620,8 +635,8 @@ public class ClatCoordinatorTest {
throw new IOException();
}
}
checkNotStartClat(new FailureDependencies(), true /* verifyTunFd */,
false /* verifyPacketSockFd */, false /* verifyRawSockFd */);
checkNotStartClat(new FailureDependencies(), true /* needToCloseTunFd */,
false /* needToClosePacketSockFd */, false /* needToCloseRawSockFd */);
}
@Test
@@ -632,8 +647,8 @@ public class ClatCoordinatorTest {
throw new IOException();
}
}
checkNotStartClat(new FailureDependencies(), true /* verifyTunFd */,
false /* verifyPacketSockFd */, false /* verifyRawSockFd */);
checkNotStartClat(new FailureDependencies(), true /* needToCloseTunFd */,
false /* needToClosePacketSockFd */, false /* needToCloseRawSockFd */);
}
@Test
@@ -644,8 +659,8 @@ public class ClatCoordinatorTest {
throw new IOException();
}
}
checkNotStartClat(new FailureDependencies(), true /* verifyTunFd */,
true /* verifyPacketSockFd */, false /* verifyRawSockFd */);
checkNotStartClat(new FailureDependencies(), true /* needToCloseTunFd */,
true /* needToClosePacketSockFd */, false /* needToCloseRawSockFd */);
}
@Test
@@ -657,8 +672,8 @@ public class ClatCoordinatorTest {
throw new IOException();
}
}
checkNotStartClat(new FailureDependencies(), true /* verifyTunFd */,
true /* verifyPacketSockFd */, true /* verifyRawSockFd */);
checkNotStartClat(new FailureDependencies(), true /* needToCloseTunFd */,
true /* needToClosePacketSockFd */, true /* needToCloseRawSockFd */);
}
@Test
@@ -669,8 +684,8 @@ public class ClatCoordinatorTest {
throw new IOException();
}
}
checkNotStartClat(new FailureDependencies(), true /* verifyTunFd */,
true /* verifyPacketSockFd */, true /* verifyRawSockFd */);
checkNotStartClat(new FailureDependencies(), true /* needToCloseTunFd */,
true /* needToClosePacketSockFd */, true /* needToCloseRawSockFd */);
}
@Test
@@ -682,8 +697,8 @@ public class ClatCoordinatorTest {
throw new IOException();
}
}
checkNotStartClat(new FailureDependencies(), true /* verifyTunFd */,
true /* verifyPacketSockFd */, true /* verifyRawSockFd */);
checkNotStartClat(new FailureDependencies(), true /* needToCloseTunFd */,
true /* needToClosePacketSockFd */, true /* needToCloseRawSockFd */);
}
@Test
@@ -697,7 +712,7 @@ public class ClatCoordinatorTest {
throw new IOException();
}
}
checkNotStartClat(new FailureDependencies(), true /* verifyTunFd */,
true /* verifyPacketSockFd */, true /* verifyRawSockFd */);
checkNotStartClat(new FailureDependencies(), true /* needToCloseTunFd */,
true /* needToClosePacketSockFd */, true /* needToCloseRawSockFd */);
}
}