From b6e02f7b0978a2c0b4ae4aaafc7d9907267410be Mon Sep 17 00:00:00 2001 From: Alex Buynytskyy Date: Tue, 17 Mar 2020 18:12:23 -0700 Subject: [PATCH] Removing race condition accessing shared binder object. Test: incrementally installing two apks at the same time Bug: b/150411019 Change-Id: I81231edf7a32470542ec529aa305b4f9fb2b80e3 --- core/java/android/content/pm/IDataLoader.aidl | 8 ++--- .../service/dataloader/DataLoaderService.java | 34 +++++++++---------- .../server/pm/DataLoaderManagerService.java | 2 +- .../server/pm/PackageInstallerSession.java | 7 ++-- services/incremental/IncrementalService.cpp | 2 +- .../test/IncrementalServiceTest.cpp | 9 ++--- 6 files changed, 31 insertions(+), 31 deletions(-) diff --git a/core/java/android/content/pm/IDataLoader.aidl b/core/java/android/content/pm/IDataLoader.aidl index 6a2658d72e41b..5ec63415d3d3d 100644 --- a/core/java/android/content/pm/IDataLoader.aidl +++ b/core/java/android/content/pm/IDataLoader.aidl @@ -29,9 +29,9 @@ oneway interface IDataLoader { void create(int id, in DataLoaderParamsParcel params, in FileSystemControlParcel control, IDataLoaderStatusListener listener); - void start(); - void stop(); - void destroy(); + void start(int id); + void stop(int id); + void destroy(int id); - void prepareImage(in InstallationFileParcel[] addedFiles, in @utf8InCpp String[] removedFiles); + void prepareImage(int id, in InstallationFileParcel[] addedFiles, in @utf8InCpp String[] removedFiles); } diff --git a/core/java/android/service/dataloader/DataLoaderService.java b/core/java/android/service/dataloader/DataLoaderService.java index d4db79eda7625..0170726b31d6b 100644 --- a/core/java/android/service/dataloader/DataLoaderService.java +++ b/core/java/android/service/dataloader/DataLoaderService.java @@ -102,21 +102,18 @@ public abstract class DataLoaderService extends Service { } private class DataLoaderBinderService extends IDataLoader.Stub { - private int mId; - @Override public void create(int id, @NonNull DataLoaderParamsParcel params, @NonNull FileSystemControlParcel control, @NonNull IDataLoaderStatusListener listener) throws RuntimeException { - mId = id; try { if (!nativeCreateDataLoader(id, control, params, listener)) { - Slog.e(TAG, "Failed to create native loader for " + mId); + Slog.e(TAG, "Failed to create native loader for " + id); } } catch (Exception ex) { - Slog.e(TAG, "Failed to create native loader for " + mId, ex); - destroy(); + Slog.e(TAG, "Failed to create native loader for " + id, ex); + destroy(id); throw new RuntimeException(ex); } finally { // Closing FDs. @@ -150,30 +147,31 @@ public abstract class DataLoaderService extends Service { } @Override - public void start() { - if (!nativeStartDataLoader(mId)) { - Slog.e(TAG, "Failed to start loader: " + mId); + public void start(int id) { + if (!nativeStartDataLoader(id)) { + Slog.e(TAG, "Failed to start loader: " + id); } } @Override - public void stop() { - if (!nativeStopDataLoader(mId)) { - Slog.w(TAG, "Failed to stop loader: " + mId); + public void stop(int id) { + if (!nativeStopDataLoader(id)) { + Slog.w(TAG, "Failed to stop loader: " + id); } } @Override - public void destroy() { - if (!nativeDestroyDataLoader(mId)) { - Slog.w(TAG, "Failed to destroy loader: " + mId); + public void destroy(int id) { + if (!nativeDestroyDataLoader(id)) { + Slog.w(TAG, "Failed to destroy loader: " + id); } } @Override - public void prepareImage(InstallationFileParcel[] addedFiles, String[] removedFiles) { - if (!nativePrepareImage(mId, addedFiles, removedFiles)) { - Slog.w(TAG, "Failed to prepare image for data loader: " + mId); + public void prepareImage(int id, InstallationFileParcel[] addedFiles, + String[] removedFiles) { + if (!nativePrepareImage(id, addedFiles, removedFiles)) { + Slog.w(TAG, "Failed to prepare image for data loader: " + id); } } } diff --git a/services/core/java/com/android/server/pm/DataLoaderManagerService.java b/services/core/java/com/android/server/pm/DataLoaderManagerService.java index 8eb773a2d0f81..09baf6e0a817c 100644 --- a/services/core/java/com/android/server/pm/DataLoaderManagerService.java +++ b/services/core/java/com/android/server/pm/DataLoaderManagerService.java @@ -213,7 +213,7 @@ public class DataLoaderManagerService extends SystemService { void destroy() { try { - mDataLoader.destroy(); + mDataLoader.destroy(mId); } catch (RemoteException ignored) { } mContext.unbindService(this); diff --git a/services/core/java/com/android/server/pm/PackageInstallerSession.java b/services/core/java/com/android/server/pm/PackageInstallerSession.java index 28d7c13d5b132..308bc41aeb6c3 100644 --- a/services/core/java/com/android/server/pm/PackageInstallerSession.java +++ b/services/core/java/com/android/server/pm/PackageInstallerSession.java @@ -2582,12 +2582,13 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { if (manualStartAndDestroy) { // IncrementalFileStorages will call start after all files are // created in IncFS. - dataLoader.start(); + dataLoader.start(dataLoaderId); } break; } case IDataLoaderStatusListener.DATA_LOADER_STARTED: { dataLoader.prepareImage( + dataLoaderId, addedFiles.toArray( new InstallationFileParcel[addedFiles.size()]), removedFiles.toArray(new String[removedFiles.size()])); @@ -2602,7 +2603,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { dispatchStreamValidateAndCommit(); } if (manualStartAndDestroy) { - dataLoader.destroy(); + dataLoader.destroy(dataLoaderId); } break; } @@ -2612,7 +2613,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { new PackageManagerException(INSTALL_FAILED_MEDIA_UNAVAILABLE, "Failed to prepare image.")); if (manualStartAndDestroy) { - dataLoader.destroy(); + dataLoader.destroy(dataLoaderId); } break; } diff --git a/services/incremental/IncrementalService.cpp b/services/incremental/IncrementalService.cpp index 727593664895d..e4524151a89e9 100644 --- a/services/incremental/IncrementalService.cpp +++ b/services/incremental/IncrementalService.cpp @@ -941,7 +941,7 @@ bool IncrementalService::startDataLoader(MountId mountId) const { if (!dataloader) { return false; } - status = dataloader->start(); + status = dataloader->start(mountId); if (!status.isOk()) { return false; } diff --git a/services/incremental/test/IncrementalServiceTest.cpp b/services/incremental/test/IncrementalServiceTest.cpp index 6002226f96307..f5b88d93c8de6 100644 --- a/services/incremental/test/IncrementalServiceTest.cpp +++ b/services/incremental/test/IncrementalServiceTest.cpp @@ -100,10 +100,11 @@ public: const sp&) override { return binder::Status::ok(); } - binder::Status start() override { return binder::Status::ok(); } - binder::Status stop() override { return binder::Status::ok(); } - binder::Status destroy() override { return binder::Status::ok(); } - binder::Status prepareImage(const std::vector&, + binder::Status start(int32_t) override { return binder::Status::ok(); } + binder::Status stop(int32_t) override { return binder::Status::ok(); } + binder::Status destroy(int32_t) override { return binder::Status::ok(); } + binder::Status prepareImage(int32_t, + const std::vector&, const std::vector&) override { return binder::Status::ok(); }