[foreign-abi] minimizing the surface of restricted native access

[Maurizio Cimadamore]

Hi Ioannis,
thanks you very much for your precious and honest feedback - some 
responses inline below:

On 27/11/2019 18:32, Ioannis Tsakpinis wrote:
> Hey Maurizio,
>
> Some thoughts based on my experience as the maintainer of LWJGL:
>
>> But, thanks to the Nothing region, we now have a way out: the
>> dereference operation can be made safe, if the MemoryAddress generated
>> by the VarHandle (upon read) is backed by the special Nothing region.
>> This means that the user won't be able to dereference such an address,
>> but this might be ok in cases where the address just needs to be written
>> somewhere else, or passed to some native library.
> Sounds good for opaque addresses/handles.
>
>> MemoryAddress rebase(MemorySegment)
>>
>> The semantics of this method is simple: if the address is contained in
>> the supplied segment, this method returns a _new_ address that is
>> re-interpreted as an offset into the supplied segment.
> Sounds good for managed segments.
Ok - so far so good then :-)
>
>> But what if a client really wants to dereference a _random_ pointer
>> generated by a library? In this case, the client has no well-known
>> segment which can be used as a rebase target, so, what can be done? This
>> is where we need to veer into restricted native access territory, and
>> provide some escape hatch by which an untrusted address can be made
>> trusted again. There are many ways to do that; perhaps the simplest is
>> to add a new method on MemoryAddress:
>>
>> MemoryAddress asUnchecked()
>>
>> which returns a new memory address which point to same location as the
>> original address, but whose access is neither spatially, nor temporally
>> checked.
> This doesn't sound so good. :)
>
> There are many functions/structs in LWJGL that fall into this category.
> Almost always, there's additional information available that defines the
> returned pointer's spatial bounds. The most common cases are:
>
> a) The user specifies the buffer size as a parameter to the function.
> b) The buffer size is returned by the function (as the return value or as a
> separate output parameter).
> c) The struct contains another member, which specifies the buffer's size.
> d) It's a null-terminated string.
>
> In idiomatic LWJGL code, these pointers are wrapped in NIO buffers (of the
> appropriate type) with the correct capacity according to that additional
> information. Accessing memory via those buffers is then perfectly safe and
> subject to the standard bound checking within NIO. As an example, a very
> common OpenGL function:
>
> void *glMapBufferRange(
>      GLenum target,
>     GLintptr offset,
>     GLsizeiptr length,
>     GLbitfield access
> );
>
> It maps a buffer object (which lives in GPU memory) to host-addressable
> memory. If the offset/length parameters are invalid, NULL is returned. If
> not, we get a pointer that is wrapped in a ByteBuffer with capacity equal
> to length.
>
> I have not used the memory access API yet, but I guess the equivalent here
> would be to create a MemorySegment that starts at the returned pointer and
> ends after length bytes. That way the mapped memory would be safely
> accessible, just like any other managed segment.
>
> Having to go through the Everything segment and dropping spatial bound
> checking would feel like a regression as an LWJGL user. It would also mean
> that the majority of LWJGL bindings would fall under "untrusted/unsafe"
> territory. Off the top of my head, functions like glMapBufferRange exist in
> OpenGL, OpenGL ES, OpenCL, Vulkan, CUDA, Assimp, bgfx, every custom memory
> allocator and possibly many more.

All the points you make are very valid - and it's exactly the kind of 
feedback I was hoping to get; what I tried to do with 'asUnchecked' is, 
in essence, a _lumpy_ move. In reality there's a much bigger space - 
since you have both spatial _and_ temporal bounds, which can be 
(independently) be enabled/disabled. This leads to a 2x2 matrix - where 
one corner (everything is checked) is where normal, non-native clients 
of the memory access API would live all the time; whereas the dual 
corner (nothing is checked) is what I described as the Everything segment.

Your examples seem to suggest that jumping from the former quadrant to 
the latter is too sudden of a transition, and that the other quadrants 
are important too. That might well be the case, and it is true that some 
of the safety belt can be added back (e.g. if the bounds are known from 
a separate parameter). But _there is_ a transition, nevertheless, as we 
have moved, subtly, from a world where spatial and temporal bounds were 
guaranteed to be 100% correct by construction to a world where, even if 
there are bounds in place, the bounds could be wrong (as these bounds 
are injected after the fact).

So, maybe we will devise different API points to slice and dice the 
safety belts with the desired level of granularity; or maybe you will 
have first have to go _fully unchecked_ and then add some degree of 
safety back into the mix. Either way, when you start playing this game, 
all bets are off - and my main claim here is that having an _explicit_ 
mechanism which signal this transition is better than, say, implicitly 
assume that all addresses returned by the native library are some kind 
of free-for-all (e.g. unchecked by default).

>
> Two more notes:
>
> 1. Temporarily, the pointer returned by glMapBufferRange lives until a call
> to glUnmapBuffer. I don't have a good answer for how to automatically close
> the corresponding MemorySegment. It should be like: mark the MemorySegment
> as closed, memory barrier (to flush any writes to the buffer and avoid
> reordering), call the glUnmapBuffer function.
This is another very good point; whatever mechanism we come up with, it 
has to allow for 'closing' the segment - in the sense allowing for 
making the segment unavailable for further uses. That is, if create_foo 
function gives you a MemoryAddress that is attached to a segment, when 
you pass that same address to the dual destroy_foo function, there 
should be a way to mark that segment as closed. The model I proposed did 
not allowed for close() and - I realize now - because part of me was 
still thinking in terms of close() == free. In reality closing a segment 
is orthogonal to freeing up memory - and however we want to model native 
unchecked addresses and segments, they should support a way to say - no 
more please.
>
> 2. Bounds checking is so important to LWJGL, that it's literally impossible
> to generate a binding, without specifying the relationships between function
> parameters or struct members. We also use libclang to parse headers and
> automate most of the process, but that's not enough. Every pointer
> parameter or return value must then be annotated with metadata that
> precisely define its bounds. In the worst case, the bounds cannot be derived
> and the pointer is marked as unsafe (in that case it is mapped to an opaque
> pointer, not a buffer). Thankfully, this occurs extremely rarely, in badly
> designed C APIs.
You touch a very valid point here, and one that it's been on my mind too 
- some of the stuff we're talking about ultimately falls on the 
shoulders of the binder generator; the memory access API, and the low 
level SystemABI interface should provide the binder generator enough 
hooks so that it is possible e.g. to attach dynamic bounds to an 
otherwise unchecked pointer; or that it is possible to close an 
unchecked segment. But this is not something that I expect to happen 
auto-magically. Our basic, raw jextract might just give you back a raw 
pointer - but I would expect ,more advanced binder generators to do 
exactly the kind of things you are describing. Which also means that we 
need to make sure that, whatever extraction API we come up with (see 
related writeup [1]), it is sufficiently expressive for you to build 
your stuff on top of - and we probably could use some guidance in that 
(the API I have in the document is really a strawman).
>
>> There is, of course a limitation: this model assumes that a client will
>> never need/want to 'free' a random pointer obtained by calling a native
>> library: since the Nothing segment is always alive (by definition), it
>> cannot be closed. While this restriction will be ok in most cases, there
>> will be some APIs requiring this - examples are strdup, vasprintf, which
>> require the client to specifically 'free' the given address. That said,
>> this problem doesn't seem too difficult to solve: for instance a client
>> can request a native MethodHandle which points to the 'free' stdlib
>> function, and then can call it directly on the required address! I think
>> such an approach would actually be preferable to an approach where
>> MemorySegment::close() secretly implies calling free() on some address -
>> which might be the case now, but might not be later on (e.g. if we
>> replace the allocator used internally by the memory access API).
> Related to the above, there needs to be an alternative to the stdlib-backed
> MemorySegment::allocateNative. We've seen this before:
>
> ByteBuffer::allocateDirect is too expensive for short allocations. Users
> allocate a big buffer, then do sub-allocations out of that buffer. They
> soon end up writing a "memory allocator" in Java. That memory allocator is
> bad (in at least one important dimension).

We are super aware of this issue. In fact over the last few months I 
didn't do much else other than staring at the ByteBuffer allocation 
code, and benchmarking to see where the numbers were coming from. The 
reason as to why ByteBuffer doesn't work well allocation-wise is, as 
always, complex; part of it is in the allocator itself - as you point 
out, malloc is not the greatest choice, nor the one that offers highest 
throughput and/or scalability. Part of it is in the memory zeroing 
process; the VM code which does the zeroing needs to touch all the 
pages, which results in abysimal performances as the allocation size 
goes up. Then there's the Java issues - ByteBuffer provides no 
deterministic deallocation, which means a Cleaner has to be attached to 
every direct buffer. Every Cleaner object takes up some space and then 
there's some cleaner-related space in the ByteBuffer too. This can 
easily lead to GC-related issues in allocation intensive code - in fact 
ByteBuffer even has an exponential backoff logic (!!) to keep retrying 
allocating a direct buffer in case the allocation limit is hit and there 
are some cleaners laying around; needless to say, this process inserts 
even more latency in the picture.

Fixing the allocator-side of things (e.g. replacing malloc/free with 
something else) w/o fixing the API issues is not gonna solve the problem 
in full. The main goal of the memory access API is to provide 
predictable performances, mainly by providing deterministic deallocation 
- that is, no cleaners. In other words, when you allocate a native 
segment, not much is allocated on the heap; and when we'll move to 
inline classes, it'll be even better - since both MemorySegment and 
MemoryAddress are Valhalla-ready; you only need to allocate one mutable 
scope object - which is in practice reused e.g. among slices and such.

But we know also that fixing the API side of things alone doesn't itself 
address all the performance issues; that is why, as I mentioned earlier 
in another mailing list, we've been toying with an alternate allocation 
strategy that is looking very promising (albeit this is in a very early 
stage). So, I hope that in few weeks, more pieces of the puzzle will be 
made more publicly available, so that you can appreciate where we're 
trying to aim.


>
> In LWJGL there are two alternatives:
>
> 1. Use a specialized memory allocator (jemalloc, rpmalloc, etc). Either
> change the default, or use it where it makes sense. Again, this means
> "random" pointers returned, which need to be bounded and trusted.
> 2. There is a "stack allocation" API. I won't expand on this before I see
> your proposal (mentioned in another thread recently), but it's critically
> important (both in terms of performance and of writing clean code).
>
>> Thoughts?
> I highly appreciate your efforts to make the memory access API as safe as
> possible. We strive for it in LWJGL too. However, lets also address the
> elephant in the room. There's going to be a way to create NULL pointers in
> Panama. Users are going to pass that NULL pointer to a function parameter
> that does not accept NULLs [1]. Users are going to pass all kinds of illegal
> parameter values. The JVM is going to crash, a lot, and you're probably
> going to be blamed for it. There are also a million different ways to crash
> the JVM process once native API access becomes easy, that have nothing to
> do with the bindings themselves. Writing a compute shader that does
> nasty things, or even a shader that's legitimate but the GPU driver doesn't
> like this week. Calling OpenGL functions in the wrong order (you can't
> really validate a massive state machine on every function call to avoid the
> crash).
>
> Anyway, I'm just saying there needs to be a balance between safety and API
> usability. I'm not saying giving up on security, but perfect crash-freedom
> is a lost cause.

Point taken. I believe there is a space where crash-freedom makes total 
sense (and I hope you agree with me on this): memory access API clients 
that have nothing to do with native code. Think about a library that 
models multidimensional arrays a la Python NDArray. This API could 
decide to use the memory access API and its var handles to manage 
tensors memory, and access to their elements. And I think it would be 
very unfair to the developers of this API if the best guarantees the 
memory access could provide was: "sorry, there will be crashes sometimes".

I understand that interacting with native libraries (either via Panama 
or JNI) is a whole different game - and things there can crash left and 
right, whether we like it or not. But I think there is still value in 
distinguishing between: (a) I've got a crash because I passed something 
really bad to the function vs. (b) I've got a crash because I _thought_ 
this segment had these bounds and in reality it does not. The former is, 
in my mental model, more akin to when you call a Java method and you get 
some exception; the latter is more similar to a failure in an unchecked 
cast operation - you thought an object was a List<String> (and the VM 
had to trust you on that, since there's no reified generics) - but, 
sorry it turned out to be a List<Integer>. There's a word for this in 
the language - heap pollution, and a mandatory warning generated for 
these potentially bad casts at compile time. Now, I don't want to 
stretch the analogy too far - but I think that when you take a 'Nothing' 
pointer and you say: trust me, this thing is going to be at least 64 
bytes - well, it's kind of like doing an unchecked cast - call it 'bound 
pollution'. And the problem about pollution-related failures is that 
they don't tend to happen on the spot - they often linger for a long 
time, only to cause problems later, sometimes in a seemingly unrelated 
piece of code. So, I think that striving to have a clean transition 
between safe-land and unchecked-land still looks like a good rule of 
thumb to me.

Thanks
Maurizio

[1] - http://cr.openjdk.java.net/~mcimadamore/panama/jextract_distilled.html

>
> - Ioannis
>
> [1] This is another aspect that LWJGL deals with manually after libclang
> automatic-generation. Pointer parameters are explicitly annotated as
> nullable, otherwise we check for NULL and throw at runtime.