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

Commit 9dab9730 authored by Valerie Hau's avatar Valerie Hau
Browse files

Only send surfaces to Listener that registered or applied transaction 

We must not leak surface controls to processes that shouldn't know about
them.  With this change, we limit the listeners that receive a callback
for a surface control to those that 1) registered the surface control
for callback or 2) received and merged a transaction containing that surface
control to apply

Bug: 139439952
Test: build, boot, IPC_test, SurfaceFlinger_test, libsurfaceflinger_unittest

Change-Id: I4eccc3e72d60729c2f3aa7788db0c5c39fbf46b7
parent dd3d0268

libs/gui/ISurfaceComposer.cpp

+6 −3
Original line number Diff line number Diff line
@@ -69,7 +69,7 @@ public: 
                                     const sp<IBinder>& applyToken,

                                     const InputWindowCommands& commands,

                                     int64_t desiredPresentTime,

                                     const client_cache_t& uncacheBuffer,

                                     const client_cache_t& uncacheBuffer, bool hasListenerCallbacks,

                                     const std::vector<ListenerCallbacks>& listenerCallbacks) {

        Parcel data, reply;

        data.writeInterfaceToken(ISurfaceComposer::getInterfaceDescriptor());
@@ -90,6 +90,7 @@ public: 
        data.writeInt64(desiredPresentTime);

        data.writeStrongBinder(uncacheBuffer.token.promote());

        data.writeUint64(uncacheBuffer.id);

        data.writeBool(hasListenerCallbacks);



        if (data.writeVectorSize(listenerCallbacks) == NO_ERROR) {

            for (const auto& [listener, callbackIds] : listenerCallbacks) {
@@ -1039,6 +1040,8 @@ status_t BnSurfaceComposer::onTransact( 
            uncachedBuffer.token = data.readStrongBinder();

            uncachedBuffer.id = data.readUint64();



            bool hasListenerCallbacks = data.readBool();



            std::vector<ListenerCallbacks> listenerCallbacks;

            int32_t listenersSize = data.readInt32();

            for (int32_t i = 0; i < listenersSize; i++) {
@@ -1047,9 +1050,9 @@ status_t BnSurfaceComposer::onTransact( 
                data.readInt64Vector(&callbackIds);

                listenerCallbacks.emplace_back(listener, callbackIds);

            }



            setTransactionState(state, displays, stateFlags, applyToken, inputWindowCommands,

                                desiredPresentTime, uncachedBuffer, listenerCallbacks);

                                desiredPresentTime, uncachedBuffer, hasListenerCallbacks,

                                listenerCallbacks);

            return NO_ERROR;

        }

        case BOOT_FINISHED: {

libs/gui/LayerState.cpp

+24 −3
Original line number Diff line number Diff line
@@ -86,7 +86,6 @@ status_t layer_state_t::write(Parcel& output) const 
    memcpy(output.writeInplace(16 * sizeof(float)),

           colorTransform.asArray(), 16 * sizeof(float));

    output.writeFloat(cornerRadius);

    output.writeBool(hasListenerCallbacks);

    output.writeStrongBinder(cachedBuffer.token.promote());

    output.writeUint64(cachedBuffer.id);

    output.writeParcelable(metadata);
@@ -95,6 +94,22 @@ status_t layer_state_t::write(Parcel& output) const 
    output.writeUint32(static_cast<uint32_t>(bgColorDataspace));

    output.writeBool(colorSpaceAgnostic);



    auto err = output.writeVectorSize(listeners);

    if (err) {

        return err;

    }



    for (auto listener : listeners) {

        err = output.writeStrongBinder(listener.transactionCompletedListener);

        if (err) {

            return err;

        }

        err = output.writeInt64Vector(listener.callbackIds);

        if (err) {

            return err;

        }

    }



    return NO_ERROR;

}
@@ -156,7 +171,6 @@ status_t layer_state_t::read(const Parcel& input) 


    colorTransform = mat4(static_cast<const float*>(input.readInplace(16 * sizeof(float))));

    cornerRadius = input.readFloat();

    hasListenerCallbacks = input.readBool();

    cachedBuffer.token = input.readStrongBinder();

    cachedBuffer.id = input.readUint64();

    input.readParcelable(&metadata);
@@ -165,6 +179,14 @@ status_t layer_state_t::read(const Parcel& input) 
    bgColorDataspace = static_cast<ui::Dataspace>(input.readUint32());

    colorSpaceAgnostic = input.readBool();



    int32_t numListeners = input.readInt32();

    listeners.clear();

    for (int i = 0; i < numListeners; i++) {

        auto listener = input.readStrongBinder();

        std::vector<CallbackId> callbackIds;

        input.readInt64Vector(&callbackIds);

        listeners.emplace_back(listener, callbackIds);

    }

    return NO_ERROR;

}
@@ -361,7 +383,6 @@ void layer_state_t::merge(const layer_state_t& other) { 
    }

    if (other.what & eHasListenerCallbacksChanged) {

        what |= eHasListenerCallbacksChanged;

        hasListenerCallbacks = other.hasListenerCallbacks;

    }



#ifndef NO_INPUT

libs/gui/SurfaceComposerClient.cpp

+65 −16
Original line number Diff line number Diff line
@@ -362,6 +362,33 @@ status_t SurfaceComposerClient::Transaction::readFromParcel(const Parcel* parcel 
        displayStates.add(displayState);

    }



    count = static_cast<size_t>(parcel->readUint32());

    if (count > parcel->dataSize()) {

        return BAD_VALUE;

    }

    std::unordered_map<sp<ITransactionCompletedListener>, CallbackInfo, TCLHash> listenerCallbacks;

    listenerCallbacks.reserve(count);

    for (size_t i = 0; i < count; i++) {

        sp<ITransactionCompletedListener> listener =

                interface_cast<ITransactionCompletedListener>(parcel->readStrongBinder());

        size_t numCallbackIds = parcel->readUint32();

        if (numCallbackIds > parcel->dataSize()) {

            return BAD_VALUE;

        }

        for (size_t j = 0; j < numCallbackIds; j++) {

            listenerCallbacks[listener].callbackIds.insert(parcel->readInt64());

        }

        size_t numSurfaces = parcel->readUint32();

        if (numSurfaces > parcel->dataSize()) {

            return BAD_VALUE;

        }

        for (size_t j = 0; j < numSurfaces; j++) {

            sp<SurfaceControl> surface;

            surface = SurfaceControl::readFromParcel(parcel);

            listenerCallbacks[listener].surfaceControls.insert(surface);

        }

    }



    count = static_cast<size_t>(parcel->readUint32());

    if (count > parcel->dataSize()) {

        return BAD_VALUE;
@@ -389,10 +416,9 @@ status_t SurfaceComposerClient::Transaction::readFromParcel(const Parcel* parcel 
    mContainsBuffer = containsBuffer;

    mDesiredPresentTime = desiredPresentTime;

    mDisplayStates = displayStates;

    mListenerCallbacks = listenerCallbacks;

    mComposerStates = composerStates;

    mInputWindowCommands = inputWindowCommands;

    // listener callbacks contain function pointer addresses and may not be safe to parcel.

    mListenerCallbacks.clear();

    return NO_ERROR;

}
@@ -408,6 +434,19 @@ status_t SurfaceComposerClient::Transaction::writeToParcel(Parcel* parcel) const 
        displayState.write(*parcel);

    }



    parcel->writeUint32(static_cast<uint32_t>(mListenerCallbacks.size()));

    for (auto const& [listener, callbackInfo] : mListenerCallbacks) {

        parcel->writeStrongBinder(ITransactionCompletedListener::asBinder(listener));

        parcel->writeUint32(static_cast<uint32_t>(callbackInfo.callbackIds.size()));

        for (auto callbackId : callbackInfo.callbackIds) {

            parcel->writeInt64(callbackId);

        }

        parcel->writeUint32(static_cast<uint32_t>(callbackInfo.surfaceControls.size()));

        for (auto surfaceControl : callbackInfo.surfaceControls) {

            surfaceControl->writeToParcel(parcel);

        }

    }



    parcel->writeUint32(static_cast<uint32_t>(mComposerStates.size()));

    for (auto const& [surfaceHandle, composerState] : mComposerStates) {

        parcel->writeStrongBinder(surfaceHandle);
@@ -441,6 +480,11 @@ SurfaceComposerClient::Transaction& SurfaceComposerClient::Transaction::merge(Tr 
        mListenerCallbacks[listener].callbackIds.insert(std::make_move_iterator(

                                                                callbackIds.begin()),

                                                        std::make_move_iterator(callbackIds.end()));

        // register surface controls for this listener that is merging

        for (const auto& surfaceControl : surfaceControls) {

            registerSurfaceControlForCallback(surfaceControl);

        }



        mListenerCallbacks[listener]

                .surfaceControls.insert(std::make_move_iterator(surfaceControls.begin()),

                                        std::make_move_iterator(surfaceControls.end()));
@@ -479,7 +523,7 @@ void SurfaceComposerClient::doDropReferenceTransaction(const sp<IBinder>& handle 


    composerStates.add(s);

    sp<IBinder> applyToken = IInterface::asBinder(TransactionCompletedListener::getIInstance());

    sf->setTransactionState(composerStates, displayStates, 0, applyToken, {}, -1, {}, {});

    sf->setTransactionState(composerStates, displayStates, 0, applyToken, {}, -1, {}, false, {});

}



void SurfaceComposerClient::doUncacheBufferTransaction(uint64_t cacheId) {
@@ -490,7 +534,7 @@ void SurfaceComposerClient::doUncacheBufferTransaction(uint64_t cacheId) { 
    uncacheBuffer.id = cacheId;



    sp<IBinder> applyToken = IInterface::asBinder(TransactionCompletedListener::getIInstance());

    sf->setTransactionState({}, {}, 0, applyToken, {}, -1, uncacheBuffer, {});

    sf->setTransactionState({}, {}, 0, applyToken, {}, -1, uncacheBuffer, false, {});

}



void SurfaceComposerClient::Transaction::cacheBuffers() {
@@ -539,8 +583,8 @@ status_t SurfaceComposerClient::Transaction::apply(bool synchronous) { 


    sp<ISurfaceComposer> sf(ComposerService::getComposerService());



    bool hasListenerCallbacks = !mListenerCallbacks.empty();

    std::vector<ListenerCallbacks> listenerCallbacks;



    // For every listener with registered callbacks

    for (const auto& [listener, callbackInfo] : mListenerCallbacks) {

        auto& [callbackIds, surfaceControls] = callbackInfo;
@@ -548,19 +592,24 @@ status_t SurfaceComposerClient::Transaction::apply(bool synchronous) { 
            continue;

        }



        if (surfaceControls.empty()) {

            listenerCallbacks.emplace_back(IInterface::asBinder(listener), std::move(callbackIds));



        // If the listener has any SurfaceControls set on this Transaction update the surface state

        } else {

            // If the listener has any SurfaceControls set on this Transaction update the surface

            // state

            for (const auto& surfaceControl : surfaceControls) {

                layer_state_t* s = getLayerState(surfaceControl);

                if (!s) {

                    ALOGE("failed to get layer state");

                    continue;

                }

                std::vector<CallbackId> callbacks(callbackIds.begin(), callbackIds.end());

                s->what |= layer_state_t::eHasListenerCallbacksChanged;

            s->hasListenerCallbacks = true;

                s->listeners.emplace_back(IInterface::asBinder(listener), callbacks);

            }

        }

    }



    mListenerCallbacks.clear();



    cacheBuffers();
@@ -598,7 +647,7 @@ status_t SurfaceComposerClient::Transaction::apply(bool synchronous) { 
    sf->setTransactionState(composerStates, displayStates, flags, applyToken, mInputWindowCommands,

                            mDesiredPresentTime,

                            {} /*uncacheBuffer - only set in doUncacheBufferTransaction*/,

                            listenerCallbacks);

                            hasListenerCallbacks, listenerCallbacks);

    mInputWindowCommands.clear();

    mStatus = NO_ERROR;

    return NO_ERROR;

libs/gui/include/gui/ISurfaceComposer.h

+1 −1
Original line number Diff line number Diff line
@@ -140,7 +140,7 @@ public: 
                                     const sp<IBinder>& applyToken,

                                     const InputWindowCommands& inputWindowCommands,

                                     int64_t desiredPresentTime,

                                     const client_cache_t& uncacheBuffer,

                                     const client_cache_t& uncacheBuffer, bool hasListenerCallbacks,

                                     const std::vector<ListenerCallbacks>& listenerCallbacks) = 0;



    /* signal that we're done booting.

libs/gui/include/gui/LayerState.h

+3 −2
Original line number Diff line number Diff line
@@ -23,6 +23,7 @@ 
#include <utils/Errors.h>



#include <gui/IGraphicBufferProducer.h>

#include <gui/ITransactionCompletedListener.h>

#include <math/mat4.h>



#ifndef NO_INPUT
@@ -123,7 +124,6 @@ struct layer_state_t { 
            surfaceDamageRegion(),

            api(-1),

            colorTransform(mat4()),

            hasListenerCallbacks(false),

            bgColorAlpha(0),

            bgColorDataspace(ui::Dataspace::UNKNOWN),

            colorSpaceAgnostic(false) {
@@ -186,7 +186,6 @@ struct layer_state_t { 
    sp<NativeHandle> sidebandStream;

    mat4 colorTransform;



    bool hasListenerCallbacks;

#ifndef NO_INPUT

    InputWindowInfo inputInfo;

#endif
@@ -203,6 +202,8 @@ struct layer_state_t { 
    // A color space agnostic layer means the color of this layer can be

    // interpreted in any color space.

    bool colorSpaceAgnostic;



    std::vector<ListenerCallbacks> listeners;

};



struct ComposerState {