D11236: [KCrash] Establish socket to allow change of ptrace scope

[Oswald Buddenhagen]

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


  yay, i finally have "some" time for this. ^^
  
  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.
  
  the socket code seemed like an unnecessary complication at first, but then i realized that launching via kdeinit makes it impossible to just talk to the child via stdout/stderr or at least pass a file descriptor of a socketpair end. that's something that could be actually addressed in the klauncher api, because it's possible to pass open fds to other processes over sockets.

INLINE COMMENTS

> kcrash.cpp:655
>              //if the process was started directly, use waitpid(), as it's a child...
>              while (waitpid(-1, nullptr, 0) != pid) {}
>          } else {

you need to cover that branch as well.

> kcrash.cpp:668
> +            // create socket path to transfer ptrace scope and open connection
> +            const QByteArray socketpath = QStringLiteral("%1/kcrash_%2").arg(QDir::tempPath()).arg(pid).toLocal8Bit();
> +            int sockfd = openDrKonqiSocket(socketpath);

use RuntimeLocation, as the kdeinit-related code (at start of setCrashHandler()) does.

> kcrash.cpp:870
> +
> +    if (bind(sockfd, (struct sockaddr *)&drkonqi_server, sizeof(drkonqi_server)) < 0) {
> +        perror("Warning: bind() for communication with DrKonqi failed");

you need to unlink first, otherwise stale sockets will throw you off.

> kcrash.cpp:884
> +{
> +    fd_set set;
> +    FD_ZERO(&set);

why don't you use poll()? this code is linux-specific anyway, so you can rely on posix functions from the previous decade.

> kcrash.cpp:904
> +        clsockfd = accept(sockfd, (struct sockaddr *)&drkonqi_client, &cllength);
> +    } while (clsockfd == -1 && (errno == EINTR || errno == EAGAIN));
> +    if (clsockfd < 0)

EAGAIN is a workaround for some unspecified broken non-linux systems, so you can drop that here.

> kcrash.cpp:915
> +    if (ucred.pid != pid) {
> +        perror("Warning: peer pid does not match DrKonqi pid");
> +        return -1;

that's bogus use of perror()

REPOSITORY
  R285 KCrash

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

To: croick, #frameworks, ossi
Cc: dfaure, lepagevalleeemmanuel, kde-frameworks-devel, sitter, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20190114/9aed5839/attachment-0001.html>