Fix segfault on no restart args
Needs RevisionPublic

Authored by jpalecek on Sun, May 17, 5:52 PM.

Details

Reviewers
dfaure
Group Reviewers
Frameworks
Summary

When an application is crashing through KCrash, but has no restart
argument list, KCrash itself crashes because s_autoRestartCommandLine
is null (when it wants to print the command line to stdout). This
patch fixes that by making sure s_autoRestartCommandLine is always
non-null.

Depends on D29810
Signed-off-by: Jiří Paleček <jpalecek@web.de>

Diff Detail

Repository
R285 KCrash
Branch
for-upstream
Lint
Lint OK
Unit
No Unit Test Coverage
jpalecek created this revision.Sun, May 17, 5:52 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptSun, May 17, 5:52 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
jpalecek requested review of this revision.Sun, May 17, 5:52 PM
jpalecek updated this revision to Diff 83138.Sun, May 24, 1:27 AM
jpalecek edited the summary of this revision. (Show Details)

Edited the commit msg, also prune the dependencies I want to abandon

dfaure requested changes to this revision.Wed, May 27, 9:39 PM

I'm a bit confused by the bug this is fixing. Surely this doesn't happen to all cases of crashes without autorestart enabled??

Also, it sounds like a null check might be enough.

src/kcrash.cpp
107

Should this become 1 then? Otherwise the for loop won't delete that new new char*[1]

272

insert(0) reads like prepend, but args is empty anyway, why not just use append?

275

We use { ... } braces also around single-line statements in KF5.

Can you do the same for the if above too?

This revision now requires changes to proceed.Wed, May 27, 9:39 PM

I'm a bit confused by the bug this is fixing. Surely this doesn't happen to all cases of crashes without autorestart enabled??

Also, it sounds like a null check might be enough.

No. It is a crash where s_autoRestartCommandLine was null. The actual crash happened in the logging code, but it is also used in startProcess and I think going for non-null s_autoRestartCommandLine is actually easier than faffing with null pointer checks and special casing in the signal handler code. Even with no restart arguments, you still need argv to hold the executable name to restart, so why not maintain it uniformly? It only costs a few bytes of heap.

src/kcrash.cpp
107

No. The invariant is that s_autoRestartArgc is the number of arguments, not counting the last null. The new char*[1] is deleted after the loop.

272

Well I was thinking about resize(), then found there wasn't any. However, if we want to be clear, maybe args = { QString() } would be the best?

(In reality, the effect should be args = { filePath }, but that comes with the next line.)

Can you explain how to trigger this crash in the first place? Which application triggers it?

src/kcrash.cpp
272

I like your suggestion.