Revert "SurfaceFlinger: Validate layers before casting."
This reverts commit 4293e9d75d5b0d033126f529ab61dd1f4a8d8420.
Reason for revert: it broke CtsViewTestCases:android.view.cts.ASurfaceControlTest
Bug: 129768960
Change-Id: I4b97869eefdf9108cf2e0e33656037780e5376f7
diff --git a/libs/gui/include/gui/SurfaceControl.h b/libs/gui/include/gui/SurfaceControl.h
index 23bfc02..55efcbf 100644
--- a/libs/gui/include/gui/SurfaceControl.h
+++ b/libs/gui/include/gui/SurfaceControl.h
@@ -84,9 +84,6 @@
explicit SurfaceControl(const sp<SurfaceControl>& other);
- SurfaceControl(const sp<SurfaceComposerClient>& client, const sp<IBinder>& handle,
- const sp<IGraphicBufferProducer>& gbp, bool owned);
-
private:
// can't be copied
SurfaceControl& operator = (SurfaceControl& rhs);
@@ -95,6 +92,12 @@
friend class SurfaceComposerClient;
friend class Surface;
+ SurfaceControl(
+ const sp<SurfaceComposerClient>& client,
+ const sp<IBinder>& handle,
+ const sp<IGraphicBufferProducer>& gbp,
+ bool owned);
+
~SurfaceControl();
sp<Surface> generateSurfaceLocked() const;
diff --git a/services/surfaceflinger/Client.cpp b/services/surfaceflinger/Client.cpp
index 508be13..e54b460 100644
--- a/services/surfaceflinger/Client.cpp
+++ b/services/surfaceflinger/Client.cpp
@@ -76,9 +76,18 @@
uint32_t flags, const sp<IBinder>& parentHandle,
LayerMetadata metadata, sp<IBinder>* handle,
sp<IGraphicBufferProducer>* gbp) {
+ sp<Layer> parent = nullptr;
+ if (parentHandle != nullptr) {
+ auto layerHandle = reinterpret_cast<Layer::Handle*>(parentHandle.get());
+ parent = layerHandle->owner.promote();
+ if (parent == nullptr) {
+ return NAME_NOT_FOUND;
+ }
+ }
+
// We rely on createLayer to check permissions.
return mFlinger->createLayer(name, this, w, h, format, flags, std::move(metadata), handle, gbp,
- parentHandle);
+ &parent);
}
status_t Client::createWithSurfaceParent(const String8& name, uint32_t w, uint32_t h,
diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp
index 1716c17..7965245 100644
--- a/services/surfaceflinger/Layer.cpp
+++ b/services/surfaceflinger/Layer.cpp
@@ -125,6 +125,7 @@
}
mFrameTracker.logAndResetStats(mName);
+
mFlinger->onLayerDestroyed();
}
diff --git a/services/surfaceflinger/RefreshRateOverlay.cpp b/services/surfaceflinger/RefreshRateOverlay.cpp
index 240c84e..e70bfe4 100644
--- a/services/surfaceflinger/RefreshRateOverlay.cpp
+++ b/services/surfaceflinger/RefreshRateOverlay.cpp
@@ -31,9 +31,9 @@
const status_t ret =
mFlinger.createLayer(String8("RefreshRateOverlay"), mClient, 0, 0,
PIXEL_FORMAT_RGBA_8888, ISurfaceComposerClient::eFXSurfaceColor,
- LayerMetadata(), &mIBinder, &mGbp, nullptr);
+ LayerMetadata(), &mIBinder, &mGbp, &mLayer);
if (ret) {
- ALOGE("failed to create color layer");
+ ALOGE("failed to color layer");
return false;
}
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index 530ec30..fccd910 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -3418,20 +3418,12 @@
const sp<IBinder>& handle,
const sp<IGraphicBufferProducer>& gbc,
const sp<Layer>& lbc,
- const sp<IBinder>& parentHandle,
+ const sp<Layer>& parent,
bool addToCurrentState)
{
// add this layer to the current state list
{
Mutex::Autolock _l(mStateLock);
- sp<Layer> parent;
- if (parentHandle != nullptr) {
- parent = fromHandle(parentHandle);
- if (parent == nullptr) {
- return NAME_NOT_FOUND;
- }
- }
-
if (mNumLayers >= MAX_LAYERS) {
ALOGE("AddClientLayer failed, mNumLayers (%zu) >= MAX_LAYERS (%zu)", mNumLayers,
MAX_LAYERS);
@@ -4042,10 +4034,10 @@
return flags;
}
-status_t SurfaceFlinger::createLayer(
- const String8& name, const sp<Client>& client, uint32_t w, uint32_t h, PixelFormat format,
- uint32_t flags, LayerMetadata metadata, sp<IBinder>* handle,
- sp<IGraphicBufferProducer>* gbp, const sp<IBinder>& parentHandle) {
+status_t SurfaceFlinger::createLayer(const String8& name, const sp<Client>& client, uint32_t w,
+ uint32_t h, PixelFormat format, uint32_t flags,
+ LayerMetadata metadata, sp<IBinder>* handle,
+ sp<IGraphicBufferProducer>* gbp, sp<Layer>* parent) {
if (int32_t(w|h) < 0) {
ALOGE("createLayer() failed, w or h is negative (w=%d, h=%d)",
int(w), int(h));
@@ -4110,14 +4102,12 @@
return result;
}
- mLayersByLocalBinderToken.emplace((*handle)->localBinder(), layer);
-
if (primaryDisplayOnly) {
layer->setPrimaryDisplayOnly();
}
bool addToCurrentState = callingThreadHasUnscopedSurfaceFlingerAccess();
- result = addClientLayer(client, *handle, *gbp, layer, parentHandle,
+ result = addClientLayer(client, *handle, *gbp, layer, *parent,
addToCurrentState);
if (result != NO_ERROR) {
return result;
@@ -4235,16 +4225,6 @@
mCurrentState.layersSortedByZ.remove(layer);
}
markLayerPendingRemovalLocked(layer);
-
- auto it = mLayersByLocalBinderToken.begin();
- while (it != mLayersByLocalBinderToken.end()) {
- if (it->second == layer) {
- it = mLayersByLocalBinderToken.erase(it);
- } else {
- it++;
- }
- }
-
layer.clear();
}
@@ -5514,50 +5494,34 @@
const bool mChildrenOnly;
};
- int reqWidth = 0;
- int reqHeight = 0;
- sp<Layer> parent;
+ auto layerHandle = reinterpret_cast<Layer::Handle*>(layerHandleBinder.get());
+ auto parent = layerHandle->owner.promote();
+
+ if (parent == nullptr || parent->isRemovedFromCurrentState()) {
+ ALOGE("captureLayers called with a removed parent");
+ return NAME_NOT_FOUND;
+ }
+
+ const int uid = IPCThreadState::self()->getCallingUid();
+ const bool forSystem = uid == AID_GRAPHICS || uid == AID_SYSTEM;
+ if (!forSystem && parent->getCurrentState().flags & layer_state_t::eLayerSecure) {
+ ALOGW("Attempting to capture secure layer: PERMISSION_DENIED");
+ return PERMISSION_DENIED;
+ }
+
Rect crop(sourceCrop);
- std::unordered_set<sp<Layer>, ISurfaceComposer::SpHash<Layer>> excludeLayers;
+ if (sourceCrop.width() <= 0) {
+ crop.left = 0;
+ crop.right = parent->getBufferSize(parent->getCurrentState()).getWidth();
+ }
- {
- Mutex::Autolock _l(mStateLock);
+ if (sourceCrop.height() <= 0) {
+ crop.top = 0;
+ crop.bottom = parent->getBufferSize(parent->getCurrentState()).getHeight();
+ }
- parent = fromHandle(layerHandleBinder);
- if (parent == nullptr || parent->isRemovedFromCurrentState()) {
- ALOGE("captureLayers called with an invalid or removed parent");
- return NAME_NOT_FOUND;
- }
-
- const int uid = IPCThreadState::self()->getCallingUid();
- const bool forSystem = uid == AID_GRAPHICS || uid == AID_SYSTEM;
- if (!forSystem && parent->getCurrentState().flags & layer_state_t::eLayerSecure) {
- ALOGW("Attempting to capture secure layer: PERMISSION_DENIED");
- return PERMISSION_DENIED;
- }
-
- if (sourceCrop.width() <= 0) {
- crop.left = 0;
- crop.right = parent->getBufferSize(parent->getCurrentState()).getWidth();
- }
-
- if (sourceCrop.height() <= 0) {
- crop.top = 0;
- crop.bottom = parent->getBufferSize(parent->getCurrentState()).getHeight();
- }
- reqWidth = crop.width() * frameScale;
- reqHeight = crop.height() * frameScale;
-
- for (const auto& handle : excludeHandles) {
- sp<Layer> excludeLayer = fromHandle(handle);
- if (excludeLayer != nullptr) {
- excludeLayers.emplace(excludeLayer);
- } else {
- ALOGW("Invalid layer handle passed as excludeLayer to captureLayers");
- return NAME_NOT_FOUND;
- }
- }
- } // mStateLock
+ int32_t reqWidth = crop.width() * frameScale;
+ int32_t reqHeight = crop.height() * frameScale;
// really small crop or frameScale
if (reqWidth <= 0) {
@@ -5567,6 +5531,18 @@
reqHeight = 1;
}
+ std::unordered_set<sp<Layer>, ISurfaceComposer::SpHash<Layer>> excludeLayers;
+ for (const auto& handle : excludeHandles) {
+ BBinder* local = handle->localBinder();
+ if (local != nullptr) {
+ auto layerHandle = reinterpret_cast<Layer::Handle*>(local);
+ excludeLayers.emplace(layerHandle->owner.promote());
+ } else {
+ ALOGW("Invalid layer handle passed as excludeLayer to captureLayers");
+ return NAME_NOT_FOUND;
+ }
+ }
+
LayerRenderArea renderArea(this, parent, crop, reqWidth, reqHeight, reqDataspace, childrenOnly);
auto traverseLayers = [parent, childrenOnly,
&excludeLayers](const LayerVector::Visitor& visitor) {
@@ -5937,18 +5913,6 @@
mFlinger->setInputWindowsFinished();
}
-sp<Layer> SurfaceFlinger::fromHandle(const sp<IBinder>& handle) {
- BBinder *b = handle->localBinder();
- if (b == nullptr) {
- return nullptr;
- }
- auto it = mLayersByLocalBinderToken.find(b);
- if (it != mLayersByLocalBinderToken.end()) {
- return it->second.promote();
- }
- return nullptr;
-}
-
} // namespace android
#if defined(__gl_h_)
diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h
index c0c0880..7e8e836 100644
--- a/services/surfaceflinger/SurfaceFlinger.h
+++ b/services/surfaceflinger/SurfaceFlinger.h
@@ -313,8 +313,6 @@
return mTransactionCompletedThread;
}
- sp<Layer> fromHandle(const sp<IBinder>& handle) REQUIRES(mStateLock);
-
private:
friend class BufferLayer;
friend class BufferQueueLayer;
@@ -589,10 +587,9 @@
/* ------------------------------------------------------------------------
* Layer management
*/
- status_t createLayer(
- const String8& name, const sp<Client>& client, uint32_t w, uint32_t h,
- PixelFormat format, uint32_t flags, LayerMetadata metadata,
- sp<IBinder>* handle, sp<IGraphicBufferProducer>* gbp, const sp<IBinder>& parentHandle);
+ status_t createLayer(const String8& name, const sp<Client>& client, uint32_t w, uint32_t h,
+ PixelFormat format, uint32_t flags, LayerMetadata metadata,
+ sp<IBinder>* handle, sp<IGraphicBufferProducer>* gbp, sp<Layer>* parent);
status_t createBufferQueueLayer(const sp<Client>& client, const String8& name, uint32_t w,
uint32_t h, uint32_t flags, LayerMetadata metadata,
@@ -624,7 +621,7 @@
const sp<IBinder>& handle,
const sp<IGraphicBufferProducer>& gbc,
const sp<Layer>& lbc,
- const sp<IBinder>& parentHandle,
+ const sp<Layer>& parent,
bool addToCurrentState);
// Traverse through all the layers and compute and cache its bounds.
@@ -997,9 +994,6 @@
std::map<wp<IBinder>, sp<DisplayDevice>> mDisplays;
std::unordered_map<DisplayId, sp<IBinder>> mPhysicalDisplayTokens;
- // protected by mStateLock
- std::unordered_map<BBinder*, wp<Layer>> mLayersByLocalBinderToken;
-
// don't use a lock for these, we don't care
int mDebugRegion = 0;
bool mDebugDisableHWC = false;
diff --git a/services/surfaceflinger/tests/Android.bp b/services/surfaceflinger/tests/Android.bp
index d3c87bf..f121a95 100644
--- a/services/surfaceflinger/tests/Android.bp
+++ b/services/surfaceflinger/tests/Android.bp
@@ -19,7 +19,6 @@
srcs: [
"BufferGenerator.cpp",
"Credentials_test.cpp",
- "InvalidHandles_test.cpp",
"Stress_test.cpp",
"SurfaceInterceptor_test.cpp",
"Transaction_test.cpp",
diff --git a/services/surfaceflinger/tests/InvalidHandles_test.cpp b/services/surfaceflinger/tests/InvalidHandles_test.cpp
deleted file mode 100644
index 42d1f5a..0000000
--- a/services/surfaceflinger/tests/InvalidHandles_test.cpp
+++ /dev/null
@@ -1,67 +0,0 @@
-/*
- * Copyright (C) 2019 The Android Open Source Project
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-#include <binder/Binder.h>
-
-#include <gtest/gtest.h>
-
-#include <gui/ISurfaceComposer.h>
-#include <gui/SurfaceComposerClient.h>
-#include <private/gui/ComposerService.h>
-#include <ui/Rect.h>
-
-namespace android {
-namespace {
-
-class NotALayer : public BBinder {};
-
-/**
- * For all of these tests we make a SurfaceControl with an invalid layer handle
- * and verify we aren't able to trick SurfaceFlinger.
- */
-class InvalidHandleTest : public ::testing::Test {
-protected:
- sp<SurfaceComposerClient> mScc;
- sp<SurfaceControl> mNotSc;
- void SetUp() override {
- mScc = new SurfaceComposerClient;
- ASSERT_EQ(NO_ERROR, mScc->initCheck());
- mNotSc = makeNotSurfaceControl();
- }
-
- sp<SurfaceControl> makeNotSurfaceControl() {
- return new SurfaceControl(mScc, new NotALayer(), nullptr, true);
- }
-};
-
-TEST_F(InvalidHandleTest, createSurfaceInvalidHandle) {
- auto notSc = makeNotSurfaceControl();
- ASSERT_EQ(nullptr,
- mScc->createSurface(String8("lolcats"), 19, 47, PIXEL_FORMAT_RGBA_8888, 0,
- notSc.get())
- .get());
-}
-
-TEST_F(InvalidHandleTest, captureLayersInvalidHandle) {
- sp<ISurfaceComposer> sf(ComposerService::getComposerService());
- sp<GraphicBuffer> outBuffer;
-
- ASSERT_EQ(NAME_NOT_FOUND,
- sf->captureLayers(mNotSc->getHandle(), &outBuffer, Rect::EMPTY_RECT, 1.0f));
-}
-
-} // namespace
-} // namespace android
diff --git a/services/surfaceflinger/tests/SurfaceFlinger_test.filter b/services/surfaceflinger/tests/SurfaceFlinger_test.filter
index a8d09ea..be862c9 100644
--- a/services/surfaceflinger/tests/SurfaceFlinger_test.filter
+++ b/services/surfaceflinger/tests/SurfaceFlinger_test.filter
@@ -1,5 +1,5 @@
{
"presubmit": {
- "filter": "CredentialsTest.*:SurfaceFlingerStress.*:SurfaceInterceptorTest.*:LayerTransactionTest.*:LayerTypeTransactionTest.*:LayerUpdateTest.*:GeometryLatchingTest.*:CropLatchingTest.*:ChildLayerTest.*:ScreenCaptureTest.*:ScreenCaptureChildOnlyTest.*:DereferenceSurfaceControlTest.*:BoundlessLayerTest.*:MultiDisplayLayerBoundsTest.*:InvalidHandleTest.*"
+ "filter": "CredentialsTest.*:SurfaceFlingerStress.*:SurfaceInterceptorTest.*:LayerTransactionTest.*:LayerTypeTransactionTest.*:LayerUpdateTest.*:GeometryLatchingTest.*:CropLatchingTest.*:ChildLayerTest.*:ScreenCaptureTest.*:ScreenCaptureChildOnlyTest.*:DereferenceSurfaceControlTest.*:BoundlessLayerTest.*:MultiDisplayLayerBoundsTest.*"
}
}