Skip to content
This repository

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Sigsegv as exception #187

Merged
merged 14 commits into from over 1 year ago
deadalnix

When core.nullpointererror is imported in a project, it transform null deference into NullPointerError and other segfault in SignalError on linux x86 and x86_64 .

If the idea is successful, and implemented on other systems (windows, macOS, freeBSD) it could become the standard behavior. For now, it only behave that way when the module is explicitly imported.

This is realted to : #181 that should be included before.

Martin Nowak
Collaborator
  • Translating signals to exceptions is highly questionable.
    It was already a bad decision on windows and we shouldn't
    try to emulate it.

  • I'm rather worried about the ABI instabilities of ucontext_t.

  • Signal handler's are set per process.

    • you need to do something with existing handlers
    • what if a segfault occurs in a non-D thread
src/core/nullpointererror.d
((60 lines not shown))
  60
+		REG_EDX,
  61
+		REG_ECX,
  62
+		REG_EAX,
  63
+		REG_TRAPNO,
  64
+		REG_ERR,
  65
+		REG_EIP,
  66
+		REG_CS,
  67
+		REG_EFL,
  68
+		REG_UESP,
  69
+		REG_SS
  70
+	}
  71
+}
  72
+
  73
+// Init
  74
+
  75
+shared static this() {
2
Martin Nowak Collaborator

This is odd.
It means that this mechanism is activated by importing the module.
Instead it should be a function like installHandler.

deadalnix
deadalnix added a note March 28, 2012

That is fine to me. It can definitively be changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
deadalnix

Translating signals to exceptions is highly questionable. It was already a bad decision on windows and we shouldn't try to emulate it.

I think your point here is very weak. No argument whatsoever here. Let me state why I think it is a good idea.

  • it allow to recover from a null pointer deference. This isn't an error that screw up the state of your program and it is definitively recoverable. Throw allow to recover.
  • it provide a stacktrace for when things goes wrong. Even if you are not running tje program using a debugger.
  • you can exit your program nicely on null deference.
  • If you call something that is @safe, unless things goes REALLY wrong, in way that your program can do little about, you can now ensure that things will not crash. (deferencing null is @safe ).

Additionally, it opens some doors :

  • it is now possible to implement thing in a consistent manner on windows and linux, making easier to write cross plateform code.
  • the proposed code allow to do something else than throw. This can evolve in custom handler depending on the memory block (something similar to libsigsegv).
  • This type of tricks are mandatory to implement concurrent GC. It is a direction we want to go in.

Information have been asked to linux hackers, ucontext_t is supported, even if not very documented. You can ask feep if you doubt my claim.

About existing handler, I think an user playing with signal handler is big enough to know what he/she is doing and be sure to not collide with this. If the segfault occur in non-D thread (or even in non D code) things will work as well if you recover. If you throw this will go wrong, as D's Exception are not compatible with C/C++. Anyway, without the Exception, things goes wrong as well. A global thread local flag can be set to ensure this behavior doesn't trigger in non D threads.

Martin Nowak
Collaborator

it allow to recover from a null pointer deference.

No you cannot because you corrupt your complete program state at the point
you do not return from the signal. You might only call async safe functions after
that.

http://www.google.com/search?q=bug%20signal%20handler%20deadlock

provide a stacktrace for when things go

If you want to improve the current behavior then try to print
useful information from within the signal handler.
That will be hard but it might be possible, among other functions
you cannot use malloc/printf.

deadalnix

I think you totally misunderstood the piece of code you are commenting. The whole point of the code in the handler is to provide a way for another piece of code OUTSIDE the handler, to handle the segfault.

The state of the program isn't corrupted, so it is safe to throw and even to recover from it. I even have a piece of code where I use mprotect to trigger segfault, and totally recover without throwing, using the exact same method.

It is recoverable, just try it if you don't believe me.

Martin Nowak
Collaborator

Yeah, you only reset the instruction pointer but that isn't much different
from performing a longjmp from inside the signal handler.

You still interrupt functions at arbitrary points, and exception handling
won't help you with scope cleanup because the code is not prepared
to handle them. These are similar issues as with exception-unsafe code,
only that the compiler will make it even worse and it may occur at
any instruction.

Logger logger;
void setLogger()
{
    logger = new Logger;
    scope (failure) logger = null;
    logger.init(getSomething());
}

If getSomething caused a segfault your program is corrupted.
Nobody will clean up the initializer, because the compiler thinks
getSomething is nothrow.

Likewise if a segfault happened in malloc you may not call it again.
It isn't async-safe and you have left it in whatever state it was.

deadalnix

No, this isn't like longjmp ! In that case, you know that the whole program is back in the right state (longjmp doesn't guarantee that).

And the only registers altered are trash registers, so it is safe in regard of try catch blocks. For you example, it currently works (tested as well) with the code generated by dmd.

I feel like you are making up problems that doesn't even exists here, and you don't even consider the advantages that such a mechanism provide.

JakobOvrum

If the idea is successful, and implemented on other systems (windows, macOS, freeBSD) it could become the standard behavior.

This is already implemented on Windows as the default behaviour. This needs more thought; with this patch it will even throw different exceptions on Linux and Windows, which is a completely unnecessary inconsistency.

deadalnix

Yes, this is inconsistent. And yes, this needs to be made consistent with windows.

The point is that the exception thrown on windows is system specific. I would rather think that both should throw a NullPointerError on null deference, so we have a system agnostic way of catching this. This definitively need to converge.

I didn't wanted to change existing behavior on windows, so this approach can be tested without breaking anything. Ultimately, this is linked to the problem of @safe that allow unsafe things to be done when passed large objects/pointers that are null. What must be done to solve that problem is something we have to decide before changing the existing behavior, or we risk to change it twice, which is something we want to avoid.

FeepingCreature

For reference, I cite ##kernel on freenode: http://pastebin.com/VEeZYPRJ . So it seems to be "officially" supported.

The main advantage over longjmp is that it definitely ends up with the handler function called from a state that is "safe", since it reuses the kernel's existing return-to-previous-context mechanism. So you don't have to emulate whatever cleanup the kernel does at signal handler exit.

Andrei Alexandrescu
Owner

Undecided on what to do about this. Should we discuss the matter further in the newsgroup? I know @WalterBright has a dim view on converting null pointer accesses into exceptions.

Andrei Alexandrescu
Owner

(it ain't rebased either :o))

deadalnix

Indeed, this pull request involve a language design decision, and may be discussed in the newsgroup. The problem to be fixed is in fact much larger than what this pull request does, it is about the whole null deference handling in D.

This pull request open the door for unified behavior between linux and windows. It is also easy to provide a callback to do whatever is wanted (HALT if one think is better, or throw). It is even possible to recover in some situation (concurrent GC is a great use case of that capability for instance).

Note that I use this in all code I produce in D until then, and it have been of great help.

deadalnix

I rebased the pull request. git wasn't able to merge the stuff by itself.

Martin Nowak
Collaborator

There are still tons of issues.

Your signal handler is global.

  • Do you throw D exceptions in any language?
  • What to do when you overwrite an existing signal?

This applies to all D executables and all D libraries.

  • Would you want libfontconfig to steal your sigsegv handler?

You'll get deadlocks which are even worse than crashing.

  • What to do w.r.t. signal-unsafe functions?

You cannot recover without unwinding/destruction support.

  • How could this be implemented with enregistered variables?

  • Why shall we make a non-standard unsafe function which will cause difficult to find
    bugs the default behavior when you can make it a library?

This has absolutely no place in production code because it is unpredictable and unreliable.

deadalnix

I'm sorry, but I don't think most of this are problems.

First, yes, it throw D exception, but not in any language, because IT IS IN D RUNTIME. So it throw D exception in D. If you interface D with other languages, then you have to make sure that the interfacing make sense. This is no news and have nothing to do with that pull request specifically.

As of 3rd party lib stealing the signal handler, it is a possibility. However, the signal mechanism is already used in druntime. So either we consider this is a problem, or we don't. But this is rather stupid to state this is a problem when this is done all over the place.

Many other remarks show that you don't understand what is going on here. The whole mechanism is here to set up a function call on top of the instruction that caused the fault. So everything happen in userland, not in the signal handler. Signal usafe function will work properly, and no deadlock will occur.

I'm sorry, but I see nothing but FUD in your comment. You should come with actual fact here.

FeepingCreature

I have to agree with this. See my earlier paste from ##kernel - it's standard, albeit uncommon. The technique works on any operating system that uses the basic x86 stackframe layout, which, I think, is more systems than D runs on - and it's not like "but this only works on Linux" stopped people when it came to supporting SEH. And the entire point of this is to sidestep the issue of signal safety. Let's not get into flames - but please make sure you have read the entire thing before objecting.

deadalnix

@dawgfoto please excuse me for the rudeness of my previous message. I let it here to keep the discussion understandable, but It was way too aggressive. I'm sorry.

To restate thing in a more neutral way : I have nothing against this not being included, but it have to be discussed and the decision must be taken based on actual hard facts. Please feel free to ask any question about the technical aspect of things, because your post contained inaccurate informations. As I'm pretty sure it was not done on purpose, I guess we simply have a misunderstanding. So let's start again on good basis, and please forgive my tone.

Andrei Alexandrescu
Owner

@deadalnix Thanks very much. All - let's keep the good spirits going! I'll ask Walter what he thinks about the idea. Is this implementable on all major OSs?

deadalnix

On windows, the system already throw an Exception when this happen. A similar mechanism can be implement on windows without much problems.

I'm not qualified enough on FreeBSD or macOS to answer that question.

Martin Nowak
Collaborator

Many other remarks show that you don't understand what is going on here. The whole mechanism is here to set up a function call on top of the instruction that caused the fault. So everything happen in userland, not in the signal handler. Signal usafe function will work properly, and no deadlock will occur.

void* p = void; // uninitialized
core.stdc.free(p);
// malloc.c
void free(void* p)
{
    // ...
    rlock_acquire(&malloc_mtx);
    size_t len = *cast(size_t)(p-1); // SIGSEGV
}

Now your preemptively exited a signal-unsafe function and you have no way of repairing it.
From now on every call to malloc/realloc/calloc/free might dead lock.

A set of functions that you may call is listed in signal(7) - Async-signal-safe functions.

it allow to recover from a null pointer deference. This isn't an error that screw up the state of your program and it is definitively recoverable. Throw allow to recover.

If your talking about recovering you need a mechanism to unwind the stack and restore state.
The one used for synchronous exceptions doesn't scale to asynchronous exceptions
because the compiler has to dump all variables to the stack in order to access them from exception handlers.

See my earlier paste from ##kernel - it's standard, albeit uncommon.
Altering the instruction pointer and continuing execution somewhere else is not the issue.

Martin Nowak
Collaborator

Is this implementable on all major OSs?

It's common that sigreturn restores the CPU context from the signal's ucontext_t as far as security allows.
FreeBSD - sys_sigreturn
OSX

deadalnix

I understand your example with malloc/free. You have to understand that in this case, whatever happen, you are doomed. Either the program crash either it is in an inconsistent state. It is a situation where catching the NullPointerError make no sense t all, because you can't recover from it.

Not having this behavior will not solve the problem, because you'll also be in an inconsistent state or you'll crash. This isn't any better and I don't see how this pull request is making things worse. A invalid call to a system function have been made, whatever comes out from that is either a crash or a inconsistent state.

Note that those function are signal unsafe. And this pull request don't change in any way if signal are send or received, it just change how they are handled. At the moment the code present in this pull request start to execute, the arm is already done, and the program is already in a beyond repair state. It is in that state because of the signal, not because of the code present in the pull request, so I don't see how it is an argument for or against it.

Another fact here is that the exception is thrown from C code and C don't handle exception. This problem isn't specific to this pull request, this is a problem that can occur every time an exception is thrown throw C code. And this problem is unsolvable, as C don't support exceptions. It is up to the programmer to ensure that exception are not throw throw C code.

This is exactly why I inherited NullPointerError from Error and not from Exception. They are not always recoverable. But they always are in @safe code.

Martin Nowak
Collaborator

This isn't any better

On an automated system deadlocks are worse than failures.
So we'd need either an opt-in or an opt-out switch.

By the way could you rephrase the purpose of translating signals to errors?
If it's error reporting you could do way more advanced stuff using dumps, execve, fork and ptrace.
For example you could restart the process in an error-reporting mode. You could also fork it first and
enable ptrace so that you may inspect memory from the reporter.

immutable pid = fork();
if (pid)
{
    char[11] buf=void;
    format(buf, pid);

    const char* args[3];
    args[0] = argv[0]; // C argv (this has issues with chdir, deleted images, changed rights...)
    args[1] = "--druntime-report-error";
    args[2] = buf.ptr;
    execve(args[0], args.ptr, environ);
}
else
{
    ptrace(PT_TRACE_ME);
    abort();
}
FeepingCreature

Personally: exceptions/errors have backtraces. Backtraces are immensely useful (no, gdb is not the answer). It'd also add consistency with Windows.

Look. Of course we can get backtraces for segfaults under linux with effort. But this patch allows us to get backtraces without effort, and that level of trivial convenience has a quality of its own.

About your free example: worst case, you can always inspect the stack, see if you're called from free(), and manually unlock. In point of fact, I think C free under Linux can tell that its argument is not a valid pointer and give a proper error [edit my mistake: it does segfault]. That aside, without this patch it just dies. With this patch it maybe dies if the exception is uncaught. All it does is add the option to do proper cleanup. And generally speaking, if you're catching an Error you deserve what you get. It's a big red flag that says "This guy think he know what he doin".

Martin Nowak
Collaborator

The point is that creating a backtrace might be enough to do way more harm.
Reliable crash information can be generated from another process (FF, Chrome, Ubuntu, OSX, Windows...).
If we come up with a solid solution it might be useful for deployed applications too.

How about a library solution, for example?
http://code.google.com/p/google-breakpad/

deadalnix

I also think that should evolve to provide a custom handler for advanced user.

As of now, it is in opt-in, you have to include this module somewhere to « activate » it.

Martin Nowak
Collaborator

Why doesn't this simply restore BP and call _d_throwc from within the signal handler?
That would also allow us to use sigaltstack for handling stack overflows.

deadalnix

Throwing from the signal handler isn't safe. You aren't in a standard execution flow, and the kernel knows it. Yes the code can be extended to manage stack overflows, and should be IMO.

Martin Nowak
Collaborator

Throwing from the signal handler isn't safe.

For the same reason that this mechanism is unsafe or am I missing something.

You are in a standard execution flow, and the kernel knows it.

What do you mean by that?

deadalnix

Throw from the signal handler is always unsafe, because you aren't in a regular execution flow.

This mecanism is unsafe in situation that are unsafe anyway, with or without. For instance, calling free with an invalid parameter will cause this mecanism to be unsafe. But it is unsafe anyway.

In @safe code, this mecanism is 100% safe. Throwing from the signal handler isn't in such a situation.

Martin Nowak
Collaborator

Throw from the signal handler is always unsafe, because you aren't in a regular execution flow.

Do you mean the signal stack frame?
We don't need to execute on the original stack to unwind it.

This mecanism is unsafe in situation that are unsafe anyway, with or without. For instance, calling free with an invalid parameter will cause this mecanism to be unsafe. But it is unsafe anyway.

One wouldn't trap a segmentation fault if the executed code was alright.

In @safe code, this mecanism is 100% safe. Throwing from the signal handler isn't in such a situation.

A segfault in @safe code implies that you invoked that code with invalid arguments or an invalid program state. The reason executing any further code, within or after the signal handler, is unsafe remains the same.
Because you exited code with an asynchronous exception any invariant might be broken, malloc being a simple example.
Because the fault might be a result of an earlier error one cannot assume localized corruption.

http://www.gnu.org/software/libc/manual/html_node/Nonreentrancy.html#Nonreentrancy
http://www.gnu.org/software/libc/manual/html_node/Defining-Handlers.html#Defining-Handlers
http://pubs.opengroup.org/onlinepubs/009695399/xrat/xsh_chap02.html#tag_03_02_04_04
http://austingroupbugs.net/view.php?id=516

FeepingCreature

Do you mean the signal stack frame?
We don't need to execute on the original stack to unwind it.

To my knowledge, it may be possible to unwind from a signal handler, but there's nothing in the spec that guarantees it's safe (or that the stack even exists between signal handler and rest of function). In fact, the only guaranteed safe way to exit a signal handler is via return. Thus this hack.

src/core/nullpointererror.d
... ...
@@ -0,0 +1,283 @@
  1
+/**
  2
+ * Written in the D programming language.
1
David Nadlinger Collaborator
klickverbot added a note July 18, 2012

Should not be part of the doc comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/core/nullpointererror.d
... ...
@@ -0,0 +1,283 @@
  1
+/**
  2
+ * Written in the D programming language.
  3
+ * Handle page protection error using Errors. NullPointerError is throw when deferencing null. A system dependant error is throw in other cases.
2
David Nadlinger Collaborator
klickverbot added a note July 18, 2012

Spelling (dependant).

Alex Rønne Petersen Collaborator
alexrp added a note July 18, 2012

(and dereferencing, not deferencing)

I'd reword to:

Handle page protection errors using Errors. NullPointerError is thrown when dereferencing null. A system-dependent error is thrown in other cases.

Also see my comments about NullPointerError.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/core/nullpointererror.d
... ...
@@ -0,0 +1,283 @@
  1
+/**
  2
+ * Written in the D programming language.
  3
+ * Handle page protection error using Errors. NullPointerError is throw when deferencing null. A system dependant error is throw in other cases.
  4
+ * Note : Only linux on x86 and x86_64 is supported for now.
2
David Nadlinger Collaborator
klickverbot added a note July 18, 2012

Remove the space in front of :, capitalize Linux.

Alex Rønne Petersen Collaborator
alexrp added a note July 18, 2012

and s/is/are/.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/core/nullpointererror.d
... ...
@@ -0,0 +1,283 @@
  1
+/**
  2
+ * Written in the D programming language.
  3
+ * Handle page protection error using Errors. NullPointerError is throw when deferencing null. A system dependant error is throw in other cases.
  4
+ * Note : Only linux on x86 and x86_64 is supported for now.
  5
+ *
  6
+ * Copyright: Copyright Digital Mars 2000 - 2012.
1
David Nadlinger Collaborator
klickverbot added a note July 18, 2012

If you wrote the code, it's most probably not »Copyright Digital Mars 2000 - 2012.«.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/core/nullpointererror.d
((87 lines not shown))
  87
+	static REG_TYPE saved_RDI, saved_RSI;
  88
+	
  89
+	extern(C)
  90
+	void handleSignal(int signum, siginfo_t* info, void* contextPtr) {
  91
+		auto context = cast(ucontext_t*)contextPtr;
  92
+		
  93
+		// Save registers into global thread local, to allow recovery.
  94
+		saved_RDI = context.uc_mcontext.gregs[REG_RDI];
  95
+		saved_RSI = context.uc_mcontext.gregs[REG_RSI];
  96
+		
  97
+		// Hijack current context so we call our handler.
  98
+		auto rip = context.uc_mcontext.gregs[REG_RIP];
  99
+		auto addr = cast(REG_TYPE) info.si_addr;
  100
+		context.uc_mcontext.gregs[REG_RDI] = addr;
  101
+		context.uc_mcontext.gregs[REG_RSI] = rip;
  102
+		context.uc_mcontext.gregs[REG_RIP] = (rip != addr)?(cast(REG_TYPE) &sigsegv_userspace_handler + 0x04):(cast(REG_TYPE) &sigsegv_userspace_handler);
8
David Nadlinger Collaborator
klickverbot added a note July 18, 2012

This can be shortened, no? Also, the line is somewhat long.

deadalnix
deadalnix added a note July 18, 2012

It cannot be that much shortened. It is necessary to handle the specific case where the memory access is done to execute the code. IE :

function() foo = null;
foo();

Martin Nowak Collaborator
MartinNowak added a note July 18, 2012

Please use two different functions instead of jumping to an offset.

deadalnix
deadalnix added a note July 18, 2012

I understand your concern, but this lead to massive code duplication, I'm not sure this is really better. What do other people think ?

David Nadlinger Collaborator
klickverbot added a note July 18, 2012

@deadalnix: cast(REG_TYPE) &sigsegv_userspace_handler + ((rip != addr) ? 0x4 : 0)? But I really just wanted to mention that we generally try to keep lines below ~100 characters in druntime/Phobos.

David Nadlinger Collaborator
klickverbot added a note July 18, 2012

Oh, and if you stick with this, you should definitely document the fact that you rely on the offset in the implementation below, so that nobody accidentally breaks the code in the future.

deadalnix
deadalnix added a note July 18, 2012

OK, I'll rewrite thing to get shorter lines/

I'll add a comment here about the offset trick, one is already present in the function the code jump to. I agree this is not trivial and is important to mention.

Martin Nowak Collaborator
MartinNowak added a note July 18, 2012

but this lead to massive code duplication

No duplication needed.
Just create two naked asm thunks that pass over to sigsegv_userspace_handler.
This is also the right place to add the missing stack alignment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/core/nullpointererror.d
((214 lines not shown))
  214
+			pop EBP;
  215
+			ret;
  216
+		}
  217
+	}
  218
+	
  219
+	// The return value is stored in EAX and EDX, so this function restore the correct value for theses registers.
  220
+	REG_TYPE[2] restore_registers() {
  221
+		return [saved_EAX, saved_EDX];
  222
+	}
  223
+}
  224
+
  225
+// This should be calculated by druntime.
  226
+enum PAGE_SIZE = 4096;
  227
+
  228
+// The first 64Kb are reserved for detecting null pointer deferences.
  229
+enum MEMORY_RESERVED_FOR_NULL_DEFERENCE = 4096 * 16;
1
Alex Rønne Petersen Collaborator
alexrp added a note July 18, 2012

s/DEFERENCE/DEREFERENCE/g

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/core/nullpointererror.d
... ...
@@ -0,0 +1,283 @@
  1
+/**
  2
+ * Written in the D programming language.
  3
+ * Handle page protection error using Errors. NullPointerError is throw when deferencing null. A system dependant error is throw in other cases.
  4
+ * Note : Only linux on x86 and x86_64 is supported for now.
  5
+ *
  6
+ * Copyright: Copyright Digital Mars 2000 - 2012.
  7
+ * License: Distributed under the
  8
+ *      $(LINK2 http://www.boost.org/LICENSE_1_0.txt, Boost Software License 1.0).
  9
+ *    (See accompanying file LICENSE_1_0.txt)
  10
+ * Authors:   Amaury SECHET, FeepingCreature, Vladimir Panteleev
  11
+ * Source: $(DRUNTIMESRC src/core/nullpointererror.d)
  12
+ */
  13
+module core.nullpointererror;
  14
+
  15
+version(linux) {
1
David Nadlinger Collaborator
klickverbot added a note July 18, 2012

Generally, new code in druntime and Phobos should follow the style conventions, i.e. braces on a separate line, and 4 space indentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/core/nullpointererror.d
((221 lines not shown))
  221
+		return [saved_EAX, saved_EDX];
  222
+	}
  223
+}
  224
+
  225
+// This should be calculated by druntime.
  226
+enum PAGE_SIZE = 4096;
  227
+
  228
+// The first 64Kb are reserved for detecting null pointer deferences.
  229
+enum MEMORY_RESERVED_FOR_NULL_DEFERENCE = 4096 * 16;
  230
+
  231
+// User space handler
  232
+
  233
+void sigsegv_userspace_process(void* address) {
  234
+	// The first page is protected to detect null deference.
  235
+	if((cast(size_t) address) < MEMORY_RESERVED_FOR_NULL_DEFERENCE) {
  236
+		throw new NullPointerError();
13
Alex Rønne Petersen Collaborator
alexrp added a note July 18, 2012

Not NullPointerError. A seg fault isn't necessarily a null dereference. Please call it MemoryError or something like that.

deadalnix
deadalnix added a note July 18, 2012

That is why a check is performed. In the general case, a SignalError is thrown.

As mentioned below, this should definitively evolve into something that check for StackOverflow and allow 3rd party code to hook its logic on a precise memory page.

Alex Rønne Petersen Collaborator
alexrp added a note July 18, 2012

I understand, but dereferencing a value like 0x10 would yield a NullPointerError too, which would be very confusing. Perhaps InvalidPointerError would be more accurate.

deadalnix
deadalnix added a note July 18, 2012

class A {
int a;
int b;
}

A a = null;
a.b; // null dereference with an address > 0 .

More generally, Linux define the file /proc/sys/vm/mmap_min_addr that define how much memory is reserved by the system to handle null pointer dereferences. It seems to me that this is reasonable to use the same value here.

Alex Rønne Petersen Collaborator
alexrp added a note July 18, 2012

I'm not sure I follow. The way I understand it,

auto foo = cast(ubyte*)0x10;
*foo = 1;

would result in a NullPointerError with your current code, no?

This should maybe be SegmentationFaultError. For instance, another way to get this would be when accessing freed memory.

deadalnix
deadalnix added a note July 18, 2012

@alexrp yes, this will be detected as null dereference. Such a code is unsafe and such a pointer can only be forged the way you did it. Except for null, no function will ever give you such a pointer.

@FeepingCreature no. No memory can be mapped at this address. Never ever. The only way you can get a pointer to such an address it either to have a null pointer or to set it manually like @alexrp does.

@deadalnix My apologies. You are correct.

Alex Rønne Petersen Collaborator
alexrp added a note July 18, 2012

I think you're a little too focused on @safe code. Keep in mind that this is a systems language; SafeD is just a subset.

Consider:

ubyte* ptr = func(); // returns null due to some silly bug
ubyte val = *(ptr + 0x10);

This is effectively the same as my first example, but shows a case where it's more likely to happen. You don't have to forge an invalid address intentionally for this to happen.

I'm willing to compromise: Make an InvalidPointerError class and a NullPointerError class (which derives from InvalidPointerError). Throw the latter when you know that the pointer was null (and nothing else); otherwise, the former.

deadalnix
deadalnix added a note July 18, 2012

Expect for names, this is what is done. NullPointerError inherit from SignalError . InvalidPointerError is a better name, I'll rename that, but the mechanism is here.

Alex Rønne Petersen Collaborator
alexrp added a note July 18, 2012

The way I read it, anything lower than MEMORY_RESERVED_FOR_NULL_DEFERENCE will yield a NullPointerError...? That is to say, my above example will throw the misleading NullPointerError. That's what I don't want. When I dereference a value like 0x10, I want an InvalidPointerError; otherwise (when the pointer was definitely null and nothing else), a NullPointerError.

deadalnix
deadalnix added a note July 18, 2012

I see, I misunderstood your point above.

A NullPointerError IS a SignalError (that will be renamed into InvalidPointerError as you suggested).

Your example above is, to me, suitable for a NullPointerError, because func did returned null and you dereference based on that. See the code as :
ubyte* ptr = func(); // returns null
ubyte val = ptr[0x10];

This type of stuff will generate a NullPointerSomething in almost every language, even if the address accessed isn't specifically null. This is why /proc/sys/vm/mmap_min_addr is often 64k instead of 4k.

Accessing this memory can only be done by forging a pointer manually or basing a dereference on null (as you did in the example above).

Alex Rønne Petersen Collaborator
alexrp added a note July 18, 2012

Well, the only languages I know that throw NPE/NRE on any invalid memory access are Java and C# (.NET in general). Java does it because Java doesn't have pointers in the first place, so anything else wouldn't make sense. C# does it due to Java heritage and because pointers were an afterthought in the design. In fact, I've had plenty of "wat" moments in C# when my invalid pointer accesses resulted in a NullReferenceException when nothing was null. I find it very unintuitive, personally, because when I write pointer operations I don't think about what they may be at a higher level; I think about what they actually are on machine level.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/core/nullpointererror.d
((219 lines not shown))
  219
+	// The return value is stored in EAX and EDX, so this function restore the correct value for theses registers.
  220
+	REG_TYPE[2] restore_registers() {
  221
+		return [saved_EAX, saved_EDX];
  222
+	}
  223
+}
  224
+
  225
+// This should be calculated by druntime.
  226
+enum PAGE_SIZE = 4096;
  227
+
  228
+// The first 64Kb are reserved for detecting null pointer deferences.
  229
+enum MEMORY_RESERVED_FOR_NULL_DEFERENCE = 4096 * 16;
  230
+
  231
+// User space handler
  232
+
  233
+void sigsegv_userspace_process(void* address) {
  234
+	// The first page is protected to detect null deference.
1
Alex Rønne Petersen Collaborator
alexrp added a note July 18, 2012

dereference

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/core/nullpointererror.d
((255 lines not shown))
  255
+	this(int signum, Throwable next, string file = __FILE__, size_t line = __LINE__) {
  256
+		_signum = signum;
  257
+		super("", file, line, next);
  258
+	}
  259
+	
  260
+	/**
  261
+	 * Property that returns the signal number.
  262
+	 */
  263
+	@property
  264
+	int signum() const {
  265
+		return _signum;
  266
+	}
  267
+}
  268
+
  269
+/**
  270
+ * Throw on null pointer deference.
1
Alex Rønne Petersen Collaborator
alexrp added a note July 18, 2012

dereference

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/core/nullpointererror.d
... ...
@@ -0,0 +1,283 @@
  1
+/**
  2
+ * Written in the D programming language.
  3
+ * Handle page protection error using Errors. NullPointerError is throw when deferencing null. A system dependant error is throw in other cases.
  4
+ * Note : Only linux on x86 and x86_64 is supported for now.
  5
+ *
  6
+ * Copyright: Copyright Digital Mars 2000 - 2012.
  7
+ * License: Distributed under the
  8
+ *      $(LINK2 http://www.boost.org/LICENSE_1_0.txt, Boost Software License 1.0).
  9
+ *    (See accompanying file LICENSE_1_0.txt)
  10
+ * Authors:   Amaury SECHET, FeepingCreature, Vladimir Panteleev
  11
+ * Source: $(DRUNTIMESRC src/core/nullpointererror.d)
  12
+ */
  13
+module core.nullpointererror;
2
Alex Rønne Petersen Collaborator
alexrp added a note July 18, 2012

I don't quite like this module name (see my comment about NullPointerError below). Some ideas: core.error, core.segfault, ...

deadalnix
deadalnix added a note July 18, 2012

Agreed. I'll rename that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Martin Nowak
Collaborator

To my knowledge, it may be possible to unwind from a signal handler

It's the same as doing it after sigreturn.

but there's nothing in the spec that guarantees it's safe

Because it isn't, but I don't see a reason why it would be safer after sigreturn, do you?

(or that the stack even exists between signal handler and rest of function).

Trapping a signal doesn't alter any memory mapping.

In fact, the only guaranteed safe way to exit a signal handler is via return. Thus this hack.

You can abort(3) the process, reraise(3) the signal or execve(2) a different image.
Those are safe operations, but sigreturn(2)ing from the handler, longjmp(3)ing to a restore point or unwinding from the handler are not because the error signal indicates a corrupted process. A null pointer dereference isn't any different because one doesn't know if it's a consequence of an earlier corruption.

The open questions are how much to risk for extended error information, how to make it as portable as possible and how to make it configurable.
As I've pointed to earlier it's also feasible to do this in a safe manner.

FeepingCreature
In fact, the only guaranteed safe way to exit a signal handler is via return. Thus this hack.

You can abort(3) the process, reraise(3) the signal or execve(2) a different image.
Those are safe operations, but sigreturn(2)ing from the handler, longjmp(3)ing to a restore point or unwinding from the handler are not because the error signal indicates a corrupted process.

My apologies, I meant "safe way to exit a signal handler while preserving the stack", and I specifically meant "safer than just unwinding through the signal handler because you never know what the kernel might have done setting up the signal." I agree that it is generally unsafe to continue a program after potential corruption has occurred, but a) null pointer access as opposed to just any segment violation generally indicates an error that is probably not destructive since it seems unlikely that other memory would have been overwritten, and b) this still gives us the chance to do cleanup and print a stack trace, which is incredibly useful (and no, gdb is not a substitute). And hey, if you want to propose some better way of doing backtraces, as the open source people say - patches welcome.

I just think the ultimate decision how to handle errors should lie with the programmer, not the runtime.

deadalnix

First of all, let's consider when this thing can be triggered. You'll find 2 cases :
1/ When a null dereference occurs. This can be anywhere, even in @safe code, this is common and non destructive.
2/ in @system code, when things get corrupted.

As far as I understand, nobody have any argument against situation 1.

Situation 2 is the case where the program cannot be recoverable, as @dawgfoto states. This is true, but in a first place, the program isn't screwed by this patch, it is screwed way before. In such a situation, how the fact the an Error percolate is worse than the program aborting abruptly ?

Situation 2 look like more an argument against @system code than against this proposal precisely. @system code can screw up the integrity of the program, this is know fact, and as soon as @system is around ANYTHING can happen.

src/core/nullpointererror.d
((139 lines not shown))
  139
+			
  140
+			pop RDI;
  141
+			
  142
+			// Restore trash registers value.
  143
+			pop R11;
  144
+			pop R10;
  145
+			pop R9;
  146
+			pop R8;
  147
+			pop RDX;
  148
+			pop RCX;
  149
+			pop RAX;
  150
+			popf;		// Restore flags.
  151
+			
  152
+			// Return
  153
+			pop RBP;
  154
+			ret;
4
Martin Nowak Collaborator
MartinNowak added a note July 18, 2012

Why are you saving/restoring the error state?
You cannot continue execution at the failing instruction, i.e. sigsegv_userspace_process must not return.
That also implies you can scratch the code vs. data distinction too.

deadalnix
deadalnix added a note July 24, 2012

If the segfault is triggered by an mprotect you can get back in the program just like if nothing happened. The goal is to allow any code to be plugged here, and not just throw exception, and full recovery is actually possible in many cases (and yes, this is tested).

Martin Nowak Collaborator
MartinNowak added a note July 25, 2012

If the segfault is triggered by an mprotect you can get back in the program just like if nothing happened.

Right, but what's the use case for accessing protected pages?
Are you thinking of GC read/write-barriers?

If you want to support arbitrary code you need to go the full way, e.g. saving and defaulting the FPU state.

deadalnix
deadalnix added a note July 30, 2012

Isn't the FPU supposed to be saved by the callee ? If not, indeed, I have to add that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Martin Nowak
Collaborator

safer than just unwinding through the signal handler

Let's end that branch of the discussion. I just proposed that unwinding in the signal handler would allow us to use sigaltstack(2) and extend this to stack overflows.

I just think the ultimate decision how to handle errors should lie with the programmer, not the runtime.

Right, but this pull is about what error handling schemes the runtime offers.
Though I do think this one would be useful for certain development workflows.

Considering null pointer dereferencing.

@safe void foo(Bar bar)
{
    bar.func1();
    bar.func2();
}

If foo is called with a corrupted bar instance then calling func1 might do arbitrary damage before calling func2 might trap a null pointer access.

Has anyone considered to leverage the unittestSegvHandler. This wouldn't be nearly as controversial and could be done with two simple core.runtime functions enable/disableBacktraceOnSignals.

Andrei Alexandrescu
Owner

Since discussion is in flux, I'm deferring approval to a mini-committee of the union of all reviewers of this. Generally I'm favorable to merging this in if it just works and is robust.

Walter Bright
Owner

As Andrei mentioned, I don't think this is a good idea. While I did it for Windows long ago, it hasn't panned out to be a useful or desirable feature. It's also a hard thing to get right, and requiring D implementations to do this could put a heavy burden on an implementation and a port (very few programmers are knowledgable enough to write such code).

Alex Rønne Petersen
Collaborator
alexrp commented July 23, 2012

@WalterBright Given that this module only activates itself when imported, it doesn't place any particular requirement on D implementations. Implementations that don't yet support it can simply do a static assert(false);.

I think the idea behind the module is fine since it is opt-in and not forced (like access violation errors on Windows...). But I still object to throwing NullPointerError for memory accesses that aren't actually accessing the value 0x0.

Martin Nowak
Collaborator
  • There are still many implementation issues not yet addressed.
  • We already have a more robust solution for backtraces on signals unittestSegvHandler.
  • I don't like the idea to allocate and throw an Error object, risking deadlocks, only to advise that running any catch code is unsafe.
deadalnix

As @dawgfoto mentioned, some changes have to be made (see comments on code itself).

The Error object allocation is an issue that can be solved by preallocating the Error, or halting depending on the stack trace (I prefers the second one).

@WalterBright I don't think you can judge the usefulness of that only based on the windows version of it. First, the behavior isn't consistent across platforms, so it is avoided, and it don't separate what is a null dereference and a memory corruption. It a mechanism that is present in already successful languages and has proven itself useful and successful.

What do you think make that feature useful in java or C#, but not in D ?

@andralex Where do you want the discussion to take place ?

Vladimir Panteleev

While I did it for Windows long ago, it hasn't panned out to be a useful or desirable feature.

Why? I do not recall any arguments against the feature, but I know many favor it. Re-running the program under a debugger on Windows is a good deal more cumbersome than with gdb.

I don't like the idea to allocate and throw an Error object, risking deadlocks

OutOfMemoryError and InvalidMemoryOperationError both are thrown using their .init, avoiding any kind of memory allocations.

JakobOvrum

OutOfMemoryError and InvalidMemoryOperationError both are thrown using their .init, avoiding any kind of memory allocations.

Being really pedantic here, but the .init of a class type is just a null reference. I think you meant to say they are pre-allocated.

JakobOvrum

Yes, I meant classinfo.init. See here: https://github.com/D-Programming-Language/druntime/blob/master/src/core/exception.d#L502

Wow, that's a really neat trick.

Martin Nowak
Collaborator

but I know many favor it. Re-running the program under a debugger

There is no need to conflate error diagnosis and exceptions.

both are thrown using their .init, avoiding any kind of memory allocations

That's avoids the allocation part but not the throw part.

Vladimir Panteleev

I favor this pull request specifically for its aid in error diagnosis. Personally, I would be equally happy with D programs printing a stack trace and immediately exiting upon a SIGSEGV.

Andrei Alexandrescu
Owner

I think it's difficult to put this in the language; as Walter said, this puts pressure on alternative implementations. But I think putting it in etc/linux would be a great way to test it out there and see how it works for people. What do you all think?

Vladimir Panteleev

Just saw the edit:

That's avoids the allocation part but not the throw part.

What is the exact problem with throw? You can see the implementation here: https://github.com/D-Programming-Language/druntime/blob/master/src/rt/deh2.d#L183

Walter Bright
Owner

I haven't found any use for it under Windows, nor have I seen any.

I know that some programs rely on catching null pointer exceptions in Java, but I think that such is execrable program design. I also recall Andrei's arguments about using it to debug when using a debugger is impractical. Putting it in etc/linux as an unofficial/experimental module may be the best way.

Vladimir Panteleev

I think the usefulness boils down to "When the program crashes, give me a sensible amount of information that can help me diagnose the problem right away, so I won't have to waste time setting up a debugger and trying to reproduce the crash there". As has been discussed previously, some classes of bugs in some classes of applications can take hours or more to reproduce. The same reasons why we print stack traces on unhandled exceptions generally, really.

deadalnix

@WalterBright yes, relying on NPE in java isn't really a good idea. It isn't in D either.

However, as both Java and D have references types set as null by default, NPE WILL occur in any non trivial program. We should either prevent the proliferation of null (hello Rust - and to be fair, I think they are right, but it is another topic) or provide a way to handle it in the language.

Not providing any mecanism for that in C/C++ has proven to be a bad idea. It is the source of many security flaws, and some very bad ones (root exploit for instance often are created by unchecked null).

@andralex putting that in etc/linux and see how it goes is actually a really good idea.

src/core/nullpointererror.d
((106 lines not shown))
  106
+	
  107
+	// This function must be called with faulting address in RDI and original RIP in RSI.
  108
+	void sigsegv_userspace_handler() {
  109
+		asm {
  110
+			naked;
  111
+			
  112
+			// Handle the stack for an invalid function call (segfault at RIP).
  113
+			push RBP;
  114
+			mov RBP, RSP;
  115
+			
  116
+			// We jump directly here if we are in a valid function call case.
  117
+			push RSI;	// return address (original RIP).
  118
+			push RBP;	// old RBP
  119
+			mov RBP, RSP;
  120
+			
  121
+			pushf;		// Save flags.
1
Martin Nowak Collaborator
MartinNowak added a note July 25, 2012

pushfd/pushfq

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/core/nullpointererror.d
((135 lines not shown))
  135
+			push RAX;
  136
+			
  137
+			call restore_RSI;
  138
+			mov RSI, RAX;
  139
+			
  140
+			pop RDI;
  141
+			
  142
+			// Restore trash registers value.
  143
+			pop R11;
  144
+			pop R10;
  145
+			pop R9;
  146
+			pop R8;
  147
+			pop RDX;
  148
+			pop RCX;
  149
+			pop RAX;
  150
+			popf;		// Restore flags.
1
Martin Nowak Collaborator
MartinNowak added a note July 25, 2012

popfd/popfq

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/core/nullpointererror.d
((116 lines not shown))
  116
+			// We jump directly here if we are in a valid function call case.
  117
+			push RSI;	// return address (original RIP).
  118
+			push RBP;	// old RBP
  119
+			mov RBP, RSP;
  120
+			
  121
+			pushf;		// Save flags.
  122
+			push RAX;	// RAX, RCX, RDX, and R8 to R11 are trash registers and must be preserved as local variables.
  123
+			push RCX;
  124
+			push RDX;
  125
+			push R8;
  126
+			push R9;
  127
+			push R10;
  128
+			push R11;
  129
+			
  130
+			// Parameter address is already set as RAX.
  131
+			call sigsegv_userspace_process;
2
Martin Nowak Collaborator
MartinNowak added a note July 25, 2012

The stack is probably not aligned at the point of this call.

deadalnix
deadalnix added a note July 30, 2012

It for 64bits but isn't for 32, you are right.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
deadalnix

I updated the pull request according to remarks of people here.

src/etc/linux/nullpointererror.d
... ...
@@ -0,0 +1,287 @@
  1
+/**
  2
+ * Handle page protection error using Errors. NullPointerError is throw when deferencing null. A system dependant error is throw in other cases.
1
JakobOvrum
JakobOvrum added a note July 31, 2012

is thrown
dereferencing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/etc/linux/nullpointererror.d
((7 lines not shown))
  7
+ *    (See accompanying file LICENSE_1_0.txt)
  8
+ * Authors:   Amaury SECHET, FeepingCreature, Vladimir Panteleev
  9
+ * Source: $(DRUNTIMESRC src/etc/linux/nullpointererror.d)
  10
+ */
  11
+module etc.linux.nullpointererror;
  12
+
  13
+version(linux) {
  14
+
  15
+private :
  16
+import core.sys.posix.signal;
  17
+import core.sys.posix.ucontext;
  18
+
  19
+// Missing details from Druntime
  20
+
  21
+version(X86_64) {
  22
+	enum {
1
JakobOvrum
JakobOvrum added a note July 31, 2012

Phobos uses 4 indendation levels.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/etc/linux/nullpointererror.d
... ...
@@ -0,0 +1,287 @@
  1
+/**
  2
+ * Handle page protection error using Errors. NullPointerError is throw when deferencing null. A system dependant error is throw in other cases.
  3
+ * Note: Only x86 and x86_64 are supported for now.
  4
+ *
  5
+ * License: Distributed under the
  6
+ *      $(LINK2 http://www.boost.org/LICENSE_1_0.txt, Boost Software License 1.0).
  7
+ *    (See accompanying file LICENSE_1_0.txt)
  8
+ * Authors:   Amaury SECHET, FeepingCreature, Vladimir Panteleev
  9
+ * Source: $(DRUNTIMESRC src/etc/linux/nullpointererror.d)
  10
+ */
  11
+module etc.linux.nullpointererror;
  12
+
  13
+version(linux) {
  14
+
  15
+private :
1
JakobOvrum
JakobOvrum added a note July 31, 2012

The style guide does not cover this, but I'm fairly sure the convention is having no space between the private and the : tokens.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/etc/linux/nullpointererror.d
((90 lines not shown))
  90
+		
  91
+		// Save registers into global thread local, to allow recovery.
  92
+		saved_RDI = context.uc_mcontext.gregs[REG_RDI];
  93
+		saved_RSI = context.uc_mcontext.gregs[REG_RSI];
  94
+		
  95
+		// Hijack current context so we call our handler.
  96
+		auto rip = context.uc_mcontext.gregs[REG_RIP];
  97
+		auto addr = cast(REG_TYPE) info.si_addr;
  98
+		context.uc_mcontext.gregs[REG_RDI] = addr;
  99
+		context.uc_mcontext.gregs[REG_RSI] = rip;
  100
+		context.uc_mcontext.gregs[REG_RIP] = (rip != addr)?(cast(REG_TYPE) &sigsegv_data_handler):(cast(REG_TYPE) &sigsegv_code_handler);
  101
+	}
  102
+	
  103
+	// All handler functions must be called with faulting address in RDI and original RIP in RSI.
  104
+	
  105
+	// This functionis called when the segfault's cause is to call an invalid function pointer.
1
JakobOvrum
JakobOvrum added a note July 31, 2012

Missing space

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/etc/linux/nullpointererror.d
((96 lines not shown))
  96
+		auto rip = context.uc_mcontext.gregs[REG_RIP];
  97
+		auto addr = cast(REG_TYPE) info.si_addr;
  98
+		context.uc_mcontext.gregs[REG_RDI] = addr;
  99
+		context.uc_mcontext.gregs[REG_RSI] = rip;
  100
+		context.uc_mcontext.gregs[REG_RIP] = (rip != addr)?(cast(REG_TYPE) &sigsegv_data_handler):(cast(REG_TYPE) &sigsegv_code_handler);
  101
+	}
  102
+	
  103
+	// All handler functions must be called with faulting address in RDI and original RIP in RSI.
  104
+	
  105
+	// This functionis called when the segfault's cause is to call an invalid function pointer.
  106
+	void sigsegv_code_handler() {
  107
+		asm {
  108
+			naked;
  109
+			
  110
+			// Handle the stack for an invalid function call (segfault at RIP).
  111
+			// With the return pointer, the stack is now alligned.
1
JakobOvrum
JakobOvrum added a note July 31, 2012

aligned

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/etc/linux/nullpointererror.d
((149 lines not shown))
  149
+			pop R11;
  150
+			pop R10;
  151
+			pop R9;
  152
+			pop R8;
  153
+			pop RDX;
  154
+			pop RCX;
  155
+			pop RAX;
  156
+			popfq;		// Restore flags.
  157
+			
  158
+			// Return
  159
+			pop RBP;
  160
+			ret;
  161
+		}
  162
+	}
  163
+	
  164
+	// The return value is stored in EAX and EDX, so this function restore the correct value for theses registers.
1
JakobOvrum
JakobOvrum added a note July 31, 2012

restore -> restores

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/etc/linux/nullpointererror.d
((246 lines not shown))
  246
+enum MEMORY_RESERVED_FOR_NULL_DEREFERENCE = 4096 * 16;
  247
+
  248
+// User space handler
  249
+void sigsegv_userspace_process(void* address) {
  250
+	// The first page is protected to detect null dereferences.
  251
+	if((cast(size_t) address) < MEMORY_RESERVED_FOR_NULL_DEREFERENCE) {
  252
+		throw new NullPointerError();
  253
+	}
  254
+	
  255
+	throw new InvalidPointerError();
  256
+}
  257
+
  258
+public :
  259
+
  260
+/**
  261
+ * Thrown on posix system when a signal is recieved. Is only throw for SIGSEGV.
1
JakobOvrum
JakobOvrum added a note July 31, 2012

systems
thrown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/etc/linux/nullpointererror.d
((259 lines not shown))
  259
+
  260
+/**
  261
+ * Thrown on posix system when a signal is recieved. Is only throw for SIGSEGV.
  262
+ */
  263
+class InvalidPointerError : Error {
  264
+	this(string file = __FILE__, size_t line = __LINE__, Throwable next = null) {
  265
+		super("", file, line, next);
  266
+	}
  267
+	
  268
+	this(Throwable next, string file = __FILE__, size_t line = __LINE__) {
  269
+		super("", file, line, next);
  270
+	}
  271
+}
  272
+
  273
+/**
  274
+ * Throw on null pointer dereferences.
1
JakobOvrum
JakobOvrum added a note July 31, 2012

Thrown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Andrei Alexandrescu
Owner

posix.mak seems to have conflicts.

An issue came up at work that brought about evidence that this may add significant practical value. In certain embedded uses (either statically or dynamically linked) it's important to avoid segfaulting. This module would go a long way toward making that possible.

@deadalnix, could you please bring this in shape (it's not rebased, won't build etc). Then let's merge it with experimental title. Thanks!

deadalnix

@andralex What do you mean by merging with experimental title ? It is rebased and fixes has been made to solve conflicts.

Sean Kelly
Collaborator
Sean Kelly
Collaborator
deadalnix

@complexmath Yes, it does jump into a subroutine after the signal handler complete.

It does work on linux (x86 and x86_64).

Andrei Alexandrescu
Owner

I strongly think we should move forward with this. Any encumbrances left? Any wrinkles to iron out? If not, please rebase and let's get this puppy in the wild.

Andrei Alexandrescu
Owner

Oh, it's already rebased. @complexmath: OK to merge?

Sean Kelly
Collaborator

Yep. GitHub is telling me this can't be automatically merged. Any idea why?

Alex Rønne Petersen
Collaborator

That just means we do need a rebase.

src/etc/linux/memoryerror.d
((223 lines not shown))
  223
+			
  224
+			// Restore register values and return.
  225
+			call restore_registers;
  226
+			
  227
+			pop ECX;
  228
+			popfd;		// Restore flags.
  229
+			
  230
+			// Return
  231
+			pop EBP;
  232
+			ret;
  233
+		}
  234
+	}
  235
+	
  236
+	// The return value is stored in EAX and EDX, so this function restore the correct value for theses registers.
  237
+	REG_TYPE[2] restore_registers() {
  238
+		return [saved_EAX, saved_EDX];
2
Alex Rønne Petersen Collaborator
alexrp added a note September 02, 2012

This will result in a GC allocation. Just so you know. I don't know if it's a problem in this particular case.

WTF I didn't knew that, but you are right, _d_arrayliteral is called and gc alloc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/etc/linux/memoryerror.d
((237 lines not shown))
  237
+	REG_TYPE[2] restore_registers() {
  238
+		return [saved_EAX, saved_EDX];
  239
+	}
  240
+}
  241
+
  242
+// This should be calculated by druntime.
  243
+enum PAGE_SIZE = 4096;
  244
+
  245
+// The first 64Kb are reserved for detecting null pointer dereferencess.
  246
+enum MEMORY_RESERVED_FOR_NULL_DEREFERENCE = 4096 * 16;
  247
+
  248
+// User space handler
  249
+void sigsegv_userspace_process(void* address) {
  250
+	// The first page is protected to detect null dereferences.
  251
+	if((cast(size_t) address) < MEMORY_RESERVED_FOR_NULL_DEREFERENCE) {
  252
+		throw new NullPointerError();
16
Alex Rønne Petersen Collaborator
alexrp added a note September 02, 2012

I still don't agree with this. Dereferencing 0x1 is not dereferencing a null pointer. You are giving the programmer an incorrect error.

Sean Kelly Collaborator

The code still won't fire unless the system detected a segfault though, right?

Alex Rønne Petersen Collaborator
alexrp added a note September 02, 2012

The code will trigger on all segmentation faults.

My gripe here is that the code throws NullPointerError for any dereference of a pointer in the range 0 - 64k. A null pointer is a null pointer. It is not 1, 2, 3, or any other integer value. Ever. The code should throw NullPointerError for 0 and 0 alone. Any other pointer should result in InvalidPointerError.

Andrei Alexandrescu Owner

@alexrp Actually accessing a field off the null pointer will trigger that. It's really same thing.

David Nadlinger Collaborator

@andralex: The point is that *(cast(ubyte*)0x100) = 1 will give you a NullPointerError, even if it's not a null pointer. I'm not sure whether there is any benefit to having NullPointerError along InvalidPointerError at all – it only seems to create confusion.

Here is the reasoning :

As Andrei said, accessing a field of a null reference will trigger a page fault at address 0 + offset of the field. This is why linux kernel specifically reserve memory at 0 to detect null dereferences.

I see a point in discriminating null dereferences and other pointer errors. In the current state of D, you can dereference null really easily almost anywhere. Other memory errors are result of memory corruption.

The (cast(ubyte)0x100) = 1 is not a good example IMO because it is artificial. Pointer come from allocated memory, not constant that you cast. And no memory is ever allocated at 0 by the linux kernel.

As a result, you only trigger the NullPointerError on dereferencing null, or by building thing specifically to trigger that behavior, like @klickverbot did.

A NullPointerError is something you should expect to see on a regular basis, considering how D is done. Other memory error is something that must scare you very much because it means somewhere you have corrupted memory.

Alex Rønne Petersen Collaborator
alexrp added a note September 03, 2012

Look... it doesn't matter where pointers come from. The point here is that this module's behavior is inconsistent.

Suppose I access some field inside a null object. The object's layout is small, so I get a NullPointerError. Now suppose I have a class with a very large (several megabytes) static array inside it. I access the last element of this array on a null object. Now I get an InvalidMemoryError.

Can you not see why this is a can of worms and will just cause confusion?

Not to mention, you can't maintain the same behavior across OSs - this reserved area is going to vary, harming future extension of this module!!

This module is supposed to provide a reliable mechanism to deal with invalid memory accesses, but it doesn't, until this stuff is fixed to be less Linux- and high-level-view-centric so it's actually future-proof and has a well-defined interface.

Just to be clear, I have nothing against this module and what it seeks to do. But I have absolutely everything against this one line of code that likes to pretend that assert(cast(void*)0x1 == null) passes.

Andrei Alexandrescu Owner

I think it's safe to drop this. IMHO objects with large embedded arrays should be, in fact, disallowed statically. Or if they're not, we don't guarantee integrity.

Alex Rønne Petersen Collaborator
alexrp added a note September 03, 2012

@andralex wait, what? Why?

Andrei Alexandrescu Owner

We should cater for the common case, and the common case is that someone writes e.g.

if (object.field == 42) { ... }

Throwing a sort of an array dereference exception would be just confusing.

David Nadlinger Collaborator

@alexrp: Think about the implications concerning @safe-ty – you can read from an arbitrary memory location in safe code via dereferencing a null pointer to an object containing such a humongous array.

Alex Rønne Petersen Collaborator
alexrp added a note September 03, 2012

Throwing a sort of an array dereference exception would be just confusing.

That's not what I'm saying we should... I'm saying that throwing NullPointerError is inconsistent, unreliable, and confusing. I say this a systems programmer who knows what the resulting assembly actually does. As such, I want an InvalidMemoryError because I am not dereferencing anything that is the value 0x0.

Andrei Alexandrescu Owner

@alexrp: are you saying you just want a change of name? (1) I'm glad this is the only thing that's under debate! (2) Although I understand your argument, I personally think NullPointerError is better than InvalidMemoryError.

David Nadlinger Collaborator

@andralex: The proposal already includes InvalidPointerError, but in addition a NullPointerError subclass which is thrown if the faulty address was particularly low.

Alex Rønne Petersen Collaborator
alexrp added a note September 03, 2012

Just look at the code I commented on to begin with. I want that check to check for null and nothing else. That's all I'm asking for.

I'm willing to 'compromise' and just have InvalidPointerError for all cases (so no special-casing for close-to-null values) too.

Alex Rønne Petersen Collaborator
alexrp added a note September 03, 2012

After some discussion on IRC, I have another proposal: Drop NullPointerError entirely and just attach RIP and the memory location that was attempted accessed (this information is trivially available across pretty much all relevant OS/arch combinations) to InvalidMemoryError and call it a day. You'll get a nice exception whenever you do an invalid memory access, and you can trivially ask (both programmatically and in a debugger) whether it's a null access.

Everyone wins?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/etc/linux/memoryerror.d
((246 lines not shown))
  246
+enum MEMORY_RESERVED_FOR_NULL_DEREFERENCE = 4096 * 16;
  247
+
  248
+// User space handler
  249
+void sigsegv_userspace_process(void* address) {
  250
+	// The first page is protected to detect null dereferences.
  251
+	if((cast(size_t) address) < MEMORY_RESERVED_FOR_NULL_DEREFERENCE) {
  252
+		throw new NullPointerError();
  253
+	}
  254
+	
  255
+	throw new InvalidPointerError();
  256
+}
  257
+
  258
+public :
  259
+
  260
+/**
  261
+ * Thrown on posix system when a signal is recieved. Is only throw for SIGSEGV.
1
Alex Rønne Petersen Collaborator
alexrp added a note September 02, 2012

"Thrown on POSIX systems when a SIGSEGV signal is received."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/etc/linux/memoryerror.d
((259 lines not shown))
  259
+
  260
+/**
  261
+ * Thrown on posix system when a signal is recieved. Is only throw for SIGSEGV.
  262
+ */
  263
+class InvalidPointerError : Error {
  264
+	this(string file = __FILE__, size_t line = __LINE__, Throwable next = null) {
  265
+		super("", file, line, next);
  266
+	}
  267
+	
  268
+	this(Throwable next, string file = __FILE__, size_t line = __LINE__) {
  269
+		super("", file, line, next);
  270
+	}
  271
+}
  272
+
  273
+/**
  274
+ * Throw on null pointer dereferences.
1
Alex Rønne Petersen Collaborator
alexrp added a note September 02, 2012

"Thrown on null pointer dereferences."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/etc/linux/memoryerror.d
((178 lines not shown))
  178
+		
  179
+		// Save registers into global thread local, to allow recovery.
  180
+		saved_EAX = context.uc_mcontext.gregs[REG_EAX];
  181
+		saved_EDX = context.uc_mcontext.gregs[REG_EDX];
  182
+		
  183
+		// Hijack current context so we call our handler.
  184
+		auto eip = context.uc_mcontext.gregs[REG_EIP];
  185
+		auto addr = cast(REG_TYPE) info.si_addr;
  186
+		context.uc_mcontext.gregs[REG_EAX] = addr;
  187
+		context.uc_mcontext.gregs[REG_EDX] = eip;
  188
+		context.uc_mcontext.gregs[REG_EIP] = (eip != addr)?(cast(REG_TYPE) &sigsegv_code_handler + 0x03):(cast(REG_TYPE) &sigsegv_data_handler);
  189
+	}
  190
+	
  191
+	// All handler functions must be called with faulting address in EAX and original EIP in EDX.
  192
+	
  193
+	// This functionis called when the segfault's cause is to call an invalid function pointer.
1
Alex Rønne Petersen Collaborator
alexrp added a note September 02, 2012

"function is"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
deadalnix

Rebased and everything.

Martin Nowak
Collaborator

Rebased and everything.

Nice.

Some final nitpicks

  • win32.mak MANIFEST and import is missing, see xattr.d
  • offering a register/unregister function instead of using shared static this would be more flexible
  • it seems that the REG_ enums are redefined in glibc, so we could put them in core.sys.linux.sys.ucontext
  • please untabify and delete-trailing-whitespace
  • most of druntime is written with opening braces on a separate line, so if you don't mind it would be good to keep this for new code
Jonathan M Davis
Collaborator

most of druntime is written with opening braces on a separate line, so if you don't mind it would be good to keep this for new code

All druntime and Phobos code needs to follow this rule. If any code doesn't, it's because it's older code which hasn't been fixed, or it managed to slip passed the reviewers. It's one of the few formatting rules that we require (the major one other being the number of characters per line - soft limit of 80, hard limit of 120).

Alex Rønne Petersen
Collaborator

I think that it would be a good idea to attach some extra information to the errors this module throws. Specifically, I'd like the instruction address which triggered the error and the specific memory location that the instruction tried to access.

deadalnix

@dawgfoto can you explain me what should I modify into win32.mak ? I don't really gt it, because this code is not usable in any way on windows.

I have made all other requested changes have been made except register/unregister .

Martin Nowak
Collaborator

@dawgfoto can you explain me what should I modify into win32.mak ? I don't really gt it, because this code is not usable in any way on windows.

Because Walter and/or other people use Windows to create the distribution files (dmd.zip).

Martin Nowak
Collaborator

We're really close, but please get rid of the shared static this() import initialization trick.
There are good use cases to only set a handler for a limited region of code. With a hardcoded initialization one cannot do this. You might also want to avoid the conflict with the unittest handler.

deadalnix

@dawgfoto Is it correct now in regard of win32.mak and win64.mak ?

The shared this method has been replaced with register and unregister.

As the unittest signal handler does unregister itself, it shouldn't be a problem.

src/core/sys/posix/ucontext.d
... ...
@@ -39,6 +39,32 @@ version( linux )
39 39
 
40 40
     version( X86_64 )
41 41
     {
  42
+        enum {
1
Alex Rønne Petersen Collaborator
alexrp added a note September 25, 2012

Brace on separate line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/core/sys/posix/ucontext.d
... ...
@@ -94,6 +120,28 @@ version( linux )
94 120
     }
95 121
     else version( X86 )
96 122
     {
  123
+        enum {
1
Alex Rønne Petersen Collaborator
alexrp added a note September 25, 2012

Ditto.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/etc/linux/memoryerror.d
((29 lines not shown))
  29
+
  30
+    return sigaction(SIGSEGV, &action, oldptr);
  31
+}
  32
+
  33
+int unregister_memory_error_handler()
  34
+{
  35
+    auto oldptr = cast(sigaction_t*) &old_sigaction;
  36
+
  37
+    return sigaction(SIGSEGV, oldptr, null);
  38
+}
  39
+
  40
+// Sighandler space
  41
+
  42
+alias typeof({ucontext_t uc; return uc.uc_mcontext.gregs[0];}()) REG_TYPE;
  43
+
  44
+version(X86_64) {
1
Alex Rønne Petersen Collaborator
alexrp added a note September 25, 2012

Brace on separate line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/etc/linux/memoryerror.d
((68 lines not shown))
  68
+    {
  69
+        asm {
  70
+            naked;
  71
+
  72
+            // Handle the stack for an invalid function call (segfault at RIP).
  73
+            // With the return pointer, the stack is now alligned.
  74
+            push RBP;
  75
+            mov RBP, RSP;
  76
+
  77
+            jmp sigsegv_data_handler;
  78
+        }
  79
+    }
  80
+
  81
+    void sigsegv_data_handler()
  82
+    {
  83
+        asm {
2
Alex Rønne Petersen Collaborator
alexrp added a note September 25, 2012

Ditto.

Alex Rønne Petersen Collaborator
alexrp added a note September 25, 2012

Ditto.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/etc/linux/memoryerror.d
((54 lines not shown))
  54
+        saved_RSI = context.uc_mcontext.gregs[REG_RSI];
  55
+
  56
+        // Hijack current context so we call our handler.
  57
+        auto rip = context.uc_mcontext.gregs[REG_RIP];
  58
+        auto addr = cast(REG_TYPE) info.si_addr;
  59
+        context.uc_mcontext.gregs[REG_RDI] = addr;
  60
+        context.uc_mcontext.gregs[REG_RSI] = rip;
  61
+        context.uc_mcontext.gregs[REG_RIP] = (rip != addr)?(cast(REG_TYPE) &sigsegv_data_handler):(cast(REG_TYPE) &sigsegv_code_handler);
  62
+    }
  63
+
  64
+    // All handler functions must be called with faulting address in RDI and original RIP in RSI.
  65
+
  66
+    // This function is called when the segfault's cause is to call an invalid function pointer.
  67
+    void sigsegv_code_handler()
  68
+    {
  69
+        asm {
2
Alex Rønne Petersen Collaborator
alexrp added a note September 25, 2012

Ditto.

Alex Rønne Petersen Collaborator
alexrp added a note September 25, 2012

Ditto.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/etc/linux/memoryerror.d
((122 lines not shown))
  122
+            pop RBP;
  123
+            ret;
  124
+        }
  125
+    }
  126
+
  127
+    // The return value is stored in EAX and EDX, so this function restore the correct value for theses registers.
  128
+    REG_TYPE restore_RDI()
  129
+    {
  130
+        return saved_RDI;
  131
+    }
  132
+
  133
+    REG_TYPE restore_RSI()
  134
+    {
  135
+        return saved_RSI;
  136
+    }
  137
+} else version(X86) {
2
Alex Rønne Petersen Collaborator
alexrp added a note September 25, 2012

Ditto (x2).

Andrei Alexandrescu Owner

@alexrp to save yourself work just write "throughout" the second time around and don't mention it again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Alex Rønne Petersen
Collaborator

@deadalnix The makefile changes look correct to me.

Alex Rønne Petersen
Collaborator

@deadalnix Actually, this is failing on Windows: http://d.puremagic.com/test-results/pull.ghtml?runid=300885

You shouldn't add the module to the list of modules to build on Windows (SRCS), just the list of modules to zip (MANIFEST).

deadalnix

@alexrp new line added and module removed from SRCS on windows.

Andrei Alexandrescu
Owner

I'm assigning this to @dawgfoto. Thanks all!

Andrei Alexandrescu
Owner

What's the deal with the Win32 build error?

src/etc/linux/memoryerror.d
... ...
@@ -0,0 +1,277 @@
  1
+/**
  2
+ * Handle page protection error using Errors. NullPointerError is throw when deferencing null. A system dependant error is throw in other cases.
1

Some corrections:
errors
thrown (twice)
dereferencing
system-dependent

It's also a good idea to use $(D symbol) when referencing symbols.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/etc/linux/memoryerror.d
((1 lines not shown))
  1
+/**
  2
+ * Handle page protection error using Errors. NullPointerError is throw when deferencing null. A system dependant error is throw in other cases.
  3
+ * Note: Only x86 and x86_64 are supported for now.
  4
+ *
  5
+ * License: Distributed under the
  6
+ *      $(LINK2 http://www.boost.org/LICENSE_1_0.txt, Boost Software License 1.0).
  7
+ *    (See accompanying file LICENSE_1_0.txt)
  8
+ * Authors:   Amaury SECHET, FeepingCreature, Vladimir Panteleev
  9
+ * Source: $(DRUNTIMESRC src/etc/linux/memory.d)
  10
+ */
  11
+module etc.linux.memoryerror;
  12
+
  13
+version(linux)
  14
+{
  15
+
  16
+private :
1

The Phobos code I've seen does not put a space between the two tokens in code like this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Alex Rønne Petersen
Collaborator

@deadalnix can you address @JakobOvrum's points?

src/etc/linux/memoryerror.d
((6 lines not shown))
  6
+ *      $(LINK2 http://www.boost.org/LICENSE_1_0.txt, Boost Software License 1.0).
  7
+ *    (See accompanying file LICENSE_1_0.txt)
  8
+ * Authors:   Amaury SECHET, FeepingCreature, Vladimir Panteleev
  9
+ * Source: $(DRUNTIMESRC src/etc/linux/memory.d)
  10
+ */
  11
+module etc.linux.memoryerror;
  12
+
  13
+version(linux)
  14
+{
  15
+
  16
+private :
  17
+import core.sys.posix.signal;
  18
+import core.sys.posix.ucontext;
  19
+
  20
+// Register and unregister memory error handler.
  21
+private shared sigaction_t old_sigaction;
1
Martin Nowak Collaborator

You might want to use __gshared here to avoid casting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/etc/linux/memoryerror.d
((28 lines not shown))
  28
+    
  29
+    auto oldptr = cast(sigaction_t*) &old_sigaction;
  30
+
  31
+    return sigaction(SIGSEGV, &action, oldptr);
  32
+}
  33
+
  34
+int unregister_memory_error_handler()
  35
+{
  36
+    auto oldptr = cast(sigaction_t*) &old_sigaction;
  37
+
  38
+    return sigaction(SIGSEGV, oldptr, null);
  39
+}
  40
+
  41
+// Sighandler space
  42
+
  43
+alias typeof({ucontext_t uc; return uc.uc_mcontext.gregs[0];}()) REG_TYPE;
1
Martin Nowak Collaborator

You can use the init property to abbreviate the alias.
alias typeof(ucontext_t.init.uc_mcontext.gregs[0]) REG_TYPE;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/etc/linux/memoryerror.d
((48 lines not shown))
  48
+
  49
+    extern(C)
  50
+    void handleSignal(int signum, siginfo_t* info, void* contextPtr)
  51
+    {
  52
+        auto context = cast(ucontext_t*)contextPtr;
  53
+
  54
+        // Save registers into global thread local, to allow recovery.
  55
+        saved_RDI = context.uc_mcontext.gregs[REG_RDI];
  56
+        saved_RSI = context.uc_mcontext.gregs[REG_RSI];
  57
+
  58
+        // Hijack current context so we call our handler.
  59
+        auto rip = context.uc_mcontext.gregs[REG_RIP];
  60
+        auto addr = cast(REG_TYPE) info.si_addr;
  61
+        context.uc_mcontext.gregs[REG_RDI] = addr;
  62
+        context.uc_mcontext.gregs[REG_RSI] = rip;
  63
+        context.uc_mcontext.gregs[REG_RIP] = (rip != addr)?(cast(REG_TYPE) &sigsegv_data_handler):(cast(REG_TYPE) &sigsegv_code_handler);
1
Martin Nowak Collaborator

The line length is 129!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/etc/linux/memoryerror.d
((145 lines not shown))
  145
+
  146
+    extern(C)
  147
+    void handleSignal(int signum, siginfo_t* info, void* contextPtr)
  148
+    {
  149
+        auto context = cast(ucontext_t*)contextPtr;
  150
+
  151
+        // Save registers into global thread local, to allow recovery.
  152
+        saved_EAX = context.uc_mcontext.gregs[REG_EAX];
  153
+        saved_EDX = context.uc_mcontext.gregs[REG_EDX];
  154
+
  155
+        // Hijack current context so we call our handler.
  156
+        auto eip = context.uc_mcontext.gregs[REG_EIP];
  157
+        auto addr = cast(REG_TYPE) info.si_addr;
  158
+        context.uc_mcontext.gregs[REG_EAX] = addr;
  159
+        context.uc_mcontext.gregs[REG_EDX] = eip;
  160
+        context.uc_mcontext.gregs[REG_EIP] = (eip != addr)?(cast(REG_TYPE) &sigsegv_code_handler + 0x03):(cast(REG_TYPE) &sigsegv_data_handler);
1
Martin Nowak Collaborator

line length

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Martin Nowak
Collaborator

Please stick to camel casing for function names.

deadalnix

What is the hard limit for line length ? What is the policy to split ternary operator on multiple line ?

Jonathan M Davis
Collaborator

What is the hard limit for line length ?

There is a soft character limit of 80 characters per line and a hard limit of 120. So, most lines should be within 80 characters but no lines can exceed 120.

What is the policy to split ternary operator on multiple line ?

There is none. For the most part, we don't have much in the way of style rules about formatting. They boil down primarily to using 4 spaces per indent (no tabs) and putting braces on their own line. For most of the rest, it's primarily a matter of making sure that the style in the file is reasonably consistent, and there probably aren't enough ternary operators being used to even set a precedent within most files. Personally, if the line with a ternary operator is too long, I do

auto result = condition
              ? branch1
              : branch2

but I expect that there's code in druntime and/or Phobos which formats that differently. At minimum, if Andrei were doing it, he'd only indent the 2nd and 3rd lines by 4 spaces rather than lining them up with the previous line. It all depends on who wrote the code and what file it's in (as the formatting style varies from file to file).

Martin Nowak MartinNowak merged commit c5b30a5 into from November 29, 2012
Martin Nowak MartinNowak closed this November 29, 2012
Martin Nowak
Collaborator

OK let's give it a try.
I've added the missing win{32,64}.mak rules here and here.

Andrei Alexandrescu
Owner

historical moment!

deadalnix

This one have been epic :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
13  posix.mak
@@ -267,7 +267,9 @@ MANIFEST= \
267 267
 	src/rt/util/console.d \
268 268
 	src/rt/util/hash.d \
269 269
 	src/rt/util/string.d \
270  
-	src/rt/util/utf.d
  270
+	src/rt/util/utf.d \
  271
+	\
  272
+	src/etc/linux/memoryerror.d
271 273
 
272 274
 GC_MODULES = gc/gc gc/gcalloc gc/gcbits gc/gcstats gc/gcx
273 275
 
@@ -398,7 +400,9 @@ SRC_D_MODULES = \
398 400
 	rt/typeinfo/ti_ulong \
399 401
 	rt/typeinfo/ti_ushort \
400 402
 	rt/typeinfo/ti_void \
401  
-	rt/typeinfo/ti_wchar
  403
+	rt/typeinfo/ti_wchar \
  404
+	\
  405
+	etc/linux/memoryerror
402 406
 
403 407
 # NOTE: trace.d and cover.d are not necessary for a successful build
404 408
 #       as both are used for debugging features (profiling and coverage)
@@ -538,7 +542,9 @@ COPY=\
538 542
 	$(IMPDIR)/core/sys/windows/dll.d \
539 543
 	$(IMPDIR)/core/sys/windows/stacktrace.d \
540 544
 	$(IMPDIR)/core/sys/windows/threadaux.d \
541  
-	$(IMPDIR)/core/sys/windows/windows.d
  545
+	$(IMPDIR)/core/sys/windows/windows.d \
  546
+	\
  547
+	$(IMPDIR)/etc/linux/memoryerror.d
542 548
 
543 549
 SRCS=$(addprefix src/,$(addsuffix .d,$(SRC_D_MODULES)))
544 550
 
@@ -587,6 +593,7 @@ copydir:
587 593
 	-mkdir -p $(IMPDIR)/core/sys/osx/mach
588 594
 	-mkdir -p $(IMPDIR)/core/sys/freebsd/sys
589 595
 	-mkdir -p $(IMPDIR)/core/sys/linux/sys
  596
+	-mkdir -p $(IMPDIR)/etc/linux
590 597
 
591 598
 copy: $(COPY)
592 599
 
50  src/core/sys/posix/ucontext.d
@@ -40,6 +40,33 @@ version( linux )
40 40
 
41 41
     version( X86_64 )
42 42
     {
  43
+        enum
  44
+        {
  45
+            REG_R8 = 0,
  46
+            REG_R9,
  47
+            REG_R10,
  48
+            REG_R11,
  49
+            REG_R12,
  50
+            REG_R13,
  51
+            REG_R14,
  52
+            REG_R15,
  53
+            REG_RDI,
  54
+            REG_RSI,
  55
+            REG_RBP,
  56
+            REG_RBX,
  57
+            REG_RDX,
  58
+            REG_RAX,
  59
+            REG_RCX,
  60
+            REG_RSP,
  61
+            REG_RIP,
  62
+            REG_EFL,
  63
+            REG_CSGSFS,     /* Actually short cs, gs, fs, __pad0.  */
  64
+            REG_ERR,
  65
+            REG_TRAPNO,
  66
+            REG_OLDMASK,
  67
+            REG_CR2
  68
+        }
  69
+
43 70
         private
44 71
         {
45 72
             struct _libc_fpxreg
@@ -95,6 +122,29 @@ version( linux )
95 122
     }
96 123
     else version( X86 )
97 124
     {
  125
+        enum
  126
+        {
  127
+            REG_GS = 0,
  128
+            REG_FS,
  129
+            REG_ES,
  130
+            REG_DS,
  131
+            REG_EDI,
  132
+            REG_ESI,
  133
+            REG_EBP,
  134
+            REG_ESP,
  135
+            REG_EBX,
  136
+            REG_EDX,
  137
+            REG_ECX,
  138
+            REG_EAX,
  139
+            REG_TRAPNO,
  140
+            REG_ERR,
  141
+            REG_EIP,
  142
+            REG_CS,
  143
+            REG_EFL,
  144
+            REG_UESP,
  145
+            REG_SS
  146
+        }
  147
+
98 148
         private
99 149
         {
100 150
             struct _libc_fpreg
277  src/etc/linux/memoryerror.d
... ...
@@ -0,0 +1,277 @@
  1
+/**
  2
+ * Handle page protection error using errors. $(D NullPointerError) is thrown when dereferencing null. A system-dependant error is thrown in other cases.
  3
+ * Note: Only x86 and x86_64 are supported for now.
  4
+ *
  5
+ * License: Distributed under the
  6
+ *      $(LINK2 http://www.boost.org/LICENSE_1_0.txt, Boost Software License 1.0).
  7
+ *    (See accompanying file LICENSE_1_0.txt)
  8
+ * Authors:   Amaury SECHET, FeepingCreature, Vladimir Panteleev
  9
+ * Source: $(DRUNTIMESRC src/etc/linux/memory.d)
  10
+ */
  11
+module etc.linux.memoryerror;
  12
+
  13
+version(linux)
  14
+{
  15
+
  16
+private:
  17
+import core.sys.posix.signal;
  18
+import core.sys.posix.ucontext;
  19
+
  20
+// Register and unregister memory error handler.
  21
+private __gshared sigaction_t old_sigaction;
  22
+
  23
+int registerMemoryErrorHandler()
  24
+{
  25
+    sigaction_t action;
  26
+    action.sa_sigaction = &handleSignal;
  27
+    action.sa_flags = SA_SIGINFO;
  28
+    
  29
+    auto oldptr = &old_sigaction;
  30
+
  31
+    return sigaction(SIGSEGV, &action, oldptr);
  32
+}
  33
+
  34
+int unregisterMemoryErrorHandler()
  35
+{
  36
+    auto oldptr = &old_sigaction;
  37
+
  38
+    return sigaction(SIGSEGV, oldptr, null);
  39
+}
  40
+
  41
+// Sighandler space
  42
+
  43
+alias typeof(ucontext_t.init.uc_mcontext.gregs[0]) REG_TYPE;
  44
+
  45
+version(X86_64)
  46
+{
  47
+    static REG_TYPE savedRDI, savedRSI;
  48
+
  49
+    extern(C)
  50
+    void handleSignal(int signum, siginfo_t* info, void* contextPtr)
  51
+    {
  52
+        auto context = cast(ucontext_t*)contextPtr;
  53
+
  54
+        // Save registers into global thread local, to allow recovery.
  55
+        savedRDI = context.uc_mcontext.gregs[REG_RDI];
  56
+        savedRSI = context.uc_mcontext.gregs[REG_RSI];
  57
+
  58
+        // Hijack current context so we call our handler.
  59
+        auto rip = context.uc_mcontext.gregs[REG_RIP];
  60
+        auto addr = cast(REG_TYPE) info.si_addr;
  61
+        context.uc_mcontext.gregs[REG_RDI] = addr;
  62
+        context.uc_mcontext.gregs[REG_RSI] = rip;
  63
+        context.uc_mcontext.gregs[REG_RIP] = cast(REG_TYPE) ((rip != addr)?&sigsegvDataHandler:&sigsegvCodeHandler);
  64
+    }
  65
+
  66
+    // All handler functions must be called with faulting address in RDI and original RIP in RSI.
  67
+
  68
+    // This function is called when the segfault's cause is to call an invalid function pointer.
  69
+    void sigsegvCodeHandler()
  70
+    {
  71
+        asm
  72
+        {
  73
+            naked;
  74
+
  75
+            // Handle the stack for an invalid function call (segfault at RIP).
  76
+            // With the return pointer, the stack is now alligned.
  77
+            push RBP;
  78
+            mov RBP, RSP;
  79
+
  80
+            jmp sigsegvDataHandler;
  81
+        }
  82
+    }
  83
+
  84
+    void sigsegvDataHandler()
  85
+    {
  86
+        asm
  87
+        {
  88
+            naked;
  89
+
  90
+            push RSI;   // return address (original RIP).
  91
+            push RBP;   // old RBP
  92
+            mov RBP, RSP;
  93
+
  94
+            pushfq;     // Save flags.
  95
+            push RAX;   // RAX, RCX, RDX, and R8 to R11 are trash registers and must be preserved as local variables.
  96
+            push RCX;
  97
+            push RDX;
  98
+            push R8;
  99
+            push R9;
  100
+            push R10;
  101
+            push R11;    // With 10 pushes, the stack is still aligned.
  102
+
  103
+            // Parameter address is already set as RAX.
  104
+            call sigsegvUserspaceProcess;
  105
+
  106
+            // Restore RDI and RSI values.
  107
+            call restoreRDI;
  108
+            push RAX;   // RDI is in RAX. It is pushed and will be poped back to RDI.
  109
+
  110
+            call restoreRSI;
  111
+            mov RSI, RAX;
  112
+
  113
+            pop RDI;
  114
+
  115
+            // Restore trash registers value.
  116
+            pop R11;
  117
+            pop R10;
  118
+            pop R9;
  119
+            pop R8;
  120
+            pop RDX;
  121
+            pop RCX;
  122
+            pop RAX;
  123
+            popfq;      // Restore flags.
  124
+
  125
+            // Return