- On Linux recent kernels only allow attaching a debugger to a process if
- the right to attach to processes is enabled system-wide
- the debugger is an ancestor of the debuggee
- or the debuggee sets a tracer process by a call to prctl(PR_SET_PTRACER, debugger_pid, 0, 0, 0);
- DrKonqi will ask the debuggee by a socket connection to set a new ptracer. This is required if an external debugger is started (using the usually hidden Debug button).
- When the debugger has finished, the access is requested back to DrKonqi to allow internal backtraces again.
Details
- Reviewers
ossi - Group Reviewers
Plasma: Workspaces Frameworks - Commits
- R871:9f3cfcff960c: Request change of ptrace scope from KCrash
- try to attach gdb or KDevelop to the process by using the Debug button: operation will not be permitted
- apply corresponding patches to KCrash D11236 and DrKonqi
- the process can now be debugged
- after detaching the debugger again, a backtrace can be created using the DrKonqi dialog as usual
Diff Detail
- Repository
- R871 DrKonqi
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comments directed at licensing and non-Linux cases.
src/queryptrace.cpp | ||
---|---|---|
1 ↗ | (On Diff #30979) | Missing file header / license bits |
49 ↗ | (On Diff #30979) | For non-Linuxen, include a null implementation here. |
src/queryptrace.h | ||
1 ↗ | (On Diff #30979) | Missing a file header / license bits |
5 ↗ | (On Diff #30979) | You used qint64 elsewhere |
7 ↗ | (On Diff #30979) | In general, and for type safety, I'd avoid using #ifdefs to define an API. There should probably be only the one declaration of queryPtrace. |
src/queryptrace.cpp | ||
---|---|---|
26 ↗ | (On Diff #30979) | Please honour TMPDIR or, even better, use QTemporaryDir (this will also make the generated path unpredictable), and create the socket inside. |
Thank you for your comments, totally forgot about licensing.
src/queryptrace.cpp | ||
---|---|---|
26 ↗ | (On Diff #30979) | QTemporaryDir will not work, since the location is set in KCrash. |
src/debuggerlaunchers.cpp | ||
---|---|---|
57 | according to my reading of the documentation (https://www.kernel.org/doc/Documentation/security/Yama.txt) and the kernel source, this is unnecessary in this branch, as descendants of the assigned ptracer have the right as well (that's why the "normal" backtrace creation works without your patch), and not even detaching breaks the chain. | |
src/queryptrace.cpp | ||
26 ↗ | (On Diff #30979) | the location is still bogus, though. see comment on the other diff. |
67 ↗ | (On Diff #34671) | use poll() here, too. |
68 ↗ | (On Diff #34671) | the current version of the complementary patch just echoes back the pid, so this cannot possibly work. |
src/queryptrace.h | ||
22 ↗ | (On Diff #34671) | no. "on linux, tell the process to allow the debugger to attach to it". |
23 ↗ | (On Diff #34671) | that's a rather misleading function name. setPtracer() would reflect the purpose and upstream naming. |
src/tests/crashtest/crashtest.cpp | ||
138 | only if kdeinit was started from the same console, which you cannot assume. also, this change wouldn't belong into this patch anyway. |
mark handled issues as done here as well.
src/ptracer.cpp | ||
---|---|---|
48 | see complementary patch | |
61 | cruft | |
69 | i'd compare to 22, because we know that the other side sends that. | |
70 | i'd use memcmp(), as it's a fixed-size data block. | |
src/ptracer.h | ||
23 | -r | |
src/tests/crashtest/crashtest.cpp | ||
133 | this addition isn't used or explained anywhere for all i can tell. |
- use debuggee pid instead of DrKonqi pid for socket name
- make sure to read/write all bytes from/to socket
src/ptracer.cpp | ||
---|---|---|
66 | you really could just use != here ... ^^ | |
79 | ditto | |
83 | a complementary warning if any of the steps goes wrong would seem appropriate. | |
src/tests/crashtest/crashtest.cpp | ||
139 | add comment: this can be disabled to be able to test kcrash's real default behavior. | |
146 | move that up, so the AlwaysDirectly stuff is in one block. |
- change order and add comment to kdeinit option in crashtest
- add warning if debuggee does not respond as expected
src/ptracer.cpp | ||
---|---|---|
66 | wow... |
will also need to wait for commit message update, like the other change.
src/ptracer.cpp | ||
---|---|---|
93 | not wrong per se, but mildly misleading, as it suggests the problem is on the other end, which is not necessarily true, this being a catch-all. "cannot set ptracer" would cover all bases (while still not being particularly helpful). |
you got the parent-child ordering wrong in the commit message. ^^
src/ptracer.cpp | ||
---|---|---|
62 | this looks stupid, i'd swap the order. |
you got the parent-child ordering wrong in the commit message. ^^
What do you mean? The (internal) debugger is a child of the debuggee.
$ pstree -pn $(pidof crashtest) crashtest(17220)─┬─{crashtest}(17221) ├─{crashtest}(17222) └─drkonqi(17223)─┬─{drkonqi}(17224) ├─{drkonqi}(17225) ├─{drkonqi}(17231) └─gdb(17293)─┬─{gdb}(17294) ├─{gdb}(17295) └─{gdb}(17296)
yes, that's why you need setPtracer() - if crashtest was *under* gdb, that would be unnecessary. ;)
(i'm still referring to the enumeration of cases, if that wasn't clear.)
src/ptracer.cpp | ||
---|---|---|
64 | i meant this line ... |