diff options
43 files changed, 438 insertions, 275 deletions
diff --git a/cmds/servicemanager/ServiceManager.cpp b/cmds/servicemanager/ServiceManager.cpp index 1f9892a262..e80c3210f0 100644 --- a/cmds/servicemanager/ServiceManager.cpp +++ b/cmds/servicemanager/ServiceManager.cpp @@ -213,17 +213,18 @@ Status ServiceManager::addService(const std::string& name, const sp<IBinder>& bi return Status::fromExceptionCode(Status::EX_ILLEGAL_STATE); } - auto entry = mNameToService.emplace(name, Service { + // Overwrite the old service if it exists + mNameToService[name] = Service { .binder = binder, .allowIsolated = allowIsolated, .dumpPriority = dumpPriority, .debugPid = ctx.debugPid, - }); + }; auto it = mNameToRegistrationCallback.find(name); if (it != mNameToRegistrationCallback.end()) { for (const sp<IServiceCallback>& cb : it->second) { - entry.first->second.guaranteeClient = true; + mNameToService[name].guaranteeClient = true; // permission checked in registerForNotifications cb->onRegistration(name, binder); } diff --git a/cmds/servicemanager/test_sm.cpp b/cmds/servicemanager/test_sm.cpp index 25245beaf7..fb9f9df856 100644 --- a/cmds/servicemanager/test_sm.cpp +++ b/cmds/servicemanager/test_sm.cpp @@ -135,6 +135,26 @@ TEST(AddService, HappyOverExistingService) { IServiceManager::DUMP_FLAG_PRIORITY_DEFAULT).isOk()); } +TEST(AddService, OverwriteExistingService) { + auto sm = getPermissiveServiceManager(); + sp<IBinder> serviceA = getBinder(); + EXPECT_TRUE(sm->addService("foo", serviceA, false /*allowIsolated*/, + IServiceManager::DUMP_FLAG_PRIORITY_DEFAULT).isOk()); + + sp<IBinder> outA; + EXPECT_TRUE(sm->getService("foo", &outA).isOk()); + EXPECT_EQ(serviceA, outA); + + // serviceA should be overwritten by serviceB + sp<IBinder> serviceB = getBinder(); + EXPECT_TRUE(sm->addService("foo", serviceB, false /*allowIsolated*/, + IServiceManager::DUMP_FLAG_PRIORITY_DEFAULT).isOk()); + + sp<IBinder> outB; + EXPECT_TRUE(sm->getService("foo", &outB).isOk()); + EXPECT_EQ(serviceB, outB); +} + TEST(AddService, NoPermissions) { std::unique_ptr<MockAccess> access = std::make_unique<NiceMock<MockAccess>>(); diff --git a/libs/binder/include/binder/Nullable.h b/libs/binder/include/binder/Nullable.h deleted file mode 100644 index a98583dc16..0000000000 --- a/libs/binder/include/binder/Nullable.h +++ /dev/null @@ -1,44 +0,0 @@ -/* - * Copyright (C) 2020 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. - */ -#pragma once - -#include <optional> -#include <utility> - -namespace android { - -namespace aidl { - -// nullable/make_nullable provide source-level compatibility between std::opional and std::unique_ptr -// usage: -// nullable<Foo> a; -// nullable<Foo> b = make_nullable<Foo>(...); -// auto c = make_nullable<Foo>(...); -// c.reset(); -// c = make_nullable<Foo>(...); -// c = std::move(a); - -template <typename T> -using nullable = std::optional<T>; - -template <typename T, typename... Args> -inline nullable<T> make_nullable(Args&&... args) { - return std::make_optional<T>(std::forward<Args>(args)...); -} - -} // namespace aidl - -} // namespace android
\ No newline at end of file diff --git a/libs/binder/ndk/ibinder.cpp b/libs/binder/ndk/ibinder.cpp index d287290a8d..7d9fd51cc2 100644 --- a/libs/binder/ndk/ibinder.cpp +++ b/libs/binder/ndk/ibinder.cpp @@ -168,7 +168,7 @@ status_t ABBinder::onTransact(transaction_code_t code, const Parcel& data, Parce binder_status_t status = getClass()->onTransact(this, code, &in, &out); return PruneStatusT(status); - } else if (code == SHELL_COMMAND_TRANSACTION) { + } else if (code == SHELL_COMMAND_TRANSACTION && getClass()->handleShellCommand != nullptr) { int in = data.readFileDescriptor(); int out = data.readFileDescriptor(); int err = data.readFileDescriptor(); diff --git a/libs/binder/ndk/ibinder_internal.h b/libs/binder/ndk/ibinder_internal.h index 57794279f2..902fe7934d 100644 --- a/libs/binder/ndk/ibinder_internal.h +++ b/libs/binder/ndk/ibinder_internal.h @@ -110,13 +110,13 @@ struct AIBinder_Class { const ::android::String16& getInterfaceDescriptor() const { return mInterfaceDescriptor; } // required to be non-null, implemented for every class - const AIBinder_Class_onCreate onCreate; - const AIBinder_Class_onDestroy onDestroy; - const AIBinder_Class_onTransact onTransact; + const AIBinder_Class_onCreate onCreate = nullptr; + const AIBinder_Class_onDestroy onDestroy = nullptr; + const AIBinder_Class_onTransact onTransact = nullptr; // optional methods for a class - AIBinder_onDump onDump; - AIBinder_handleShellCommand handleShellCommand; + AIBinder_onDump onDump = nullptr; + AIBinder_handleShellCommand handleShellCommand = nullptr; private: // This must be a String16 since BBinder virtual getInterfaceDescriptor returns a reference to diff --git a/libs/binder/ndk/tests/iface.cpp b/libs/binder/ndk/tests/iface.cpp index 64832f3081..a5889856fc 100644 --- a/libs/binder/ndk/tests/iface.cpp +++ b/libs/binder/ndk/tests/iface.cpp @@ -118,7 +118,7 @@ IFoo::~IFoo() { AIBinder_Weak_delete(mWeakBinder); } -binder_status_t IFoo::addService(const char* instance) { +AIBinder* IFoo::getBinder() { AIBinder* binder = nullptr; if (mWeakBinder != nullptr) { @@ -132,8 +132,18 @@ binder_status_t IFoo::addService(const char* instance) { AIBinder_Weak_delete(mWeakBinder); } mWeakBinder = AIBinder_Weak_new(binder); + + // WARNING: it is important that this class does not implement debug or + // shell functions because it does not use special C++ wrapper + // functions, and so this is how we test those functions. } + return binder; +} + +binder_status_t IFoo::addService(const char* instance) { + AIBinder* binder = getBinder(); + binder_status_t status = AServiceManager_addService(binder, instance); // Strong references we care about kept by remote process AIBinder_decStrong(binder); diff --git a/libs/binder/ndk/tests/include/iface/iface.h b/libs/binder/ndk/tests/include/iface/iface.h index cdf5493216..d9dd64b8a6 100644 --- a/libs/binder/ndk/tests/include/iface/iface.h +++ b/libs/binder/ndk/tests/include/iface/iface.h @@ -30,6 +30,9 @@ class IFoo : public virtual ::android::RefBase { static AIBinder_Class* kClass; + // binder representing this interface with one reference count + AIBinder* getBinder(); + // Takes ownership of IFoo binder_status_t addService(const char* instance); static ::android::sp<IFoo> getService(const char* instance, AIBinder** outBinder = nullptr); diff --git a/libs/binder/ndk/tests/libbinder_ndk_unit_test.cpp b/libs/binder/ndk/tests/libbinder_ndk_unit_test.cpp index 332b4ca5b8..1424b6cfca 100644 --- a/libs/binder/ndk/tests/libbinder_ndk_unit_test.cpp +++ b/libs/binder/ndk/tests/libbinder_ndk_unit_test.cpp @@ -165,11 +165,10 @@ int lazyService(const char* instance) { return 1; // should not return } -// This is too slow -// TEST(NdkBinder, GetServiceThatDoesntExist) { -// sp<IFoo> foo = IFoo::getService("asdfghkl;"); -// EXPECT_EQ(nullptr, foo.get()); -// } +TEST(NdkBinder, GetServiceThatDoesntExist) { + sp<IFoo> foo = IFoo::getService("asdfghkl;"); + EXPECT_EQ(nullptr, foo.get()); +} TEST(NdkBinder, CheckServiceThatDoesntExist) { AIBinder* binder = AServiceManager_checkService("asdfghkl;"); @@ -184,6 +183,26 @@ TEST(NdkBinder, CheckServiceThatDoesExist) { AIBinder_decStrong(binder); } +TEST(NdkBinder, UnimplementedDump) { + sp<IFoo> foo = IFoo::getService(IFoo::kSomeInstanceName); + ASSERT_NE(foo, nullptr); + AIBinder* binder = foo->getBinder(); + EXPECT_EQ(OK, AIBinder_dump(binder, STDOUT_FILENO, nullptr, 0)); + AIBinder_decStrong(binder); +} + +TEST(NdkBinder, UnimplementedShell) { + // libbinder_ndk doesn't support calling shell, so we are calling from the + // libbinder across processes to the NDK service which doesn't implement + // shell + static const sp<android::IServiceManager> sm(android::defaultServiceManager()); + sp<IBinder> testService = sm->getService(String16(IFoo::kSomeInstanceName)); + + Vector<String16> argsVec; + EXPECT_EQ(OK, IBinder::shellCommand(testService, STDIN_FILENO, STDOUT_FILENO, STDERR_FILENO, + argsVec, nullptr, nullptr)); +} + TEST(NdkBinder, DoubleNumber) { sp<IFoo> foo = IFoo::getService(IFoo::kSomeInstanceName); ASSERT_NE(foo, nullptr); diff --git a/libs/gui/BitTube.cpp b/libs/gui/BitTube.cpp index ef7a6f54d9..351af652e2 100644 --- a/libs/gui/BitTube.cpp +++ b/libs/gui/BitTube.cpp @@ -86,6 +86,10 @@ void BitTube::setReceiveFd(base::unique_fd&& receiveFd) { mReceiveFd = std::move(receiveFd); } +void BitTube::setSendFd(base::unique_fd&& sendFd) { + mSendFd = std::move(sendFd); +} + ssize_t BitTube::write(void const* vaddr, size_t size) { ssize_t err, len; do { @@ -115,6 +119,11 @@ status_t BitTube::writeToParcel(Parcel* reply) const { status_t result = reply->writeDupFileDescriptor(mReceiveFd); mReceiveFd.reset(); + if (result != NO_ERROR) { + return result; + } + result = reply->writeDupFileDescriptor(mSendFd); + mSendFd.reset(); return result; } @@ -126,6 +135,13 @@ status_t BitTube::readFromParcel(const Parcel* parcel) { ALOGE("BitTube::readFromParcel: can't dup file descriptor (%s)", strerror(error)); return -error; } + mSendFd.reset(dup(parcel->readFileDescriptor())); + if (mSendFd < 0) { + mSendFd.reset(); + int error = errno; + ALOGE("BitTube::readFromParcel: can't dup file descriptor (%s)", strerror(error)); + return -error; + } return NO_ERROR; } diff --git a/libs/gui/DisplayEventDispatcher.cpp b/libs/gui/DisplayEventDispatcher.cpp index 51fbb9748b..2cc7c34c40 100644 --- a/libs/gui/DisplayEventDispatcher.cpp +++ b/libs/gui/DisplayEventDispatcher.cpp @@ -89,12 +89,8 @@ status_t DisplayEventDispatcher::scheduleVsync() { return OK; } -void DisplayEventDispatcher::requestLatestConfig() { - status_t status = mReceiver.requestLatestConfig(); - if (status) { - ALOGW("Failed enable config events, status=%d", status); - return; - } +void DisplayEventDispatcher::injectEvent(const DisplayEventReceiver::Event& event) { + mReceiver.sendEvents(&event, 1); } int DisplayEventDispatcher::getFd() const { @@ -156,6 +152,9 @@ bool DisplayEventDispatcher::processPendingEvents(nsecs_t* outTimestamp, dispatchConfigChanged(ev.header.timestamp, ev.header.displayId, ev.config.configId, ev.config.vsyncPeriod); break; + case DisplayEventReceiver::DISPLAY_EVENT_NULL: + dispatchNullEvent(ev.header.timestamp, ev.header.displayId); + break; default: ALOGW("dispatcher %p ~ ignoring unknown event type %#x", this, ev.header.type); break; @@ -167,4 +166,5 @@ bool DisplayEventDispatcher::processPendingEvents(nsecs_t* outTimestamp, } return gotVsync; } + } // namespace android diff --git a/libs/gui/DisplayEventReceiver.cpp b/libs/gui/DisplayEventReceiver.cpp index 1fed509003..f2b0962eb7 100644 --- a/libs/gui/DisplayEventReceiver.cpp +++ b/libs/gui/DisplayEventReceiver.cpp @@ -79,14 +79,6 @@ status_t DisplayEventReceiver::requestNextVsync() { return NO_INIT; } -status_t DisplayEventReceiver::requestLatestConfig() { - if (mEventConnection != nullptr) { - mEventConnection->requestLatestConfig(); - return NO_ERROR; - } - return NO_INIT; -} - ssize_t DisplayEventReceiver::getEvents(DisplayEventReceiver::Event* events, size_t count) { return DisplayEventReceiver::getEvents(mDataChannel.get(), events, count); @@ -98,6 +90,10 @@ ssize_t DisplayEventReceiver::getEvents(gui::BitTube* dataChannel, return gui::BitTube::recvObjects(dataChannel, events, count); } +ssize_t DisplayEventReceiver::sendEvents(Event const* events, size_t count) { + return DisplayEventReceiver::sendEvents(mDataChannel.get(), events, count); +} + ssize_t DisplayEventReceiver::sendEvents(gui::BitTube* dataChannel, Event const* events, size_t count) { diff --git a/libs/gui/IDisplayEventConnection.cpp b/libs/gui/IDisplayEventConnection.cpp index aa74bfd3f8..c0e246fa15 100644 --- a/libs/gui/IDisplayEventConnection.cpp +++ b/libs/gui/IDisplayEventConnection.cpp @@ -26,8 +26,7 @@ enum class Tag : uint32_t { STEAL_RECEIVE_CHANNEL = IBinder::FIRST_CALL_TRANSACTION, SET_VSYNC_RATE, REQUEST_NEXT_VSYNC, - REQUEST_LATEST_CONFIG, - LAST = REQUEST_LATEST_CONFIG, + LAST = REQUEST_NEXT_VSYNC, }; } // Anonymous namespace @@ -54,11 +53,6 @@ public: callRemoteAsync<decltype(&IDisplayEventConnection::requestNextVsync)>( Tag::REQUEST_NEXT_VSYNC); } - - void requestLatestConfig() override { - callRemoteAsync<decltype(&IDisplayEventConnection::requestLatestConfig)>( - Tag::REQUEST_LATEST_CONFIG); - } }; // Out-of-line virtual method definition to trigger vtable emission in this translation unit (see @@ -80,8 +74,6 @@ status_t BnDisplayEventConnection::onTransact(uint32_t code, const Parcel& data, return callLocal(data, reply, &IDisplayEventConnection::setVsyncRate); case Tag::REQUEST_NEXT_VSYNC: return callLocalAsync(data, reply, &IDisplayEventConnection::requestNextVsync); - case Tag::REQUEST_LATEST_CONFIG: - return callLocalAsync(data, reply, &IDisplayEventConnection::requestLatestConfig); } } diff --git a/libs/gui/LayerState.cpp b/libs/gui/LayerState.cpp index 9c0ad9d4e5..7742503575 100644 --- a/libs/gui/LayerState.cpp +++ b/libs/gui/LayerState.cpp @@ -509,7 +509,8 @@ status_t CaptureArgs::write(Parcel& output) const { status_t status = output.writeInt32(static_cast<int32_t>(pixelFormat)) ?: output.write(sourceCrop) ?: output.writeFloat(frameScale) ?: - output.writeBool(captureSecureLayers); + output.writeBool(captureSecureLayers) ?: + output.writeInt32(uid); return status; } @@ -518,7 +519,8 @@ status_t CaptureArgs::read(const Parcel& input) { status_t status = input.readInt32(&format) ?: input.read(sourceCrop) ?: input.readFloat(&frameScale) ?: - input.readBool(&captureSecureLayers); + input.readBool(&captureSecureLayers) ?: + input.readInt32(&uid); pixelFormat = static_cast<ui::PixelFormat>(format); return status; diff --git a/libs/gui/include/gui/DisplayEventDispatcher.h b/libs/gui/include/gui/DisplayEventDispatcher.h index f210c34196..eb5b00418a 100644 --- a/libs/gui/include/gui/DisplayEventDispatcher.h +++ b/libs/gui/include/gui/DisplayEventDispatcher.h @@ -31,7 +31,7 @@ public: status_t initialize(); void dispose(); status_t scheduleVsync(); - void requestLatestConfig(); + void injectEvent(const DisplayEventReceiver::Event& event); int getFd() const; virtual int handleEvent(int receiveFd, int events, void* data); @@ -48,6 +48,9 @@ private: bool connected) = 0; virtual void dispatchConfigChanged(nsecs_t timestamp, PhysicalDisplayId displayId, int32_t configId, nsecs_t vsyncPeriod) = 0; + // AChoreographer-specific hook for processing null-events so that looper + // can be properly poked. + virtual void dispatchNullEvent(nsecs_t timestamp, PhysicalDisplayId displayId) = 0; bool processPendingEvents(nsecs_t* outTimestamp, PhysicalDisplayId* outDisplayId, uint32_t* outCount); diff --git a/libs/gui/include/gui/DisplayEventReceiver.h b/libs/gui/include/gui/DisplayEventReceiver.h index 8d49184caf..0e10d1ad8e 100644 --- a/libs/gui/include/gui/DisplayEventReceiver.h +++ b/libs/gui/include/gui/DisplayEventReceiver.h @@ -53,6 +53,7 @@ public: DISPLAY_EVENT_VSYNC = fourcc('v', 's', 'y', 'n'), DISPLAY_EVENT_HOTPLUG = fourcc('p', 'l', 'u', 'g'), DISPLAY_EVENT_CONFIG_CHANGED = fourcc('c', 'o', 'n', 'f'), + DISPLAY_EVENT_NULL = fourcc('n', 'u', 'l', 'l'), }; struct Event { @@ -130,6 +131,7 @@ public: * sendEvents write events to the queue and returns how many events were * written. */ + ssize_t sendEvents(Event const* events, size_t count); static ssize_t sendEvents(gui::BitTube* dataChannel, Event const* events, size_t count); /* @@ -146,12 +148,6 @@ public: */ status_t requestNextVsync(); - /* - * requestLatestConfig() force-requests the current config for the primary - * display. - */ - status_t requestLatestConfig(); - private: sp<IDisplayEventConnection> mEventConnection; std::unique_ptr<gui::BitTube> mDataChannel; diff --git a/libs/gui/include/gui/IDisplayEventConnection.h b/libs/gui/include/gui/IDisplayEventConnection.h index 674aafd81c..cff22a368a 100644 --- a/libs/gui/include/gui/IDisplayEventConnection.h +++ b/libs/gui/include/gui/IDisplayEventConnection.h @@ -51,11 +51,6 @@ public: * requestNextVsync() schedules the next vsync event. It has no effect if the vsync rate is > 0. */ virtual void requestNextVsync() = 0; // Asynchronous - - /* - * requestLatestConfig() requests the config for the primary display. - */ - virtual void requestLatestConfig() = 0; // Asynchronous }; class BnDisplayEventConnection : public SafeBnInterface<IDisplayEventConnection> { diff --git a/libs/gui/include/gui/LayerState.h b/libs/gui/include/gui/LayerState.h index bbf827f0cd..6a304ef2d8 100644 --- a/libs/gui/include/gui/LayerState.h +++ b/libs/gui/include/gui/LayerState.h @@ -314,12 +314,14 @@ static inline int compare_type(const DisplayState& lhs, const DisplayState& rhs) bool ValidateFrameRate(float frameRate, int8_t compatibility, const char* functionName); struct CaptureArgs { + const static int32_t UNSET_UID = -1; virtual ~CaptureArgs() = default; ui::PixelFormat pixelFormat{ui::PixelFormat::RGBA_8888}; Rect sourceCrop; float frameScale{1}; bool captureSecureLayers{false}; + int32_t uid{UNSET_UID}; virtual status_t write(Parcel& output) const; virtual status_t read(const Parcel& input); diff --git a/libs/gui/include/private/gui/BitTube.h b/libs/gui/include/private/gui/BitTube.h index 13c01623b7..80485184bd 100644 --- a/libs/gui/include/private/gui/BitTube.h +++ b/libs/gui/include/private/gui/BitTube.h @@ -58,6 +58,9 @@ public: // resets this BitTube's receive file descriptor to receiveFd void setReceiveFd(base::unique_fd&& receiveFd); + // resets this BitTube's send file descriptor to sendFd + void setSendFd(base::unique_fd&& sendFd); + // send objects (sized blobs). All objects are guaranteed to be written or the call fails. template <typename T> static ssize_t sendObjects(BitTube* tube, T const* events, size_t count) { @@ -85,7 +88,7 @@ private: // the message, excess data is silently discarded. ssize_t read(void* vaddr, size_t size); - base::unique_fd mSendFd; + mutable base::unique_fd mSendFd; mutable base::unique_fd mReceiveFd; static ssize_t sendObjects(BitTube* tube, void const* events, size_t count, size_t objSize); diff --git a/libs/nativedisplay/AChoreographer.cpp b/libs/nativedisplay/AChoreographer.cpp index ebc8909179..ff1b5e6d34 100644 --- a/libs/nativedisplay/AChoreographer.cpp +++ b/libs/nativedisplay/AChoreographer.cpp @@ -136,6 +136,7 @@ private: void dispatchHotplug(nsecs_t timestamp, PhysicalDisplayId displayId, bool connected) override; void dispatchConfigChanged(nsecs_t timestamp, PhysicalDisplayId displayId, int32_t configId, nsecs_t vsyncPeriod) override; + void dispatchNullEvent(nsecs_t, PhysicalDisplayId) override; void scheduleCallbacks(); @@ -170,7 +171,7 @@ Choreographer* Choreographer::getForThread() { Choreographer::Choreographer(const sp<Looper>& looper) : DisplayEventDispatcher(looper, ISurfaceComposer::VsyncSource::eVsyncSourceApp, - ISurfaceComposer::ConfigChanged::eConfigChangedDispatch), + ISurfaceComposer::ConfigChanged::eConfigChangedSuppress), mLooper(looper), mThreadId(std::this_thread::get_id()) { std::lock_guard<std::mutex> _l(gChoreographers.lock); @@ -294,8 +295,14 @@ void Choreographer::scheduleLatestConfigRequest() { } else { // If the looper thread is detached from Choreographer, then refresh rate // changes will be handled in AChoreographer_handlePendingEvents, so we - // need to redispatch a config from SF - requestLatestConfig(); + // need to wake up the looper thread by writing to the write-end of the + // socket the looper is listening on. + // Fortunately, these events are small so sending packets across the + // socket should be atomic across processes. + DisplayEventReceiver::Event event; + event.header = DisplayEventReceiver::Event::Header{DisplayEventReceiver::DISPLAY_EVENT_NULL, + PhysicalDisplayId(0), systemTime()}; + injectEvent(event); } } @@ -373,27 +380,14 @@ void Choreographer::dispatchHotplug(nsecs_t, PhysicalDisplayId displayId, bool c // displays. When multi-display choreographer is properly supported, then // PhysicalDisplayId should no longer be ignored. void Choreographer::dispatchConfigChanged(nsecs_t, PhysicalDisplayId displayId, int32_t configId, - nsecs_t vsyncPeriod) { + nsecs_t) { ALOGV("choreographer %p ~ received config change event (displayId=%s, configId=%d).", this, to_string(displayId).c_str(), configId); +} - const nsecs_t lastPeriod = mLatestVsyncPeriod; - std::vector<RefreshRateCallback> callbacks{}; - { - std::lock_guard<std::mutex> _l{mLock}; - for (auto& cb : mRefreshRateCallbacks) { - callbacks.push_back(cb); - cb.firstCallbackFired = true; - } - } - - for (auto& cb : callbacks) { - if (!cb.firstCallbackFired || (vsyncPeriod > 0 && vsyncPeriod != lastPeriod)) { - cb.callback(vsyncPeriod, cb.data); - } - } - - mLatestVsyncPeriod = vsyncPeriod; +void Choreographer::dispatchNullEvent(nsecs_t, PhysicalDisplayId) { + ALOGV("choreographer %p ~ received null event.", this); + handleRefreshRateUpdates(); } void Choreographer::handleMessage(const Message& message) { diff --git a/libs/renderengine/OWNERS b/libs/renderengine/OWNERS index c00fbbaad2..b44456b705 100644 --- a/libs/renderengine/OWNERS +++ b/libs/renderengine/OWNERS @@ -1,2 +1,4 @@ +alecmouri@google.com +jreck@google.com lpy@google.com stoza@google.com diff --git a/libs/ui/OWNERS b/libs/ui/OWNERS index 203a7395e8..b1317b169e 100644 --- a/libs/ui/OWNERS +++ b/libs/ui/OWNERS @@ -1,6 +1,7 @@ +alecmouri@google.com chrisforbes@google.com +jreck@google.com lpy@google.com mathias@google.com romainguy@google.com stoza@google.com -vhau@google.com diff --git a/services/inputflinger/dispatcher/InputDispatcher.cpp b/services/inputflinger/dispatcher/InputDispatcher.cpp index 5ba5ad8282..44f26b0de8 100644 --- a/services/inputflinger/dispatcher/InputDispatcher.cpp +++ b/services/inputflinger/dispatcher/InputDispatcher.cpp @@ -3947,7 +3947,7 @@ void InputDispatcher::setFocusedDisplay(int32_t displayId) { } mFocusedDisplayId = displayId; - // Sanity check + // Find new focused window and validate sp<InputWindowHandle> newFocusedWindowHandle = getValueByKey(mFocusedWindowHandlesByDisplay, displayId); notifyFocusChangedLocked(oldFocusedWindowHandle, newFocusedWindowHandle); diff --git a/services/inputflinger/tests/BlockingQueue_test.cpp b/services/inputflinger/tests/BlockingQueue_test.cpp index 0dea8d7861..fd9d9d5bd3 100644 --- a/services/inputflinger/tests/BlockingQueue_test.cpp +++ b/services/inputflinger/tests/BlockingQueue_test.cpp @@ -26,7 +26,7 @@ namespace android { // --- BlockingQueueTest --- /** - * Sanity check of basic pop and push operation. + * Validate basic pop and push operation. */ TEST(BlockingQueueTest, Queue_AddAndRemove) { constexpr size_t capacity = 10; diff --git a/services/surfaceflinger/Layer.cpp b/services/surfaceflinger/Layer.cpp index 001eab7f96..eb33175b0e 100644 --- a/services/surfaceflinger/Layer.cpp +++ b/services/surfaceflinger/Layer.cpp @@ -39,6 +39,7 @@ #include <gui/LayerDebugInfo.h> #include <gui/Surface.h> #include <math.h> +#include <private/android_filesystem_config.h> #include <renderengine/RenderEngine.h> #include <stdint.h> #include <stdlib.h> @@ -139,6 +140,14 @@ Layer::Layer(const LayerCreationArgs& args) mCallingPid = args.callingPid; mCallingUid = args.callingUid; + + if (mCallingUid == AID_GRAPHICS || mCallingUid == AID_SYSTEM) { + // If the system didn't send an ownerUid, use the callingUid for the ownerUid. + mOwnerUid = args.metadata.getInt32(METADATA_OWNER_UID, mCallingUid); + } else { + // A create layer request from a non system request cannot specify the owner uid + mOwnerUid = mCallingUid; + } } void Layer::onFirstRef() { @@ -1665,8 +1674,8 @@ void Layer::dumpFrameEvents(std::string& result) { } void Layer::dumpCallingUidPid(std::string& result) const { - StringAppendF(&result, "Layer %s (%s) pid:%d uid:%d\n", getName().c_str(), getType(), - mCallingPid, mCallingUid); + StringAppendF(&result, "Layer %s (%s) callingPid:%d callingUid:%d ownerUid:%d\n", + getName().c_str(), getType(), mCallingPid, mCallingUid, mOwnerUid); } void Layer::onDisconnect() { @@ -2339,6 +2348,8 @@ void Layer::writeToProtoCommonState(LayerProto* layerInfo, LayerVector::StateSet } layerInfo->set_is_relative_of(state.isRelativeOf); + + layerInfo->set_owner_uid(mOwnerUid); } if (traceFlags & SurfaceTracing::TRACE_INPUT) { diff --git a/services/surfaceflinger/Layer.h b/services/surfaceflinger/Layer.h index 99b1bb199a..8d8ab6dbae 100644 --- a/services/surfaceflinger/Layer.h +++ b/services/surfaceflinger/Layer.h @@ -436,9 +436,7 @@ public: // Deprecated, please use compositionengine::Output::belongsInOutput() // instead. // TODO(lpique): Move the remaining callers (screencap) to the new function. - bool belongsToDisplay(uint32_t layerStack, bool isPrimaryDisplay) const { - return getLayerStack() == layerStack && (!mPrimaryDisplayOnly || isPrimaryDisplay); - } + bool belongsToDisplay(uint32_t layerStack) const { return getLayerStack() == layerStack; } FloatRect getBounds(const Region& activeTransparentRegion) const; FloatRect getBounds() const; @@ -967,6 +965,8 @@ public: */ virtual bool needsInputInfo() const { return hasInputInfo(); } + uid_t getOwnerUid() { return mOwnerUid; } + protected: compositionengine::OutputLayer* findOutputLayerForDisplay(const DisplayDevice*) const; @@ -1089,6 +1089,10 @@ private: pid_t mCallingPid; uid_t mCallingUid; + // The owner of the layer. If created from a non system process, it will be the calling uid. + // If created from a system process, the value can be passed in. + uid_t mOwnerUid; + // The current layer is a clone of mClonedFrom. This means that this layer will update it's // properties based on mClonedFrom. When mClonedFrom latches a new buffer for BufferLayers, // this layer will update it's buffer. When mClonedFrom updates it's drawing state, children, diff --git a/services/surfaceflinger/RegionSamplingThread.cpp b/services/surfaceflinger/RegionSamplingThread.cpp index 255a1f2450..0157a7f576 100644 --- a/services/surfaceflinger/RegionSamplingThread.cpp +++ b/services/surfaceflinger/RegionSamplingThread.cpp @@ -429,7 +429,7 @@ void RegionSamplingThread::captureSample() { bounds.top, bounds.right, bounds.bottom); visitor(layer); }; - mFlinger.traverseLayersInLayerStack(layerStack, filterVisitor); + mFlinger.traverseLayersInLayerStack(layerStack, CaptureArgs::UNSET_UID, filterVisitor); }; sp<GraphicBuffer> buffer = nullptr; diff --git a/services/surfaceflinger/Scheduler/EventThread.cpp b/services/surfaceflinger/Scheduler/EventThread.cpp index 2020e0cb29..846190c7eb 100644 --- a/services/surfaceflinger/Scheduler/EventThread.cpp +++ b/services/surfaceflinger/Scheduler/EventThread.cpp @@ -137,6 +137,7 @@ void EventThreadConnection::onFirstRef() { status_t EventThreadConnection::stealReceiveChannel(gui::BitTube* outChannel) { outChannel->setReceiveFd(mChannel.moveReceiveFd()); + outChannel->setSendFd(base::unique_fd(dup(mChannel.getSendFd()))); return NO_ERROR; } @@ -150,11 +151,6 @@ void EventThreadConnection::requestNextVsync() { mEventThread->requestNextVsync(this); } -void EventThreadConnection::requestLatestConfig() { - ATRACE_NAME("requestLatestConfig"); - mEventThread->requestLatestConfig(this); -} - status_t EventThreadConnection::postEvent(const DisplayEventReceiver::Event& event) { ssize_t size = DisplayEventReceiver::sendEvents(&mChannel, &event, 1); return size < 0 ? status_t(size) : status_t(NO_ERROR); @@ -267,28 +263,6 @@ void EventThread::requestNextVsync(const sp<EventThreadConnection>& connection) } } -void EventThread::requestLatestConfig(const sp<EventThreadConnection>& connection) { - std::lock_guard<std::mutex> lock(mMutex); - if (connection->mForcedConfigChangeDispatch) { - return; - } - connection->mForcedConfigChangeDispatch = true; - auto pendingConfigChange = - std::find_if(std::begin(mPendingEvents), std::end(mPendingEvents), - [&](const DisplayEventReceiver::Event& event) { - return event.header.type == - DisplayEventReceiver::DISPLAY_EVENT_CONFIG_CHANGED; - }); - - // If we didn't find a pending config change event, then push out the - // latest one we've ever seen. - if (pendingConfigChange == std::end(mPendingEvents)) { - mPendingEvents.push_back(mLastConfigChangeEvent); - } - - mCondition.notify_all(); -} - void EventThread::onScreenReleased() { std::lock_guard<std::mutex> lock(mMutex); if (!mVSyncState || mVSyncState->synthetic) { @@ -364,9 +338,6 @@ void EventThread::threadMain(std::unique_lock<std::mutex>& lock) { mInterceptVSyncsCallback(event->header.timestamp); } break; - case DisplayEventReceiver::DISPLAY_EVENT_CONFIG_CHANGED: - mLastConfigChangeEvent = *event; - break; } } @@ -379,10 +350,6 @@ void EventThread::threadMain(std::unique_lock<std::mutex>& lock) { vsyncRequested |= connection->vsyncRequest != VSyncRequest::None; if (event && shouldConsumeEvent(*event, connection)) { - if (event->header.type == DisplayEventReceiver::DISPLAY_EVENT_CONFIG_CHANGED && - connection->mForcedConfigChangeDispatch) { - connection->mForcedConfigChangeDispatch = false; - } consumers.push_back(connection); } @@ -458,9 +425,7 @@ bool EventThread::shouldConsumeEvent(const DisplayEventReceiver::Event& event, return true; case DisplayEventReceiver::DISPLAY_EVENT_CONFIG_CHANGED: { - const bool oneTimeDispatch = connection->mForcedConfigChangeDispatch; - return oneTimeDispatch || - connection->mConfigChanged == ISurfaceComposer::eConfigChangedDispatch; + return connection->mConfigChanged == ISurfaceComposer::eConfigChangedDispatch; } case DisplayEventReceiver::DISPLAY_EVENT_VSYNC: diff --git a/services/surfaceflinger/Scheduler/EventThread.h b/services/surfaceflinger/Scheduler/EventThread.h index 64acbd72d0..49f624c35e 100644 --- a/services/surfaceflinger/Scheduler/EventThread.h +++ b/services/surfaceflinger/Scheduler/EventThread.h @@ -81,19 +81,13 @@ public: status_t stealReceiveChannel(gui::BitTube* outChannel) override; status_t setVsyncRate(uint32_t rate) override; void requestNextVsync() override; // asynchronous - void requestLatestConfig() override; // asynchronous // Called in response to requestNextVsync. const ResyncCallback resyncCallback; VSyncRequest vsyncRequest = VSyncRequest::None; - ISurfaceComposer::ConfigChanged mConfigChanged = + const ISurfaceComposer::ConfigChanged mConfigChanged = ISurfaceComposer::ConfigChanged::eConfigChangedSuppress; - // Store whether we need to force dispatching a config change separately - - // if mConfigChanged ever changes before the config change is dispatched - // then we still need to propagate an initial config to the app if we - // haven't already. - bool mForcedConfigChangeDispatch = false; private: virtual void onFirstRef(); @@ -129,10 +123,6 @@ public: virtual void setVsyncRate(uint32_t rate, const sp<EventThreadConnection>& connection) = 0; // Requests the next vsync. If resetIdleTimer is set to true, it resets the idle timer. virtual void requestNextVsync(const sp<EventThreadConnection>& connection) = 0; - // Dispatches the most recent configuration - // Usage of this method assumes that only the primary internal display - // supports multiple display configurations. - virtual void requestLatestConfig(const sp<EventThreadConnection>& connection) = 0; // Retrieves the number of event connections tracked by this EventThread. virtual size_t getEventThreadConnectionCount() = 0; @@ -153,7 +143,6 @@ public: status_t registerDisplayEventConnection(const sp<EventThreadConnection>& connection) override; void setVsyncRate(uint32_t rate, const sp<EventThreadConnection>& connection) override; void requestNextVsync(const sp<EventThreadConnection>& connection) override; - void requestLatestConfig(const sp<EventThreadConnection>& connection) override; // called before the screen is turned off from main thread void onScreenReleased() override; @@ -201,7 +190,6 @@ private: std::vector<wp<EventThreadConnection>> mDisplayEventConnections GUARDED_BY(mMutex); std::deque<DisplayEventReceiver::Event> mPendingEvents GUARDED_BY(mMutex); - DisplayEventReceiver::Event mLastConfigChangeEvent GUARDED_BY(mMutex); // VSYNC state of connected display. struct VSyncState { diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp index 9ff57df572..ca9f6296d9 100644 --- a/services/surfaceflinger/SurfaceFlinger.cpp +++ b/services/surfaceflinger/SurfaceFlinger.cpp @@ -4958,11 +4958,13 @@ status_t SurfaceFlinger::CheckTransactCodeCredentials(uint32_t code) { // special permissions. case SET_FRAME_RATE: case GET_DISPLAY_BRIGHTNESS_SUPPORT: + // captureLayers and captureDisplay will handle the permission check in the function + case CAPTURE_LAYERS: + case CAPTURE_DISPLAY: case SET_DISPLAY_BRIGHTNESS: { return OK; } - case CAPTURE_LAYERS: - case CAPTURE_DISPLAY: + case ADD_REGION_SAMPLING_LISTENER: case REMOVE_REGION_SAMPLING_LISTENER: { // codes that require permission check @@ -5434,10 +5436,33 @@ static Dataspace pickDataspaceFromColorMode(const ColorMode colorMode) { } } +static status_t validateScreenshotPermissions(const CaptureArgs& captureArgs) { + IPCThreadState* ipc = IPCThreadState::self(); + const int pid = ipc->getCallingPid(); + const int uid = ipc->getCallingUid(); + if (uid == AID_GRAPHICS || PermissionCache::checkPermission(sReadFramebuffer, pid, uid)) { + return OK; + } + + // If the caller doesn't have the correct permissions but is only attempting to screenshot + // itself, we allow it to continue. + if (captureArgs.uid == uid) { + return OK; + } + + ALOGE("Permission Denial: can't take screenshot pid=%d, uid=%d", pid, uid); + return PERMISSION_DENIED; +} + status_t SurfaceFlinger::captureDisplay(const DisplayCaptureArgs& args, ScreenCaptureResults& captureResults) { ATRACE_CALL(); + status_t validate = validateScreenshotPermissions(args); + if (validate != OK) { + return validate; + } + if (!args.displayToken) return BAD_VALUE; wp<DisplayDevice> displayWeak; @@ -5466,8 +5491,8 @@ status_t SurfaceFlinger::captureDisplay(const DisplayCaptureArgs& args, args.useIdentityTransform, args.captureSecureLayers); }); - auto traverseLayers = [this, layerStack](const LayerVector::Visitor& visitor) { - traverseLayersInLayerStack(layerStack, visitor); + auto traverseLayers = [this, args, layerStack](const LayerVector::Visitor& visitor) { + traverseLayersInLayerStack(layerStack, args.uid, visitor); }; return captureScreenCommon(std::move(renderAreaFuture), traverseLayers, reqSize, args.pixelFormat, captureResults); @@ -5541,7 +5566,7 @@ status_t SurfaceFlinger::captureDisplay(uint64_t displayOrLayerStack, }); auto traverseLayers = [this, layerStack](const LayerVector::Visitor& visitor) { - traverseLayersInLayerStack(layerStack, visitor); + traverseLayersInLayerStack(layerStack, CaptureArgs::UNSET_UID, visitor); }; return captureScreenCommon(std::move(renderAreaFuture), traverseLayers, size, @@ -5552,6 +5577,11 @@ status_t SurfaceFlinger::captureLayers(const LayerCaptureArgs& args, ScreenCaptureResults& captureResults) { ATRACE_CALL(); + status_t validate = validateScreenshotPermissions(args); + if (validate != OK) { + return validate; + } + ui::Size reqSize; sp<Layer> parent; Rect crop(args.sourceCrop); @@ -5625,19 +5655,19 @@ status_t SurfaceFlinger::captureLayers(const LayerCaptureArgs& args, } bool childrenOnly = args.childrenOnly; - RenderAreaFuture renderAreaFuture = promise::defer([=]() -> std::unique_ptr<RenderArea> { return std::make_unique<LayerRenderArea>(*this, parent, crop, reqSize, dataspace, childrenOnly, displayViewport, captureSecureLayers); }); - auto traverseLayers = [parent, childrenOnly, - &excludeLayers](const LayerVector::Visitor& visitor) { + auto traverseLayers = [parent, args, &excludeLayers](const LayerVector::Visitor& visitor) { parent->traverseChildrenInZOrder(LayerVector::StateSet::Drawing, [&](Layer* layer) { if (!layer->isVisible()) { return; - } else if (childrenOnly && layer == parent.get()) { + } else if (args.childrenOnly && layer == parent.get()) { + return; + } else if (args.uid != CaptureArgs::UNSET_UID && args.uid != layer->getOwnerUid()) { return; } @@ -5859,22 +5889,25 @@ void SurfaceFlinger::State::traverseInReverseZOrder(const LayerVector::Visitor& layersSortedByZ.traverseInReverseZOrder(stateSet, visitor); } -void SurfaceFlinger::traverseLayersInLayerStack(ui::LayerStack layerStack, +void SurfaceFlinger::traverseLayersInLayerStack(ui::LayerStack layerStack, const int32_t uid, const LayerVector::Visitor& visitor) { // We loop through the first level of layers without traversing, // as we need to determine which layers belong to the requested display. for (const auto& layer : mDrawingState.layersSortedByZ) { - if (!layer->belongsToDisplay(layerStack, false)) { + if (!layer->belongsToDisplay(layerStack)) { continue; } // relative layers are traversed in Layer::traverseInZOrder layer->traverseInZOrder(LayerVector::StateSet::Drawing, [&](Layer* layer) { - if (!layer->belongsToDisplay(layerStack, false)) { + if (layer->getPrimaryDisplayOnly()) { return; } if (!layer->isVisible()) { return; } + if (uid != CaptureArgs::UNSET_UID && layer->getOwnerUid() != uid) { + return; + } visitor(layer); }); } diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h index 2e85c1b85b..1acfda9916 100644 --- a/services/surfaceflinger/SurfaceFlinger.h +++ b/services/surfaceflinger/SurfaceFlinger.h @@ -728,7 +728,9 @@ private: sp<DisplayDevice> getDisplayByIdOrLayerStack(uint64_t displayOrLayerStack) REQUIRES(mStateLock); sp<DisplayDevice> getDisplayByLayerStack(uint64_t layerStack) REQUIRES(mStateLock); - void traverseLayersInLayerStack(ui::LayerStack, const LayerVector::Visitor&); + // If the uid provided is not UNSET_UID, the traverse will skip any layers that don't have a + // matching ownerUid + void traverseLayersInLayerStack(ui::LayerStack, const int32_t uid, const LayerVector::Visitor&); sp<StartPropertySetThread> mStartPropertySetThread; diff --git a/services/surfaceflinger/layerproto/LayerProtoParser.cpp b/services/surfaceflinger/layerproto/LayerProtoParser.cpp index 8fce0c9bf3..aef670da33 100644 --- a/services/surfaceflinger/layerproto/LayerProtoParser.cpp +++ b/services/surfaceflinger/layerproto/LayerProtoParser.cpp @@ -115,6 +115,7 @@ LayerProtoParser::Layer LayerProtoParser::generateLayer(const LayerProto& layerP } layer.cornerRadiusCrop = generateFloatRect(layerProto.corner_radius_crop()); layer.shadowRadius = layerProto.shadow_radius(); + layer.ownerUid = layerProto.owner_uid(); return layer; } @@ -276,7 +277,7 @@ std::string LayerProtoParser::Region::to_string(const char* what) const { std::string LayerProtoParser::Layer::to_string() const { std::string result; - StringAppendF(&result, "+ %s (%s)\n", type.c_str(), name.c_str()); + StringAppendF(&result, "+ %s (%s) uid=%d\n", type.c_str(), name.c_str(), ownerUid); result.append(transparentRegion.to_string("TransparentRegion").c_str()); result.append(visibleRegion.to_string("VisibleRegion").c_str()); result.append(damageRegion.to_string("SurfaceDamageRegion").c_str()); diff --git a/services/surfaceflinger/layerproto/include/layerproto/LayerProtoParser.h b/services/surfaceflinger/layerproto/include/layerproto/LayerProtoParser.h index 52b916555f..c48354fe95 100644 --- a/services/surfaceflinger/layerproto/include/layerproto/LayerProtoParser.h +++ b/services/surfaceflinger/layerproto/include/layerproto/LayerProtoParser.h @@ -114,6 +114,7 @@ public: LayerMetadata metadata; LayerProtoParser::FloatRect cornerRadiusCrop; float shadowRadius; + uid_t ownerUid; std::string to_string() const; }; diff --git a/services/surfaceflinger/layerproto/layers.proto b/services/surfaceflinger/layerproto/layers.proto index 8458d54356..41d6d08e11 100644 --- a/services/surfaceflinger/layerproto/layers.proto +++ b/services/surfaceflinger/layerproto/layers.proto @@ -123,6 +123,8 @@ message LayerProto { bool is_relative_of = 51; // Layer's background blur radius in pixels. int32 background_blur_radius = 52; + + uint32 owner_uid = 53; } message PositionProto { diff --git a/services/surfaceflinger/tests/LayerTransaction_test.cpp b/services/surfaceflinger/tests/LayerTransaction_test.cpp index 0ef4150bfe..8d715e1597 100644 --- a/services/surfaceflinger/tests/LayerTransaction_test.cpp +++ b/services/surfaceflinger/tests/LayerTransaction_test.cpp @@ -18,7 +18,6 @@ #pragma clang diagnostic push #pragma clang diagnostic ignored "-Wconversion" -#include <private/android_filesystem_config.h> #include <thread> #include "LayerTransactionTest.h" @@ -26,40 +25,6 @@ namespace android { using android::hardware::graphics::common::V1_1::BufferUsage; -TEST_F(LayerTransactionTest, SetFlagsSecureEUidSystem) { - sp<SurfaceControl> layer; - ASSERT_NO_FATAL_FAILURE(layer = createLayer("test", 32, 32)); - ASSERT_NO_FATAL_FAILURE(fillBufferQueueLayerColor(layer, Color::RED, 32, 32)); - - sp<ISurfaceComposer> composer = ComposerService::getComposerService(); - Transaction() - .setFlags(layer, layer_state_t::eLayerSecure, layer_state_t::eLayerSecure) - .apply(true); - ASSERT_EQ(PERMISSION_DENIED, composer->captureDisplay(mCaptureArgs, mCaptureResults)); - - UIDFaker f(AID_SYSTEM); - - // By default the system can capture screenshots with secure layers but they - // will be blacked out - ASSERT_EQ(NO_ERROR, composer->captureDisplay(mCaptureArgs, mCaptureResults)); - - { - SCOPED_TRACE("as system"); - auto shot = screenshot(); - shot->expectColor(Rect(0, 0, 32, 32), Color::BLACK); - } - - // Here we pass captureSecureLayers = true and since we are AID_SYSTEM we should be able - // to receive them...we are expected to take care with the results. - DisplayCaptureArgs args; - args.displayToken = mDisplay; - args.captureSecureLayers = true; - ASSERT_EQ(NO_ERROR, composer->captureDisplay(args, mCaptureResults)); - ASSERT_EQ(true, mCaptureResults.capturedSecureLayers); - ScreenCapture sc(mCaptureResults.buffer); - sc.expectColor(Rect(0, 0, 32, 32), Color::RED); -} - TEST_F(LayerTransactionTest, SetTransformToDisplayInverse_BufferState) { sp<SurfaceControl> layer; ASSERT_NO_FATAL_FAILURE( diff --git a/services/surfaceflinger/tests/ScreenCapture_test.cpp b/services/surfaceflinger/tests/ScreenCapture_test.cpp index 149e4d7f78..690f758c15 100644 --- a/services/surfaceflinger/tests/ScreenCapture_test.cpp +++ b/services/surfaceflinger/tests/ScreenCapture_test.cpp @@ -74,6 +74,42 @@ protected: std::unique_ptr<ScreenCapture> mCapture; }; +TEST_F(ScreenCaptureTest, SetFlagsSecureEUidSystem) { + sp<SurfaceControl> layer; + ASSERT_NO_FATAL_FAILURE( + layer = createLayer("test", 32, 32, + ISurfaceComposerClient::eSecure | + ISurfaceComposerClient::eFXSurfaceBufferQueue)); + ASSERT_NO_FATAL_FAILURE(fillBufferQueueLayerColor(layer, Color::RED, 32, 32)); + + Transaction().show(layer).setLayer(layer, INT32_MAX).apply(true); + + sp<ISurfaceComposer> composer = ComposerService::getComposerService(); + ASSERT_EQ(PERMISSION_DENIED, composer->captureDisplay(mCaptureArgs, mCaptureResults)); + + UIDFaker f(AID_SYSTEM); + + // By default the system can capture screenshots with secure layers but they + // will be blacked out + ASSERT_EQ(NO_ERROR, composer->captureDisplay(mCaptureArgs, mCaptureResults)); + + { + SCOPED_TRACE("as system"); + auto shot = screenshot(); + shot->expectColor(Rect(0, 0, 32, 32), Color::BLACK); + } + + // Here we pass captureSecureLayers = true and since we are AID_SYSTEM we should be able + // to receive them...we are expected to take care with the results. + DisplayCaptureArgs args; + args.displayToken = mDisplay; + args.captureSecureLayers = true; + ASSERT_EQ(NO_ERROR, composer->captureDisplay(args, mCaptureResults)); + ASSERT_TRUE(mCaptureResults.capturedSecureLayers); + ScreenCapture sc(mCaptureResults.buffer); + sc.expectColor(Rect(0, 0, 32, 32), Color::RED); +} + TEST_F(ScreenCaptureTest, CaptureSingleLayer) { LayerCaptureArgs captureArgs; captureArgs.layerHandle = mBGSurfaceControl->getHandle(); @@ -494,6 +530,128 @@ TEST_F(ScreenCaptureTest, CaputureSecureLayer) { mCapture->expectColor(Rect(30, 30, 60, 60), Color::RED); } +TEST_F(ScreenCaptureTest, CaptureDisplayWithUid) { + uid_t fakeUid = 12345; + + DisplayCaptureArgs captureArgs; + captureArgs.displayToken = mDisplay; + + sp<SurfaceControl> layer; + ASSERT_NO_FATAL_FAILURE(layer = createLayer("test layer", 32, 32, + ISurfaceComposerClient::eFXSurfaceBufferQueue, + mBGSurfaceControl.get())); + ASSERT_NO_FATAL_FAILURE(fillBufferQueueLayerColor(layer, Color::RED, 32, 32)); + + Transaction().show(layer).setLayer(layer, INT32_MAX).apply(); + + // Make sure red layer with the background layer is screenshot. + ScreenCapture::captureDisplay(&mCapture, captureArgs); + mCapture->expectColor(Rect(0, 0, 32, 32), Color::RED); + mCapture->expectBorder(Rect(0, 0, 32, 32), {63, 63, 195, 255}); + + // From non system uid, can't request screenshot without a specified uid. + UIDFaker f(fakeUid); + sp<ISurfaceComposer> composer = ComposerService::getComposerService(); + ASSERT_EQ(PERMISSION_DENIED, composer->captureDisplay(captureArgs, mCaptureResults)); + + // Make screenshot request with current uid set. No layers were created with the current + // uid so screenshot will be black. + captureArgs.uid = fakeUid; + ScreenCapture::captureDisplay(&mCapture, captureArgs); + mCapture->expectColor(Rect(0, 0, 32, 32), Color::BLACK); + mCapture->expectBorder(Rect(0, 0, 32, 32), Color::BLACK); + + sp<SurfaceControl> layerWithFakeUid; + // Create a new layer with the current uid + ASSERT_NO_FATAL_FAILURE(layerWithFakeUid = + createLayer("new test layer", 32, 32, + ISurfaceComposerClient::eFXSurfaceBufferQueue, + mBGSurfaceControl.get())); + ASSERT_NO_FATAL_FAILURE(fillBufferQueueLayerColor(layerWithFakeUid, Color::GREEN, 32, 32)); + Transaction() + .show(layerWithFakeUid) + .setLayer(layerWithFakeUid, INT32_MAX) + .setPosition(layerWithFakeUid, 128, 128) + .apply(); + + // Screenshot from the fakeUid caller with the uid requested allows the layer + // with that uid to be screenshotted. Everything else is black + ScreenCapture::captureDisplay(&mCapture, captureArgs); + mCapture->expectColor(Rect(128, 128, 160, 160), Color::GREEN); + mCapture->expectBorder(Rect(128, 128, 160, 160), Color::BLACK); +} + +TEST_F(ScreenCaptureTest, CaptureLayerWithUid) { + uid_t fakeUid = 12345; + + sp<SurfaceControl> layer; + ASSERT_NO_FATAL_FAILURE(layer = createLayer("test layer", 32, 32, + ISurfaceComposerClient::eFXSurfaceBufferQueue, + mBGSurfaceControl.get())); + ASSERT_NO_FATAL_FAILURE(fillBufferQueueLayerColor(layer, Color::RED, 32, 32)); + + Transaction().show(layer).setLayer(layer, INT32_MAX).apply(); + + LayerCaptureArgs captureArgs; + captureArgs.layerHandle = mBGSurfaceControl->getHandle(); + captureArgs.childrenOnly = false; + + // Make sure red layer with the background layer is screenshot. + ScreenCapture::captureLayers(&mCapture, captureArgs); + mCapture->expectColor(Rect(0, 0, 32, 32), Color::RED); + mCapture->expectBorder(Rect(0, 0, 32, 32), {63, 63, 195, 255}); + + // From non system uid, can't request screenshot without a specified uid. + std::unique_ptr<UIDFaker> uidFaker = std::make_unique<UIDFaker>(fakeUid); + + sp<ISurfaceComposer> composer = ComposerService::getComposerService(); + ASSERT_EQ(PERMISSION_DENIED, composer->captureLayers(captureArgs, mCaptureResults)); + + // Make screenshot request with current uid set. No layers were created with the current + // uid so screenshot will be black. + captureArgs.uid = fakeUid; + ScreenCapture::captureLayers(&mCapture, captureArgs); + mCapture->expectColor(Rect(0, 0, 32, 32), Color::TRANSPARENT); + mCapture->expectBorder(Rect(0, 0, 32, 32), Color::TRANSPARENT); + + sp<SurfaceControl> layerWithFakeUid; + // Create a new layer with the current uid + ASSERT_NO_FATAL_FAILURE(layerWithFakeUid = + createLayer("new test layer", 32, 32, + ISurfaceComposerClient::eFXSurfaceBufferQueue, + mBGSurfaceControl.get())); + ASSERT_NO_FATAL_FAILURE(fillBufferQueueLayerColor(layerWithFakeUid, Color::GREEN, 32, 32)); + Transaction() + .show(layerWithFakeUid) + .setLayer(layerWithFakeUid, INT32_MAX) + .setPosition(layerWithFakeUid, 128, 128) + // reparent a layer that was created with a different uid to the new layer. + .reparent(layer, layerWithFakeUid->getHandle()) + .apply(); + + // Screenshot from the fakeUid caller with the uid requested allows the layer + // with that uid to be screenshotted. The child layer is skipped since it was created + // from a different uid. + ScreenCapture::captureLayers(&mCapture, captureArgs); + mCapture->expectColor(Rect(128, 128, 160, 160), Color::GREEN); + mCapture->expectBorder(Rect(128, 128, 160, 160), Color::TRANSPARENT); + + // Clear fake calling uid so it's back to system. + uidFaker = nullptr; + // Screenshot from the test caller with the uid requested allows the layer + // with that uid to be screenshotted. The child layer is skipped since it was created + // from a different uid. + ScreenCapture::captureLayers(&mCapture, captureArgs); + mCapture->expectColor(Rect(128, 128, 160, 160), Color::GREEN); + mCapture->expectBorder(Rect(128, 128, 160, 160), Color::TRANSPARENT); + + // Screenshot from the fakeUid caller with no uid requested allows everything to be screenshot. + captureArgs.uid = -1; + ScreenCapture::captureLayers(&mCapture, captureArgs); + mCapture->expectColor(Rect(128, 128, 160, 160), Color::RED); + mCapture->expectBorder(Rect(128, 128, 160, 160), {63, 63, 195, 255}); +} + // In the following tests we verify successful skipping of a parent layer, // so we use the same verification logic and only change how we mutate // the parent layer to verify that various properties are ignored. diff --git a/services/surfaceflinger/tests/unittests/CompositionTest.cpp b/services/surfaceflinger/tests/unittests/CompositionTest.cpp index 6472428c55..4843f059dc 100644 --- a/services/surfaceflinger/tests/unittests/CompositionTest.cpp +++ b/services/surfaceflinger/tests/unittests/CompositionTest.cpp @@ -240,7 +240,8 @@ void CompositionTest::captureScreenComposition() { ui::Dataspace::V0_SRGB, ui::Transform::ROT_0); auto traverseLayers = [this](const LayerVector::Visitor& visitor) { - return mFlinger.traverseLayersInLayerStack(mDisplay->getLayerStack(), visitor); + return mFlinger.traverseLayersInLayerStack(mDisplay->getLayerStack(), + CaptureArgs::UNSET_UID, visitor); }; // TODO: Eliminate expensive/real allocation if possible. diff --git a/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h b/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h index 345577d387..b349144729 100644 --- a/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h +++ b/services/surfaceflinger/tests/unittests/TestableSurfaceFlinger.h @@ -342,9 +342,9 @@ public: outSyncFd, regionSampling, captureResults); } - auto traverseLayersInLayerStack(ui::LayerStack layerStack, + auto traverseLayersInLayerStack(ui::LayerStack layerStack, int32_t uid, const LayerVector::Visitor& visitor) { - return mFlinger->SurfaceFlinger::traverseLayersInLayerStack(layerStack, visitor); + return mFlinger->SurfaceFlinger::traverseLayersInLayerStack(layerStack, uid, visitor); } auto getDisplayNativePrimaries(const sp<IBinder>& displayToken, diff --git a/services/surfaceflinger/tests/utils/ScreenshotUtils.h b/services/surfaceflinger/tests/utils/ScreenshotUtils.h index 56628f88bc..081d18b6b8 100644 --- a/services/surfaceflinger/tests/utils/ScreenshotUtils.h +++ b/services/surfaceflinger/tests/utils/ScreenshotUtils.h @@ -31,18 +31,22 @@ public: captureScreen(sc, SurfaceComposerClient::getInternalDisplayToken()); } - static void captureScreen(std::unique_ptr<ScreenCapture>* sc, sp<IBinder> displayToken) { + static void captureDisplay(std::unique_ptr<ScreenCapture>* sc, + const DisplayCaptureArgs& captureArgs) { const auto sf = ComposerService::getComposerService(); SurfaceComposerClient::Transaction().apply(true); - DisplayCaptureArgs args; - args.displayToken = displayToken; - ScreenCaptureResults captureResults; - ASSERT_EQ(NO_ERROR, sf->captureDisplay(args, captureResults)); + ASSERT_EQ(NO_ERROR, sf->captureDisplay(captureArgs, captureResults)); *sc = std::make_unique<ScreenCapture>(captureResults.buffer); } + static void captureScreen(std::unique_ptr<ScreenCapture>* sc, sp<IBinder> displayToken) { + DisplayCaptureArgs args; + args.displayToken = displayToken; + captureDisplay(sc, args); + } + static void captureLayers(std::unique_ptr<ScreenCapture>* sc, const LayerCaptureArgs& captureArgs) { sp<ISurfaceComposer> sf(ComposerService::getComposerService()); diff --git a/services/vibratorservice/VibratorHalController.cpp b/services/vibratorservice/VibratorHalController.cpp index 03e51aea7c..a9da74fea3 100644 --- a/services/vibratorservice/VibratorHalController.cpp +++ b/services/vibratorservice/VibratorHalController.cpp @@ -91,7 +91,7 @@ std::shared_ptr<HalWrapper> HalConnector::connect(std::shared_ptr<CallbackSchedu template <typename T> HalResult<T> HalController::processHalResult(HalResult<T> result, const char* functionName) { if (result.isFailed()) { - ALOGE("%s failed: Vibrator HAL not available", functionName); + ALOGE("%s failed: %s", functionName, result.errorMessage()); std::lock_guard<std::mutex> lock(mConnectedHalMutex); mConnectedHal->tryReconnect(); } diff --git a/services/vibratorservice/VibratorHalWrapper.cpp b/services/vibratorservice/VibratorHalWrapper.cpp index 1210110e83..ee891def11 100644 --- a/services/vibratorservice/VibratorHalWrapper.cpp +++ b/services/vibratorservice/VibratorHalWrapper.cpp @@ -67,6 +67,10 @@ bool isStaticCastValid(Effect effect) { // ------------------------------------------------------------------------------------------------- +const constexpr char* STATUS_T_ERROR_MESSAGE_PREFIX = "status_t = "; +const constexpr char* STATUS_V_1_0_ERROR_MESSAGE_PREFIX = + "android::hardware::vibrator::V1_0::Status = "; + template <typename T> HalResult<T> HalResult<T>::fromStatus(binder::Status status, T data) { if (status.exceptionCode() == binder::Status::EX_UNSUPPORTED_OPERATION) { @@ -75,7 +79,7 @@ HalResult<T> HalResult<T>::fromStatus(binder::Status status, T data) { if (status.isOk()) { return HalResult<T>::ok(data); } - return HalResult<T>::failed(); + return HalResult<T>::failed(std::string(status.toString8().c_str())); } template <typename T> @@ -86,24 +90,32 @@ HalResult<T> HalResult<T>::fromStatus(V1_0::Status status, T data) { case V1_0::Status::UNSUPPORTED_OPERATION: return HalResult<T>::unsupported(); default: - return HalResult<T>::failed(); + return HalResult<T>::failed(STATUS_V_1_0_ERROR_MESSAGE_PREFIX + toString(status)); } } template <typename T> template <typename R> HalResult<T> HalResult<T>::fromReturn(hardware::Return<R>& ret, T data) { - return ret.isOk() ? HalResult<T>::ok(data) : HalResult<T>::failed(); + return ret.isOk() ? HalResult<T>::ok(data) : HalResult<T>::failed(ret.description()); } template <typename T> template <typename R> HalResult<T> HalResult<T>::fromReturn(hardware::Return<R>& ret, V1_0::Status status, T data) { - return ret.isOk() ? HalResult<T>::fromStatus(status, data) : HalResult<T>::failed(); + return ret.isOk() ? HalResult<T>::fromStatus(status, data) + : HalResult<T>::failed(ret.description()); } // ------------------------------------------------------------------------------------------------- +HalResult<void> HalResult<void>::fromStatus(status_t status) { + if (status == android::OK) { + return HalResult<void>::ok(); + } + return HalResult<void>::failed(STATUS_T_ERROR_MESSAGE_PREFIX + statusToString(status)); +} + HalResult<void> HalResult<void>::fromStatus(binder::Status status) { if (status.exceptionCode() == binder::Status::EX_UNSUPPORTED_OPERATION) { return HalResult<void>::unsupported(); @@ -111,7 +123,7 @@ HalResult<void> HalResult<void>::fromStatus(binder::Status status) { if (status.isOk()) { return HalResult<void>::ok(); } - return HalResult<void>::failed(); + return HalResult<void>::failed(std::string(status.toString8().c_str())); } HalResult<void> HalResult<void>::fromStatus(V1_0::Status status) { @@ -121,13 +133,13 @@ HalResult<void> HalResult<void>::fromStatus(V1_0::Status status) { case V1_0::Status::UNSUPPORTED_OPERATION: return HalResult<void>::unsupported(); default: - return HalResult<void>::failed(); + return HalResult<void>::failed(STATUS_V_1_0_ERROR_MESSAGE_PREFIX + toString(status)); } } template <typename R> HalResult<void> HalResult<void>::fromReturn(hardware::Return<R>& ret) { - return ret.isOk() ? HalResult<void>::ok() : HalResult<void>::failed(); + return ret.isOk() ? HalResult<void>::ok() : HalResult<void>::failed(ret.description()); } // ------------------------------------------------------------------------------------------------- @@ -149,8 +161,7 @@ private: // ------------------------------------------------------------------------------------------------- HalResult<void> AidlHalWrapper::ping() { - // TODO(b/153415251): Investigate why IBinder::pingBinder() is returning false even on success. - return getCapabilitiesInternal().isFailed() ? HalResult<void>::failed() : HalResult<void>::ok(); + return HalResult<void>::fromStatus(IInterface::asBinder(getHal())->pingBinder()); } void AidlHalWrapper::tryReconnect() { @@ -456,15 +467,15 @@ HalResult<milliseconds> HidlHalWrapperV1_3::performEffect( } HalResult<Capabilities> HidlHalWrapperV1_3::getCapabilitiesInternal() { + Capabilities capabilities = Capabilities::NONE; + sp<V1_3::IVibrator> hal = getHal(); auto amplitudeResult = hal->supportsAmplitudeControl(); if (!amplitudeResult.isOk()) { - return HalResult<Capabilities>::failed(); + return HalResult<Capabilities>::fromReturn(amplitudeResult, capabilities); } auto externalControlResult = hal->supportsExternalControl(); - Capabilities capabilities = Capabilities::NONE; - if (amplitudeResult.withDefault(false)) { capabilities |= Capabilities::AMPLITUDE_CONTROL; } diff --git a/services/vibratorservice/include/vibratorservice/VibratorHalWrapper.h b/services/vibratorservice/include/vibratorservice/VibratorHalWrapper.h index a4fa8693fe..6e36bd6350 100644 --- a/services/vibratorservice/include/vibratorservice/VibratorHalWrapper.h +++ b/services/vibratorservice/include/vibratorservice/VibratorHalWrapper.h @@ -35,8 +35,10 @@ template <typename T> class HalResult { public: static HalResult<T> ok(T value) { return HalResult(value); } - static HalResult<T> failed() { return HalResult(/* unsupported= */ false); } - static HalResult<T> unsupported() { return HalResult(/* unsupported= */ true); } + static HalResult<T> failed(std::string msg) { + return HalResult(std::move(msg), /* unsupported= */ false); + } + static HalResult<T> unsupported() { return HalResult("", /* unsupported= */ true); } static HalResult<T> fromStatus(binder::Status status, T data); static HalResult<T> fromStatus(hardware::vibrator::V1_0::Status status, T data); @@ -53,13 +55,17 @@ public: bool isOk() const { return !mUnsupported && mValue.has_value(); } bool isFailed() const { return !mUnsupported && !mValue.has_value(); } bool isUnsupported() const { return mUnsupported; } + const char* errorMessage() const { return mErrorMessage.c_str(); } private: std::optional<T> mValue; + std::string mErrorMessage; bool mUnsupported; - explicit HalResult(T value) : mValue(std::make_optional(value)), mUnsupported(false) {} - explicit HalResult(bool unsupported) : mValue(), mUnsupported(unsupported) {} + explicit HalResult(T value) + : mValue(std::make_optional(value)), mErrorMessage(), mUnsupported(false) {} + explicit HalResult(std::string errorMessage, bool unsupported) + : mValue(), mErrorMessage(std::move(errorMessage)), mUnsupported(unsupported) {} }; // Empty result of a call to the Vibrator HAL wrapper. @@ -67,11 +73,10 @@ template <> class HalResult<void> { public: static HalResult<void> ok() { return HalResult(); } - static HalResult<void> failed() { return HalResult(/* failed= */ true); } - static HalResult<void> unsupported() { - return HalResult(/* failed= */ false, /* unsupported= */ true); - } + static HalResult<void> failed(std::string msg) { return HalResult(std::move(msg)); } + static HalResult<void> unsupported() { return HalResult(/* unsupported= */ true); } + static HalResult<void> fromStatus(status_t status); static HalResult<void> fromStatus(binder::Status status); static HalResult<void> fromStatus(hardware::vibrator::V1_0::Status status); @@ -81,13 +86,17 @@ public: bool isOk() const { return !mUnsupported && !mFailed; } bool isFailed() const { return !mUnsupported && mFailed; } bool isUnsupported() const { return mUnsupported; } + const char* errorMessage() const { return mErrorMessage.c_str(); } private: + std::string mErrorMessage; bool mFailed; bool mUnsupported; - explicit HalResult(bool failed = false, bool unsupported = false) - : mFailed(failed), mUnsupported(unsupported) {} + explicit HalResult(bool unsupported = false) + : mErrorMessage(), mFailed(false), mUnsupported(unsupported) {} + explicit HalResult(std::string errorMessage) + : mErrorMessage(std::move(errorMessage)), mFailed(true), mUnsupported(false) {} }; // ------------------------------------------------------------------------------------------------- diff --git a/services/vibratorservice/test/VibratorHalControllerTest.cpp b/services/vibratorservice/test/VibratorHalControllerTest.cpp index 2d5554941c..8155df0377 100644 --- a/services/vibratorservice/test/VibratorHalControllerTest.cpp +++ b/services/vibratorservice/test/VibratorHalControllerTest.cpp @@ -257,10 +257,10 @@ TEST_F(VibratorHalControllerTest, TestUnsupportedApiResultDoNotResetHalConnectio TEST_F(VibratorHalControllerTest, TestFailedApiResultResetsHalConnection) { setHalExpectations(MAX_ATTEMPTS, std::vector<CompositeEffect>(), - vibrator::HalResult<void>::failed(), - vibrator::HalResult<vibrator::Capabilities>::failed(), - vibrator::HalResult<std::vector<Effect>>::failed(), - vibrator::HalResult<milliseconds>::failed()); + vibrator::HalResult<void>::failed("message"), + vibrator::HalResult<vibrator::Capabilities>::failed("message"), + vibrator::HalResult<std::vector<Effect>>::failed("message"), + vibrator::HalResult<milliseconds>::failed("message")); ASSERT_EQ(0, mConnectCounter); @@ -286,7 +286,7 @@ TEST_F(VibratorHalControllerTest, TestFailedApiResultReturnsSuccessAfterRetries) InSequence seq; EXPECT_CALL(*mMockHal.get(), ping()) .Times(Exactly(1)) - .WillRepeatedly(Return(vibrator::HalResult<void>::failed())); + .WillRepeatedly(Return(vibrator::HalResult<void>::failed("message"))); EXPECT_CALL(*mMockHal.get(), tryReconnect()).Times(Exactly(1)); EXPECT_CALL(*mMockHal.get(), ping()) .Times(Exactly(1)) @@ -351,11 +351,11 @@ TEST_F(VibratorHalControllerTest, TestScheduledCallbackSurvivesReconnection) { }); EXPECT_CALL(*mMockHal.get(), ping()) .Times(Exactly(1)) - .WillRepeatedly(Return(vibrator::HalResult<void>::failed())); + .WillRepeatedly(Return(vibrator::HalResult<void>::failed("message"))); EXPECT_CALL(*mMockHal.get(), tryReconnect()).Times(Exactly(1)); EXPECT_CALL(*mMockHal.get(), ping()) .Times(Exactly(1)) - .WillRepeatedly(Return(vibrator::HalResult<void>::failed())); + .WillRepeatedly(Return(vibrator::HalResult<void>::failed("message"))); EXPECT_CALL(*mMockHal.get(), tryReconnect()).Times(Exactly(1)); } diff --git a/services/vibratorservice/test/VibratorHalWrapperAidlTest.cpp b/services/vibratorservice/test/VibratorHalWrapperAidlTest.cpp index ab5ab1dee2..3e06c95620 100644 --- a/services/vibratorservice/test/VibratorHalWrapperAidlTest.cpp +++ b/services/vibratorservice/test/VibratorHalWrapperAidlTest.cpp @@ -116,19 +116,16 @@ ACTION(TriggerCallbackInArg2) { } TEST_F(VibratorHalWrapperAidlTest, TestPing) { - { - InSequence seq; - EXPECT_CALL(*mMockHal.get(), getCapabilities(_)) - .Times(Exactly(3)) - .WillOnce(Return(Status::fromExceptionCode(Status::Exception::EX_SECURITY))) - .WillOnce(Return( - Status::fromExceptionCode(Status::Exception::EX_UNSUPPORTED_OPERATION))) - .WillRepeatedly(Return(Status())); - } + EXPECT_CALL(*mMockHal.get(), onAsBinder()) + .Times(Exactly(2)) + .WillRepeatedly(Return(mMockBinder.get())); + EXPECT_CALL(*mMockBinder.get(), pingBinder()) + .Times(Exactly(2)) + .WillOnce(Return(android::OK)) + .WillRepeatedly(Return(android::DEAD_OBJECT)); - ASSERT_TRUE(mWrapper->ping().isFailed()); - ASSERT_TRUE(mWrapper->ping().isOk()); ASSERT_TRUE(mWrapper->ping().isOk()); + ASSERT_TRUE(mWrapper->ping().isFailed()); } TEST_F(VibratorHalWrapperAidlTest, TestOnWithCallbackSupport) { |