Avoid accessing failed HIDL Return contents
It's invalid to attempt to read the contents of a HIDL Return object
that has isOk() return false. This leads to a crash, so instead map
isOk() == false to DEAD_OBJECT in places where the return status is
used, and avoid otherwise avoid referencing the contents of the Return.
To facilitate testing, also add some additional informational logs for
cases when the sensor handles have changed after HAL restart.
Bug: 133497927
Test: use customized 2.0 HAL that crashes in activate call, confirm the
HAL crash doesn't trigger system server crash due to invalid use
of Return object
Change-Id: Ic094999ef20d90b538f757e34167155e83711b3d
diff --git a/services/sensorservice/SensorDevice.cpp b/services/sensorservice/SensorDevice.cpp
index fcd4f29..4072cff 100644
--- a/services/sensorservice/SensorDevice.cpp
+++ b/services/sensorservice/SensorDevice.cpp
@@ -45,7 +45,9 @@
ANDROID_SINGLETON_STATIC_INSTANCE(SensorDevice)
-static status_t StatusFromResult(Result result) {
+namespace {
+
+status_t statusFromResult(Result result) {
switch (result) {
case Result::OK:
return OK;
@@ -71,6 +73,8 @@
INTERNAL_WAKE = 1 << 16,
};
+} // anonymous namespace
+
void SensorsHalDeathReceivier::serviceDied(
uint64_t /* cookie */,
const wp<::android::hidl::base::V1_0::IBase>& /* service */) {
@@ -105,7 +109,7 @@
initializeSensorList();
mIsDirectReportSupported =
- (checkReturn(mSensors->unregisterDirectChannel(-1)) != Result::INVALID_OPERATION);
+ (checkReturnAndGetStatus(mSensors->unregisterDirectChannel(-1)) != INVALID_OPERATION);
}
void SensorDevice::initializeSensorList() {
@@ -122,7 +126,7 @@
convertToSensor(list[i], &sensor);
// Sanity check and clamp power if it is 0 (or close)
if (sensor.power < minPowerMa) {
- ALOGE("Reported power %f not deemed sane, clamping to %f",
+ ALOGI("Reported power %f not deemed sane, clamping to %f",
sensor.power, minPowerMa);
sensor.power = minPowerMa;
}
@@ -217,10 +221,10 @@
mWakeLockQueue != nullptr && mEventQueueFlag != nullptr &&
mWakeLockQueueFlag != nullptr);
- status_t status = StatusFromResult(checkReturn(mSensors->initialize(
+ status_t status = checkReturnAndGetStatus(mSensors->initialize(
*mEventQueue->getDesc(),
*mWakeLockQueue->getDesc(),
- new SensorsCallback())));
+ new SensorsCallback()));
if (status != NO_ERROR) {
connectionStatus = HalConnectionStatus::FAILED_TO_CONNECT;
@@ -270,6 +274,8 @@
bool didChange = false;
if (oldSensorList.size() != newSensorList.size()) {
+ ALOGI("Sensor list size changed from %zu to %zu", oldSensorList.size(),
+ newSensorList.size());
didChange = true;
}
@@ -281,6 +287,7 @@
if (prevSensor.handle == newSensor.handle) {
found = true;
if (!sensorIsEquivalent(prevSensor, newSensor)) {
+ ALOGI("Sensor %s not equivalent to previous version", newSensor.name);
didChange = true;
}
}
@@ -289,6 +296,7 @@
if (!found) {
// Could not find the new sensor in the old list of sensors, the lists must
// have changed.
+ ALOGI("Sensor %s (handle %d) did not exist before", newSensor.name, newSensor.handle);
didChange = true;
}
}
@@ -423,7 +431,7 @@
convertToSensorEvents(events, dynamicSensorsAdded, buffer);
err = (ssize_t)events.size();
} else {
- err = StatusFromResult(result);
+ err = statusFromResult(result);
}
});
@@ -621,7 +629,7 @@
if (actuateHardware) {
ALOGD_IF(DEBUG_CONNECTIONS, "\t>>> actuating h/w activate handle=%d enabled=%d", handle,
enabled);
- err = StatusFromResult(checkReturn(mSensors->activate(handle, enabled)));
+ err = checkReturnAndGetStatus(mSensors->activate(handle, enabled));
ALOGE_IF(err, "Error %s sensor %d (%s)", enabled ? "activating" : "disabling", handle,
strerror(-err));
@@ -694,9 +702,8 @@
if (prevBestBatchParams != info.bestBatchParams) {
ALOGD_IF(DEBUG_CONNECTIONS, "\t>>> actuating h/w BATCH 0x%08x %" PRId64 " %" PRId64, handle,
info.bestBatchParams.mTSample, info.bestBatchParams.mTBatch);
- err = StatusFromResult(
- checkReturn(mSensors->batch(
- handle, info.bestBatchParams.mTSample, info.bestBatchParams.mTBatch)));
+ err = checkReturnAndGetStatus(mSensors->batch(
+ handle, info.bestBatchParams.mTSample, info.bestBatchParams.mTBatch));
if (err != NO_ERROR) {
ALOGE("sensor batch failed %p 0x%08x %" PRId64 " %" PRId64 " err=%s",
mSensors.get(), handle, info.bestBatchParams.mTSample,
@@ -720,7 +727,7 @@
if (mSensors == nullptr) return NO_INIT;
if (isClientDisabled(ident)) return INVALID_OPERATION;
ALOGD_IF(DEBUG_CONNECTIONS, "\t>>> actuating h/w flush %d", handle);
- return StatusFromResult(checkReturn(mSensors->flush(handle)));
+ return checkReturnAndGetStatus(mSensors->flush(handle));
}
bool SensorDevice::isClientDisabled(void* ident) {
@@ -753,16 +760,14 @@
const int sensor_handle = mActivationCount.keyAt(i);
ALOGD_IF(DEBUG_CONNECTIONS, "\t>> reenable actuating h/w sensor enable handle=%d ",
sensor_handle);
- status_t err = StatusFromResult(
- checkReturn(mSensors->batch(
- sensor_handle,
- info.bestBatchParams.mTSample,
- info.bestBatchParams.mTBatch)));
+ status_t err = checkReturnAndGetStatus(mSensors->batch(
+ sensor_handle,
+ info.bestBatchParams.mTSample,
+ info.bestBatchParams.mTBatch));
ALOGE_IF(err, "Error calling batch on sensor %d (%s)", sensor_handle, strerror(-err));
if (err == NO_ERROR) {
- err = StatusFromResult(
- checkReturn(mSensors->activate(sensor_handle, 1 /* enabled */)));
+ err = checkReturnAndGetStatus(mSensors->activate(sensor_handle, 1 /* enabled */));
ALOGE_IF(err, "Error activating sensor %d (%s)", sensor_handle, strerror(-err));
}
@@ -810,14 +815,13 @@
Event ev;
convertFromSensorEvent(*injected_sensor_event, &ev);
- return StatusFromResult(checkReturn(mSensors->injectSensorData(ev)));
+ return checkReturnAndGetStatus(mSensors->injectSensorData(ev));
}
status_t SensorDevice::setMode(uint32_t mode) {
if (mSensors == nullptr) return NO_INIT;
- return StatusFromResult(
- checkReturn(mSensors->setOperationMode(
- static_cast<hardware::sensors::V1_0::OperationMode>(mode))));
+ return checkReturnAndGetStatus(mSensors->setOperationMode(
+ static_cast<hardware::sensors::V1_0::OperationMode>(mode)));
}
int32_t SensorDevice::registerDirectChannel(const sensors_direct_mem_t* memory) {
@@ -855,7 +859,7 @@
if (result == Result::OK) {
ret = channelHandle;
} else {
- ret = StatusFromResult(result);
+ ret = statusFromResult(result);
}
}));
return ret;
@@ -894,12 +898,12 @@
checkReturn(mSensors->configDirectReport(sensorHandle, channelHandle, rate,
[&ret, rate] (auto result, auto token) {
if (rate == RateLevel::STOP) {
- ret = StatusFromResult(result);
+ ret = statusFromResult(result);
} else {
if (result == Result::OK) {
ret = token;
} else {
- ret = StatusFromResult(result);
+ ret = statusFromResult(result);
}
}
}));
@@ -1016,5 +1020,10 @@
}
}
+status_t SensorDevice::checkReturnAndGetStatus(const Return<Result>& ret) {
+ checkReturn(ret);
+ return (!ret.isOk()) ? DEAD_OBJECT : statusFromResult(ret);
+}
+
// ---------------------------------------------------------------------------
}; // namespace android