Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 8e6f31c1 authored by Brian Duddie's avatar Brian Duddie
Browse files

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
parent 1db00e0a

services/sensorservice/SensorDevice.cpp

+34 −25
Original line number Diff line number Diff line
@@ -45,7 +45,9 @@ namespace android { 


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 @@ enum EventQueueFlagBitsInternal : uint32_t { 
    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 @@ SensorDevice::SensorDevice() 
    initializeSensorList();



    mIsDirectReportSupported =

           (checkReturn(mSensors->unregisterDirectChannel(-1)) != Result::INVALID_OPERATION);

            (checkReturnAndGetStatus(mSensors->unregisterDirectChannel(-1)) != INVALID_OPERATION);

}



void SensorDevice::initializeSensorList() {
@@ -122,7 +126,7 @@ void SensorDevice::initializeSensorList() { 
                    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 @@ SensorDevice::HalConnectionStatus SensorDevice::connectHidlServiceV2_0() { 
                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 SensorDevice::sensorHandlesChanged(const Vector<sensor_t>& oldSensorList, 
    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 @@ bool SensorDevice::sensorHandlesChanged(const Vector<sensor_t>& oldSensorList, 
            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 @@ bool SensorDevice::sensorHandlesChanged(const Vector<sensor_t>& oldSensorList, 
        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 @@ ssize_t SensorDevice::pollHal(sensors_event_t* buffer, size_t count) { 
                        convertToSensorEvents(events, dynamicSensorsAdded, buffer);

                        err = (ssize_t)events.size();

                    } else {

                        err = StatusFromResult(result);

                        err = statusFromResult(result);

                    }

                });
@@ -621,7 +629,7 @@ status_t SensorDevice::activateLocked(void* ident, int handle, int enabled) { 
    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 @@ status_t SensorDevice::batchLocked(void* ident, int handle, int flags, int64_t s 
    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 @@ status_t SensorDevice::flush(void* ident, int handle) { 
    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 @@ void SensorDevice::enableAllSensors() { 
        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(

        status_t err = checkReturnAndGetStatus(mSensors->batch(

                sensor_handle,

                info.bestBatchParams.mTSample,

                    info.bestBatchParams.mTBatch)));

                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 @@ status_t SensorDevice::injectSensorData( 
    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 @@ int32_t SensorDevice::registerDirectChannel(const sensors_direct_mem_t* memory) 
                if (result == Result::OK) {

                    ret = channelHandle;

                } else {

                    ret = StatusFromResult(result);

                    ret = statusFromResult(result);

                }

            }));

    return ret;
@@ -894,12 +898,12 @@ int32_t SensorDevice::configureDirectChannel(int32_t sensorHandle, 
    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 @@ void SensorDevice::handleHidlDeath(const std::string & detail) { 
    }

}



status_t SensorDevice::checkReturnAndGetStatus(const Return<Result>& ret) {

    checkReturn(ret);

    return (!ret.isOk()) ? DEAD_OBJECT : statusFromResult(ret);

}



// ---------------------------------------------------------------------------

}; // namespace android

services/sensorservice/SensorDevice.h

+2 −2
Original line number Diff line number Diff line
@@ -213,12 +213,12 @@ private: 


    static void handleHidlDeath(const std::string &detail);

    template<typename T>

    static Return<T> checkReturn(Return<T> &&ret) {

    static void checkReturn(const Return<T>& ret) {

        if (!ret.isOk()) {

            handleHidlDeath(ret.description());

        }

        return std::move(ret);

    }

    static status_t checkReturnAndGetStatus(const Return<Result>& ret);

    //TODO(b/67425500): remove waiter after bug is resolved.

    sp<SensorDeviceUtils::HidlServiceRegistrationWaiter> mRestartWaiter;