Crash in sddmthemeinstaller invalid use of errorString
ClosedPublic

Authored by johngehrig on Apr 27 2019, 3:28 AM.

Details

Summary

The destruction of KAuth::ExecuteJob* "job" is not handled properly when passed as a parameter to KMessageBox.

An intermediate QString is created containing the error text. This value is passed into the KMessageBox and qWarning() so that the object "job" is no longer an input. The console error is now first so an error message displayed even in the event of a GUI crash.

BUG: 406718
BUG: 404609
FIXED-IN: 5.16.0

Test Plan

The patch has been applied to my system and the crash no longer occurs.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
johngehrig requested review of this revision.Apr 27 2019, 3:28 AM
johngehrig created this revision.
johngehrig added a reviewer: ngraham.
johngehrig set the repository for this revision to R123 SDDM Configuration Panel (KCM).
johngehrig removed a subscriber: ngraham.
Restricted Application added a project: Plasma. · View Herald TranscriptApr 27 2019, 3:29 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript

It looks like my alternate error "unknown error" isn't showing up in the dialog... So this isn't quite right yet.

I will fix this together with any other feedback. The segfault does appears to be resolved.

That's maybe KAuth bug or KCoreAddons one, when job fails it should have errorString. Also check jobs does not delete itself.

Thanks for the patch! Let me know when the other issue is resolved too. @anthonyfieroni offered up some good ideas.

davidedmundson requested changes to this revision.Apr 28 2019, 10:41 PM

That's not why its crashing.

You have this.

https://blogs.kde.org/2009/03/26/how-crash-almost-every-qtkde-application-and-how-fix-it-0

Kmessagebox hides a nested event loop.
Easiest fix, use the non static version that doesn't have the nested event loop.

This revision now requires changes to proceed.Apr 28 2019, 10:41 PM

Or swap the qWarning line with the messagebox

We want the log regardless

@anthonyfieroni Yes, I think there also an issue with KAuth here. This machine never gets to the auth prompt, which works fine on another machine.

I don't think "jobs" is deleting itself, otherwise we would segfault on "jobs->error()". No?

@davidedmundson Just so that I understand... You are saying "this" (parent window of dialog) is deleted out from under KMessageBox while it is in a waiting state?

I don't think that is the problem, but I will investigate and report back... From what I have observed, we segfault on "job->errorString()".

A simple case which should also crash:

+ QString errMessage = job->error();
-  KMessageBox::sorry(nullptr, i18n("Unable to install theme"), job->errorString());
- qWarning() << job->error() << job->errorString();

How would flipping the order help with the crash? Although logging first, then displaying the dialog seems like a good improvement.

The API for errorString() calls out that it should not be called when error() == 0. The code does not explicitly check for this scenario.

Somehow users (myself and others) can get into a state where !rc && error() == 0, and we are calling errorString() anyways. What's the harm in adding a line of defensive code so that we conform to the API?

What's the harm in adding a line of defensive code so that we conform to the API?

First step to fixing a bug is understanding the bug.

Your description of what is happening does not match the code.

From KJob.

int KJob::error() const
{
    return d_func()->error;
}

QString KJob::errorText() const
{
    return d_func()->errorText;
}

You can't have a state where one of those is callable and not the other.

davidedmundson added a comment.EditedApr 29 2019, 7:25 PM

You are saying "this" (parent window of dialog) is deleted out from under KMessageBox while it is in a waiting state?

~ish

We know KJob calls deleteLater at the end of KJob::exec()
We know KMessageBox spawns a new event loop
I think it's deleting your job when that new event loop starts.

Normally there is magic guarding of deferred deletion inside nested event loops.
However because app.exec() isn't running, it's not nested, it's the primary event loop.

But we can find out.

put a breakpoint on KAuth::ExecuteJob~ExecuteJob in gdb
and try to trigger your crash.

(or run in valgrind)

Thanks for the clarification! I understand what you are saying :)

I see errorString() has a fallback path stating the app is broken for invalid errror() values. Agreed, this isn't right...

Yes, setting a breakpoint at the destructor for job was my plan. I will report back once I can run the steps on the failing machine again. This issue only reproduces on one of my machines.

johngehrig updated this revision to Diff 57581.May 5 2019, 6:02 AM
johngehrig edited the summary of this revision. (Show Details)
johngehrig edited the test plan for this revision. (Show Details)

Ordering changed, qWarning first.
job->errorString() displayed for all values of job->error().
A QString object was created, so job is no longer passed to KMessageBox.

Sorry for the delay... I was finally able to get the crashing machine set up again.

Yes, it looks like the descuctor for job was called while the KMessageBox was in the process of being painted onscreen (breakpoint in kauthexecutejob.cpp).

I tried making job a QPointer<KAuth::ExecuteJob> and QSharedPointer with the original qWarning/KMessageBox lines, but that didn't help. I can't say I'm familiar enough with the intricacies of how KDE/Qt handles object allocations and events to explain...

davidedmundson accepted this revision.May 14 2019, 7:58 PM
This revision is now accepted and ready to land.May 14 2019, 7:58 PM
ngraham accepted this revision.May 14 2019, 8:32 PM
ngraham edited the summary of this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.