Usage of global scope by Linker is unsafe for pointers to `static` data in loaded library

[Maurizio Cimadamore]

Filed:
https://bugs.openjdk.org/browse/JDK-8340641

Cheers
Maurizio

On 23/09/2024 10:47, Maurizio Cimadamore wrote:
>
> Hi,
> the problem you describe here is that the caller _knows_ (e.g. by 
> reading the API javadoc) that a pointer returned by a native library 
> call points to static data and has, therefore, the same lifetime as 
> the library itself.
>
> The linker cannot know this. The linker only sees a call that returns 
> a pointer - the pointer can have any size and any lifetime. It is up 
> to the caller to "sanitize" that call, by overriding the global 
> lifetime with a more appropriate one (e.g. the one used to load the 
> library). But there's no way for the Linker to "guess" - maybe the 
> native function does a malloc, and returns a pointer to it - the 
> lifetime could also be much smaller than that of the native library. 
> For this reason, changing the default downcall behavior is not an 
> option: while the interpretation you suggest might be ok for functions 
> returning data, it is certainly not ok for all the other functions.
>
> To override the lifetime, the MemorySegment::reinterpret method can be 
> used. It is true that reinterpret accepts and Arena, and I think your 
> main issue is that, at the point when the call is made, you don't have 
> this Arena. The reason as to why reinterpret wants an Arena and not a 
> Scope is explained here: 
> https://github.com/openjdk/panama-foreign/pull/812. The problem is 
> that we don't have a way (at least for now) to set the lifetime of a 
> memory segment w/o _also_ altering its thread confinement. That is, a 
> client might expect that setting a scope on a segment might leave all 
> the other segment properties unchanged, but that's not the case: the 
> segment confinement is also changed.
>
> As an alternative, we could also have a linker option to mark 
> downcalls that return pointers to static data (so that the downcall 
> will reassign the scope of the returned segment as you desire). While 
> this might be effective, if you don't control the creation of the 
> downcall method handle, it will not be a good/general solution (but 
> maybe it's enough?)
>
> Maurizio
>
>
>
>
>
>
>
> On 21/09/2024 16:54, 
> some-java-user-99206970363698485155 at vodafonemail.de wrote:
>>
>> Hello,
>>
>> as mentioned in the Javadoc, for downcalls and upcalls Linker uses 
>> the global scope for the result respectively the arguments. This is 
>> problematic for pointers to `static` data in C because it leads to 
>> unsafe code.
>> Specifically, consider this case:
>>
>>  1. Load a library using SymbolLookup
>>  2. Obtain the address of a function in that library
>>  3. Call that function using Linker; the function returns a pointer
>>     to `static` data
>>  4. Use the function result
>>
>> If I understand it correctly that pointer to the `static` data 
>> actually points within the data of the loaded library (and not some 
>> newly allocated heap memory). But I am not that familiar with C, so 
>> please correct me if I am wrong.
>>
>> The problem is now if you accidentally unload the library by closing 
>> the Arena which was used for loading it, either explicitly or by the 
>> garbage collector in case of Arena.ofAuto(), while the function 
>> result is still is use.
>> Because the Linker used the global scope, if you use the `static` 
>> data returned by the function it will crash the JVM because it is not 
>> detected that the original Arena had been closed, and the library had 
>> been unloaded.
>>
>>
>> What makes this worse is that if you are just given a SymbolLookup, 
>> or even a MemorySegment obtained from it, you don't have access to 
>> the original Arena used to load the library, so you cannot even 
>> manually fix this unsafety this by using `MemorySegment#reinterpet` 
>> to change the Arena.
>>
>>
>> Maybe it would therefore be safer if Linker for downcalls used the 
>> scope of the given function pointer MemorySegment, instead of the 
>> global scope. What do you think?
>> For upcall arguments I am not sure if there is a way to fix this 
>> unsafety.
>>
>>
>> If the behavior for downcalls cannot be changed, could you then 
>> please at least add a 'restricted' `MemorySegment#withScope(Scope)` 
>> (maybe with additional 'cleanup' argument) or similar, so that you 
>> can manually change the scope of the downcall result, without needing 
>> a reference to the original Arena (which is currently required for 
>> `MemorySegment#reinterpet`)?
>> Though personally I would prefer if the default downcall behavior was 
>> changed, because manually having to change the scope of the downcall 
>> result every time is error-prone and easy to forget.
>>
>>
>> Kind regards 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/panama-dev/attachments/20240923/aa8a05fd/attachment.htm>