Page MenuHome GnuPG

Kleopatra: Crash when quitting Application on Windows
Closed, ResolvedPublic

Description

When exiting Kleopatra and choosing to really quit Kleopatra the application does not cleanly quit but crashes. Since the effect -> Kleo is quit. Is the same for the user most users have not noticed this, but it is noticeable in the Event Viewer log. This error exists at least since 3.1.21, Gpg4win-4.0.0 I have not tested older versions. Prio high as my policy was the reproducable crashes should always have high priority, even if they are not noticable.

The initial analysis done by a User of Gpg4win is:

Wenn man Kleopatra komplett schließt, also
bei meiner englischen Lokalisierung auf Quit, dann kommt die Frage ob
nur das Fenster minimiert werden, oder ob der Prozess komplett beendet
werden soll.

Wählt man letzteres, dann scheint es auch so als ob Kleopatra _sauber_
beendet wurde - das Problem ist, dem ist aber nicht so, das habe ich
direkt gemerkt als über dem Mauszeiger der bekannte Kreis erscheint
ist, der in diesem Fall einen Absturz signalisiert hatte, und im
Hintergrund der Handler für AppCrashes (keine Ahnung wie die PE-Datei
genau heißt) schon lief.

Folgende Informationen kann ich euch weiterleiten:

1.) Der Crash tritt immer an der selben Stelle im Code-Segment von
libKF5ConfigWidgets.dll auf, hier ein Auszug des AppFault-Eintrages
aus dem Event Viewer (Windows Logs -> Application):

Faulting application name: kleopatra.exe, version: 3.1.22.0, time
stamp: 0x00000000
Faulting module name: libKF5ConfigWidgets.dll, version: 0.0.0.0, time
stamp: 0x00000000
Exception code: 0xc0000005
Fault offset: 0x00034ef0
Faulting process id: 0x1208

Der Exception Code ist nur allzu bekannt, Access Violation, also eine
Zugriffsverletzung - oh wie klingt das widerlich im Deutschen (nun
gut, ich spreche und denke 95% der Zeit nur noch in Englisch, obwohl
ich in Deutschland geboren wurde.. das verändert einen schon recht
deutlich)

Aus Lust und Laune habe ich die betroffene DLL mal in IDA geladen, da
ASLR hier scheinbar nicht richtig funktioniert habt, entweder weil Ihr
per API die Mitigations komplett deaktiviert (aus welchem Grund auch
immer), oder wegen einem isoliertem Problem auf meinem System, ist ja
auch egal - die Adresse, an der die Exception auftritt bleibt die
selbe - 0x00034ef0.

Also, die Bibliothek in IDA geladen, und zu der Adresse navigiert, wo
der Code eine Access Violation auslöst:

Siehe da, es handelt sich um den Export mit dem Ordinal 483, eine
Funktion namens
"_ZN28QExplicitlySharedDataPointerI13KSharedConfigED1Ev", die C++
Compiler Namensdekoration des Funktionsnamens habe ich nicht entfernt,
aber Ihr solltet mit all den Informationen die Funktion sehr einfach
finden können.

An der Adresse 0x00034ef0 finden wir auch ganz zufällig folgenden
Assembler Code:

.text:00034EF0 8B 01 mov eax, [ecx]

Da es sich um eine Funktion mit der Aufrufkonvention __fastcall
handelt, handelt es sich hier bei dem Register ecx um Argument #1 der
Funktion, die eckigen Klammern [] bedeuten dass hier ein Pointer
dereferenziert wird - wahrscheinlich ein Nullpointer oder eine Adresse
die auf einen invaliden Speicherbereich zeigt, so dass der Kernel bzw.
die CPU hier eine Exception / Ausnahme wirft.

"Live debuggt" habe ich das ganze nicht, aber ein Großteil der Arbeit
habe ich ja schon für euch getan.

Jetzt muss nur noch rausgefunden werden, warum ein ein falsches
Argument übergeben wird.

Noch etwas: IDA ist nicht immer korrekt mit den Schätzungen der
Calling Convention, es könnte also auch __thiscall, also eine
virtuelle Funktion einer C++ Klasse sein, dann wäre das ecx-Register
_IMMER_ ein Zeiger auf die zugehörige Klasse - warum dieser ein
nullptr oder ähnliches sein sollte, kann ich mir beim besten Willen
nicht erklären - da müsste schon arg viel aus dem Ruder laufen.

Ich hoffe meine Informationen konnten euch erst einmal in die richtige
Richtung leiten, wenn Ihr mehr Informationen braucht oder Ihr den
Fehler nicht reproduzieren könnt, dann meldet euch unter dieser E-Mail
Adresse.

Ich habe das Problem lokal erst einmal gefixt, in dem ich die Funktion
einfach mit 0x90 Instruktionen überschrieben habe - also NOOP / do
nothing - die Funktion hatte, mit dem Absturz, ja eh keinen Nutzen.

Details

Version
master

Event Timeline

aheinecke triaged this task as Normal priority.May 2 2022, 10:59 AM
aheinecke created this task.
aheinecke raised the priority of this task from Normal to High.May 2 2022, 11:01 AM
aheinecke updated the task description. (Show Details)

Looks like somebody is writing to the shared config after it has been destroyed already. Probably some global object that is destroyed by the runtime on shutdown.

So after updating to latest kf5 this still happens. The windows event log tells me that the crash occurs from libkf5configwidgets.dll
Does not seem to happen on Linux, i have run it with valgrind to and only found other crashes on linux shutdown when closing the window.

After some debugging and reading the code I did not find any way that the KSharedConfigPtr could be accessed when it was NULL. It is never saved in a member variable. The only place it was saved was in the static thread_local variable of defaultConfig in kcolorschemehelpers_p.h. Since this was irritating because I understood QExplicitlySharedDataPointer to already handle thread_local ness

From the docs: https://doc.qt.io/qt-5/qexplicitlyshareddatapointer.html

QExplicitlySharedDataPointer<T> makes writing your own explicitly shared classes easy. QExplicitlySharedDataPointer implements thread-safe reference counting, ensuring that adding QExplicitlySharedDataPointers to your reentrant classes won't make them non-reentrant.

So I tried removing the thread_local marker and tada it no longer crashed. Maybe this issue existed since c0cc6b8a200aa54099b9efc8eb7ac782b400fd93 in kconfigwidgets from january 2019 which introduced that.

But I really do not understand the issue here completely. It was more a case of experimental fixing. I used x64debug but it really did not help much since it also could not read the GCC symbols.

aheinecke changed the task status from Open to Testing.May 12 2022, 12:37 PM
aheinecke reassigned this task from aheinecke to ikloecker.

Ingo: I have now assigned this to you. If you can explain why my fix is required in a way that upstream would accept the change we should try to get this into KConfigWidgets proper. But I do not think that I can convincingly explain why it is required or what happens here.

Hmm, according to lxr.kde.org this is the only usage of a static thread_local KSharedConfigPtr variable in all of (indexed) KDE. OTOH, there's also exactly one usage of a static KSharedConfigPtr variable in all of KDE (in plasma/plasma-nm/libs/editor/configuration.cpp).

@aheinecke I suggest to open an MR for this at https://invent.kde.org/frameworks/kconfigwidgets and see how the discussion goes. Either the change is accepted or other proposals are made to fix the crash in Kleopatra.

On second thought: Let me open the MR.

ikloecker moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.

The KDE crowd think that this is likely a bug mingw. duckducking "mingw thread_local crash" give many hits, e.g. https://sourceforge.net/p/mingw-w64/bugs/727/.

Ways to go forward:
a) Keep our patch locally. I think it's okay for Kleopatra because we don't need thread local storage for KColorScheme. It should only be used from the main thread.
b) Do what Milian suggested:

You can try and see if QThreadStorage behaves better on mingw and then use that - but if you do add a clear marker that this is just to workaround a mingw bug.

I'd go for a) for now. The above bug report for mingw has been filed 4 years ago. It doesn't seem to have high priority.

ikloecker moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.

@aheinecke I think this task can be closed.

werner moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Jun 9 2022, 5:05 PM

Ok. Let us resolve this. The patch is in kconfigwidgets without a version marker and I already added a patch-next next to it for future versions of kconfigwidgets. Should be no problem to keep.

ebo moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Apr 5 2023, 1:55 PM