Request change of ptrace scope from KCrash
ClosedPublic

Authored by croick on Mar 11 2018, 3:06 PM.

Details

Summary
  • 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.
Test Plan
  • 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
Branch
ptracer
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 7186
Build 7204: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
adridg added a subscriber: adridg.May 22 2018, 9:40 AM

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.

maximilianocuria added inline comments.
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.

croick updated this revision to Diff 34670.May 22 2018, 7:46 PM
  • add license text
  • use QDir::tempPath()
  • ifdefs to definition
croick marked 6 inline comments as done.May 22 2018, 7:49 PM

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.

croick updated this revision to Diff 34671.May 22 2018, 8:33 PM
croick marked an inline comment as done.
  • check size of socket path
ossi requested changes to this revision.Jan 14 2019, 4:41 PM
ossi added a subscriber: ossi.
ossi added inline comments.
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
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.

26 ↗(On Diff #30979)

the location is still bogus, though. see comment on the other diff.

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.

This revision now requires changes to proceed.Jan 14 2019, 4:41 PM
croick updated this revision to Diff 49607.Jan 16 2019, 10:14 AM
croick edited the summary of this revision. (Show Details)
croick edited the test plan for this revision. (Show Details)
  • rename queryPtrace to setPtracer
  • add a kdeinit option in crashtest
ossi requested changes to this revision.Jan 16 2019, 11:00 AM

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.

This revision now requires changes to proceed.Jan 16 2019, 11:00 AM
croick updated this revision to Diff 49618.Jan 16 2019, 11:56 AM
  • use debuggee pid instead of DrKonqi pid for socket name
  • make sure to read/write all bytes from/to socket
croick marked 11 inline comments as done.Jan 16 2019, 12:11 PM

read() and write() were assumed to work right away, I added a loop around.

src/tests/crashtest/crashtest.cpp
133

It's used to unset the AlwaysDirectly flag. It allows testing using with direct start and kdeinit start.

138

Agreed, I added an option instead and would commit that separately.

ossi requested changes to this revision.Jan 16 2019, 3:52 PM
ossi added inline comments.
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.

This revision now requires changes to proceed.Jan 16 2019, 3:52 PM
croick updated this revision to Diff 49649.Jan 16 2019, 4:07 PM
croick marked an inline comment as done.
  • change order and add comment to kdeinit option in crashtest
  • add warning if debuggee does not respond as expected
croick updated this revision to Diff 49651.Jan 16 2019, 4:15 PM
croick marked 5 inline comments as done.
  • emit warning if no socket is available
croick added inline comments.Jan 16 2019, 4:15 PM
src/ptracer.cpp
66

wow...

ossi added a comment.Jan 16 2019, 4:33 PM

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

croick updated this revision to Diff 49705.Jan 17 2019, 11:48 AM
croick edited the summary of this revision. (Show Details)
  • adjust warning and check for EINTR when polling
ossi added a comment.Jan 17 2019, 12:07 PM

you got the parent-child ordering wrong in the commit message. ^^

src/ptracer.cpp
62

this looks stupid, i'd swap the order.

croick updated this revision to Diff 49710.Jan 17 2019, 12:23 PM
  • swap lines, declare array for response message where needed

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)
ossi added a comment.Jan 17 2019, 1:24 PM

What do you mean? The (internal) debugger is a child of the debuggee.

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 ...
(but your change was good, too).

croick updated this revision to Diff 49717.Jan 17 2019, 1:39 PM
  • swap variable declaration
croick marked an inline comment as done.Jan 17 2019, 1:39 PM
croick edited the summary of this revision. (Show Details)Jan 17 2019, 2:07 PM
croick edited the test plan for this revision. (Show Details)
croick edited the summary of this revision. (Show Details)Jan 20 2019, 9:43 PM
croick edited the summary of this revision. (Show Details)Jan 23 2019, 11:07 PM
ossi accepted this revision.Jan 24 2019, 11:14 AM

note that for extra correctness it should be "ancestor" not "parent".

This revision is now accepted and ready to land.Jan 24 2019, 11:14 AM
croick retitled this revision from [DrKonqi] Request change of ptrace scope from KCrash to Request change of ptrace scope from KCrash.Jan 25 2019, 9:07 AM
croick edited the summary of this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.