D11236: [KCrash] Establish socket to allow change of ptracer

[Christoph Roick]

This revision was automatically updated to reflect the committed changes.
Closed by commit R285:2af59ef51f8c: [KCrash] Establish socket to allow change 
of ptracer (authored by croick).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D11236?vs=49711=50214#toc

REPOSITORY
  R285 KCrash

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11236?vs=49711=50214

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

AFFECTED FILES
  src/kcrash.cpp

To: croick, #frameworks, ossi
Cc: dfaure, lepagevalleeemmanuel, kde-frameworks-devel, sitter, michaelh, 
ngraham, bruns

D11236: [KCrash] Establish socket to allow change of ptracer

[Christoph Roick]

croick edited the summary of this revision.

REPOSITORY
  R285 KCrash

BRANCH
  ptracer

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

To: croick, #frameworks, ossi
Cc: dfaure, lepagevalleeemmanuel, kde-frameworks-devel, sitter, michaelh, 
ngraham, bruns

D11236: [KCrash] Establish socket to allow change of ptracer

[Oswald Buddenhagen]

ossi accepted this revision.
ossi added a comment.
This revision is now accepted and ready to land.


  here too, it should be "ancestor" not "parent" for extra correctness.
  amending the code comment is optional, too.

INLINE COMMENTS

> kcrash.cpp:662
> +// For now that will be DrKonqi, which may ask to transfer the 
> ptrace scope to
> +// a debugger using a socket.
>  #ifndef PR_SET_PTRACER

you could make it more specific by saying "a debugger it is not an ancestor of 
(because it started it via kdeinit or startDetached())"

REPOSITORY
  R285 KCrash

BRANCH
  ptracer

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

To: croick, #frameworks, ossi
Cc: dfaure, lepagevalleeemmanuel, kde-frameworks-devel, sitter, michaelh, 
ngraham, bruns

D11236: [KCrash] Establish socket to allow change of ptracer

[Christoph Roick]

croick added a comment.


  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.

REPOSITORY
  R285 KCrash

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

To: croick, #frameworks, ossi
Cc: dfaure, lepagevalleeemmanuel, kde-frameworks-devel, sitter, michaelh, 
ngraham, bruns

D11236: [KCrash] Establish socket to allow change of ptracer

[Christoph Roick]

croick edited the summary of this revision.

REPOSITORY
  R285 KCrash

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

To: croick, #frameworks, ossi
Cc: dfaure, lepagevalleeemmanuel, kde-frameworks-devel, sitter, michaelh, 
ngraham, bruns

D11236: [KCrash] Establish socket to allow change of ptracer

[Oswald Buddenhagen]

ossi added a comment.


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

REPOSITORY
  R285 KCrash

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

To: croick, #frameworks, ossi
Cc: dfaure, lepagevalleeemmanuel, kde-frameworks-devel, sitter, michaelh, 
ngraham, bruns

D11236: [KCrash] Establish socket to allow change of ptracer

[Christoph Roick]

croick added a comment.


  > the 3rd line is still wrong ...
  
  So I used the word "tracer" now. Otherwise I don't know what you mean.

REPOSITORY
  R285 KCrash

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

To: croick, #frameworks, ossi
Cc: dfaure, lepagevalleeemmanuel, kde-frameworks-devel, sitter, michaelh, 
ngraham, bruns

D11236: [KCrash] Establish socket to allow change of ptracer

[Christoph Roick]

croick edited the summary of this revision.

REPOSITORY
  R285 KCrash

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

To: croick, #frameworks, ossi
Cc: dfaure, lepagevalleeemmanuel, kde-frameworks-devel, sitter, michaelh, 
ngraham, bruns

D11236: [KCrash] Establish socket to allow change of ptracer

[Oswald Buddenhagen]

ossi added a comment.


  the 3rd line is still wrong ...

REPOSITORY
  R285 KCrash

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

To: croick, #frameworks, ossi
Cc: dfaure, lepagevalleeemmanuel, kde-frameworks-devel, sitter, michaelh, 
ngraham, bruns

D11236: [KCrash] Establish socket to allow change of ptracer

[Oswald Buddenhagen]

ossi added a comment.


  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.

REPOSITORY
  R285 KCrash

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

To: croick, #frameworks, ossi
Cc: dfaure, lepagevalleeemmanuel, kde-frameworks-devel, sitter, michaelh, 
ngraham, bruns

D11236: [KCrash] Establish socket to allow change of ptracer

[Christoph Roick]

croick edited the summary of this revision.
croick edited the test plan for this revision.

REPOSITORY
  R285 KCrash

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

To: croick, #frameworks, ossi
Cc: dfaure, lepagevalleeemmanuel, kde-frameworks-devel, sitter, michaelh, 
ngraham, bruns

D11236: [KCrash] Establish socket to allow change of ptracer

[Christoph Roick]

croick added a comment.


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

REPOSITORY
  R285 KCrash

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

To: croick, #frameworks, ossi
Cc: dfaure, lepagevalleeemmanuel, kde-frameworks-devel, sitter, michaelh, 
ngraham, bruns

D11236: [KCrash] Establish socket to allow change of ptracer

[Oswald Buddenhagen]

ossi added inline comments.

INLINE COMMENTS

> kcrash.cpp:658
> +// when launching drkonqi. Note that DrKonqi will SIGSTOP this 
> process in the meantime
> +// and only send SIGCONT when it is about to attach a debugger.
>  #ifdef Q_OS_LINUX

ok. but come to think of it, maybe that should be revised - that SIGSTOPping 
sounds a tad paranoid to start with. feel like researching the history of that?

REPOSITORY
  R285 KCrash

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

To: croick, #frameworks, ossi
Cc: dfaure, lepagevalleeemmanuel, kde-frameworks-devel, sitter, michaelh, 
ngraham, bruns

D11236: [KCrash] Establish socket to allow change of ptracer

[Christoph Roick]

croick marked an inline comment as done.

REPOSITORY
  R285 KCrash

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

To: croick, #frameworks, ossi
Cc: dfaure, lepagevalleeemmanuel, kde-frameworks-devel, sitter, michaelh, 
ngraham, bruns

D11236: [KCrash] Establish socket to allow change of ptracer

[Christoph Roick]

croick updated this revision to Diff 49711.
croick marked an inline comment as done.
croick added a comment.


  - invert debugger-debuggee hierarchy in comment

REPOSITORY
  R285 KCrash

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11236?vs=49650=49711

BRANCH
  ptracer

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

AFFECTED FILES
  src/kcrash.cpp

To: croick, #frameworks, ossi
Cc: dfaure, lepagevalleeemmanuel, kde-frameworks-devel, sitter, michaelh, 
ngraham, bruns

D11236: [KCrash] Establish socket to allow change of ptracer

[Oswald Buddenhagen]

ossi added a comment.


  commit message inverted here as well.

INLINE COMMENTS

> kcrash.cpp:658
> +// when launching drkonqi. Note that DrKonqi will SIGSTOP this 
> process in the meantime
> +// and only send SIGCONT when it is to be attached by a debugger.
>  #ifdef Q_OS_LINUX

"when it is about to attach a debugger".

REPOSITORY
  R285 KCrash

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

To: croick, #frameworks, ossi
Cc: dfaure, lepagevalleeemmanuel, kde-frameworks-devel, sitter, michaelh, 
ngraham, bruns

D11236: [KCrash] Establish socket to allow change of ptracer

[Christoph Roick]

croick edited the summary of this revision.

REPOSITORY
  R285 KCrash

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

To: croick, #frameworks, ossi
Cc: dfaure, lepagevalleeemmanuel, kde-frameworks-devel, sitter, michaelh, 
ngraham, bruns

D11236: [KCrash] Establish socket to allow change of ptracer

[Christoph Roick]

croick marked 2 inline comments as done.
croick added a comment.


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

INLINE COMMENTS

> ossi wrote in kcrash.cpp:657
> ok, so now it's claimed to apply to the entire block. but how can that be? 
> thepollDrKonqiSocket() couldn't possibly be called then ...

True, actually DrKonqi puts the debugger in a block between SIGCONT and SIGSTOP.

REPOSITORY
  R285 KCrash

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

To: croick, #frameworks, ossi
Cc: dfaure, lepagevalleeemmanuel, kde-frameworks-devel, sitter, michaelh, 
ngraham, bruns

D11236: [KCrash] Establish socket to allow change of ptracer

[Christoph Roick]

croick updated this revision to Diff 49650.
croick marked 2 inline comments as done.
croick added a comment.


  - modify comment and remove check for EAGAIN

REPOSITORY
  R285 KCrash

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11236?vs=49617=49650

BRANCH
  ptracer

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

AFFECTED FILES
  src/kcrash.cpp

To: croick, #frameworks, ossi
Cc: dfaure, lepagevalleeemmanuel, kde-frameworks-devel, sitter, michaelh, 
ngraham, bruns

D11236: [KCrash] Establish socket to allow change of ptracer

[Oswald Buddenhagen]

ossi added a comment.


  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?

INLINE COMMENTS

> kcrash.cpp:657
>  // Wait forever until the started process exits. This code path is 
> executed
> -// when launching drkonqi. Note that drkonqi will stop this process 
> in the meantime.
> -if (directly) {
> -//if the process was started directly, use waitpid(), as it's a 
> child...
> -while (waitpid(-1, nullptr, 0) != pid) {}
> -} else {
> +// when launching drkonqi. Note that DrKonqi will SIGSTOP this 
> process in the meantime
> +// and a backtrace will end here.

ok, so now it's claimed to apply to the entire block. but how can that be? 
thepollDrKonqiSocket() couldn't possibly be called then ...

> kcrash.cpp:907
> +r = poll(, 1, 1000); // wait for 1 second for a request by DrKonqi
> +} while (r == -1 && (errno == EINTR || errno == EAGAIN));
> +// only continue if POLLIN event returned

remove eagain here as well.

REPOSITORY
  R285 KCrash

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

To: croick, #frameworks, ossi
Cc: dfaure, lepagevalleeemmanuel, kde-frameworks-devel, sitter, michaelh, 
ngraham, bruns

D11236: [KCrash] Establish socket to allow change of ptracer

[Christoph Roick]

croick marked 10 inline comments as done.
croick added a comment.


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

INLINE COMMENTS

> ossi wrote in kcrash.cpp:674
> please consistently put a space after the // marker. also, stick to the 
> file's capitalization style.

That comment was just copied from the old code.

> ossi wrote in kcrash.cpp:686
> hmm, what about the last sentence? it seems to me that some adjustment 
> (possibly just additional comments) is required.

see line 656

REPOSITORY
  R285 KCrash

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

To: croick, #frameworks, ossi
Cc: dfaure, lepagevalleeemmanuel, kde-frameworks-devel, sitter, michaelh, 
ngraham, bruns

D11236: [KCrash] Establish socket to allow change of ptracer

[Christoph Roick]

croick updated this revision to Diff 49617.
croick marked 2 inline comments as done.
croick added a comment.


  - unlink old sockets before binding
  - remove superfluous EAGAIN check
  - use debuggee pid instead of DrKonqi pid for socket name
  - fix comments

REPOSITORY
  R285 KCrash

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11236?vs=49609=49617

BRANCH
  ptracer

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

AFFECTED FILES
  src/kcrash.cpp

To: croick, #frameworks, ossi
Cc: dfaure, lepagevalleeemmanuel, kde-frameworks-devel, sitter, michaelh, 
ngraham, bruns

D11236: [KCrash] Establish socket to allow change of ptracer

[Oswald Buddenhagen]

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


  mostly minor issues left.
  
  please explicitly mark all handled issues as done - you'll notice on the way 
that you didn't address some of them.
  
  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).

INLINE COMMENTS

> kcrash.cpp:668
> +
> QStringLiteral("%1/kcrash_%2").arg(QStandardPaths::writableLocation(QStandardPaths::RuntimeLocation))
> +  .arg(pid));
> +int sockfd = openDrKonqiSocket(socketpath);

not really significant, but imo it's backwards that you let the drkonqi pid 
rather than the kcrash pid determine the socket name.

> kcrash.cpp:674
> +if (directly) {
> +//if the process was started directly, use waitpid(), as 
> it's a child...
> +while ((running = waitpid(pid, nullptr, WNOHANG) != pid) && 
> pollDrKonqiSocket(pid, sockfd) >= 0) {}

please consistently put a space after the // marker. also, stick to the file's 
capitalization style.

> kcrash.cpp:686
> +// Wait forever until the started process exits. This code path is 
> executed
> +// when launching drkonqi. Note that drkonqi will SIGSTOP this 
> process in the meantime.
> +if (running) {

hmm, what about the last sentence? it seems to me that some adjustment 
(possibly just additional comments) is required.

> kcrash.cpp:870
> +{
> +struct sockaddr_un drkonqi_server;
> +

i'd move that down, before the member init.

REPOSITORY
  R285 KCrash

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

To: croick, #frameworks, ossi
Cc: dfaure, lepagevalleeemmanuel, kde-frameworks-devel, sitter, michaelh, 
ngraham, bruns

D11236: [KCrash] Establish socket to allow change of ptracer

[Christoph Roick]

croick updated this revision to Diff 49609.
croick retitled this revision from "[KCrash] Establish socket to allow change 
of ptrace scope" to "[KCrash] Establish socket to allow change of ptracer".
croick edited the summary of this revision.
croick added a comment.


  - use poll instead of select
  - set ptracer pid and poll for request for the directly started DrKonqi as 
well and make the ifdef-section more readable
  - replace bogus perror

REPOSITORY
  R285 KCrash

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11236?vs=43560=49609

BRANCH
  ptracer

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

AFFECTED FILES
  src/kcrash.cpp

To: croick, #frameworks, ossi
Cc: dfaure, lepagevalleeemmanuel, kde-frameworks-devel, sitter, michaelh, 
ngraham, bruns