D29814: Fix segfault on no restart args

[David Faure]

dfaure closed this revision.

REPOSITORY
  R285 KCrash

REVISION DETAIL
  https://phabricator.kde.org/D29814

To: jpalecek, #frameworks, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29814: Fix segfault on no restart args

[David Faure]

dfaure accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R285 KCrash

BRANCH
  for-upstream

REVISION DETAIL
  https://phabricator.kde.org/D29814

To: jpalecek, #frameworks, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29814: Fix segfault on no restart args

[Jiří Paleček]

jpalecek added a comment.


  In D29814#674730 , @dfaure wrote:
  
  > This question is still without an answer: "Can you explain how to trigger 
this crash in the first place? Which application triggers it?"
  
  
  That's because I don't know in which binary I have seen it. The patch itself 
is more than 6 months old and I don't remember what I was doing then. Not that 
I would care that much, IMHO the code should be resilient enough not to crash 
when someone does weird things with `QApplication`. After all, if apps had no 
bugs, there wouldn't be need for kcrash in the first place.
  
  The comment on one line says that `tst_QX11Info::startupId does QApplication 
app(argc, nullptr)` and this would certainly trigger it (should the following 
execution crash). That comment convinced me this usage is at least sort-of 
valid and so I wrote the patch.

REPOSITORY
  R285 KCrash

REVISION DETAIL
  https://phabricator.kde.org/D29814

To: jpalecek, #frameworks, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29814: Fix segfault on no restart args

[David Faure]

dfaure added a comment.


  This question is still without an answer: "Can you explain how to trigger 
this crash in the first place? Which application triggers it?"

REPOSITORY
  R285 KCrash

REVISION DETAIL
  https://phabricator.kde.org/D29814

To: jpalecek, #frameworks, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29814: Fix segfault on no restart args

[Jiří Paleček]

jpalecek updated this revision to Diff 83232.
jpalecek added a comment.


  Remove contamination from another patch made by the last upload

REPOSITORY
  R285 KCrash

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29814?vs=83214=83232

BRANCH
  for-upstream

REVISION DETAIL
  https://phabricator.kde.org/D29814

AFFECTED FILES
  src/kcrash.cpp

To: jpalecek, #frameworks, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29814: Fix segfault on no restart args

[David Faure]

dfaure added a comment.


  This commit seems to include the changes requested in the other review...

REPOSITORY
  R285 KCrash

REVISION DETAIL
  https://phabricator.kde.org/D29814

To: jpalecek, #frameworks, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29814: Fix segfault on no restart args

[Jiří Paleček]

jpalecek updated this revision to Diff 83214.
jpalecek added a comment.


  Polished the style as requested

REPOSITORY
  R285 KCrash

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29814?vs=83138=83214

BRANCH
  for-upstream

REVISION DETAIL
  https://phabricator.kde.org/D29814

AFFECTED FILES
  src/kcrash.cpp

To: jpalecek, #frameworks, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29814: Fix segfault on no restart args

[David Faure]

dfaure added a comment.


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

INLINE COMMENTS

> jpalecek wrote in kcrash.cpp: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.)

I like your suggestion.

REPOSITORY
  R285 KCrash

REVISION DETAIL
  https://phabricator.kde.org/D29814

To: jpalecek, #frameworks, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29814: Fix segfault on no restart args

[Jiří Paleček]

jpalecek added a comment.


  In D29814#674013 , @dfaure wrote:
  
  > 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.

INLINE COMMENTS

> dfaure wrote in kcrash.cpp:107
> Should this become 1 then? Otherwise the `for` loop won't delete that new 
> `new char*[1]`

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.

> dfaure wrote in kcrash.cpp:272
> insert(0) reads like prepend, but args is empty anyway, why not just use 
> append?

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.)

REPOSITORY
  R285 KCrash

REVISION DETAIL
  https://phabricator.kde.org/D29814

To: jpalecek, #frameworks, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29814: Fix segfault on no restart args

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


  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.

INLINE COMMENTS

> kcrash.cpp:107
>  static char *s_appPath = nullptr;
>  static int s_autoRestartArgc = 0;
> +static char **s_autoRestartCommandLine = new char*[1]{ nullptr };

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

> kcrash.cpp:272
> +if (args.isEmpty()) // edge case: tst_QX11Info::startupId does 
> QApplication app(argc, nullptr)...
> +args.insert(0, QString());
> +

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

> kcrash.cpp:275
> +args[0] = filePath; // replace argv[0] with full path above
> +for (int arg = 0; arg < s_autoRestartArgc; arg++)
> +delete [] s_autoRestartCommandLine[arg];

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

Can you do the same for the `if` above too?

REPOSITORY
  R285 KCrash

REVISION DETAIL
  https://phabricator.kde.org/D29814

To: jpalecek, #frameworks, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29814: Fix segfault on no restart args

[Jiří Paleček]

jpalecek added a reviewer: dfaure.

REPOSITORY
  R285 KCrash

REVISION DETAIL
  https://phabricator.kde.org/D29814

To: jpalecek, #frameworks, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29814: Fix segfault on no restart args

[Jiří Paleček]

jpalecek updated this revision to Diff 83138.
jpalecek edited the summary of this revision.
jpalecek added a dependency: D29810: Don't use the setenv function after fork.
jpalecek added a comment.


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

REPOSITORY
  R285 KCrash

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29814?vs=83027=83138

BRANCH
  for-upstream

REVISION DETAIL
  https://phabricator.kde.org/D29814

AFFECTED FILES
  src/kcrash.cpp

To: jpalecek, #frameworks
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29814: Fix segfault on no restart args

[Jiří Paleček]

jpalecek added a reviewer: Frameworks.

REPOSITORY
  R285 KCrash

REVISION DETAIL
  https://phabricator.kde.org/D29814

To: jpalecek, #frameworks
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29814: Fix segfault on no restart args

[Jiří Paleček]

jpalecek created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
jpalecek requested review of this revision.

REVISION 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 D29813 
  Signed-off-by: Jiří Paleček 

REPOSITORY
  R285 KCrash

BRANCH
  for-upstream

REVISION DETAIL
  https://phabricator.kde.org/D29814

AFFECTED FILES
  src/kcrash.cpp

To: jpalecek
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns