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

[Maurizio Cimadamore]

On Wed, 29 Jun 2022 13:40:01 GMT, Jorn Vernee <jvernee 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.

Looks good - I've added some comments, esp. in the javadoc

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>```

src/java.base/share/classes/java/lang/foreign/VaList.java line 99:

> 97:      * @throws WrongThreadException if this method is called from a thread other than the thread owning
> 98:      * the {@linkplain #session() session} associated with this variable argument list.
> 99:      * @throws NoSuchElementException if an out-of-bounds read is detected.

I'd insert a link to the safety section, where the link text is "out-of-bounds".

src/java.base/share/classes/java/lang/foreign/VaList.java line 202:

> 200:      * restricted methods, and use safe and supported functionalities, where possible.
> 201:      *
> 202:      * @implNote variable argument lists created using this method can not detect out-of-bound reads.

Links here too

src/java.base/share/classes/jdk/internal/foreign/abi/x64/sysv/SysVVaList.java line 291:

> 289:     }
> 290: 
> 291:     private void checkRegAreaElement(MemoryLayout layout, TypeClass typeClass) {

should this be `checkRegSaveAreaElement` ?

test/jdk/java/foreign/valist/VaListTest.java line 827:

> 825: 
> 826:     @DataProvider
> 827:     public static Object[][] overflow() {

nice test!

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

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