Crash when using KIdentityManagement::IdentityManager

[Volker Krause]

On Mittwoch, 14. April 2021 21:12:11 CEST David Faure wrote:
> 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.c
> > > > > pp
> > > > > #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.

You can easily try that with something like
 inline ConstIterator myBegin() const { return begin(); }

You need to try really hard to make that generate a symbol (actually calling 
it, all optimizations off, and some extra dummy content like debug output 
triggered it here), but even that symbol is then local/not exported. This is 
the same for "normal" inline code, template code or special methods marked as 
"= default" in the header.

That's at least gcc/clang, but I remember MSVC being even stricter when 
encountering things like templates with export macros (something that given 
the above is nonsensical, and thus rightfully rejected).

On the consuming side this means the compiler has to assume there is no 
available symbol it can refer to when seeing inline code, and thus generates a 
local one itself. Even if there would be one already, the compiler doesn't 
know that, only the linker would be able to notice, and at that point the 
compiler has created facts already anyway.

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

pimpl'ing could be overcome here by a private method returning a reference to 
the vector, and using that in the inline code. Such a method might be the 
better and simpler API in the first place anyway.

Regards,
Volker
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-pim/attachments/20210415/91f453e8/attachment.sig>