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

[Maurizio Cimadamore]

On 14/02/2019 15:18, Maurizio Cimadamore wrote:
> Why not using the Array.assign API point, which already performs all 
> these checks?

Similarly, I think References.OfStruct::set should use Struct.assign - 
as there's always a chance of mis-using the setter MH to write a struct 
that is bigger or smaller than the expected one.

Maurizio

>
> Maurizio
>
> On 14/02/2019 14:31, Jorn Vernee wrote:
>> Okay, I have implemented the check in References.OfArray::set.
>>
>> Updated webrev: 
>> http://cr.openjdk.java.net/~jvernee/panama/webrevs/8218772/webrev.01/
>>
>> I was also tinkering a long time with simplifying the original code, 
>> but kept running into issues with bounds checks in MemoryBoundInfo. 
>> The current bounds checking code isn't really suited for dealing with 
>> 0 length range checks.
>>
>> Jorn
>>
>> Jorn Vernee schreef op 2019-02-13 19:49:
>>> Jorn Vernee schreef op 2019-02-13 19:44:
>>>> Henry Jen schreef op 2019-02-13 19:19:
>>>>> On Feb 13, 2019, at 9:42 AM, Jorn Vernee <jbvernee at xs4all.nl> wrote:
>>>>>>
>>>>>> I don't think this bug is just a symptom of the bulk copy of 
>>>>>> Arrays. Note that you can also cause an overwrite when you have a 
>>>>>> struct with 2 ints, take a pointer to the first one, cast it to a 
>>>>>> long pointer, and write to it.
>>>>>>
>>>>>
>>>>> This is perfectly OK. The working theory is that as long as you
>>>>> operated in the allocated memory region, you should be able to 
>>>>> cast as
>>>>> needed.
>>>>>
>>>>> In you array case earlier, that’s not OK as the type of array has a
>>>>> length, so I would expect an exception. But if you cast the 
>>>>> pointer to
>>>>> an Array of 3, then it should be able to overwrite the int. After 
>>>>> all,
>>>>> [2i32]i32 and [3i32] and i32i32i32 are the same size of memory block.
>>>>
>>>> The problem is that there is no way to know the size of the target
>>>> array type once we're in References.OfArray::set, since the target
>>>> pointer might not actually have that type. The earliest location where
>>>> we do know this size is in RuntimeSupport, when retrieving the pointer
>>>> to the field.
>>>
>>> Actually, maybe we do know the type... I'm wondering about use of
>>> unsafe set operations in boxing code. I will just try it out and see.
>>>
>>> Jorn
>>>
>>>> Jorn
>>>>
>>>>> Cheers,
>>>>> Henry
>>>>>
>>>>>
>>>>>> It simply seems incorrect to me that a pointer to a struct's 
>>>>>> field provides access outside of that field's memory.
>>>>>>
>>>>>> ---
>>>>>>
>>>>>> FWIW, I think the Array abstraction is a useful one. It signals 
>>>>>> the difference between having a chunk of memory (Array) vs. 
>>>>>> having just a cursor into memory (Pointer), and as such, I think 
>>>>>> it's fine to say that writing an Array constitutes a copy of the 
>>>>>> array vs. just a copy of the pointer. If a bulk-copy is not 
>>>>>> wanted, users can use the elementPointer() instead (same with 
>>>>>> Struct::ptr).
>>>>>>
>>>>>> The native types do not map perfectly into Java types, so there 
>>>>>> are some things that have to be learned when using the API. I 
>>>>>> think the pitfall here is the assumption that, since in C an 
>>>>>> array is just a pointer in a lot of cases, and setting a pointer 
>>>>>> does not incur a copy, the Array type in Java must also really be 
>>>>>> just a Pointer, and setting it should not incur a copy. But in 
>>>>>> the API we have 2 distinct types, Pointer and Array, so I don't 
>>>>>> think it's unreasonable to say that those 2 will behave differently.
>>>>>>
>>>>>> Jorn
>>>>>>
>>>>>> Maurizio Cimadamore schreef op 2019-02-13 17:57:
>>>>>>> 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