From c601e875cfa7d673f1567c71087036631e42d272 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Fl=C3=B6ser?= Date: Fri, 10 Nov 2017 20:38:33 +0100 Subject: [PATCH] [platforms/drm] At safety checks for the properties Summary: The AMS code accesses elements in a vector which might not be valid. This change refactors the code to be more robust, especially the DrmPlane, which started to crash after adding transformation support. BUG: 386490 Reviewers: #kwin, #plasma, fvogt, subdiff Subscribers: plasma-devel, kwin Tags: #kwin Differential Revision: https://phabricator.kde.org/D8752 --- plugins/platforms/drm/drm_object.cpp | 19 ++++++++--- plugins/platforms/drm/drm_object.h | 31 +++++++++-------- .../platforms/drm/drm_object_connector.cpp | 4 +-- plugins/platforms/drm/drm_object_crtc.cpp | 7 ++-- plugins/platforms/drm/drm_object_plane.cpp | 33 ++++++++++++++----- plugins/platforms/drm/egl_gbm_backend.cpp | 3 ++ 6 files changed, 65 insertions(+), 32 deletions(-) diff --git a/plugins/platforms/drm/drm_object.cpp b/plugins/platforms/drm/drm_object.cpp index 79a473caa0..48d297fa9e 100644 --- a/plugins/platforms/drm/drm_object.cpp +++ b/plugins/platforms/drm/drm_object.cpp @@ -41,9 +41,14 @@ DrmObject::~DrmObject() delete p; } +void DrmObject::setPropertyNames(QVector &&vector) +{ + m_propsNames = std::move(vector); + m_props.fill(nullptr, m_propsNames.size()); +} + void DrmObject::initProp(int n, drmModeObjectProperties *properties, QVector enumNames) { - m_props.resize(m_propsNames.size()); for (unsigned int i = 0; i < properties->count_props; ++i) { drmModePropertyRes *prop = drmModeGetProperty(m_backend->fd(), properties->props[i]); if (!prop) { @@ -58,10 +63,10 @@ void DrmObject::initProp(int n, drmModeObjectProperties *properties, QVectorpropId(), value) <= 0) { - qCWarning(KWIN_DRM) << "Adding property" << m_propsNames[prop] << "to atomic commit failed for object" << this; + if (drmModeAtomicAddProperty(req, m_id, property->propId(), property->value()) <= 0) { + qCWarning(KWIN_DRM) << "Adding property" << property->name() << "to atomic commit failed for object" << this; return false; } return true; @@ -72,7 +77,11 @@ bool DrmObject::atomicPopulate(drmModeAtomicReq *req) bool ret = true; for (int i = 0; i < m_props.size(); i++) { - ret &= atomicAddProperty(req, i, m_props[i]->value()); + auto property = m_props.at(i); + if (!property) { + continue; + } + ret &= atomicAddProperty(req, property); } if (!ret) { diff --git a/plugins/platforms/drm/drm_object.h b/plugins/platforms/drm/drm_object.h index 49ab7207fb..50ec56f347 100644 --- a/plugins/platforms/drm/drm_object.h +++ b/plugins/platforms/drm/drm_object.h @@ -53,38 +53,35 @@ public: void setOutput(DrmOutput* output) { m_output = output; } - - uint32_t propId(int prop) { - return m_props[prop]->propId(); - } - uint64_t value(int prop) { - return m_props[prop]->value(); - } bool propHasEnum(int prop, uint64_t value) const { - return m_props[prop]->hasEnum(value); + auto property = m_props.at(prop); + return property ? property->hasEnum(value) : false; } void setValue(int prop, uint64_t new_value) { Q_ASSERT(prop < m_props.size()); - m_props[prop]->setValue(new_value); + auto property = m_props.at(prop); + if (property) { + property->setValue(new_value); + } } virtual bool atomicPopulate(drmModeAtomicReq *req); protected: virtual bool initProps() = 0; // only derived classes know names and quantity of properties + void setPropertyNames(QVector &&vector); void initProp(int n, drmModeObjectProperties *properties, QVector enumNames = QVector(0)); - bool atomicAddProperty(drmModeAtomicReq *req, int prop, uint64_t value); + class Property; + bool atomicAddProperty(drmModeAtomicReq *req, Property *property); DrmBackend *m_backend; const uint32_t m_id = 0; DrmOutput *m_output = nullptr; // for comparision with received name of DRM object - QVector m_propsNames; - class Property; QVector m_props; class Property @@ -102,15 +99,18 @@ protected: return m_enumMap.contains(value); } - uint32_t propId() { + uint32_t propId() const { return m_propId; } - uint64_t value() { + uint64_t value() const { return m_value; } void setValue(uint64_t new_value) { m_value = new_value; } + const QByteArray &name() const { + return m_propName; + } private: uint32_t m_propId = 0; @@ -120,6 +120,9 @@ protected: QVector m_enumMap; QVector m_enumNames; }; + +private: + QVector m_propsNames; }; diff --git a/plugins/platforms/drm/drm_object_connector.cpp b/plugins/platforms/drm/drm_object_connector.cpp index 444ce4eefd..f671f0c3c3 100644 --- a/plugins/platforms/drm/drm_object_connector.cpp +++ b/plugins/platforms/drm/drm_object_connector.cpp @@ -51,9 +51,9 @@ bool DrmConnector::atomicInit() bool DrmConnector::initProps() { - m_propsNames = { + setPropertyNames( { QByteArrayLiteral("CRTC_ID"), - }; + }); drmModeObjectProperties *properties = drmModeObjectGetProperties(m_backend->fd(), m_id, DRM_MODE_OBJECT_CONNECTOR); if (!properties) { diff --git a/plugins/platforms/drm/drm_object_crtc.cpp b/plugins/platforms/drm/drm_object_crtc.cpp index 20b232ef86..d44ca787b2 100644 --- a/plugins/platforms/drm/drm_object_crtc.cpp +++ b/plugins/platforms/drm/drm_object_crtc.cpp @@ -48,10 +48,10 @@ bool DrmCrtc::atomicInit() bool DrmCrtc::initProps() { - m_propsNames = { + setPropertyNames({ QByteArrayLiteral("MODE_ID"), QByteArrayLiteral("ACTIVE"), - }; + }); drmModeObjectProperties *properties = drmModeObjectGetProperties(m_backend->fd(), m_id, DRM_MODE_OBJECT_CRTC); if (!properties) { @@ -81,6 +81,9 @@ void DrmCrtc::flipBuffer() bool DrmCrtc::blank() { + if (!m_output) { + return false; + } if (!m_blackBuffer) { DrmDumbBuffer *blackBuffer = m_backend->createBuffer(m_output->pixelSize()); if (!blackBuffer->map()) { diff --git a/plugins/platforms/drm/drm_object_plane.cpp b/plugins/platforms/drm/drm_object_plane.cpp index 6899769709..f9efc6161e 100644 --- a/plugins/platforms/drm/drm_object_plane.cpp +++ b/plugins/platforms/drm/drm_object_plane.cpp @@ -63,7 +63,7 @@ bool DrmPlane::atomicInit() bool DrmPlane::initProps() { - m_propsNames = { + setPropertyNames( { QByteArrayLiteral("type"), QByteArrayLiteral("SRC_X"), QByteArrayLiteral("SRC_Y"), @@ -76,7 +76,7 @@ bool DrmPlane::initProps() QByteArrayLiteral("FB_ID"), QByteArrayLiteral("CRTC_ID"), QByteArrayLiteral("rotation") - }; + }); QVector typeNames = { QByteArrayLiteral("Primary"), @@ -129,29 +129,40 @@ bool DrmPlane::initProps() DrmPlane::TypeIndex DrmPlane::type() { - uint64_t v = value(int(PropertyIndex::Type)); + auto property = m_props.at(int(PropertyIndex::Type)); + if (!property) { + return TypeIndex::Overlay; + } int typeCount = int(TypeIndex::Count); for (int i = 0; i < typeCount; i++) { - if (m_props[int(PropertyIndex::Type)]->enumMap(i) == v) { + if (property->enumMap(i) == property->value()) { return TypeIndex(i); } } return TypeIndex::Overlay; } -void DrmPlane::setNext(DrmBuffer *b){ - setValue(int(PropertyIndex::FbId), b ? b->bufferId() : 0); +void DrmPlane::setNext(DrmBuffer *b) +{ + if (auto property = m_props.at(int(PropertyIndex::FbId))) { + property->setValue(b ? b->bufferId() : 0); + } m_next = b; } void DrmPlane::setTransformation(Transformations t) { - setValue(int(PropertyIndex::Rotation), int(t)); + if (auto property = m_props.at(int(PropertyIndex::Rotation))) { + property->setValue(int(t)); + } } DrmPlane::Transformations DrmPlane::transformation() { - return Transformations(int(value(int(PropertyIndex::Rotation)))); + if (auto property = m_props.at(int(PropertyIndex::Rotation))) { + return Transformations(int(property->value())); + } + return Transformations(Transformation::Rotate0); } bool DrmPlane::atomicPopulate(drmModeAtomicReq *req) @@ -159,7 +170,11 @@ bool DrmPlane::atomicPopulate(drmModeAtomicReq *req) bool ret = true; for (int i = 1; i < m_props.size(); i++) { - ret &= atomicAddProperty(req, i, m_props[i]->value()); + auto property = m_props.at(i); + if (!property) { + continue; + } + ret &= atomicAddProperty(req, property); } if (!ret) { diff --git a/plugins/platforms/drm/egl_gbm_backend.cpp b/plugins/platforms/drm/egl_gbm_backend.cpp index 51861f3101..092cee3774 100644 --- a/plugins/platforms/drm/egl_gbm_backend.cpp +++ b/plugins/platforms/drm/egl_gbm_backend.cpp @@ -168,6 +168,9 @@ bool EglGbmBackend::resetOutput(Output &o, DrmOutput *drmOutput) } else { // destroy previous surface if (o.eglSurface != EGL_NO_SURFACE) { + if (surface() == o.eglSurface) { + setSurface(eglSurface); + } eglDestroySurface(eglDisplay(), o.eglSurface); } o.eglSurface = eglSurface;