status-im / status-desktop

Status Desktop client made in Nim & QML
https://status.app
Mozilla Public License 2.0
275 stars 76 forks source link

Make Nim models data() function safe #15324

Open endulab opened 3 weeks ago

endulab commented 3 weeks ago

During some testing there was a situation that Qml model (ObjectProxyModel) wanted to access a role in a Nim model which did not exist and the application crashed. After some investigations it turned out that data() function of the model returned nil instead of QVariant and the Qml could not handle it. The point of this task is to fix all data() functions in all Nim models. The default result value of this function should always be newQVariant().

endulab commented 3 weeks ago

FYI @micieslak

igor-sirotin commented 2 weeks ago

Might be possible to fix this on DOtherSide level. like If Nim returned null, return QVariant().

friofry commented 1 day ago

sequenceDiagram
    participant App
    participant Collectibles_model.nim
    participant qabstractitemmodel.nim
    participant qabstractlistmodel.nim
    participant Dotherside.hpp
    participant DosQAbstractItemModel.cpp

    Note right of App: Construct Model
    App->>Collectibles_model.nim: create model
    Collectibles_model.nim->>qabstractlistmodel.nim: Call dos_qabstractlistmodel_create
    qabstractlistmodel.nim->>Dotherside.hpp: Call dos_qabstractlistmodel_create
    Dotherside.hpp->>DosQAbstractItemModel.cpp: create ListModel
    DosQAbstractItemModel.cpp->>Dotherside.hpp: return ListModel
      Dotherside.hpp->>Collectibles_model.nim: Return model ptr
      Collectibles_model.nim->>App: Expose model to QML 

    Note right of App: QML reads Role from Model
    App->>DosQAbstractItemModel.cpp: Call data(index, role)
      DosQAbstractItemModel.cpp->>qabstractitemmodel.nim: Call dataCallback
    qabstractitemmodel.nim->>Collectibles_model.nim: Call data method
    Collectibles_model.nim->>qabstractitemmodel.nim: Return result
    qabstractitemmodel.nim->>Dotherside.hpp: dos_qvariant_assign
    qabstractitemmodel.nim->>DosQAbstractItemModel.cpp: Return QVariant
    DosQAbstractItemModel.cpp->>App: Return result to QML

When we access model data from QML


template<class T>
QVariant DosQAbstractGenericModel<T>::data(const QModelIndex &index, int role) const
{
    QVariant result;
    m_callbacks.data(m_modelObject, &index, role, &result);
    return result;
}

it calls qabstractitemmodel.nim

proc dataCallback(modelPtr: pointer, rawIndex: DosQModelIndex, role: cint, result: DosQVariant) {.cdecl, exportc.} =
  debugMsg("QAbstractItemModel", "dataCallback")
  let model = cast[QAbstractItemModel](modelPtr)
  let index = newQModelIndex(rawIndex, Ownership.Clone)
  let variant = data(model, index, role.int)
  if variant != nil:
    dos_qvariant_assign(result, variant.vptr)
    variant.delete

which calls collectibles_model.nim

I suspect the crash can happen in wrapper models like ObjectProxy, Renaming

QVariant ObjectProxyModel::data(const QModelIndex& index, int role) const
{
    if (!checkIndex(index, CheckIndexOption::IndexIsValid))
        return {};

    if (m_delegate && m_exposedRolesSet.contains(role)) {
        const auto proxy = this->proxyObject(index.row());
        return proxy->property(m_roleNames[role]);
    }
    return QIdentityProxyModel::data(index, role);
}

But I wasn't able to reproduce this by passing an invalid name to an expected role. because it is handled in objectproxymodel.cpp

                    ObjectProxyModel {
                        objectName: "Model2"

                      sourceModel: appMain.rootStore.globalCollectiblesModel

                      delegate: QtObject {
                          readonly property string key: model.symbol ?? ""
                          readonly property string shortName: model.collectionName ? model.collectionName : model.collectionUid ? model.collectionUid : ""
                          readonly property string symbol: shortName
                          readonly property string name: shortName
                          readonly property int category: 1 // Own
                          readonly property string xyz1: xyz
                      }

                      exposedRoles: ["key", "symbol", "shortName", "name", "category", "xyz1"]
                      expectedRoles: ["symbol", "collectionName", "collectionUid", "xyz"]
                  }