[foreign] RFR 8218772: Limit struct member pointers to size of the field

[Maurizio Cimadamore]

Hi Jorn,
I was looking at something very related to this - e.g. relationship 
between pointers and arrays, and, in general bulk-write operations for 
structs and arrays (with Pointer::set) and I thought I might add 
something to this discussion, to see if the issues that you are running 
into are just bugs, or symptoms of something deeper.

Over the last few weeks I've been toying with the idea of merging Array 
and Pointers - after all a BoundedPointer is expressive enough to 
represent both. We could e.g. setup an approach like the one below:


- Pointer has array-like accessors Pointer::get(long), 
Pointer::set(long, X)

- regular accessor defaults to zero offset - that is, Pointer::get() -> 
Pointer::get(0)

- jextract does NOT generate setters for array struct fields, or array 
global variables - only getters

- an API is provided (well, one exists already) to do bulk copy between 
different arrays/structs


While I like the unification this brings about (only one abstraction 
instead of two, Pointer and Array), and I also like the fact that we 
move towards a model where Pointer::get, Pointer::set are O(1) 
operations (with bulk operations left to higher level APIs), there is 
something that doesn't 100% convinces me: if we go down this path, the 
following layouts:

u64:u64:i32 (pointer to pointer to int)

and

u64:[5 i32] (pointer to array of int)

will effectively have the same carrier type:

Pointer<Pointer<Integer>>

The difference will be visible only upon closer inspection: we could 
call Pointer::type() obtain a LayoutType, then do LayoutType::layout() 
and see whether the pointee layout is a sequence or an address.

Now, when we perform a get() on such a pointer, we can, given the 
layout, construct the right pointer with the right size info (if any).

But what about set() ? I see different options here, none of which seems 
particularly satisfying:

1) Pointer::set should throw if you are trying to set an array-like 
pointer (which would require bulk copy)
2) Pointer::set will silently perform bulk copy of the incoming pointer 
into the pointed region

Of these, (2) is similar to what we have now, whereas (1) would be a 
stricter variant.

The thing that puzzles me is that, looking at the code it will be 
absolutely impossible to understand what's going on - e.g. if native 
arrays and native pointers map to the same Java carrier (Pointer) it 
then becomes super hard to explain/understand/predict why a given 
operation (e.g. Pointer::set) resulted in bulk copy/exception.

One of the principles I've been trying to adhere to when designing the 
foreign API is to avoid surprises - e.g. the semantics of a given 
operation should be clear from the carrier types involved in the 
operation. In that respect, maybe a pointer with an optional length() is 
tolerable, but having Pointer::set behaving differently depending on the 
result of Pointer::type() is that acceptable or too subtle?

This seems to suggest that the current Pointer vs. Array split carries 
some weight. If carrier type are different then it's easy to see for the 
user which operation might or might not be supported (or might have 
bulk-logic).

<sidebar>
On the other hand, one might argue that this is already happening with 
pointers to incomplete objects - if I have a Pointer<?> and I call get() 
I don't know if I'll get an exception or not. It again depends on 
whether the layout complete or not - e.g. if it's the void layout or, if 
it's partial layout (because it refers to an unresolved struct) an 
exception will be thrown.
</sidebar>


If we do retain Pointer vs. Array then I think we are free to decide 
whether for Pointer::set we want (1) or (2) - that is, when we have

Pointer<Array<Integer>>

or

Pointer<StructFoo>

should Pointer::set do bulk copy, or should it throw? If the latter, 
should jextract even emit setters for struct fields of type 
array/struct? (or, perhaps, emulate such setters by performing explicit 
copy, rather than by using Pointer::set internally).


These are all questions that are relevant, I believe, to the fix you 
brought up - I'm of course fine with the fix, but I'd like to also 
understand whether the bulk-copy on Pointer::set is one magic trick too 
far and also, more generally, what do Panama-land feels about the Array 
vs. Pointer split/lump.

Feedback welcome.

Cheers
Maurizio








On 13/02/2019 15:47, Jorn Vernee wrote:
> Hi,
>
> I found a bug where it was possible to overwrite trailing fields of a 
> struct by writing an oversized array to a previous array field (see 
> bug). Overwriting is also possible in other cases by forcing an 
> oversized write to a struct field. This can be fixed (relatively 
> easily) by limiting the size of memory regions of pointers to struct 
> members to the size of their fields.
>
> Please review the following.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8218772
> Webrev: 
> http://cr.openjdk.java.net/~jvernee/panama/webrevs/8218772/webrev.00/
>
> Thanks,
> Jorn