Crash when using KIdentityManagement::IdentityManager

[David Faure]

On mercredi 14 avril 2021 19:19:57 CEST Volker Krause wrote:
> On Mittwoch, 14. April 2021 19:04:46 CEST David Faure wrote:
> > On mardi 13 avril 2021 20:21:21 CEST Volker Krause wrote:
> > > On Dienstag, 13. April 2021 17:47:47 CEST Carl Schwan wrote:
> > > > Hello folks,
> > > > 
> > > > I have been trying to implement a mail client based on Akonadi
> > > > for the PinePhone. I'm currently getting blocked by a weird
> > > > crash in KIdentityManagement::IdentityManager.
> > > > 
> > > > The code can be found here:
> > > > https://invent.kde.org/carlschwan/quickmail/-/blob/master/src/main.cpp
> > > > #L
> > > > 46
> > > > 
> > > > valgrind/gdb tells me that the d pointer of the IdentityManager is
> > > > inaccessible when calling constEnd/Begin(). But then adding the exact
> > > > same
> > > > code as a test case in KIdentityManagement, it works without any
> > > > crash.
> > > > 
> > > > Does anyone with a bit more experience in C++ can help me?
> > > 
> > > Summary of the discussion on chat:
> > > 
> > > It's an ABI issue on methods returning a QVector::[const_]iterator, and
> > > KIdentityManagement being built with QT_STRICT_ITERATORS enabled (via KF
> > > default settings), while the application didn't have that set.
> > 
> > Ouch! We didn't think of that, when enabling QT_STRICT_ITERATORS.
> > 
> > > I'm wondering now what the lesson for ABI compatibility in general is
> > > from
> > > this:
> > > * do not use QVector iterator types in public API?
> > 
> > This seems like the safest option indeed.
> > 
> > > do we even do that in KF?
> > 
> > Not really. All I found was 3 typedefs, but they are not used in the API
> > (or anywhere in app code, in fact)
> > kconfig/src/core/kcoreconfigskeleton.h: typedef QHash<QString,
> > KConfigSkeletonItem *>::Iterator DictIterator;
> > kio/src/core/kacl.h:typedef QList<ACLUserPermissions>::iterator
> > ACLUserPermissionsIterator; kio/src/core/kacl.h:typedef
> > QList<ACLGroupPermissions>::iterator ACLGroupPermissionsIterator;
> > 
> > > * do not set QT_STRICT_ITERATOR? what if the consuming code has it set
> > > though?
> > 
> > Right, the mismatch can happen both ways, so this wouldn't help.
> > 
> > > * only allow QVector iterators in public API inline code (so it
> > > entirely follows QT_STRICT_ITERATOR as set on the outside)?
> > 
> > Do we have a 100% guarantee that the compiler will inline the code,
> > even with optimizations disabled? I thought this was only a hint?
> 
> Different meaning of inline. What I am referring to is code that does not
> even have exported symbols (templates being the obvious/extreme case, but
> this applies to code in header files in general). With no existing symbol
> there is no choice for the compiler either way.

Right, but the common case of a method in an exported class, like
IdentityManager, might generate a symbol even if implemented inline, right?

Assuming IdentityManager wasn't pimpl'ed, what would happen if I wrote,
as part of the IdentityManager declaration:

    [...]
    ConstIterator begin() const { return mIdentities.cbegin(); }
    [...]

    (with ConstIterator being, as it already is, a typedef for QVector<Identity>::ConstIterator)

The class IdentityManager is exported. Does this mean the compiler
will always export a symbol for begin() just in case the method isn't inlined
when building an application that calls begin()? Or can it decide to skip exporting
a symbol and rely on all compilers always inlining the call?
I admit being rather hazy on this point.

In any case, pimpl'ing kills this idea, so I suggest we make the rule simple:
no QVector/QList iterators in public API.

-- 
David Faure, faure at kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5