D11235: [DrKonqi] Request change of ptrace scope from KCrash

[Oswald Buddenhagen]

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


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

REPOSITORY
  R871 DrKonqi

BRANCH
  ptracer

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

To: croick, #plasma_workspaces, #frameworks, ossi
Cc: ossi, lepagevalleeemmanuel, maximilianocuria, adridg, plasma-devel, 
jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D11235: [DrKonqi] Request change of ptrace scope from KCrash

[Christoph Roick]

croick edited the summary of this revision.

REPOSITORY
  R871 DrKonqi

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

To: croick, #plasma_workspaces, #frameworks, ossi
Cc: ossi, lepagevalleeemmanuel, maximilianocuria, adridg, plasma-devel, 
jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D11235: [DrKonqi] Request change of ptrace scope from KCrash

[Christoph Roick]

croick edited the summary of this revision.

REPOSITORY
  R871 DrKonqi

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

To: croick, #plasma_workspaces, #frameworks, ossi
Cc: ossi, lepagevalleeemmanuel, maximilianocuria, adridg, plasma-devel, 
jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D11235: [DrKonqi] Request change of ptrace scope from KCrash

[Christoph Roick]

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

REPOSITORY
  R871 DrKonqi

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

To: croick, #plasma_workspaces, #frameworks, ossi
Cc: ossi, lepagevalleeemmanuel, maximilianocuria, adridg, plasma-devel, 
jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D11235: [DrKonqi] Request change of ptrace scope from KCrash

[Christoph Roick]

croick updated this revision to Diff 49717.
croick added a comment.


  - swap variable declaration

REPOSITORY
  R871 DrKonqi

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11235?vs=49710=49717

BRANCH
  ptracer

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

AFFECTED FILES
  src/CMakeLists.txt
  src/debuggerlaunchers.cpp
  src/debuggerlaunchers.h
  src/ptracer.cpp
  src/ptracer.h
  src/tests/crashtest/crashtest.cpp

To: croick, #plasma_workspaces, #frameworks, ossi
Cc: ossi, lepagevalleeemmanuel, maximilianocuria, adridg, plasma-devel, 
jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D11235: [DrKonqi] Request change of ptrace scope from KCrash

[Christoph Roick]

croick marked an inline comment as done.

REPOSITORY
  R871 DrKonqi

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

To: croick, #plasma_workspaces, #frameworks, ossi
Cc: ossi, lepagevalleeemmanuel, maximilianocuria, adridg, plasma-devel, 
jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D11235: [DrKonqi] Request change of ptrace scope from KCrash

[Oswald Buddenhagen]

ossi added a comment.


  In D11235#395021 , @croick wrote:
  
  > 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.)

INLINE COMMENTS

> ptracer.cpp:63
> +
> +int bytes = 0, r;
> +while (bytes < msize) {

i meant this line ...
(but your change was good, too).

REPOSITORY
  R871 DrKonqi

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

To: croick, #plasma_workspaces, #frameworks, ossi
Cc: ossi, lepagevalleeemmanuel, maximilianocuria, adridg, plasma-devel, 
jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D11235: [DrKonqi] Request change of ptrace scope from KCrash

[Christoph Roick]

croick added a comment.


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

REPOSITORY
  R871 DrKonqi

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

To: croick, #plasma_workspaces, #frameworks, ossi
Cc: ossi, lepagevalleeemmanuel, maximilianocuria, adridg, plasma-devel, 
jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D11235: [DrKonqi] Request change of ptrace scope from KCrash

[Christoph Roick]

croick updated this revision to Diff 49710.
croick added a comment.


  - swap lines, declare array for response message where needed

REPOSITORY
  R871 DrKonqi

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11235?vs=49705=49710

BRANCH
  ptracer

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

AFFECTED FILES
  src/CMakeLists.txt
  src/debuggerlaunchers.cpp
  src/debuggerlaunchers.h
  src/ptracer.cpp
  src/ptracer.h
  src/tests/crashtest/crashtest.cpp

To: croick, #plasma_workspaces, #frameworks, ossi
Cc: ossi, lepagevalleeemmanuel, maximilianocuria, adridg, plasma-devel, 
jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D11235: [DrKonqi] Request change of ptrace scope from KCrash

[Oswald Buddenhagen]

ossi added a comment.


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

INLINE COMMENTS

> ptracer.cpp:61
> +char msg[msize], rmsg[msize];
> +int bytes = 0, r;
> +sprintf(msg, "%lld", debuggerpid);

this looks stupid, i'd swap the order.

REPOSITORY
  R871 DrKonqi

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

To: croick, #plasma_workspaces, #frameworks, ossi
Cc: ossi, lepagevalleeemmanuel, maximilianocuria, adridg, plasma-devel, 
jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D11235: [DrKonqi] Request change of ptrace scope from KCrash

[Christoph Roick]

croick updated this revision to Diff 49705.
croick edited the summary of this revision.
croick added a comment.


  - adjust warning and check for EINTR when polling

REPOSITORY
  R871 DrKonqi

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11235?vs=49651=49705

BRANCH
  ptracer

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

AFFECTED FILES
  src/CMakeLists.txt
  src/debuggerlaunchers.cpp
  src/debuggerlaunchers.h
  src/ptracer.cpp
  src/ptracer.h
  src/tests/crashtest/crashtest.cpp

To: croick, #plasma_workspaces, #frameworks, ossi
Cc: ossi, lepagevalleeemmanuel, maximilianocuria, adridg, plasma-devel, 
jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D11235: [DrKonqi] Request change of ptrace scope from KCrash

[Oswald Buddenhagen]

ossi added a comment.


  will also need to wait for commit message update, like the other change.

INLINE COMMENTS

> ptracer.cpp:92
> +
> +qCWarning(DRKONQI_LOG) << "debugged process did not acknowledge setting 
> ptracer to" << debuggerpid;
> +close(sfd);

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

REPOSITORY
  R871 DrKonqi

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

To: croick, #plasma_workspaces, #frameworks, ossi
Cc: ossi, lepagevalleeemmanuel, maximilianocuria, adridg, plasma-devel, 
jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D11235: [DrKonqi] Request change of ptrace scope from KCrash

[Christoph Roick]

croick updated this revision to Diff 49651.
croick marked 5 inline comments as done.
croick added a comment.


  - emit warning if no socket is available

REPOSITORY
  R871 DrKonqi

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11235?vs=49649=49651

BRANCH
  ptracer

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

AFFECTED FILES
  src/CMakeLists.txt
  src/debuggerlaunchers.cpp
  src/debuggerlaunchers.h
  src/ptracer.cpp
  src/ptracer.h
  src/tests/crashtest/crashtest.cpp

To: croick, #plasma_workspaces, #frameworks, ossi
Cc: ossi, lepagevalleeemmanuel, maximilianocuria, adridg, plasma-devel, 
jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D11235: [DrKonqi] Request change of ptrace scope from KCrash

[Christoph Roick]

croick added inline comments.

INLINE COMMENTS

> ossi wrote in ptracer.cpp:65
> you really could just use != here ... ^^

wow...

REPOSITORY
  R871 DrKonqi

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

To: croick, #plasma_workspaces, #frameworks, ossi
Cc: ossi, lepagevalleeemmanuel, maximilianocuria, adridg, plasma-devel, 
jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D11235: [DrKonqi] Request change of ptrace scope from KCrash

[Christoph Roick]

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


  - change order and add comment to kdeinit option in crashtest
  - add warning if debuggee does not respond as expected

REPOSITORY
  R871 DrKonqi

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11235?vs=49618=49649

BRANCH
  ptracer

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

AFFECTED FILES
  src/CMakeLists.txt
  src/debuggerlaunchers.cpp
  src/debuggerlaunchers.h
  src/ptracer.cpp
  src/ptracer.h
  src/tests/crashtest/crashtest.cpp

To: croick, #plasma_workspaces, #frameworks, ossi
Cc: ossi, lepagevalleeemmanuel, maximilianocuria, adridg, plasma-devel, 
jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D11235: [DrKonqi] Request change of ptrace scope from KCrash

[Oswald Buddenhagen]

ossi requested changes to this revision.
ossi added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> ptracer.cpp:65
> +bytes += r;
> +else if (r == -1 && !(errno == EINTR))
> +break;

you really could just use != here ... ^^

> ptracer.cpp:78
> +bytes += r;
> +else if (r == -1 && !(errno == EINTR))
> +break;

ditto

> ptracer.cpp:82
> +if (bytes == msize && memcmp(msg, rmsg, msize) == 0)
> +qCInfo(DRKONQI_LOG) << "ptracer set to" << debuggerpid 
> << "by debugged process";
> +}

a complementary warning if any of the steps goes wrong would seem appropriate.

> crashtest.cpp:139
>  
> -  //start drkonqi directly so that drkonqi's output goes to the console
> +  //start drkonqi directly by default so that drkonqi's output goes to the 
> console
>KCrash::CrashFlags flags = KCrash::AlwaysDirectly;

add comment: this can be disabled to be able to test kcrash's real default 
behavior.

> crashtest.cpp:143
>  flags |= KCrash::AutoRestart;
> +  if (parser.isSet(QStringLiteral("kdeinit")))
> +flags &= ~KCrash::AlwaysDirectly;

move that up, so the AlwaysDirectly stuff is in one block.

REPOSITORY
  R871 DrKonqi

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

To: croick, #plasma_workspaces, #frameworks, ossi
Cc: ossi, lepagevalleeemmanuel, maximilianocuria, adridg, plasma-devel, 
jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D11235: [DrKonqi] Request change of ptrace scope from KCrash

[Christoph Roick]

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


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

INLINE COMMENTS

> ossi wrote in crashtest.cpp:133
> this addition isn't used or explained anywhere for all i can tell.

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

> ossi wrote in 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.

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

REPOSITORY
  R871 DrKonqi

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

To: croick, #plasma_workspaces, #frameworks, ossi
Cc: ossi, lepagevalleeemmanuel, maximilianocuria, adridg, plasma-devel, 
jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D11235: [DrKonqi] Request change of ptrace scope from KCrash

[Christoph Roick]

croick updated this revision to Diff 49618.
croick added a comment.


  - use debuggee pid instead of DrKonqi pid for socket name
  - make sure to read/write all bytes from/to socket

REPOSITORY
  R871 DrKonqi

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11235?vs=49607=49618

BRANCH
  ptracer

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

AFFECTED FILES
  src/CMakeLists.txt
  src/debuggerlaunchers.cpp
  src/debuggerlaunchers.h
  src/ptracer.cpp
  src/ptracer.h
  src/tests/crashtest/crashtest.cpp

To: croick, #plasma_workspaces, #frameworks, ossi
Cc: ossi, lepagevalleeemmanuel, maximilianocuria, adridg, plasma-devel, 
jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D11235: [DrKonqi] Request change of ptrace scope from KCrash

[Oswald Buddenhagen]

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


  mark handled issues as done here as well.

INLINE COMMENTS

> ptracer.cpp:47
> +
> QStringLiteral("%1/kcrash_%2").arg(QStandardPaths::writableLocation(QStandardPaths::RuntimeLocation))
> +  
> .arg(QCoreApplication::applicationPid());
> +

see complementary patch

> ptracer.cpp:60
> +if (write(sfd, msg, 22) > 0) {
> +fd_set set;
> +FD_ZERO();

cruft

> ptracer.cpp:68
> +if (poll(, 1, 1000) > 0 && (fd.revents & POLLIN) &&
> +read(sfd, rmsg, 22) > 0 &&
> +strncmp(msg, rmsg, 22) == 0) {

i'd compare to 22, because we know that the other side sends that.

> ptracer.cpp:69
> +read(sfd, rmsg, 22) > 0 &&
> +strncmp(msg, rmsg, 22) == 0) {
> +qCInfo(DRKONQI_LOG) << "ptracer set to" << pid << "by 
> debugged process";

i'd use memcmp(), as it's a fixed-size data block.

> ptracer.h:22
> +
> +/** On Linux, tell the process to allow the debugger to attach to itr */
> +void setPtracer(qint64 pid);

-r

> crashtest.cpp:133
>parser.addOption(QCommandLineOption(QStringLiteral("autorestart"), 
> i18n("Automatically restart")));
> +  parser.addOption(QCommandLineOption(QStringLiteral("kdeinit"), i18n("Start 
> DrKonqi using kdeinit")));
>parser.addPositionalArgument(QStringLiteral("type"), i18n("Type of 
> crash."), QStringLiteral("crash|malloc|div0|assert|threads"));

this addition isn't used or explained anywhere for all i can tell.

REPOSITORY
  R871 DrKonqi

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

To: croick, #plasma_workspaces, #frameworks, ossi
Cc: ossi, lepagevalleeemmanuel, maximilianocuria, adridg, plasma-devel, 
jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D11235: [DrKonqi] Request change of ptrace scope from KCrash

[Christoph Roick]

croick updated this revision to Diff 49607.
croick edited the summary of this revision.
croick edited the test plan for this revision.
croick added a comment.


  - rename queryPtrace to setPtracer
  - add a kdeinit option in crashtest

REPOSITORY
  R871 DrKonqi

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11235?vs=34671=49607

BRANCH
  ptracer

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

AFFECTED FILES
  src/CMakeLists.txt
  src/debuggerlaunchers.cpp
  src/debuggerlaunchers.h
  src/ptracer.cpp
  src/ptracer.h
  src/tests/crashtest/crashtest.cpp

To: croick, #plasma_workspaces, #frameworks, ossi
Cc: ossi, lepagevalleeemmanuel, maximilianocuria, adridg, plasma-devel, 
jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D11235: [DrKonqi] Request change of ptrace scope from KCrash

[Oswald Buddenhagen]

ossi requested changes to this revision.
ossi added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> debuggerlaunchers.cpp:57
>  if ( pid > 0 ) {
> +queryPtrace(pid);
>  m_monitor->startMonitoring(pid);

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.

> queryptrace.cpp:67
> +
> +if (select(sfd + 1, , NULL, NULL, ) > 0 &&
> +read(sfd, msg, 3) > 0 && strcmp(msg, "OK") == 0) {

use poll() here, too.

> queryptrace.cpp:68
> +if (select(sfd + 1, , NULL, NULL, ) > 0 &&
> +read(sfd, msg, 3) > 0 && strcmp(msg, "OK") == 0) {
> +qCInfo(DRKONQI_LOG) << "ptrace granted by debugged process";

the current version of the complementary patch just echoes back the pid, so 
this cannot possibly work.

> croick wrote in queryptrace.cpp:26
> QTemporaryDir will not work, since the location is set in KCrash.

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

> queryptrace.h:22
> +
> +/** In Linux try to make the process attach to the debugger */
> +void queryPtrace(qint64 pid);

no. "on linux, tell the process to allow the debugger to attach to it".

> queryptrace.h:23
> +/** In Linux try to make the process attach to the debugger */
> +void queryPtrace(qint64 pid);
> +

that's a rather misleading function name. setPtracer() would reflect the 
purpose and upstream naming.

> croick wrote in crashtest.cpp:138
> That reasoning does not seem to be true. The output can still be read when 
> started "normally".

only if kdeinit was started from the same console, which you cannot assume.

also, this change wouldn't belong into this patch anyway.

REPOSITORY
  R871 DrKonqi

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

To: croick, #plasma_workspaces, #frameworks, ossi
Cc: ossi, lepagevalleeemmanuel, maximilianocuria, adridg, plasma-devel, 
sukalyanbanga, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D11235: [DrKonqi] Request change of ptrace scope from KCrash

[Harald Sitter]

sitter added a subscriber: lepagevalleeemmanuel.

REPOSITORY
  R871 DrKonqi

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

To: croick, #plasma_workspaces, #frameworks
Cc: lepagevalleeemmanuel, maximilianocuria, adridg, plasma-devel, ragreen, 
Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
mart

D11235: [DrKonqi] Request change of ptrace scope from KCrash

[Christoph Roick]

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


  - check size of socket path

REPOSITORY
  R871 DrKonqi

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11235?vs=34670=34671

BRANCH
  ptracer

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

AFFECTED FILES
  src/CMakeLists.txt
  src/debuggerlaunchers.cpp
  src/debuggerlaunchers.h
  src/queryptrace.cpp
  src/queryptrace.h
  src/tests/crashtest/crashtest.cpp

To: croick, #plasma_workspaces, #frameworks
Cc: maximilianocuria, adridg, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D11235: [DrKonqi] Request change of ptrace scope from KCrash

[Christoph Roick]

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


  Thank you for your comments, totally forgot about licensing.

INLINE COMMENTS

> maximilianocuria wrote in queryptrace.cpp:26
> Please honour TMPDIR or, even better, use QTemporaryDir (this will also make 
> the generated path unpredictable), and create the socket inside.

QTemporaryDir will not work, since the location is set in KCrash.

REPOSITORY
  R871 DrKonqi

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

To: croick, #plasma_workspaces, #frameworks
Cc: maximilianocuria, adridg, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D11235: [DrKonqi] Request change of ptrace scope from KCrash

[Christoph Roick]

croick updated this revision to Diff 34670.
croick added a comment.


  - add license text
  - use `QDir::tempPath()`
  - ifdefs to definition

REPOSITORY
  R871 DrKonqi

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11235?vs=30979=34670

BRANCH
  ptracer

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

AFFECTED FILES
  src/CMakeLists.txt
  src/debuggerlaunchers.cpp
  src/debuggerlaunchers.h
  src/queryptrace.cpp
  src/queryptrace.h
  src/tests/crashtest/crashtest.cpp

To: croick, #plasma_workspaces, #frameworks
Cc: maximilianocuria, adridg, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D11235: [DrKonqi] Request change of ptrace scope from KCrash

[Maximiliano Curia]

maximilianocuria added inline comments.

INLINE COMMENTS

> queryptrace.cpp:26
> +server.sun_family = AF_UNIX;
> +sprintf(server.sun_path, "/tmp/kcrash_%lld", 
> QCoreApplication::applicationPid());
> +if (::connect(sfd, (struct sockaddr *), sl) == 0) {

Please honour TMPDIR or, even better, use QTemporaryDir (this will also make 
the generated path unpredictable), and create the socket inside.

REPOSITORY
  R871 DrKonqi

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

To: croick, #plasma_workspaces, #frameworks
Cc: maximilianocuria, adridg, plasma-devel, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D11235: [DrKonqi] Request change of ptrace scope from KCrash

[Adriaan de Groot]

adridg added a comment.


  Comments directed at licensing and non-Linux cases.

INLINE COMMENTS

> queryptrace.cpp:1
> +#include 
> +

Missing file header / license bits

> queryptrace.cpp:49
> +
> +#endif

For non-Linuxen, include a null implementation here.

> queryptrace.h:1
> +#ifndef QUERYPTRACE_H
> +#define QUERYPTRACE_H

Missing a file header / license bits

> queryptrace.h:5
> +#ifdef Q_OS_LINUX
> +void queryPtrace(long long pid);
> +#else

You used qint64 elsewhere

> queryptrace.h:7
> +#else
> +#define queryPtrace(x)
> +#endif

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.

REPOSITORY
  R871 DrKonqi

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

To: croick, #plasma_workspaces, #frameworks
Cc: adridg, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D11235: [DrKonqi] Request change of ptrace scope from KCrash

[Christoph Roick]

croick updated this revision to Diff 30979.
croick added a comment.


  - update socket file location

REPOSITORY
  R871 DrKonqi

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11235?vs=29249=30979

BRANCH
  ptracer

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

AFFECTED FILES
  src/CMakeLists.txt
  src/debuggerlaunchers.cpp
  src/debuggerlaunchers.h
  src/queryptrace.cpp
  src/queryptrace.h
  src/tests/crashtest/crashtest.cpp

To: croick, #plasma_workspaces, #frameworks
Cc: plasma-devel, ragreen, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D11235: [DrKonqi] Request change of ptrace scope from KCrash

[Christoph Roick]

croick added inline comments.

INLINE COMMENTS

> crashtest.cpp:138
>  
> -  //start drkonqi directly so that drkonqi's output goes to the console
> -  KCrash::CrashFlags flags = KCrash::AlwaysDirectly;

That reasoning does not seem to be true. The output can still be read when 
started "normally".

REPOSITORY
  R871 DrKonqi

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

To: croick, #plasma_workspaces, #frameworks
Cc: plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D11235: [DrKonqi] Request change of ptrace scope from KCrash

[Christoph Roick]

croick updated this revision to Diff 29249.
croick added a comment.


  - remove superfluous include

REPOSITORY
  R871 DrKonqi

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11235?vs=29247=29249

BRANCH
  ptracer

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

AFFECTED FILES
  src/CMakeLists.txt
  src/debuggerlaunchers.cpp
  src/debuggerlaunchers.h
  src/queryptrace.cpp
  src/queryptrace.h
  src/tests/crashtest/crashtest.cpp

To: croick, #plasma_workspaces, #frameworks
Cc: plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D11235: [DrKonqi] Request change of ptrace scope from KCrash

[Christoph Roick]

croick edited the test plan for this revision.

REPOSITORY
  R871 DrKonqi

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

To: croick, #plasma_workspaces, #frameworks
Cc: plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart

D11235: [DrKonqi] Request change of ptrace scope from KCrash

[Christoph Roick]

croick created this revision.
croick added reviewers: Plasma: Workspaces, Frameworks.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.
croick requested review of this revision.

REVISION SUMMARY
  - In Linux recent kernels only allow attaching of processes to a debugger, if
- the right to attach to processes is enabled system-wide
- the debugger is a child process of the debuggee
- or the debuggee grants access by a call to prctl(PR_SET_PTRACER, 
debugger_pid, 0, 0, 0);
  - DrKonqi cannot do this directly, so it will ask the debuggee to do so by a 
socket connection.
  - When the debugger has finished, the access is requested back to DrKonqi to 
allow internal backtraces again.

TEST PLAN
  - try to attach the debugged process to gdb or KDevelop by using the debug 
button: operation will not be permitted
  - apply corresponding patches to KCrash and DrKonqi
  - the process can now be debugged
  - after detaching the process again, a backtrace can be created using the 
DrKonqi dialog as usual

REPOSITORY
  R871 DrKonqi

BRANCH
  ptracer

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

AFFECTED FILES
  src/CMakeLists.txt
  src/debuggerlaunchers.cpp
  src/debuggerlaunchers.h
  src/queryptrace.cpp
  src/queryptrace.h
  src/tests/crashtest/crashtest.cpp

To: croick, #plasma_workspaces, #frameworks
Cc: plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol, mart