SF: Fix thread safety in ISurfaceComposer APIs
Fix a few cases of ISurfaceComposer functions looking up displays and
calling into HWC without holding mStateLock. Clean up error handling
for consistent messages and accurate status codes.
Bug: 123715322
Test: Boot (no repro, just an audit)
Change-Id: I6432e15c4166b27398137b681b4ea7c056fea5e4
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 2979322..cd78c1a 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -466,17 +466,17 @@
}
void SurfaceFlinger::destroyDisplay(const sp<IBinder>& displayToken) {
- Mutex::Autolock _l(mStateLock);
+ Mutex::Autolock lock(mStateLock);
- ssize_t index = mCurrentState.displays.indexOfKey(displayToken);
+ const ssize_t index = mCurrentState.displays.indexOfKey(displayToken);
if (index < 0) {
- ALOGE("destroyDisplay: Invalid display token %p", displayToken.get());
+ ALOGE("%s: Invalid display token %p", __FUNCTION__, displayToken.get());
return;
}
const DisplayDeviceState& state = mCurrentState.displays.valueAt(index);
- if (!state.isVirtual()) {
- ALOGE("destroyDisplay called for non-virtual display");
+ if (state.physical) {
+ ALOGE("%s: Invalid operation on physical display", __FUNCTION__);
return;
}
mInterceptor->saveDisplayDeletion(state.sequenceId);
@@ -908,20 +908,29 @@
}
int SurfaceFlinger::getActiveConfig(const sp<IBinder>& displayToken) {
- const auto display = getDisplayDevice(displayToken);
- if (!display) {
- ALOGE("getActiveConfig: Invalid display token %p", displayToken.get());
- return BAD_VALUE;
+ int activeConfig;
+ bool isPrimary;
+
+ {
+ Mutex::Autolock lock(mStateLock);
+
+ if (const auto display = getDisplayDeviceLocked(displayToken)) {
+ activeConfig = display->getActiveConfig().value();
+ isPrimary = display->isPrimary();
+ } else {
+ ALOGE("%s: Invalid display token %p", __FUNCTION__, displayToken.get());
+ return NAME_NOT_FOUND;
+ }
}
- if (display->isPrimary()) {
+ if (isPrimary) {
std::lock_guard<std::mutex> lock(mActiveConfigLock);
if (mDesiredActiveConfigChanged) {
return mDesiredActiveConfig.configId.value();
}
}
- return display->getActiveConfig().value();
+ return activeConfig;
}
void SurfaceFlinger::setDesiredActiveConfig(const ActiveConfigInfo& info) {
@@ -972,17 +981,16 @@
return BAD_VALUE;
}
- status_t result = NO_ERROR;
+ status_t result = NAME_NOT_FOUND;
postMessageSync(new LambdaMessage([&]() {
const auto display = getDisplayDeviceLocked(displayToken);
if (!display) {
ALOGE("Attempt to set allowed display configs for invalid display token %p",
displayToken.get());
- result = BAD_VALUE;
} else if (display->isVirtual()) {
ALOGW("Attempt to set allowed display configs for virtual display");
- result = BAD_VALUE;
+ result = INVALID_OPERATION;
} else {
HwcConfigIndexType config(mode);
const auto& refreshRate = mRefreshRateConfigs->getRefreshRateFromConfigId(config);
@@ -1161,7 +1169,7 @@
// Currently we only support this API for a single internal display.
if (getInternalDisplayToken() != displayToken) {
- return BAD_VALUE;
+ return NAME_NOT_FOUND;
}
memcpy(&primaries, &mInternalDisplayPrimaries, sizeof(ui::DisplayPrimaries));
@@ -1169,7 +1177,9 @@
}
ColorMode SurfaceFlinger::getActiveColorMode(const sp<IBinder>& displayToken) {
- if (const auto display = getDisplayDevice(displayToken)) {
+ Mutex::Autolock lock(mStateLock);
+
+ if (const auto display = getDisplayDeviceLocked(displayToken)) {
return display->getCompositionDisplay()->getState().colorMode;
}
return static_cast<ColorMode>(BAD_VALUE);
@@ -1185,7 +1195,7 @@
decodeColorMode(mode).c_str(), mode, displayToken.get());
return;
}
- const auto display = getDisplayDevice(displayToken);
+ const auto display = getDisplayDeviceLocked(displayToken);
if (!display) {
ALOGE("Attempt to set active color mode %s (%d) for invalid display token %p",
decodeColorMode(mode).c_str(), mode, displayToken.get());
@@ -1205,16 +1215,14 @@
status_t SurfaceFlinger::getAutoLowLatencyModeSupport(const sp<IBinder>& displayToken,
bool* outSupport) const {
- Mutex::Autolock _l(mStateLock);
-
if (!displayToken) {
- ALOGE("getAutoLowLatencyModeSupport() failed. Missing display token.");
return BAD_VALUE;
}
+
+ Mutex::Autolock lock(mStateLock);
+
const auto displayId = getPhysicalDisplayIdLocked(displayToken);
if (!displayId) {
- ALOGE("getAutoLowLatencyModeSupport() failed. Display id for display token %p not found.",
- displayToken.get());
return NAME_NOT_FOUND;
}
*outSupport = getHwComposer().hasDisplayCapability(*displayId,
@@ -1223,64 +1231,45 @@
}
void SurfaceFlinger::setAutoLowLatencyMode(const sp<IBinder>& displayToken, bool on) {
- postMessageAsync(new LambdaMessage([=] { setAutoLowLatencyModeInternal(displayToken, on); }));
-}
-
-void SurfaceFlinger::setAutoLowLatencyModeInternal(const sp<IBinder>& displayToken, bool on) {
- if (!displayToken) {
- ALOGE("setAutoLowLatencyMode() failed. Missing display token.");
- return;
- }
- const auto displayId = getPhysicalDisplayIdLocked(displayToken);
- if (!displayId) {
- ALOGE("setAutoLowLatencyMode() failed. Display id for display token %p not found.",
- displayToken.get());
- return;
- }
-
- getHwComposer().setAutoLowLatencyMode(*displayId, on);
+ postMessageAsync(new LambdaMessage([=] {
+ if (const auto displayId = getPhysicalDisplayIdLocked(displayToken)) {
+ getHwComposer().setAutoLowLatencyMode(*displayId, on);
+ } else {
+ ALOGE("%s: Invalid display token %p", __FUNCTION__, displayToken.get());
+ }
+ }));
}
status_t SurfaceFlinger::getGameContentTypeSupport(const sp<IBinder>& displayToken,
bool* outSupport) const {
- Mutex::Autolock _l(mStateLock);
-
if (!displayToken) {
- ALOGE("getGameContentTypeSupport() failed. Missing display token.");
return BAD_VALUE;
}
+
+ Mutex::Autolock lock(mStateLock);
+
const auto displayId = getPhysicalDisplayIdLocked(displayToken);
if (!displayId) {
- ALOGE("getGameContentTypeSupport() failed. Display id for display token %p not found.",
- displayToken.get());
return NAME_NOT_FOUND;
}
- std::vector<HWC2::ContentType> outSupportedContentTypes;
- getHwComposer().getSupportedContentTypes(*displayId, &outSupportedContentTypes);
- *outSupport = std::find(outSupportedContentTypes.begin(), outSupportedContentTypes.end(),
- HWC2::ContentType::Game) != outSupportedContentTypes.end();
+ std::vector<HWC2::ContentType> types;
+ getHwComposer().getSupportedContentTypes(*displayId, &types);
+
+ *outSupport = std::any_of(types.begin(), types.end(),
+ [](auto type) { return type == HWC2::ContentType::Game; });
return NO_ERROR;
}
void SurfaceFlinger::setGameContentType(const sp<IBinder>& displayToken, bool on) {
- postMessageAsync(new LambdaMessage([=] { setGameContentTypeInternal(displayToken, on); }));
-}
-
-void SurfaceFlinger::setGameContentTypeInternal(const sp<IBinder>& displayToken, bool on) {
- if (!displayToken) {
- ALOGE("setGameContentType() failed. Missing display token.");
- return;
- }
- const auto displayId = getPhysicalDisplayIdLocked(displayToken);
- if (!displayId) {
- ALOGE("setGameContentType() failed. Display id for display token %p not found.",
- displayToken.get());
- return;
- }
-
- const HWC2::ContentType type = on ? HWC2::ContentType::Game : HWC2::ContentType::None;
- getHwComposer().setContentType(*displayId, type);
+ postMessageAsync(new LambdaMessage([=] {
+ if (const auto displayId = getPhysicalDisplayIdLocked(displayToken)) {
+ const auto type = on ? HWC2::ContentType::Game : HWC2::ContentType::None;
+ getHwComposer().setContentType(*displayId, type);
+ } else {
+ ALOGE("%s: Invalid display token %p", __FUNCTION__, displayToken.get());
+ }
+ }));
}
status_t SurfaceFlinger::clearAnimationFrameStats() {
@@ -1297,15 +1286,15 @@
status_t SurfaceFlinger::getHdrCapabilities(const sp<IBinder>& displayToken,
HdrCapabilities* outCapabilities) const {
- Mutex::Autolock _l(mStateLock);
+ Mutex::Autolock lock(mStateLock);
const auto display = getDisplayDeviceLocked(displayToken);
if (!display) {
- ALOGE("getHdrCapabilities: Invalid display token %p", displayToken.get());
- return BAD_VALUE;
+ ALOGE("%s: Invalid display token %p", __FUNCTION__, displayToken.get());
+ return NAME_NOT_FOUND;
}
- // At this point the DisplayDeivce should already be set up,
+ // At this point the DisplayDevice should already be set up,
// meaning the luminance information is already queried from
// hardware composer and stored properly.
const HdrCapabilities& capabilities = display->getHdrCapabilities();
@@ -1348,39 +1337,46 @@
if (!outFormat || !outDataspace || !outComponentMask) {
return BAD_VALUE;
}
- const auto display = getDisplayDevice(displayToken);
- if (!display || !display->getId()) {
- ALOGE("getDisplayedContentSamplingAttributes: Bad display token: %p", display.get());
- return BAD_VALUE;
+
+ Mutex::Autolock lock(mStateLock);
+
+ const auto displayId = getPhysicalDisplayIdLocked(displayToken);
+ if (!displayId) {
+ return NAME_NOT_FOUND;
}
- return getHwComposer().getDisplayedContentSamplingAttributes(*display->getId(), outFormat,
+
+ return getHwComposer().getDisplayedContentSamplingAttributes(*displayId, outFormat,
outDataspace, outComponentMask);
}
status_t SurfaceFlinger::setDisplayContentSamplingEnabled(const sp<IBinder>& displayToken,
bool enable, uint8_t componentMask,
- uint64_t maxFrames) const {
- const auto display = getDisplayDevice(displayToken);
- if (!display || !display->getId()) {
- ALOGE("setDisplayContentSamplingEnabled: Bad display token: %p", display.get());
- return BAD_VALUE;
- }
+ uint64_t maxFrames) {
+ status_t result = NAME_NOT_FOUND;
- return getHwComposer().setDisplayContentSamplingEnabled(*display->getId(), enable,
- componentMask, maxFrames);
+ postMessageSync(new LambdaMessage([&] {
+ if (const auto displayId = getPhysicalDisplayIdLocked(displayToken)) {
+ result = getHwComposer().setDisplayContentSamplingEnabled(*displayId, enable,
+ componentMask, maxFrames);
+ } else {
+ ALOGE("%s: Invalid display token %p", __FUNCTION__, displayToken.get());
+ }
+ }));
+
+ return result;
}
status_t SurfaceFlinger::getDisplayedContentSample(const sp<IBinder>& displayToken,
uint64_t maxFrames, uint64_t timestamp,
DisplayedFrameStats* outStats) const {
- const auto display = getDisplayDevice(displayToken);
- if (!display || !display->getId()) {
- ALOGE("getDisplayContentSample: Bad display token: %p", displayToken.get());
- return BAD_VALUE;
+ Mutex::Autolock lock(mStateLock);
+
+ const auto displayId = getPhysicalDisplayIdLocked(displayToken);
+ if (!displayId) {
+ return NAME_NOT_FOUND;
}
- return getHwComposer().getDisplayedContentSample(*display->getId(), maxFrames, timestamp,
- outStats);
+ return getHwComposer().getDisplayedContentSample(*displayId, maxFrames, timestamp, outStats);
}
status_t SurfaceFlinger::getProtectedContentSupport(bool* outSupported) const {
@@ -1396,19 +1392,15 @@
if (!displayToken || !outIsWideColorDisplay) {
return BAD_VALUE;
}
- Mutex::Autolock _l(mStateLock);
+
+ Mutex::Autolock lock(mStateLock);
const auto display = getDisplayDeviceLocked(displayToken);
if (!display) {
- return BAD_VALUE;
+ return NAME_NOT_FOUND;
}
- // Use hasWideColorDisplay to override built-in display.
- const auto displayId = display->getId();
- if (displayId && displayId == getInternalDisplayIdLocked()) {
- *outIsWideColorDisplay = hasWideColorDisplay;
- return NO_ERROR;
- }
- *outIsWideColorDisplay = display->hasWideColorGamut();
+ *outIsWideColorDisplay =
+ display->isPrimary() ? hasWideColorDisplay : display->hasWideColorGamut();
return NO_ERROR;
}
@@ -1483,6 +1475,9 @@
if (!displayToken || !outSupport) {
return BAD_VALUE;
}
+
+ Mutex::Autolock lock(mStateLock);
+
const auto displayId = getPhysicalDisplayIdLocked(displayToken);
if (!displayId) {
return NAME_NOT_FOUND;
@@ -1492,16 +1487,22 @@
return NO_ERROR;
}
-status_t SurfaceFlinger::setDisplayBrightness(const sp<IBinder>& displayToken,
- float brightness) const {
+status_t SurfaceFlinger::setDisplayBrightness(const sp<IBinder>& displayToken, float brightness) {
if (!displayToken) {
return BAD_VALUE;
}
- const auto displayId = getPhysicalDisplayIdLocked(displayToken);
- if (!displayId) {
- return NAME_NOT_FOUND;
- }
- return getHwComposer().setDisplayBrightness(*displayId, brightness);
+
+ status_t result = NAME_NOT_FOUND;
+
+ postMessageSync(new LambdaMessage([&] {
+ if (const auto displayId = getPhysicalDisplayIdLocked(displayToken)) {
+ result = getHwComposer().setDisplayBrightness(*displayId, brightness);
+ } else {
+ ALOGE("%s: Invalid display token %p", __FUNCTION__, displayToken.get());
+ }
+ }));
+
+ return result;
}
status_t SurfaceFlinger::notifyPowerHint(int32_t hintId) {
@@ -4230,7 +4231,7 @@
void SurfaceFlinger::setPowerMode(const sp<IBinder>& displayToken, int mode) {
postMessageSync(new LambdaMessage([&]() NO_THREAD_SAFETY_ANALYSIS {
- const auto display = getDisplayDevice(displayToken);
+ const auto display = getDisplayDeviceLocked(displayToken);
if (!display) {
ALOGE("Attempt to set power mode %d for invalid display token %p", mode,
displayToken.get());
@@ -5343,10 +5344,10 @@
sp<DisplayDevice> display;
{
- Mutex::Autolock _l(mStateLock);
+ Mutex::Autolock lock(mStateLock);
display = getDisplayDeviceLocked(displayToken);
- if (!display) return BAD_VALUE;
+ if (!display) return NAME_NOT_FOUND;
// set the requested width/height to the logical display viewport size
// by default
@@ -5422,10 +5423,10 @@
uint32_t height;
ui::Transform::RotationFlags captureOrientation;
{
- Mutex::Autolock _l(mStateLock);
+ Mutex::Autolock lock(mStateLock);
display = getDisplayByIdOrLayerStack(displayOrLayerStack);
if (!display) {
- return BAD_VALUE;
+ return NAME_NOT_FOUND;
}
width = uint32_t(display->getViewport().width());
@@ -5560,7 +5561,7 @@
std::unordered_set<sp<Layer>, ISurfaceComposer::SpHash<Layer>> excludeLayers;
Rect displayViewport;
{
- Mutex::Autolock _l(mStateLock);
+ Mutex::Autolock lock(mStateLock);
parent = fromHandle(layerHandleBinder);
if (parent == nullptr || parent->isRemovedFromCurrentState()) {
@@ -5604,9 +5605,9 @@
}
}
- auto display = getDisplayByLayerStack(parent->getLayerStack());
+ const auto display = getDisplayByLayerStack(parent->getLayerStack());
if (!display) {
- return BAD_VALUE;
+ return NAME_NOT_FOUND;
}
displayViewport = display->getViewport();
@@ -5989,17 +5990,16 @@
return BAD_VALUE;
}
- status_t result = NO_ERROR;
+ status_t result = NAME_NOT_FOUND;
postMessageSync(new LambdaMessage([&]() {
const auto display = getDisplayDeviceLocked(displayToken);
if (!display) {
- result = BAD_VALUE;
ALOGE("Attempt to set desired display configs for invalid display token %p",
displayToken.get());
} else if (display->isVirtual()) {
- result = BAD_VALUE;
ALOGW("Attempt to set desired display configs for virtual display");
+ result = INVALID_OPERATION;
} else {
result = setDesiredDisplayConfigSpecsInternal(display,
scheduler::RefreshRateConfigs::
@@ -6038,12 +6038,11 @@
*outMaxRefreshRate = policy.maxRefreshRate;
return NO_ERROR;
} else if (display->isVirtual()) {
- return BAD_VALUE;
+ return INVALID_OPERATION;
} else {
const auto displayId = display->getId();
- if (!displayId) {
- return BAD_VALUE;
- }
+ LOG_FATAL_IF(!displayId);
+
*outDefaultConfig = getHwComposer().getActiveConfigIndex(*displayId);
auto vsyncPeriod = getHwComposer().getActiveConfig(*displayId)->getVsyncPeriod();
*outMinRefreshRate = 1e9f / vsyncPeriod;