Make VM Descriptor AutoCloseable
I also close it implicitly on import - which seems natural, since we
do in fact close the PFDs as part of the import, so no further use can
be made of the descriptor.
Also disallow parcelling or importing once closed.
Add some related tests. (And, while I'm here, update the test
annotations - most of our tests aren't really checking that we can run
all available OSs in the VM.)
Bug: 268613460
Test: atest MicrodroidTests
Change-Id: I0558e4d0357faa5d113e6e1c2b2f8e59ea82c784
diff --git a/javalib/api/system-current.txt b/javalib/api/system-current.txt
index 1ba479f..3170d43 100644
--- a/javalib/api/system-current.txt
+++ b/javalib/api/system-current.txt
@@ -85,7 +85,8 @@
method @NonNull public android.system.virtualmachine.VirtualMachineConfig.Builder setVmOutputCaptured(boolean);
}
- public final class VirtualMachineDescriptor implements android.os.Parcelable {
+ public final class VirtualMachineDescriptor implements java.io.Closeable android.os.Parcelable {
+ method public void close() throws java.io.IOException;
method public int describeContents();
method public void writeToParcel(@NonNull android.os.Parcel, int);
field @NonNull public static final android.os.Parcelable.Creator<android.system.virtualmachine.VirtualMachineDescriptor> CREATOR;
diff --git a/javalib/src/android/system/virtualmachine/VirtualMachine.java b/javalib/src/android/system/virtualmachine/VirtualMachine.java
index 902ed72..9902cb5 100644
--- a/javalib/src/android/system/virtualmachine/VirtualMachine.java
+++ b/javalib/src/android/system/virtualmachine/VirtualMachine.java
@@ -393,27 +393,31 @@
@NonNull String name,
@NonNull VirtualMachineDescriptor vmDescriptor)
throws VirtualMachineException {
- VirtualMachineConfig config = VirtualMachineConfig.from(vmDescriptor.getConfigFd());
File vmDir = createVmDir(context, name);
try {
- VirtualMachine vm =
- new VirtualMachine(context, name, config, VirtualizationService.getInstance());
- config.serialize(vm.mConfigFilePath);
- try {
- vm.mInstanceFilePath.createNewFile();
- } catch (IOException e) {
- throw new VirtualMachineException("failed to create instance image", e);
- }
- vm.importInstanceFrom(vmDescriptor.getInstanceImgFd());
-
- if (vmDescriptor.getEncryptedStoreFd() != null) {
+ VirtualMachine vm;
+ try (vmDescriptor) {
+ VirtualMachineConfig config = VirtualMachineConfig.from(vmDescriptor.getConfigFd());
+ vm = new VirtualMachine(context, name, config, VirtualizationService.getInstance());
+ config.serialize(vm.mConfigFilePath);
try {
- vm.mEncryptedStoreFilePath.createNewFile();
+ vm.mInstanceFilePath.createNewFile();
} catch (IOException e) {
- throw new VirtualMachineException(
- "failed to create encrypted storage image", e);
+ throw new VirtualMachineException("failed to create instance image", e);
}
- vm.importEncryptedStoreFrom(vmDescriptor.getEncryptedStoreFd());
+ vm.importInstanceFrom(vmDescriptor.getInstanceImgFd());
+
+ if (vmDescriptor.getEncryptedStoreFd() != null) {
+ try {
+ vm.mEncryptedStoreFilePath.createNewFile();
+ } catch (IOException e) {
+ throw new VirtualMachineException(
+ "failed to create encrypted storage image", e);
+ }
+ vm.importEncryptedStoreFrom(vmDescriptor.getEncryptedStoreFd());
+ }
+ } catch (IOException e) {
+ throw new VirtualMachineException(e);
}
return vm;
} catch (VirtualMachineException | RuntimeException e) {
diff --git a/javalib/src/android/system/virtualmachine/VirtualMachineDescriptor.java b/javalib/src/android/system/virtualmachine/VirtualMachineDescriptor.java
index 02475a6..1304af3 100644
--- a/javalib/src/android/system/virtualmachine/VirtualMachineDescriptor.java
+++ b/javalib/src/android/system/virtualmachine/VirtualMachineDescriptor.java
@@ -25,6 +25,9 @@
import android.os.ParcelFileDescriptor;
import android.os.Parcelable;
+import java.io.Closeable;
+import java.io.IOException;
+
/**
* A VM descriptor that captures the state of a Virtual Machine.
*
@@ -36,7 +39,8 @@
*/
// TODO(b/268613460): should implement autocloseable.
@SystemApi
-public final class VirtualMachineDescriptor implements Parcelable {
+public final class VirtualMachineDescriptor implements Parcelable, Closeable {
+ private volatile boolean mClosed = false;
@NonNull private final ParcelFileDescriptor mConfigFd;
@NonNull private final ParcelFileDescriptor mInstanceImgFd;
// File descriptor of the image backing the encrypted storage - Will be null if encrypted
@@ -50,6 +54,7 @@
@Override
public void writeToParcel(@NonNull Parcel out, int flags) {
+ checkNotClosed();
out.writeParcelable(mConfigFd, flags);
out.writeParcelable(mInstanceImgFd, flags);
out.writeParcelable(mEncryptedStoreFd, flags);
@@ -72,6 +77,7 @@
*/
@NonNull
ParcelFileDescriptor getConfigFd() {
+ checkNotClosed();
return mConfigFd;
}
@@ -80,6 +86,7 @@
*/
@NonNull
ParcelFileDescriptor getInstanceImgFd() {
+ checkNotClosed();
return mInstanceImgFd;
}
@@ -89,6 +96,7 @@
*/
@Nullable
ParcelFileDescriptor getEncryptedStoreFd() {
+ checkNotClosed();
return mEncryptedStoreFd;
}
@@ -111,4 +119,19 @@
return in.readParcelable(
ParcelFileDescriptor.class.getClassLoader(), ParcelFileDescriptor.class);
}
+
+ @Override
+ public void close() throws IOException {
+ mClosed = true;
+ // Let the compiler do the work: close everything, throw if any of them fail, skipping null.
+ try (mConfigFd;
+ mInstanceImgFd;
+ mEncryptedStoreFd) {}
+ }
+
+ private void checkNotClosed() {
+ if (mClosed) {
+ throw new IllegalStateException("Descriptor has been closed");
+ }
+ }
}
diff --git a/javalib/src/android/system/virtualmachine/VirtualMachineManager.java b/javalib/src/android/system/virtualmachine/VirtualMachineManager.java
index 7773cb5..7c9af63 100644
--- a/javalib/src/android/system/virtualmachine/VirtualMachineManager.java
+++ b/javalib/src/android/system/virtualmachine/VirtualMachineManager.java
@@ -191,7 +191,8 @@
* Imports a virtual machine from an {@link VirtualMachineDescriptor} object and associates it
* with the given name.
*
- * <p>The new virtual machine will be in the same state as the descriptor indicates.
+ * <p>The new virtual machine will be in the same state as the descriptor indicates. The
+ * descriptor is automatically closed and cannot be used again.
*
* <p>NOTE: This method may block and should not be called on the main thread.
*
diff --git a/tests/testapk/src/java/com/android/microdroid/test/MicrodroidTests.java b/tests/testapk/src/java/com/android/microdroid/test/MicrodroidTests.java
index 1030e34..f9ece56 100644
--- a/tests/testapk/src/java/com/android/microdroid/test/MicrodroidTests.java
+++ b/tests/testapk/src/java/com/android/microdroid/test/MicrodroidTests.java
@@ -18,20 +18,20 @@
import static android.system.virtualmachine.VirtualMachine.STATUS_DELETED;
import static android.system.virtualmachine.VirtualMachine.STATUS_RUNNING;
import static android.system.virtualmachine.VirtualMachine.STATUS_STOPPED;
-import static android.system.virtualmachine.VirtualMachineConfig.CPU_TOPOLOGY_ONE_CPU;
import static android.system.virtualmachine.VirtualMachineConfig.CPU_TOPOLOGY_MATCH_HOST;
+import static android.system.virtualmachine.VirtualMachineConfig.CPU_TOPOLOGY_ONE_CPU;
import static android.system.virtualmachine.VirtualMachineConfig.DEBUG_LEVEL_FULL;
import static android.system.virtualmachine.VirtualMachineConfig.DEBUG_LEVEL_NONE;
import static android.system.virtualmachine.VirtualMachineManager.CAPABILITY_NON_PROTECTED_VM;
import static android.system.virtualmachine.VirtualMachineManager.CAPABILITY_PROTECTED_VM;
-
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth.assertWithMessage;
import static com.google.common.truth.TruthJUnit.assume;
-
+import static java.nio.file.StandardCopyOption.REPLACE_EXISTING;
import static org.junit.Assert.assertThrows;
-import static java.nio.file.StandardCopyOption.REPLACE_EXISTING;
+import com.google.common.base.Strings;
+import com.google.common.truth.BooleanSubject;
import android.content.ComponentName;
import android.content.Context;
@@ -53,16 +53,11 @@
import android.system.virtualmachine.VirtualMachineManager;
import android.util.Log;
-import androidx.test.core.app.ApplicationProvider;
-
import com.android.compatibility.common.util.CddTest;
import com.android.microdroid.test.device.MicrodroidDeviceTestBase;
import com.android.microdroid.test.vmshare.IVmShareTestService;
import com.android.microdroid.testservice.ITestService;
-import com.google.common.base.Strings;
-import com.google.common.truth.BooleanSubject;
-
import org.junit.After;
import org.junit.Before;
import org.junit.Ignore;
@@ -98,6 +93,7 @@
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicReference;
+import androidx.test.core.app.ApplicationProvider;
import co.nstant.in.cbor.CborDecoder;
import co.nstant.in.cbor.model.Array;
import co.nstant.in.cbor.model.DataItem;
@@ -222,7 +218,7 @@
}
@Test
- @CddTest(requirements = {"9.17/C-1-1", "9.17/C-2-1"})
+ @CddTest(requirements = {"9.17/C-1-1"})
public void autoCloseVm() throws Exception {
assumeSupportedKernel();
@@ -253,7 +249,61 @@
}
@Test
- @CddTest(requirements = {"9.17/C-1-1", "9.17/C-2-1"})
+ @CddTest(requirements = {"9.17/C-1-1"})
+ public void autoCloseVmDescriptor() throws Exception {
+ VirtualMachineConfig config =
+ newVmConfigBuilder()
+ .setPayloadBinaryName("MicrodroidTestNativeLib.so")
+ .setDebugLevel(DEBUG_LEVEL_FULL)
+ .build();
+ VirtualMachine vm = forceCreateNewVirtualMachine("test_vm", config);
+ VirtualMachineDescriptor descriptor = vm.toDescriptor();
+
+ Parcel parcel = Parcel.obtain();
+ try (descriptor) {
+ // It should be ok to use at this point
+ descriptor.writeToParcel(parcel, 0);
+ }
+
+ // But not now - it's been closed.
+ assertThrows(IllegalStateException.class, () -> descriptor.writeToParcel(parcel, 0));
+ assertThrows(
+ IllegalStateException.class,
+ () -> getVirtualMachineManager().importFromDescriptor("imported_vm", descriptor));
+
+ // Closing again is fine.
+ descriptor.close();
+
+ // Tidy up
+ parcel.recycle();
+ }
+
+ @Test
+ @CddTest(requirements = {"9.17/C-1-1"})
+ public void vmDescriptorClosedOnImport() throws Exception {
+ VirtualMachineConfig config =
+ newVmConfigBuilder()
+ .setPayloadBinaryName("MicrodroidTestNativeLib.so")
+ .setDebugLevel(DEBUG_LEVEL_FULL)
+ .build();
+ VirtualMachine vm = forceCreateNewVirtualMachine("test_vm", config);
+ VirtualMachineDescriptor descriptor = vm.toDescriptor();
+
+ getVirtualMachineManager().importFromDescriptor("imported_vm", descriptor);
+ try {
+ // Descriptor has been implicitly closed
+ assertThrows(
+ IllegalStateException.class,
+ () ->
+ getVirtualMachineManager()
+ .importFromDescriptor("imported_vm2", descriptor));
+ } finally {
+ getVirtualMachineManager().delete("imported_vm");
+ }
+ }
+
+ @Test
+ @CddTest(requirements = {"9.17/C-1-1"})
public void vmLifecycleChecks() throws Exception {
assumeSupportedKernel();
@@ -1206,7 +1256,6 @@
VirtualMachine vmOrig = forceCreateNewVirtualMachine(vmNameOrig, config);
VmCdis origCdis = launchVmAndGetCdis(vmNameOrig);
assertThat(origCdis.instanceSecret).isNotNull();
- VirtualMachineDescriptor descriptor = vmOrig.toDescriptor();
VirtualMachineManager vmm = getVirtualMachineManager();
if (vmm.get(vmNameImport) != null) {
vmm.delete(vmNameImport);
@@ -1214,7 +1263,7 @@
// Action
// The imported VM will be fetched by name later.
- VirtualMachine unusedVmImport = vmm.importFromDescriptor(vmNameImport, descriptor);
+ vmm.importFromDescriptor(vmNameImport, vmOrig.toDescriptor());
// Asserts
VmCdis importCdis = launchVmAndGetCdis(vmNameImport);
@@ -1222,14 +1271,14 @@
}
@Test
- @CddTest(requirements = {"9.17/C-1-1", "9.17/C-2-1"})
+ @CddTest(requirements = {"9.17/C-1-1"})
public void importedVmIsEqualToTheOriginalVm_WithoutStorage() throws Exception {
TestResults testResults = importedVmIsEqualToTheOriginalVm(false);
assertThat(testResults.mEncryptedStoragePath).isEqualTo("");
}
@Test
- @CddTest(requirements = {"9.17/C-1-1", "9.17/C-2-1"})
+ @CddTest(requirements = {"9.17/C-1-1"})
public void importedVmIsEqualToTheOriginalVm_WithStorage() throws Exception {
TestResults testResults = importedVmIsEqualToTheOriginalVm(true);
assertThat(testResults.mEncryptedStoragePath).isEqualTo("/mnt/encryptedstore");
@@ -1259,14 +1308,13 @@
});
assertThat(origTestResults.mException).isNull();
assertThat(origTestResults.mAddInteger).isEqualTo(123 + 456);
- VirtualMachineDescriptor descriptor = vmOrig.toDescriptor();
VirtualMachineManager vmm = getVirtualMachineManager();
if (vmm.get(vmNameImport) != null) {
vmm.delete(vmNameImport);
}
// Action
- VirtualMachine vmImport = vmm.importFromDescriptor(vmNameImport, descriptor);
+ VirtualMachine vmImport = vmm.importFromDescriptor(vmNameImport, vmOrig.toDescriptor());
// Asserts
assertFileContentsAreEqualInTwoVms("config.xml", vmNameOrig, vmNameImport);
@@ -1290,7 +1338,7 @@
}
@Test
- @CddTest(requirements = {"9.17/C-1-1", "9.17/C-2-1"})
+ @CddTest(requirements = {"9.17/C-1-1"})
public void encryptedStorageAvailable() throws Exception {
assumeSupportedKernel();
@@ -1313,7 +1361,7 @@
}
@Test
- @CddTest(requirements = {"9.17/C-1-1", "9.17/C-2-1"})
+ @CddTest(requirements = {"9.17/C-1-1"})
public void encryptedStorageIsInaccessibleToDifferentVm() throws Exception {
assumeSupportedKernel();
@@ -1396,7 +1444,7 @@
}
@Test
- @CddTest(requirements = {"9.17/C-1-1", "9.17/C-2-1"})
+ @CddTest(requirements = {"9.17/C-1-1"})
public void encryptedStorageIsPersistent() throws Exception {
assumeSupportedKernel();