Maybe I am missing something here but wouldn't this allow any application to get ptrace access?

e.g. if a malicious program watches /tmp/kcrash_*, then writes its own pid to a new socket before kcrash writes the debugger's... now the malicious program has ptrace access.

I also think *printf isn't save to call in a signal handler. Not sure about atoi.

In D11236#239055, @sitter wrote:

Maybe I am missing something here but wouldn't this allow any application to get ptrace access?

e.g. if a malicious program watches /tmp/kcrash_*, then writes its own pid to a new socket before kcrash writes the debugger's... now the malicious program has ptrace access.

That's a valid point. In the updated revision the peer PID is checked and must match the one of DrKonqi, before prctl is called.

I also think *printf isn't save to call in a signal handler. Not sure about atoi.

atoi seems to be safe, (f)printf isn't indeed. Nevertheless it's used in the existing code already. Maybe that should be addressed in a different patch?

Thank you for your remarks!

Is there anybody who feels like reviewing this?
I'm happily using the patch on my system, but I understand that there might still be hidden issues here.

This looks like the kind of commit where a review by Oswald Buddenhagen <ossi@kde.org> would be useful.

In D11236#393063, @ossi wrote:

yay, i finally have "some" time for this. ^^

Great! :) And thanks for your input.

it took me a while to get a coherent picture of the problem and solution, because your description unnecessarily dwells on the irrelevant cases of ptrace restrictions, but omits the crucial fact that this is about tracers that are not descendant processes of drkonqi, specifically kdevelop launched via kdeinit. see also comment on DefaultDebuggerLauncher in the complementary change. please make sure to point that out in the commit messages.

Actually it's only the internal process that creates the backtraces, that is a descendant of DrKonqi. All debuggers that are launched via the "Debug" button are processes on their own (to remain alive, even if DrKonqi already quit). But I will change the commit message to point that out.

In D11236#393720, @croick wrote:

Actually it's only the internal process that creates the backtraces, that is a descendant of DrKonqi. All debuggers that are launched via the "Debug" button are processes on their own (to remain alive, even if DrKonqi already quit).

if by that you're implying that external debuggers are always launched via kdeinit, then please put that as the reason in the commit message.
irrespective of that, the "to remain alive" argument is bogus, because detaching via double fork() (as qprocess does) "protects" the child process, but does *not* break the permission inheritance.

In D11236#393789, @ossi wrote:

detaching via double fork() "protects" the child process,

but does *not* break the permission inheritance.

ok, i take that back and claim the opposite - i misinterpreted the kernel's task_struct's real_parent member's purpose.

that also means that the kernel doc is mildly misleading ("descendant" clearly refers to the current relationship after possible re-parenting, not the real ancestry at time of forking), but i suppose one is expected to know the exact definitions of the used terminology.

please explicitly mark all handled issues as done - you'll notice on the way that you didn't address some of them.

Sorry, you were just responding too quickly.

you adjusted the summary kinda the wrong way around ... but come to think of it, i was actually kinda wrong - you indeed need to list all three cases to illustrate that neither applies. the ancestry case is special only in the sense that it automatically makes the "internal" debugger work (i'd mention that in parentheses of that case's bullet point).

Actually the point I removed does not seem to be true any longer. I'm almost certain, that the prctrl(PR_SET_PTRACER, ...) was not required until some time ago when starting DrKonqi as a fork. But now the internal backtrace is not working without. I cannot tell, when that started (currently using kernel 4.20).

In D11236#394363, @croick wrote:

Actually the point I removed does not seem to be true any longer. I'm almost certain, that the prctrl(PR_SET_PTRACER, ...) was not required until some time ago when starting DrKonqi as a fork. But now the internal backtrace is not working without. I cannot tell, when that started (currently using kernel 4.20).

that sounds suspicious. i don't think the kernel's behavior did changed, and the process hierarchy presumably didn't, either. the right is handed down the ancestry, and that's irrespective of whether the tracer is a "natural" ancestor or the tracee, or was set via prctl(). maybe your sysctl settings simply changed?

that sounds suspicious. i don't think the kernel's behavior did changed, and the process hierarchy presumably didn't, either. the right is handed down the ancestry, and that's irrespective of whether the tracer is a "natural" ancestor or the tracee, or was set via prctl(). maybe your sysctl settings simply changed?

At least I didn't do that knowingly ... I will check what could be the reason.

commit message inverted here as well.

I think it's quite useful (see BUG 175362). Remaining threads remain quite busy otherwise, especially since KCrash is closing all file descriptors by default, which leads to a lot of polling of non-existant FDs in the background.
Maybe you would like to join the discussion in D18245 about this ;)

right, threads, i didn't think of that. i suppose it might make sense to pthread_kill() from within kcrash to freeze the other threads, but that will be non-atomic, and i wonder whether it wouldn't have undesirable interactions with the attached debugger and/or exiting the process.

the 3rd line is still wrong ...

the 3rd line is still wrong ...

So I used the word "tracer" now. Otherwise I don't know what you mean.

i said "3rd line", not "3rd case". it's about the ancestry; see the other change.

Ok, now I got it. After reading the YAMA doc again (more carefully) I realize that it really just is the parent that can be attached to the child by default and not the other way around.
I still wonder why I seem to recall that there was a working backtrace for the directly started DrKonqi in the tests, although the tracer was never set. Sorry about that.