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
diff --git a/libs/gui/ISurfaceComposer.cpp b/libs/gui/ISurfaceComposer.cpp
index dc161b7..5805797 100644
--- a/libs/gui/ISurfaceComposer.cpp
+++ b/libs/gui/ISurfaceComposer.cpp
@@ -69,7 +69,7 @@
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 @@
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 @@
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 @@
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: {
diff --git a/libs/gui/LayerState.cpp b/libs/gui/LayerState.cpp
index 523ed1d..e004e95 100644
--- a/libs/gui/LayerState.cpp
+++ b/libs/gui/LayerState.cpp
@@ -86,7 +86,6 @@
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 @@
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 @@
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 @@
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 @@
}
if (other.what & eHasListenerCallbacksChanged) {
what |= eHasListenerCallbacksChanged;
- hasListenerCallbacks = other.hasListenerCallbacks;
}
#ifndef NO_INPUT
diff --git a/libs/gui/SurfaceComposerClient.cpp b/libs/gui/SurfaceComposerClient.cpp
index 67dd726..6b5021d 100644
--- a/libs/gui/SurfaceComposerClient.cpp
+++ b/libs/gui/SurfaceComposerClient.cpp
@@ -366,6 +366,33 @@
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;
+ }
std::unordered_map<sp<IBinder>, ComposerState, IBinderHash> composerStates;
composerStates.reserve(count);
for (size_t i = 0; i < count; i++) {
@@ -389,10 +416,9 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
continue;
}
- listenerCallbacks.emplace_back(IInterface::asBinder(listener), std::move(callbackIds));
-
- // 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;
+ if (surfaceControls.empty()) {
+ listenerCallbacks.emplace_back(IInterface::asBinder(listener), std::move(callbackIds));
+ } 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->listeners.emplace_back(IInterface::asBinder(listener), callbacks);
}
- s->what |= layer_state_t::eHasListenerCallbacksChanged;
- s->hasListenerCallbacks = true;
}
}
+
mListenerCallbacks.clear();
cacheBuffers();
@@ -598,7 +647,7 @@
sf->setTransactionState(composerStates, displayStates, flags, applyToken, mInputWindowCommands,
mDesiredPresentTime,
{} /*uncacheBuffer - only set in doUncacheBufferTransaction*/,
- listenerCallbacks);
+ hasListenerCallbacks, listenerCallbacks);
mInputWindowCommands.clear();
mStatus = NO_ERROR;
return NO_ERROR;
diff --git a/libs/gui/include/gui/ISurfaceComposer.h b/libs/gui/include/gui/ISurfaceComposer.h
index c84910b..06be1b3 100644
--- a/libs/gui/include/gui/ISurfaceComposer.h
+++ b/libs/gui/include/gui/ISurfaceComposer.h
@@ -140,7 +140,7 @@
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.
diff --git a/libs/gui/include/gui/LayerState.h b/libs/gui/include/gui/LayerState.h
index 2eb5492..a49ed52 100644
--- a/libs/gui/include/gui/LayerState.h
+++ b/libs/gui/include/gui/LayerState.h
@@ -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 @@
surfaceDamageRegion(),
api(-1),
colorTransform(mat4()),
- hasListenerCallbacks(false),
bgColorAlpha(0),
bgColorDataspace(ui::Dataspace::UNKNOWN),
colorSpaceAgnostic(false) {
@@ -186,7 +186,6 @@
sp<NativeHandle> sidebandStream;
mat4 colorTransform;
- bool hasListenerCallbacks;
#ifndef NO_INPUT
InputWindowInfo inputInfo;
#endif
@@ -203,6 +202,8 @@
// 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 {
diff --git a/libs/gui/include/gui/SurfaceComposerClient.h b/libs/gui/include/gui/SurfaceComposerClient.h
index 8a6920c..15287e2 100644
--- a/libs/gui/include/gui/SurfaceComposerClient.h
+++ b/libs/gui/include/gui/SurfaceComposerClient.h
@@ -291,6 +291,7 @@
};
class Transaction : public Parcelable {
+ protected:
std::unordered_map<sp<IBinder>, ComposerState, IBinderHash> mComposerStates;
SortedVector<DisplayState > mDisplayStates;
std::unordered_map<sp<ITransactionCompletedListener>, CallbackInfo, TCLHash>
diff --git a/libs/gui/tests/Surface_test.cpp b/libs/gui/tests/Surface_test.cpp
index 5a121d7..d75b1ca 100644
--- a/libs/gui/tests/Surface_test.cpp
+++ b/libs/gui/tests/Surface_test.cpp
@@ -688,6 +688,7 @@
const sp<IBinder>& /*applyToken*/,
const InputWindowCommands& /*inputWindowCommands*/,
int64_t /*desiredPresentTime*/, const client_cache_t& /*cachedBuffer*/,
+ bool /*hasListenerCallbacks*/,
const std::vector<ListenerCallbacks>& /*listenerCallbacks*/) override {
}