Merge "Switch to streaming data for time zone update"

am: f093d4e13e

Change-Id: I3fd22fe47e1d7b86d4f88d527940c6812ecfa31b

services/core/java/com/android/server/timezone/FileDescriptorHelper.java

@@ -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;
-}

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,
+            Executor executor, PackageTracker packageTracker,
-            FileDescriptorHelper fileDescriptorHelper, 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(
+    public int requestInstall(ParcelFileDescriptor distroParcelFileDescriptor,
-            ParcelFileDescriptor timeZoneDistro, byte[] checkTokenBytes, ICallback callback) {
+            byte[] checkTokenBytes, ICallback callback) {
-        mPermissionHelper.enforceCallerHasPermission(REQUIRED_UPDATER_PERMISSION);
 
-        CheckToken checkToken = null;
+        boolean closeParcelFileDescriptorOnExit = true;
-        if (checkTokenBytes != null) {
+        try {
-            checkToken = createCheckTokenOrThrow(checkTokenBytes);
+            mPermissionHelper.enforceCallerHasPermission(REQUIRED_UPDATER_PERMISSION);
-        }
-        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);
 
-            // Execute the install asynchronously.
+            CheckToken checkToken = null;
-            mExecutor.execute(new InstallRunnable(timeZoneDistro, checkToken, callback));
+            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(
+        InstallRunnable(ParcelFileDescriptor distroParcelFileDescriptor, CheckToken checkToken,
-                ParcelFileDescriptor timeZoneDistro, CheckToken checkToken, ICallback callback) {
+                ICallback callback) {
-            mTimeZoneDistro = timeZoneDistro;
+            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 (ParcelFileDescriptor pfd = mDistroParcelFileDescriptor) {
-            try {
+                // The ParcelFileDescriptor owns the underlying FileDescriptor and we'll close
-                byte[] distroBytes =
+                // it at the end of the try-with-resources.
-                        RulesManagerService.this.mFileDescriptorHelper.readFully(mTimeZoneDistro);
+                final boolean isFdOwner = false;
-                TimeZoneDistro distro = new TimeZoneDistro(distroBytes);
+                InputStream is = new FileInputStream(pfd.getFileDescriptor(), isFdOwner);
+
+                TimeZoneDistro distro = new TimeZoneDistro(is);
                 int installerResult = mInstaller.stageInstallWithErrorCode(distro);
                 int resultCode = mapInstallerResultToApiCode(installerResult);
                 sendFinishedStatus(mCallback, resultCode);

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
+final class RulesManagerServiceHelperImpl implements PermissionHelper, Executor {
-        implements PermissionHelper, Executor, FileDescriptorHelper {
 
     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);
-        }
-    }
 }

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 =
-        ParcelFileDescriptor parcelFileDescriptor = createFakeParcelFileDescriptor();
+                createParcelFileDescriptor(createArbitraryBytes(1000));
-        configureParcelFileDescriptorReadSuccess(parcelFileDescriptor, expectedContent);
 
         // 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 parcelFileDescriptor1 =
-        ParcelFileDescriptor parcelFileDescriptor = createFakeParcelFileDescriptor();
+                createParcelFileDescriptor(createArbitraryBytes(1000));
-        configureParcelFileDescriptorReadSuccess(parcelFileDescriptor, expectedContent);
 
         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 =
-        ParcelFileDescriptor parcelFileDescriptor = createFakeParcelFileDescriptor();
+                createParcelFileDescriptor(createArbitraryBytes(1000));
-        configureParcelFileDescriptorReadSuccess(parcelFileDescriptor, expectedContent);
 
         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();
+        ParcelFileDescriptor parcelFileDescriptor =
-        byte[] expectedContent = createArbitraryBytes(1000);
+                createParcelFileDescriptor(createArbitraryBytes(1000));
-        configureParcelFileDescriptorReadSuccess(parcelFileDescriptor, expectedContent);
 
         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();
+        ParcelFileDescriptor parcelFileDescriptor =
-        byte[] expectedContent = createArbitraryBytes(1000);
+                createParcelFileDescriptor(createArbitraryBytes(1000));
-        configureParcelFileDescriptorReadSuccess(parcelFileDescriptor, expectedContent);
 
         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 =
-        ParcelFileDescriptor parcelFileDescriptor = createFakeParcelFileDescriptor();
+                createParcelFileDescriptor(createArbitraryBytes(1000));
-        configureParcelFileDescriptorReadSuccess(parcelFileDescriptor, expectedContent);
 
         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;
+    }
 }