[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

@@ -41,9 +41,14 @@ DrmObject::~DrmObject()
         delete p;
 }
 
+void DrmObject::setPropertyNames(QVector<QByteArray> &&vector)
+{
+    m_propsNames = std::move(vector);
+    m_props.fill(nullptr, m_propsNames.size());
+}
+
 void DrmObject::initProp(int n, drmModeObjectProperties *properties, QVector<QByteArray> 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, QVector<QBy
     }
 }
 
-bool DrmObject::atomicAddProperty(drmModeAtomicReq *req, int prop, uint64_t value)
+bool DrmObject::atomicAddProperty(drmModeAtomicReq *req, Property *property)
 {
-    if (drmModeAtomicAddProperty(req, m_id, m_props[prop]->propId(), 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) {

plugins/platforms/drm/drm_object.h

@@ -54,37 +54,34 @@ public:
         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<QByteArray> &&vector);
     void initProp(int n, drmModeObjectProperties *properties, QVector<QByteArray> enumNames = QVector<QByteArray>(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<QByteArray> m_propsNames;
-    class Property;
     QVector<Property *> 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<uint64_t> m_enumMap;
         QVector<QByteArray> m_enumNames;
     };
+
+private:
+    QVector<QByteArray> m_propsNames;
 };
 
 

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) {

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()) {

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<QByteArray> 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) {

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;