Potential deadlock mitigation.
+better error logging
DataLoaders might report user statuses from lifecycle callbacks.
Immediate processing of such might introduce infinite loops/deadlocks e.g.
DataLoader_OnStop -> reportStatus(UNRECOVERABLE) -> fsmStep -> DataLoader_OnStop
Bug: 160634487
Test: atest PackageManagerShellCommandTest PackageManagerShellCommandIncrementalTest IncrementalServiceTest PackageManagerServiceTest ChecksumsTest
Change-Id: Ic68657d7a8cd6c6855b6f5295276a42b3cb09117
diff --git a/services/core/jni/com_android_server_pm_PackageManagerShellCommandDataLoader.cpp b/services/core/jni/com_android_server_pm_PackageManagerShellCommandDataLoader.cpp
index 5a5b0a8..7b78b8d 100644
--- a/services/core/jni/com_android_server_pm_PackageManagerShellCommandDataLoader.cpp
+++ b/services/core/jni/com_android_server_pm_PackageManagerShellCommandDataLoader.cpp
@@ -44,7 +44,6 @@
namespace {
using android::base::borrowed_fd;
-using android::base::ReadFully;
using android::base::unique_fd;
using namespace std::literals;
@@ -173,7 +172,7 @@
static inline std::vector<char> readBytes(borrowed_fd fd) {
int32_t size = readLEInt32(fd);
std::vector<char> result(size);
- ReadFully(fd, result.data(), size);
+ android::base::ReadFully(fd, result.data(), size);
return result;
}
@@ -569,7 +568,7 @@
// Awaiting adb handshake.
char okay_buf[OKAY.size()];
if (!android::base::ReadFully(inout, okay_buf, OKAY.size())) {
- ALOGE("Failed to receive OKAY. Abort.");
+ ALOGE("Failed to receive OKAY. Abort. Error %d", errno);
return false;
}
if (std::string_view(okay_buf, OKAY.size()) != OKAY) {
@@ -693,12 +692,12 @@
continue;
}
if (res < 0) {
- ALOGE("Failed to poll. Abort.");
+ ALOGE("Failed to poll. Abort. Error %d", res);
mStatusListener->reportStatus(DATA_LOADER_UNRECOVERABLE);
break;
}
if (res == mEventFd) {
- ALOGE("Received stop signal. Sending EXIT to server.");
+ ALOGE("DataLoader requested to stop. Sending EXIT to server.");
sendRequest(inout, EXIT);
break;
}
@@ -712,7 +711,7 @@
auto header = readHeader(remainingData);
if (header.fileIdx == -1 && header.blockType == 0 && header.compressionType == 0 &&
header.blockIdx == 0 && header.blockSize == 0) {
- ALOGI("Stop signal received. Sending exit command (remaining bytes: %d).",
+ ALOGI("Stop command received. Sending exit command (remaining bytes: %d).",
int(remainingData.size()));
sendRequest(inout, EXIT);
@@ -721,16 +720,15 @@
}
if (header.fileIdx < 0 || header.blockSize <= 0 || header.blockType < 0 ||
header.compressionType < 0 || header.blockIdx < 0) {
- ALOGE("invalid header received. Abort.");
+ ALOGE("Invalid header received. Abort.");
mStopReceiving = true;
break;
}
+
const FileIdx fileIdx = header.fileIdx;
const android::dataloader::FileId fileId = convertFileIndexToFileId(mode, fileIdx);
if (!android::incfs::isValidFileId(fileId)) {
- ALOGE("Unknown data destination for file ID %d. "
- "Ignore.",
- header.fileIdx);
+ ALOGE("Unknown data destination for file ID %d. Ignore.", header.fileIdx);
continue;
}
@@ -738,7 +736,7 @@
if (writeFd < 0) {
writeFd.reset(this->mIfs->openForSpecialOps(fileId).release());
if (writeFd < 0) {
- ALOGE("Failed to open file %d for writing (%d). Aborting.", header.fileIdx,
+ ALOGE("Failed to open file %d for writing (%d). Abort.", header.fileIdx,
-writeFd);
break;
}
diff --git a/services/incremental/IncrementalService.cpp b/services/incremental/IncrementalService.cpp
index 886c1e5..2fa927b 100644
--- a/services/incremental/IncrementalService.cpp
+++ b/services/incremental/IncrementalService.cpp
@@ -66,6 +66,8 @@
static constexpr auto blockSize = 4096;
static constexpr auto systemPackage = "android"sv;
+ static constexpr auto userStatusDelay = 100ms;
+
static constexpr auto progressUpdateInterval = 1000ms;
static constexpr auto perUidTimeoutOffset = progressUpdateInterval * 2;
static constexpr auto minPerUidTimeout = progressUpdateInterval * 3;
@@ -2306,13 +2308,24 @@
LOG(ERROR) << "Mount ID mismatch: expected " << id() << ", but got: " << mountId;
return binder::Status::fromServiceSpecificError(-EPERM, "Mount ID mismatch.");
}
+ if (newStatus == IDataLoaderStatusListener::DATA_LOADER_UNRECOVERABLE) {
+ // User-provided status, let's postpone the handling to avoid possible deadlocks.
+ mService.addTimedJob(*mService.mTimedQueue, id(), Constants::userStatusDelay,
+ [this, newStatus]() { setCurrentStatus(newStatus); });
+ return binder::Status::ok();
+ }
+ setCurrentStatus(newStatus);
+ return binder::Status::ok();
+}
+
+void IncrementalService::DataLoaderStub::setCurrentStatus(int newStatus) {
int targetStatus, oldStatus;
DataLoaderStatusListener listener;
{
std::unique_lock lock(mMutex);
if (mCurrentStatus == newStatus) {
- return binder::Status::ok();
+ return;
}
oldStatus = mCurrentStatus;
@@ -2332,14 +2345,12 @@
<< newStatus << " (target " << targetStatus << ")";
if (listener) {
- listener->onStatusChanged(mountId, newStatus);
+ listener->onStatusChanged(id(), newStatus);
}
fsmStep();
mStatusCondition.notify_all();
-
- return binder::Status::ok();
}
binder::Status IncrementalService::DataLoaderStub::reportStreamHealth(MountId mountId,
diff --git a/services/incremental/IncrementalService.h b/services/incremental/IncrementalService.h
index 459ed32..d8f2c91 100644
--- a/services/incremental/IncrementalService.h
+++ b/services/incremental/IncrementalService.h
@@ -234,6 +234,8 @@
binder::Status onStatusChanged(MountId mount, int newStatus) final;
binder::Status reportStreamHealth(MountId mount, int newStatus) final;
+ void setCurrentStatus(int newStatus);
+
sp<content::pm::IDataLoader> getDataLoader();
bool bind();