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

Commit 4f9576bf authored by Ruben Brunk's avatar Ruben Brunk
Browse files

camera: Fix client eviction/disconnect race. 

- Add blocking wait in camera service connect call to
  prevent race when client has called disconnect while
  eviction of that client is taking place, resulting
  in early call of device initialization before all
  HAL resources are available.

Bug: 20038135
Change-Id: I7afc5bfa23612ba7f83293fa542ff983a5991230
parent a8ca9157

services/camera/libcameraservice/CameraService.cpp

+23 −4
Original line number Diff line number Diff line
@@ -866,7 +866,7 @@ status_t CameraService::handleEvictionsLocked(const String8& cameraId, int clien 
        std::shared_ptr<resource_policy::ClientDescriptor<String8, sp<BasicClient>>>* partial) {



    status_t ret = NO_ERROR;

    std::vector<sp<BasicClient>> evictedClients;

    std::vector<DescriptorPtr> evictedClients;

    DescriptorPtr clientDescriptor;

    {

        if (effectiveApiLevel == API_1) {
@@ -974,7 +974,7 @@ status_t CameraService::handleEvictionsLocked(const String8& cameraId, int clien 


            ALOGE("CameraService::connect evicting conflicting client for camera ID %s",

                    i->getKey().string());

            evictedClients.push_back(clientSp);

            evictedClients.push_back(i);



            // Log the clients evicted

            logEvent(String8::format("EVICT device %s client held by package %s (PID"
@@ -1001,12 +1001,31 @@ status_t CameraService::handleEvictionsLocked(const String8& cameraId, int clien 
    // Destroy evicted clients

    for (auto& i : evictedClients) {

        // Disconnect is blocking, and should only have returned when HAL has cleaned up

        i->disconnect(); // Clients will remove themselves from the active client list here

        i->getValue()->disconnect(); // Clients will remove themselves from the active client list

    }

    evictedClients.clear();



    IPCThreadState::self()->restoreCallingIdentity(token);



    for (const auto& i : evictedClients) {

        ALOGV("%s: Waiting for disconnect to complete for client for device %s (PID %" PRId32 ")",

                __FUNCTION__, i->getKey().string(), i->getOwnerId());

        ret = mActiveClientManager.waitUntilRemoved(i, DEFAULT_DISCONNECT_TIMEOUT_NS);

        if (ret == TIMED_OUT) {

            ALOGE("%s: Timed out waiting for client for device %s to disconnect, "

                    "current clients:\n%s", __FUNCTION__, i->getKey().string(),

                    mActiveClientManager.toString().string());

            return -EBUSY;

        }

        if (ret != NO_ERROR) {

            ALOGE("%s: Received error waiting for client for device %s to disconnect: %s (%d), "

                    "current clients:\n%s", __FUNCTION__, i->getKey().string(), strerror(-ret),

                    ret, mActiveClientManager.toString().string());

            return ret;

        }

    }



    evictedClients.clear();



    // Once clients have been disconnected, relock

    mServiceLock.lock();

services/camera/libcameraservice/CameraService.h

+3 −0
Original line number Diff line number Diff line
@@ -90,6 +90,9 @@ public: 
    // 3 second busy timeout when other clients are connecting

    static const nsecs_t DEFAULT_CONNECT_TIMEOUT_NS = 3000000000;



    // 1 second busy timeout when other clients are disconnecting

    static const nsecs_t DEFAULT_DISCONNECT_TIMEOUT_NS = 1000000000;



    // Default number of messages to store in eviction log

    static const size_t DEFAULT_EVENT_LOG_LENGTH = 100;

services/camera/libcameraservice/utils/ClientManager.h

+49 −0
Original line number Diff line number Diff line
@@ -17,7 +17,9 @@ 
#ifndef ANDROID_SERVICE_UTILS_EVICTION_POLICY_MANAGER_H

#define ANDROID_SERVICE_UTILS_EVICTION_POLICY_MANAGER_H



#include <utils/Condition.h>

#include <utils/Mutex.h>

#include <utils/Timers.h>



#include <algorithm>

#include <utility>
@@ -263,6 +265,16 @@ public: 
     */

    std::shared_ptr<ClientDescriptor<KEY, VALUE>> get(const KEY& key) const;



    /**

     * Block until the given client is no longer in the active clients list, or the timeout

     * occurred.

     *

     * Returns NO_ERROR if this succeeded, -ETIMEDOUT on a timeout, or a negative error code on

     * failure.

     */

    status_t waitUntilRemoved(const std::shared_ptr<ClientDescriptor<KEY, VALUE>> client,

            nsecs_t timeout) const;



protected:

    ~ClientManager();
@@ -284,6 +296,7 @@ private: 
    int64_t getCurrentCostLocked() const;



    mutable Mutex mLock;

    mutable Condition mRemovedCondition;

    int32_t mMaxCost;

    // LRU ordered, most recent at end

    std::vector<std::shared_ptr<ClientDescriptor<KEY, VALUE>>> mClients;
@@ -430,6 +443,7 @@ std::vector<std::shared_ptr<ClientDescriptor<KEY, VALUE>>> ClientManager<KEY, VA 
        }), mClients.end());



    mClients.push_back(client);

    mRemovedCondition.broadcast();



    return evicted;

}
@@ -487,6 +501,7 @@ template<class KEY, class VALUE> 
void ClientManager<KEY, VALUE>::removeAll() {

    Mutex::Autolock lock(mLock);

    mClients.clear();

    mRemovedCondition.broadcast();

}



template<class KEY, class VALUE>
@@ -505,6 +520,39 @@ std::shared_ptr<ClientDescriptor<KEY, VALUE>> ClientManager<KEY, VALUE>::remove( 
            return false;

        }), mClients.end());



    mRemovedCondition.broadcast();

    return ret;

}



template<class KEY, class VALUE>

status_t ClientManager<KEY, VALUE>::waitUntilRemoved(

        const std::shared_ptr<ClientDescriptor<KEY, VALUE>> client,

        nsecs_t timeout) const {

    status_t ret = NO_ERROR;

    Mutex::Autolock lock(mLock);



    bool isRemoved = false;



    // Figure out what time in the future we should hit the timeout

    nsecs_t failTime = systemTime(SYSTEM_TIME_MONOTONIC) + timeout;



    while (!isRemoved) {

        isRemoved = true;

        for (const auto& i : mClients) {

            if (i == client) {

                isRemoved = false;

            }

        }



        if (!isRemoved) {

            ret = mRemovedCondition.waitRelative(mLock, timeout);

            if (ret != NO_ERROR) {

                break;

            }

            timeout = failTime - systemTime(SYSTEM_TIME_MONOTONIC);

        }

    }



    return ret;

}
@@ -520,6 +568,7 @@ void ClientManager<KEY, VALUE>::remove( 
            }

            return false;

        }), mClients.end());

    mRemovedCondition.broadcast();

}



template<class KEY, class VALUE>