On Tue, 5 Jul 2022 11:03:30 GMT, Maurizio Cimadamore <mcimadam...@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
