diff options
4 files changed, 106 insertions, 149 deletions
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 c3b1101000b5..000000000000 --- 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 58bdeb9d11ec..804a8b79310e 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; + boolean closeParcelFileDescriptorOnExit = true; + try { + mPermissionHelper.enforceCallerHasPermission(REQUIRED_UPDATER_PERMISSION); + + CheckToken checkToken = null; + if (checkTokenBytes != null) { + checkToken = createCheckTokenOrThrow(checkTokenBytes); } - mOperationInProgress.set(true); - // Execute the install asynchronously. - mExecutor.execute(new InstallRunnable(timeZoneDistro, checkToken, callback)); + 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); - return RulesManager.SUCCESS; + // 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 15a571d6750c..482d8e2c8014 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 c3a6f07548c8..3b76d5e05e0e 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(); @@ -494,38 +500,6 @@ public class RulesManagerServiceTest { } @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; + } } |