Disable rt_trapExceptions when started inside a debugger on Linux

[adamdruppe]

The implementation is borrowed from Stack Overflow (which is permissively licensed, we must give them attribution which is the same as Boost), and follows the same pattern we do on Windows - if we start in a debugger, don't trap the exception, letting the debugger handle it when it is thrown.

This gives us an interactive debugging session at the throw point, which is of significantly more value than the limited info we get from printing the string.

See also:
http://arsdnet.net/this-week-in-d/2016-aug-07.html

[Burgos]

Very useful, thanks!

[Burgos]

Just debugger_present should be enough

[Burgos]

return false

[Burgos]

scope (exit) close(status_fd);

[dnadlinger]

A bit further D-ified:

// Ported from http://stackoverflow.com/a/24969863/1457000
bool isDebuggerPresent()
{
    import core.sys.posix.fcntl;
    import core.sys.posix.unistd;

    immutable statusFd = open("/proc/self/status", O_RDONLY);
    if (statusFd == -1) return false;
    scope(exit) close(statusFd);

    char[1024] buf = void;
    immutable numRead = read(statusFd, buf.ptr, buf.length - 1);
    if (numRead <= 0) return false;
    buf[numRead] = 0;

    immutable prefix = "TracerPid:";
    immutable tracerLine = strstr(buf.ptr, prefix.ptr);
    return tracerLine && !!atoi(tracerLine + prefix.length);
}

(Single-line blocks might of course not be druntime style.)

[adamdruppe]

Can any of you even read the spam that is the auto tester output? Where's the failure?

make[2]: *** [generated/linux/debug/32/line_trace.done] Error 1

that it? Is it comparing EXACT line numbers in the output?!?!

ugh this is why i hate even trying to contribute to D through official channels.

[dnadlinger]

This reads a bit confusing to me, as IsDebuggerPresent doesn't follow the usual convention (so makes my brain think of the WinAPI function immediately).

Furthermore, the duplicated version block is a bit clunky. Why not make it a

bool tryDetectDebugger()
{
    version (Windows) return IsDebuggerPresent();
    else version(linux) …
    else return false;
}

that is then unconditionally invoked in _d_run_main? The latter already is a big unwieldy mess.

[adamdruppe]

On Fri, Apr 14, 2017 at 12:02:24PM -0700, David Nadlinger wrote: that is then unconditionally invoked in _d_run_main?

Yeah, I can live with that, I'll do it in my next draft later.

[adamdruppe]

@klickverbot I used your modification and changed it to an unconditional try test.

[dnadlinger]

Thanks. I was secretly hoping you could come up with a better name than tryDetectDebugger to express the fact that the detection is conservative.

@MartinNowak (and the other usual suspects): It seems like reading from /proc/self/status on every application startup shouldn't be problematic, given that we just fail gracefully, or am I missing something?

[adamdruppe]

On Fri, Apr 21, 2017 at 09:44:02AM -0700, David Nadlinger wrote: Thanks. I was secretly hoping you could come up with a better name than `tryDetectDebugger` to express the fact that the detection is conservative.

oh, meh, it is a private internal function, I don't put a lot of thought into those names regardless. But I think tryDetectDebugger is decent regardless.

@MartinNowak (and the other usual suspects): It seems like reading from `/proc/self/status` on every application startup shouldn't be problematic, given that we just fail gracefully, or am I missing something?

Remember too that that's just asking the kernel for some information; the /proc filesystem is special and not subject to normal file read woes.

[Burgos]

Ping on this, this is very useful thing to have IMHO.

[adamdruppe]

Ugh, the stupid tests actually are sensitive to exact line numbers in the code.

Also, the autotester website could get two really easy improvements: 1: make the log full size, it has a css seamless but that's invalid, it needs to be on the html, and 2: make the log actually show errors somewhere easy to find instead of hidden in the middle. I'd prefer them at the end, but even a highlight of fail lines would be helpful so you can find them in a search.

[dnadlinger]

Remember too that that's just asking the kernel for some information;
the /proc filesystem is special and not subject to normal file read
woes.

I was thinking about a situation where the executed program doesn't have some capabilities enabled, but if somebody configures their system/container to kill processes if they access /proc/self, I guess they are aware that they might be in for a surprise. Should be fine.

[PetarKirov]

LGTM too

[nemanja-boric-sociomantic]

Also see this one: #1673

I think this is a fine patch because together with this patch it would actually cause system to abort and debugger to break on unhandled exception.

[adamdruppe]

So, whats the hold up?

[CyberShadow]

Would this be triggered by things such as strace?

Maybe in addition to or instead of this, controlling whether the runtime installs a top-level exception handler should be controlled by an environment variable or switch (similar to the --DRT-... ones we already have for the GC).

[schveiguy]

Would this be triggered by things such as strace?

Yes:

$ strace cat /proc/self/status
execve("/bin/cat", ["cat", "/proc/self/status"], [/* 52 vars */]) = 0
brk(0)                                  = 0x21b4000
...
write(1, "Name:\tcat\nState:\tR (running)\nTgi"..., 868Name:	cat
State:	R (running)
Tgid:	97143
Ngid:	0
Pid:	97143
PPid:	97140
TracerPid:	97140
...
close(3)                                = 0
close(1)                                = 0
close(2)                                = 0
exit_group(0)                           = ?
+++ exited with 0 +++

Note PPid (which should be pid of strace) and TracerPid are set to the same value.

[adamdruppe]

Would this be triggered by things such as strace?

strace is a debugger-lite, so it makes sense to me still.

Maybe in addition to or instead of this, controlling whether the runtime installs a top-level exception handler should be controlled by an environment variable or switch (similar to the --DRT-... ones we already have for the GC).

I would like that in addition to it, definitely. I don't want instead because in the debugger, I basically always want the signal since it is useful for that, and outside the debugger, running all the finally blocks is desired.... but cases like strace could go either way, so the switch is perfect for that.

I'll do the DRT switch myself as an additional PR after this, or a second commit in this one since they are related, regardless it isn't hard to change the if condition..

[nemanja-boric-sociomantic]

See #1538 #1674

[adamdruppe]

Question asked and answered. Still held up. Why?

[CyberShadow]

strace is a debugger-lite, so it makes sense to me still.

No, that doesn't make any sense at all. The purpose of this change is to make uncaught exceptions result in a useful debugging session when ran from an actual debugger. When the program crashes when it is run from strace, the only result is a completely unhelpful error message and user confusion.

Furthermore:

• /proc/*/status is Linux-specific, and this method won't work on other POSIX OSes.
• /proc/*/status looks like it was meant primarily to be consumed by humans, I'm not sure if its format is stable for parsing.
• Opening and reading /proc/self/status, no matter how small the overhead, is still going to be an operation that all D programs do on every startup to perform a check that in 99.99% of cases will be false
• Never minding the difference in exception handling, it clutters the strace/ltrace output of all D programs.

So, I'm really not sure this is a good idea - the Windows check (IsDebuggerPresent) simply doesn't seem to map anywhere as cleanly on Linux or POSIX.

[adamdruppe]

On Tue, Jun 20, 2017 at 08:35:35AM -0700, Vladimir Panteleev wrote: - /proc/*/status is Linux-specific, and this method won't work on other POSIX OSes.

That's why it is under `version(linux)`.

- /proc/*/status looks like it was meant primarily to be consumed by humans, I'm not sure if its format is stable for parsing. - Opening and reading /proc/self/status, no matter how small the overhead, is still going to be an operation that all D programs do on every startup to perform a check that in 99.99% of cases will be false

Those are worth it to me, but I can see your point. I can also live with the --DRT switch (see linked PRs) since I'd then just pass it there.

So, I'm really not sure this is a good idea - the Windows check (IsDebuggerPresent) simply doesn't seem to map anywhere as cleanly on Linux or POSIX.

In that case, let's just make an executive decision and close it. I (and probably many others btw) would prefer a solid "No. closed." than silence as merge conflicts creep in....

[nemanja-boric-sociomantic]

• /proc/*/status looks like it was meant primarily to be consumed by humans, I'm not sure if its format is stable for parsing.

This format should be stable for parsing, as it is concerned as an interface of the kernel, and there should be no breakage (as long as you allow for the appending to this file). Lots of programs (canonical examples would be top or netstat) are parsing these files in order to get to work.

[CyberShadow]

I (and probably many others btw) would prefer a solid "No. closed." than silence as merge conflicts creep in....

I think only Walter and Andrei can do that, and they can't review every PR individually, so it's left to the community in many cases; in cases like these where there are arguments for and against, the best answer is often unclear, so conclusions can be postponed in hope that more arguments emerge and an answer becomes clearer. Authoritative decision-making also has pitfalls - no single person can be an expert in every field, and unless it concerns core policy things such as pertaining to branding or the foundation, a sub-optimal decision that many disagree with will not sit well in the eyes of other contributors.

I agree that the switch is more useful and makes this change less necessary.

BTW, has the method of ptrace(PTRACE_TRACEME) / ptrace(PTRACE_DETACH) been considered? That might be more portable and wouldn't involve file descriptors.

[adamdruppe]

On Tue, Jun 20, 2017 at 05:59:52PM -0700, Vladimir Panteleev wrote: Authoritative decision-making also has pitfalls - no single person can be an expert in every field, and unless it concerns core policy things such as pertaining to branding or the foundation, a sub-optimal decision that many disagree with will not sit well in the eyes of other contributors.

You wouldn't necessarily have one authority for everything, you can have a bunch of people who say no (or yes) in some area they've taken ownership of. I generally trust your technical judgment, so if you said no here, I'd be ok with that.

BTW, has the method of `ptrace(PTRACE_TRACEME)` / `ptrace(PTRACE_DETACH)` been considered? That might be more portable and wouldn't involve file descriptors.

By my reading of the man page, if traceme successfully attaches, the process cannot detach itself; it'd be left to the parent process, which is likely a shell that has no idea what's going on (unless you fork() yourself... which defeats the elegance of it again). I'm nervous to do that; at least the file descriptor doesn't have unknown effects outside the startup function.

[CyberShadow]

By my reading of the man page, if traceme successfully attaches, the process cannot detach itself; it'd be left to the parent process, which is likely a shell that has no idea what's going on (unless you fork() yourself... which defeats the elegance of it again). I'm nervous to do that; at least the file descriptor doesn't have unknown effects outside the startup function.

Oops, you're right, that's definitely not going to work.

I generally trust your technical judgment, so if you said no here, I'd be ok with that.

Well, I'm not myself convinced that this is something we don't want despite the issues presented; but I guess since it's harder to take something out once it got in, it'd be safer to avoid such low-level changes we're not certain about.