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

[Maurizio Cimadamore]

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/89e2782a/attachment-0001.htm>