From 54525bf939ded4429aa3b7dd46954ed20bfa9320 Mon Sep 17 00:00:00 2001 From: Neil Fuller Date: Thu, 22 Jun 2017 14:10:29 +0100 Subject: [PATCH] Switch to streaming data for time zone update Switch to streaming data for time zone update rather than loading it into memory first. Also make sure that the ParcelFileDescriptor passed to the service is closed in all cases. make -j30 FrameworksServicesTests adb install -r -g \ "out/target/product/angler/data/app/FrameworksServicesTests/FrameworksServicesTests.apk" adb shell am instrument -e package com.android.server.timezone -w \ com.android.frameworks.servicestests \ "com.android.frameworks.servicestests/android.support.test.runner.AndroidJUnitRunner" Test: See above Test: Manual Bug: 31008728 Change-Id: Ia1e27b204697caee62deb2a3d682800350bca800 --- .../server/timezone/FileDescriptorHelper.java | 30 ----- .../server/timezone/RulesManagerService.java | 93 +++++++------ .../RulesManagerServiceHelperImpl.java | 12 +- .../timezone/RulesManagerServiceTest.java | 122 +++++++----------- 4 files changed, 107 insertions(+), 150 deletions(-) delete mode 100644 services/core/java/com/android/server/timezone/FileDescriptorHelper.java diff --git a/services/core/java/com/android/server/timezone/FileDescriptorHelper.java b/services/core/java/com/android/server/timezone/FileDescriptorHelper.java deleted file mode 100644 index c3b1101000b5a..0000000000000 --- a/services/core/java/com/android/server/timezone/FileDescriptorHelper.java +++ /dev/null @@ -1,30 +0,0 @@ -/* - * Copyright (C) 2017 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package com.android.server.timezone; - -import android.os.ParcelFileDescriptor; - -import java.io.IOException; - -/** - * An easy-to-mock interface around use of {@link ParcelFileDescriptor} for use by - * {@link RulesManagerService}. - */ -interface FileDescriptorHelper { - - byte[] readFully(ParcelFileDescriptor parcelFileDescriptor) throws IOException; -} diff --git a/services/core/java/com/android/server/timezone/RulesManagerService.java b/services/core/java/com/android/server/timezone/RulesManagerService.java index 58bdeb9d11ec5..804a8b79310ec 100644 --- a/services/core/java/com/android/server/timezone/RulesManagerService.java +++ b/services/core/java/com/android/server/timezone/RulesManagerService.java @@ -20,8 +20,8 @@ import com.android.internal.annotations.VisibleForTesting; import com.android.server.SystemService; import com.android.timezone.distro.DistroException; import com.android.timezone.distro.DistroVersion; -import com.android.timezone.distro.TimeZoneDistro; import com.android.timezone.distro.StagedDistroOperation; +import com.android.timezone.distro.TimeZoneDistro; import android.app.timezone.Callback; import android.app.timezone.DistroFormatVersion; @@ -36,7 +36,9 @@ import android.os.RemoteException; import android.util.Slog; import java.io.File; +import java.io.FileInputStream; import java.io.IOException; +import java.io.InputStream; import java.util.Arrays; import java.util.concurrent.Executor; import java.util.concurrent.atomic.AtomicBoolean; @@ -83,26 +85,22 @@ public final class RulesManagerService extends IRulesManager.Stub { private final PackageTracker mPackageTracker; private final Executor mExecutor; private final TimeZoneDistroInstaller mInstaller; - private final FileDescriptorHelper mFileDescriptorHelper; private static RulesManagerService create(Context context) { RulesManagerServiceHelperImpl helper = new RulesManagerServiceHelperImpl(context); return new RulesManagerService( helper /* permissionHelper */, helper /* executor */, - helper /* fileDescriptorHelper */, PackageTracker.create(context), new TimeZoneDistroInstaller(TAG, SYSTEM_TZ_DATA_FILE, TZ_DATA_DIR)); } // A constructor that can be used by tests to supply mocked / faked dependencies. RulesManagerService(PermissionHelper permissionHelper, - Executor executor, - FileDescriptorHelper fileDescriptorHelper, PackageTracker packageTracker, + Executor executor, PackageTracker packageTracker, TimeZoneDistroInstaller timeZoneDistroInstaller) { mPermissionHelper = permissionHelper; mExecutor = executor; - mFileDescriptorHelper = fileDescriptorHelper; mPackageTracker = packageTracker; mInstaller = timeZoneDistroInstaller; } @@ -177,55 +175,78 @@ public final class RulesManagerService extends IRulesManager.Stub { } @Override - public int requestInstall( - ParcelFileDescriptor timeZoneDistro, byte[] checkTokenBytes, ICallback callback) { - mPermissionHelper.enforceCallerHasPermission(REQUIRED_UPDATER_PERMISSION); + public int requestInstall(ParcelFileDescriptor distroParcelFileDescriptor, + byte[] checkTokenBytes, ICallback callback) { - CheckToken checkToken = null; - if (checkTokenBytes != null) { - checkToken = createCheckTokenOrThrow(checkTokenBytes); - } - synchronized (this) { - if (timeZoneDistro == null) { - throw new NullPointerException("timeZoneDistro == null"); - } - if (callback == null) { - throw new NullPointerException("observer == null"); - } - if (mOperationInProgress.get()) { - return RulesManager.ERROR_OPERATION_IN_PROGRESS; - } - mOperationInProgress.set(true); + boolean closeParcelFileDescriptorOnExit = true; + try { + mPermissionHelper.enforceCallerHasPermission(REQUIRED_UPDATER_PERMISSION); - // Execute the install asynchronously. - mExecutor.execute(new InstallRunnable(timeZoneDistro, checkToken, callback)); + CheckToken checkToken = null; + if (checkTokenBytes != null) { + checkToken = createCheckTokenOrThrow(checkTokenBytes); + } - return RulesManager.SUCCESS; + synchronized (this) { + if (distroParcelFileDescriptor == null) { + throw new NullPointerException("distroParcelFileDescriptor == null"); + } + if (callback == null) { + throw new NullPointerException("observer == null"); + } + if (mOperationInProgress.get()) { + return RulesManager.ERROR_OPERATION_IN_PROGRESS; + } + mOperationInProgress.set(true); + + // Execute the install asynchronously. + mExecutor.execute( + new InstallRunnable(distroParcelFileDescriptor, checkToken, callback)); + + // The InstallRunnable now owns the ParcelFileDescriptor, so it will close it after + // it executes (and we do not have to). + closeParcelFileDescriptorOnExit = false; + + return RulesManager.SUCCESS; + } + } finally { + // We should close() the local ParcelFileDescriptor we were passed if it hasn't been + // passed to another thread to handle. + if (distroParcelFileDescriptor != null && closeParcelFileDescriptorOnExit) { + try { + distroParcelFileDescriptor.close(); + } catch (IOException e) { + Slog.w(TAG, "Failed to close distroParcelFileDescriptor", e); + } + } } } private class InstallRunnable implements Runnable { - private final ParcelFileDescriptor mTimeZoneDistro; + private final ParcelFileDescriptor mDistroParcelFileDescriptor; private final CheckToken mCheckToken; private final ICallback mCallback; - InstallRunnable( - ParcelFileDescriptor timeZoneDistro, CheckToken checkToken, ICallback callback) { - mTimeZoneDistro = timeZoneDistro; + InstallRunnable(ParcelFileDescriptor distroParcelFileDescriptor, CheckToken checkToken, + ICallback callback) { + mDistroParcelFileDescriptor = distroParcelFileDescriptor; mCheckToken = checkToken; mCallback = callback; } @Override public void run() { + boolean success = false; // Adopt the ParcelFileDescriptor into this try-with-resources so it is closed // when we are done. - boolean success = false; - try { - byte[] distroBytes = - RulesManagerService.this.mFileDescriptorHelper.readFully(mTimeZoneDistro); - TimeZoneDistro distro = new TimeZoneDistro(distroBytes); + try (ParcelFileDescriptor pfd = mDistroParcelFileDescriptor) { + // The ParcelFileDescriptor owns the underlying FileDescriptor and we'll close + // it at the end of the try-with-resources. + final boolean isFdOwner = false; + InputStream is = new FileInputStream(pfd.getFileDescriptor(), isFdOwner); + + TimeZoneDistro distro = new TimeZoneDistro(is); int installerResult = mInstaller.stageInstallWithErrorCode(distro); int resultCode = mapInstallerResultToApiCode(installerResult); sendFinishedStatus(mCallback, resultCode); diff --git a/services/core/java/com/android/server/timezone/RulesManagerServiceHelperImpl.java b/services/core/java/com/android/server/timezone/RulesManagerServiceHelperImpl.java index 15a571d6750c7..482d8e2c80146 100644 --- a/services/core/java/com/android/server/timezone/RulesManagerServiceHelperImpl.java +++ b/services/core/java/com/android/server/timezone/RulesManagerServiceHelperImpl.java @@ -27,8 +27,7 @@ import libcore.io.Streams; /** * A single class that implements multiple helper interfaces for use by {@link RulesManagerService}. */ -final class RulesManagerServiceHelperImpl - implements PermissionHelper, Executor, FileDescriptorHelper { +final class RulesManagerServiceHelperImpl implements PermissionHelper, Executor { private final Context mContext; @@ -47,13 +46,4 @@ final class RulesManagerServiceHelperImpl // TODO Is there a better way? new Thread(runnable).start(); } - - @Override - public byte[] readFully(ParcelFileDescriptor parcelFileDescriptor) throws IOException { - try (ParcelFileDescriptor pfd = parcelFileDescriptor) { - // Read bytes - FileInputStream in = new FileInputStream(pfd.getFileDescriptor(), false /* isOwner */); - return Streams.readFully(in); - } - } } diff --git a/services/tests/servicestests/src/com/android/server/timezone/RulesManagerServiceTest.java b/services/tests/servicestests/src/com/android/server/timezone/RulesManagerServiceTest.java index c3a6f07548c82..3b76d5e05e0e5 100644 --- a/services/tests/servicestests/src/com/android/server/timezone/RulesManagerServiceTest.java +++ b/services/tests/servicestests/src/com/android/server/timezone/RulesManagerServiceTest.java @@ -30,6 +30,8 @@ import android.app.timezone.RulesManager; import android.app.timezone.RulesState; import android.os.ParcelFileDescriptor; +import java.io.File; +import java.io.FileOutputStream; import java.io.IOException; import java.util.concurrent.Executor; import javax.annotation.Nullable; @@ -61,7 +63,6 @@ public class RulesManagerServiceTest { private FakeExecutor mFakeExecutor; private PermissionHelper mMockPermissionHelper; - private FileDescriptorHelper mMockFileDescriptorHelper; private PackageTracker mMockPackageTracker; private TimeZoneDistroInstaller mMockTimeZoneDistroInstaller; @@ -69,7 +70,6 @@ public class RulesManagerServiceTest { public void setUp() { mFakeExecutor = new FakeExecutor(); - mMockFileDescriptorHelper = mock(FileDescriptorHelper.class); mMockPackageTracker = mock(PackageTracker.class); mMockPermissionHelper = mock(PermissionHelper.class); mMockTimeZoneDistroInstaller = mock(TimeZoneDistroInstaller.class); @@ -77,7 +77,6 @@ public class RulesManagerServiceTest { mRulesManagerService = new RulesManagerService( mMockPermissionHelper, mFakeExecutor, - mMockFileDescriptorHelper, mMockPackageTracker, mMockTimeZoneDistroInstaller); } @@ -273,9 +272,8 @@ public class RulesManagerServiceTest { revision); configureInstalledDistroVersion(installedDistroVersion); - byte[] expectedContent = createArbitraryBytes(1000); - ParcelFileDescriptor parcelFileDescriptor = createFakeParcelFileDescriptor(); - configureParcelFileDescriptorReadSuccess(parcelFileDescriptor, expectedContent); + ParcelFileDescriptor parcelFileDescriptor = + createParcelFileDescriptor(createArbitraryBytes(1000)); // Start an async operation so there is one in progress. The mFakeExecutor won't actually // execute it. @@ -298,24 +296,27 @@ public class RulesManagerServiceTest { public void requestInstall_operationInProgress() throws Exception { configureCallerHasPermission(); - byte[] expectedContent = createArbitraryBytes(1000); - ParcelFileDescriptor parcelFileDescriptor = createFakeParcelFileDescriptor(); - configureParcelFileDescriptorReadSuccess(parcelFileDescriptor, expectedContent); + ParcelFileDescriptor parcelFileDescriptor1 = + createParcelFileDescriptor(createArbitraryBytes(1000)); byte[] tokenBytes = createArbitraryTokenBytes(); ICallback callback = new StubbedCallback(); // First request should succeed. assertEquals(RulesManager.SUCCESS, - mRulesManagerService.requestInstall(parcelFileDescriptor, tokenBytes, callback)); + mRulesManagerService.requestInstall(parcelFileDescriptor1, tokenBytes, callback)); // Something async should be enqueued. Clear it but do not execute it so we can detect the // second request does nothing. mFakeExecutor.getAndResetLastCommand(); // Second request should fail. + ParcelFileDescriptor parcelFileDescriptor2 = + createParcelFileDescriptor(createArbitraryBytes(1000)); assertEquals(RulesManager.ERROR_OPERATION_IN_PROGRESS, - mRulesManagerService.requestInstall(parcelFileDescriptor, tokenBytes, callback)); + mRulesManagerService.requestInstall(parcelFileDescriptor2, tokenBytes, callback)); + + assertClosed(parcelFileDescriptor2); // Assert nothing async was enqueued. mFakeExecutor.assertNothingQueued(); @@ -327,9 +328,8 @@ public class RulesManagerServiceTest { public void requestInstall_badToken() throws Exception { configureCallerHasPermission(); - byte[] expectedContent = createArbitraryBytes(1000); - ParcelFileDescriptor parcelFileDescriptor = createFakeParcelFileDescriptor(); - configureParcelFileDescriptorReadSuccess(parcelFileDescriptor, expectedContent); + ParcelFileDescriptor parcelFileDescriptor = + createParcelFileDescriptor(createArbitraryBytes(1000)); byte[] badTokenBytes = new byte[2]; ICallback callback = new StubbedCallback(); @@ -340,6 +340,8 @@ public class RulesManagerServiceTest { } catch (IllegalArgumentException expected) { } + assertClosed(parcelFileDescriptor); + // Assert nothing async was enqueued. mFakeExecutor.assertNothingQueued(); verifyNoInstallerCallsMade(); @@ -369,7 +371,8 @@ public class RulesManagerServiceTest { public void requestInstall_nullCallback() throws Exception { configureCallerHasPermission(); - ParcelFileDescriptor parcelFileDescriptor = createFakeParcelFileDescriptor(); + ParcelFileDescriptor parcelFileDescriptor = + createParcelFileDescriptor(createArbitraryBytes(1000)); byte[] tokenBytes = createArbitraryTokenBytes(); ICallback callback = null; @@ -378,6 +381,8 @@ public class RulesManagerServiceTest { fail(); } catch (NullPointerException expected) {} + assertClosed(parcelFileDescriptor); + // Assert nothing async was enqueued. mFakeExecutor.assertNothingQueued(); verifyNoInstallerCallsMade(); @@ -388,9 +393,8 @@ public class RulesManagerServiceTest { public void requestInstall_asyncSuccess() throws Exception { configureCallerHasPermission(); - ParcelFileDescriptor parcelFileDescriptor = createFakeParcelFileDescriptor(); - byte[] expectedContent = createArbitraryBytes(1000); - configureParcelFileDescriptorReadSuccess(parcelFileDescriptor, expectedContent); + ParcelFileDescriptor parcelFileDescriptor = + createParcelFileDescriptor(createArbitraryBytes(1000)); CheckToken token = createArbitraryToken(); byte[] tokenBytes = token.toByteArray(); @@ -406,14 +410,14 @@ public class RulesManagerServiceTest { verifyNoInstallerCallsMade(); verifyNoPackageTrackerCallsMade(); - TimeZoneDistro expectedDistro = new TimeZoneDistro(expectedContent); - // Set up the installer. configureStageInstallExpectation(TimeZoneDistroInstaller.INSTALL_SUCCESS); // Simulate the async execution. mFakeExecutor.simulateAsyncExecutionOfLastCommand(); + assertClosed(parcelFileDescriptor); + // Verify the expected calls were made to other components. verifyStageInstallCalled(); verifyPackageTrackerCalled(token, true /* success */); @@ -426,9 +430,8 @@ public class RulesManagerServiceTest { public void requestInstall_nullTokenBytes() throws Exception { configureCallerHasPermission(); - ParcelFileDescriptor parcelFileDescriptor = createFakeParcelFileDescriptor(); - byte[] expectedContent = createArbitraryBytes(1000); - configureParcelFileDescriptorReadSuccess(parcelFileDescriptor, expectedContent); + ParcelFileDescriptor parcelFileDescriptor = + createParcelFileDescriptor(createArbitraryBytes(1000)); TestCallback callback = new TestCallback(); @@ -447,6 +450,8 @@ public class RulesManagerServiceTest { // Simulate the async execution. mFakeExecutor.simulateAsyncExecutionOfLastCommand(); + assertClosed(parcelFileDescriptor); + // Verify the expected calls were made to other components. verifyStageInstallCalled(); verifyPackageTrackerCalled(null /* expectedToken */, true /* success */); @@ -459,9 +464,8 @@ public class RulesManagerServiceTest { public void requestInstall_asyncInstallFail() throws Exception { configureCallerHasPermission(); - byte[] expectedContent = createArbitraryBytes(1000); - ParcelFileDescriptor parcelFileDescriptor = createFakeParcelFileDescriptor(); - configureParcelFileDescriptorReadSuccess(parcelFileDescriptor, expectedContent); + ParcelFileDescriptor parcelFileDescriptor = + createParcelFileDescriptor(createArbitraryBytes(1000)); CheckToken token = createArbitraryToken(); byte[] tokenBytes = token.toByteArray(); @@ -482,6 +486,8 @@ public class RulesManagerServiceTest { // Simulate the async execution. mFakeExecutor.simulateAsyncExecutionOfLastCommand(); + assertClosed(parcelFileDescriptor); + // Verify the expected calls were made to other components. verifyStageInstallCalled(); @@ -493,38 +499,6 @@ public class RulesManagerServiceTest { callback.assertResultReceived(Callback.ERROR_INSTALL_VALIDATION_ERROR); } - @Test - public void requestInstall_asyncParcelFileDescriptorReadFail() throws Exception { - configureCallerHasPermission(); - - ParcelFileDescriptor parcelFileDescriptor = createFakeParcelFileDescriptor(); - configureParcelFileDescriptorReadFailure(parcelFileDescriptor); - - CheckToken token = createArbitraryToken(); - byte[] tokenBytes = token.toByteArray(); - - TestCallback callback = new TestCallback(); - - // Request the install. - assertEquals(RulesManager.SUCCESS, - mRulesManagerService.requestInstall(parcelFileDescriptor, tokenBytes, callback)); - - // Simulate the async execution. - mFakeExecutor.simulateAsyncExecutionOfLastCommand(); - - // Verify nothing else happened. - verifyNoInstallerCallsMade(); - - // A failure to read the ParcelFileDescriptor is treated as a failure. It might be the - // result of a file system error. This is a fairly arbitrary choice. - verifyPackageTrackerCalled(token, false /* success */); - - verifyNoPackageTrackerCallsMade(); - - // Check the callback was received. - callback.assertResultReceived(Callback.ERROR_UNKNOWN_FAILURE); - } - @Test public void requestUninstall_operationInProgress() throws Exception { configureCallerHasPermission(); @@ -773,17 +747,6 @@ public class RulesManagerServiceTest { .enforceCallerHasPermission(REQUIRED_UPDATER_PERMISSION); } - private void configureParcelFileDescriptorReadSuccess(ParcelFileDescriptor parcelFileDescriptor, - byte[] content) throws Exception { - when(mMockFileDescriptorHelper.readFully(parcelFileDescriptor)).thenReturn(content); - } - - private void configureParcelFileDescriptorReadFailure(ParcelFileDescriptor parcelFileDescriptor) - throws Exception { - when(mMockFileDescriptorHelper.readFully(parcelFileDescriptor)) - .thenThrow(new IOException("Simulated failure")); - } - private void configureStageInstallExpectation(int resultCode) throws Exception { when(mMockTimeZoneDistroInstaller.stageInstallWithErrorCode(any(TimeZoneDistro.class))) @@ -827,10 +790,6 @@ public class RulesManagerServiceTest { return new CheckToken(1, new PackageVersions(1, 1)); } - private ParcelFileDescriptor createFakeParcelFileDescriptor() { - return new ParcelFileDescriptor((ParcelFileDescriptor) null); - } - private void configureDeviceSystemRulesVersion(String systemRulesVersion) throws Exception { when(mMockTimeZoneDistroInstaller.getSystemRulesVersion()).thenReturn(systemRulesVersion); } @@ -870,6 +829,10 @@ public class RulesManagerServiceTest { .thenThrow(new IOException("Simulated failure")); } + private static void assertClosed(ParcelFileDescriptor parcelFileDescriptor) { + assertFalse(parcelFileDescriptor.getFileDescriptor().valid()); + } + private static class FakeExecutor implements Executor { private Runnable mLastCommand; @@ -926,4 +889,17 @@ public class RulesManagerServiceTest { fail("Unexpected call"); } } + + private static ParcelFileDescriptor createParcelFileDescriptor(byte[] bytes) + throws IOException { + File file = File.createTempFile("pfd", null); + try (FileOutputStream fos = new FileOutputStream(file)) { + fos.write(bytes); + } + ParcelFileDescriptor pfd = + ParcelFileDescriptor.open(file, ParcelFileDescriptor.MODE_READ_ONLY); + // This should now be safe to delete. The ParcelFileDescriptor has an open fd. + file.delete(); + return pfd; + } }