[jdk19] RFR: 8289148: j.l.foreign.VaList::nextVarg call could throw IndexOutOfBoundsException or even crash the VM

[Jorn Vernee]

On Tue, 5 Jul 2022 11:03:30 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> This patch changes all VaList implementations to throw `NoSuchElementException` when out of bounds reads occur on a VaList that is created using the Java builder API. The docs are updated accordingly.
>> 
>> For VaLists that are created from native addresses, we don't know their bounds, so we can not throw exceptions in that case.
>> 
>> Testing: `jdk_foreign` test suite on all platforms that implement VaList.
>
> src/java.base/share/classes/java/lang/foreign/VaList.java line 61:
> 
>> 59:  * Particularly, variable argument lists created using {@link #make(Consumer, MemorySession)} are able to detect out-of-bounds reads,
>> 60:  * while variable argument lists crearted using {@link #ofAddress(MemoryAddress, MemorySession)} are not.
>> 61:  * <p>
> 
> Suggestion:
> 
>  * <h2>Safety considerations</h2>
>  * A variable argument list can be thought of as a cursor into a memory segment which stores one or
>  * more elements, with given layouts. The {@linkplain #nextVarg(ValueLayout.OfInt) methods} used to retrieve elements from
>  * a variable argument list update the cursor position; as such it is possible for clients to access elements
>  * outside the spatial bounds of the variable argument list.
>  * <p>
>  * A variable argument list attempts to detect out-of-bounds access on a best-effort basis.
>  * Whether this detection succeeds depends on the factory method used to create the variable argument list:
>  * <ul>
>  *     <li>Variable argument lists created <em>safely</em>, using {@link #make(Consumer, MemorySession)} are built on top of
>  *     native memory segments that are sized correctly. As such, they are capable of detecting out-of-bounds reads;</li>
>  *     <li>Variable argument lists created <em>unsafely</em>, using {@link #ofAddress(MemoryAddress, MemorySession)} are built on top
>  *     of segments whose size is unknown. As such they cannot detect out-of-bounds reads</li>
>  * <p>```

This seems a bit too much.

The class javadoc further up already describes a va list as "a stateful cursor used to iterate over a set of arguments". If that description is insufficient, I think it should be amended at that point.

Also, I don't think we have to go into describing how va lists returned by different factories are implemented (in fact, I think we should avoid that in order not to make accidental false promises when things change). It seems like noise next to the safety concerns (if someone really wants to know how the specified semantics are implemented, they can look at the code, or ask on the mailing list).

I'd suggest something simpler like this:

Suggestion:

 * It is possible for clients to access elements outside the spatial bounds of a variable argument list. Variable argument list
 * implementations will try to detect out-of-bounds reads on a best-effort basis.
 * <p>
 * Whether this detection succeeds depends on the factory method used to create the variable argument list:
 * <ul>
 *     <li>Variable argument lists created <em>safely</em>, using {@link #make(Consumer, MemorySession)} are capable of detecting out-of-bounds reads;</li>
 *     <li>Variable argument lists created <em>unsafely</em>, using {@link #ofAddress(MemoryAddress, MemorySession)} are not capable of detecting out-of-bounds reads</li>
 * </ul>
 * <p>


I'm also fine with changing the section title to "Safety considerations".

-------------

PR: https://git.openjdk.org/jdk19/pull/91