Make coop handles typesafe. (#9183)
commit475a69f7c56a3827a4eafe9fed4151b6feed05a4
authorJay Krell <jay.krell@cornell.edu>
Tue, 19 Jun 2018 13:39:18 +0000 (19 06:39 -0700)
committerGitHub <noreply@github.com>
Tue, 19 Jun 2018 13:39:18 +0000 (19 06:39 -0700)
treeea4a814e05928a6db542564400096de714511ff1
parent9b9e4f4bb6d579cd70ed8f2667a5d2edef67dc26
Make coop handles typesafe. (#9183)

* Make coop handles typesafe.

Type-safe handles are a struct with a pointer to pointer.
The only operations allowed on them are the functions/macros in handle.h, and assignment
from same handle type to same handle type (which cannot be prohibited in C, but is ok here anyway.)

Type-unsafe handles are a pointer to a struct with a pointer.
This is a leakier abstraction and besides the type-safe operations, also allows:

1. compared to NULL, instead of only MONO_HANDLE_IS_NULL
2. assigned from NULL, instead of only a handle
3. MONO_HANDLE_NEW (T) from anything, instead of only a T*
4. MONO_HANDLE_CAST from anything, instead of only another handle type
5. assigned from any void*, at least in C
6. Cast from any handle type to any handle type, without using MONO_HANDLE_CAST.
7. Cast from any handle type to any pointer type and vice versa, such as incorrect unboxing.
8. mono_object_class (handle), instead of mono_handle_class.

None of those operations were likely intended and some are crashing sort of bugs, esp.
the last two. See https://github.com/mono/mono/pull/9188 for the last two. The rest all occur also but are not necessarily harmful. Raw(NULL_HANDLE) and IsNull(NULL_HANDLE) were both access violations, so NULL just as well. MONO_HANDLE_CAST is still pretty liberal in what it allows.
NULL_HANDLE has been brought into question and this will allow grepping for it instead of the plainer NULL (i.e. returning them as handles).

The interaction of these handles with JITed code depends on the ABI to pass/return a struct
with a pointer the same as a pointer. Marshalling assumes this.
There might be some OS/CPU ABIs that do not support this.
CI suggests it is broadly but not 100% viable (disabled for non-Mac non-Windows x86, but enabled for arm, arm64, amd64, Mac/x86, Windows/x86).

* Typesafe handles enforcement brings with it a little more pointer enforcement, mostly around const and volatile. Repair a bit, and disable temporary for Windows.

 * Fix MONO_HANDLE_IS_NULL (NULL_HANDLE) to not null dereference.

 * Fix MONO_HANDLE_RAW(NULL_HANDLE) to not null dereference.
This depends on a gcc/clang extension, and a Visual C++ (C) quirk.
Otherwise we have to chose double evaluation, or C++, or assignment or cast from void* to temporary (possibly passing more types to more macros).
Another choice is a runtime assert, and double evaluation or not.
Asserts seen can be covered up by first checking MONO_HANDLE_IS_NULL, which will not null  dereference nor assert.

Prior to making RAW(NULL) not null derefence, fixing code of the form:
```gboolean is_null_handle = handle && MONO_HANDLE_IS_NULL(handle)```
to be
```gboolean is_null_handle = MONO_HANDLE_IS_NULL(handle)```
while working on this change led to failures.

* Add gcc -Werror=incompatible-pointer-types to catch some handle type mismatches.
This has slight collateral damage on other code, mostly around const and volatile, mostly in Android-specific code. Disabled for Windows/gcc at first.

* Change mono_icall_handle_new_interior to return gpointer instead of MonoObjectHandle. It isn't returning a handle and making the non-handle typesafe is unnecessary/incorrect.
Likewise change mono_handle_new_interior to return gpointer instead of MonoRawHandle (same thing).
gpointer* is more correct, gpointer is more opaque; unclear which is preferred here.
gpointer makes this a smaller semantic change, as MonoRawHandle was already equivalent, just a rename.

* Move NULL_HANDLE, NULL_HANDLE_STRING, NULL_HANDLE_ARRAY so they are near the comment that refers to all of them.

* Remove mono_handle_new_full prototype, the function does not exist.

* Remove unnecessary casts.

* Remove unused MONO_HANDLE_INIT.

* Add an assert to handle_assign.

* A refinement of this change should enable WebAssembly and skirt underlying safety less. WebAssembly does not allow the function pointer casts used here.
24 files changed:
configure.ac
mono/eglib/goutput.c
mono/eglib/gspawn.c
mono/metadata/appdomain.c
mono/metadata/cominterop.c
mono/metadata/custom-attrs.c
mono/metadata/exception.c
mono/metadata/handle.c
mono/metadata/handle.h
mono/metadata/icall.c
mono/metadata/marshal.c
mono/metadata/marshal.h
mono/metadata/object.c
mono/metadata/reflection-cache.h
mono/metadata/reflection.c
mono/metadata/remoting.c
mono/metadata/sre-encode.c
mono/metadata/sre.c
mono/metadata/threads.c
mono/metadata/w32file.c
mono/metadata/w32socket.c
mono/mini/aot-compiler.c
mono/mini/exceptions-wasm.c
mono/mini/tramp-wasm.c