| dfaure |
| Frameworks |
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>
| Lint OK |
| No Unit Test Coverage |
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 ↗ | (On Diff #83138) | Should this become 1 then? Otherwise the for loop won't delete that new new char*[1] |
| 272 ↗ | (On Diff #83138) | insert(0) reads like prepend, but args is empty anyway, why not just use append? |
| 275 ↗ | (On Diff #83138) | We use { ... } braces also around single-line statements in KF5. Can you do the same for the if above too? |
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 ↗ | (On Diff #83138) | 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 ↗ | (On Diff #83138) | 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 ↗ | (On Diff #83138) | I like your suggestion. |
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.