ArrayIndexOutOfBoundsException with JNA 4.3.0

[neilcsmith-net]

With the GStreamer 1.x bindings I'm getting multiple ArrayIndexOutOfBoundsExceptions. I think they all involve varargs methods without any other parameters. The vararg type is custom type-mapped.

Stack trace is below. We've inherited an InvocationHandler that sits before Library$Handler.invoke(), though not caused an issue before, and not actually doing anything in this case.

java.lang.ArrayIndexOutOfBoundsException: 1
	at com.sun.jna.Function.invoke(Function.java:334)
	at com.sun.jna.Library$Handler.invoke(Library.java:244)
	at org.freedesktop.gstreamer.lowlevel.GNative$Handler.invoke(GNative.java:195)
	at com.sun.proxy.$Proxy16.gst_element_link_many(Unknown Source)
	at org.freedesktop.gstreamer.Element.linkMany(Element.java:547)
	at org.freedesktop.gstreamer.TestPipe.<init>(TestPipe.java:46)
	at org.freedesktop.gstreamer.BusTest.anyMessage(BusTest.java:244)

[matthiasblaesing]

@neilcsmith-net this is reproducible without any external calls. I added a unittest to catch this (see referenced commit).

With a JNA build with this applied (and the calls to context.getTargetType() "fixed" by casting to raw Class) unittests of gstreamer-1.x core run cleanly.

@twall Could you please have a look at this and see if you agree with my fix?

[twall]

Why would you have a varargs function that takes no arguments? You can't even write that in C; the first argument gives you the starting stack pointer as well as indicating what arguments to expect on the stack.

[neilcsmith-net]

Well, the signature of the method causing the problem is

boolean gst_element_link_many(Element... elements);

This has worked in every version of JNA up until 4.3.0 Element is mapped through a custom type mapper (something inherited I would like to get rid of!). Don't seem to be able to map it as Element[]

[matthiasblaesing]

The target case is not providing zero arguments - the case is providing arguments to a pure varargs call on the java side. I think looking at the concrete example will help:

https://gstreamer.freedesktop.org/data/doc/gstreamer/head/gstreamer/html/GstElement.html#gst-element-link-many

gboolean                gst_element_link_many           (GstElement *element_1,
                                                         GstElement *element_2, ...) G_GNUC_NULL_TERMINATED;

I read this as: you need to supply gst_element_link_many with at least two elements, but potentially an unlimited number.

So the correct binding would have been (my interpretation):

boolean gst_element_link_many(Element element1, Element elemente2, Element... elements);

and the binder concluded, that looks stupid, if you have an array of Element[] and want to link these. Why would you single out the first and second element.

This is also the reason for the unittest, that I added:

#764 (review)

the first element of that test indeed needs to be present (yes I really need to really learn C), but the signature from the java perspective is more convenient.

[twall]

I see. I wrote the argument expansion from the C perspective, not the Java one. The problem is that when you write Element... you've lost the indication that the first two arguments are indeed required. I think it'd be more appropriate to make the explicit params method internal, wrap it with the syntactic sugar, and raise an appropriate usage exception if used improperly.

I'm not particularly bothered by the fix, though. You could put in a check to ensure that there's at least one parameter, to meet the C requirements, but that still wouldn't help incorrect use in the example provided. LGTM.

[neilcsmith-net]

@twall thanks! While I agree with your point that the signature should match the requirements, this particular mapping has not changed since Wayne Meissner first wrote the GStreamer 0.10 bindings in 2007/8. So, if not strictly valid, it has at least been supported by every JNA version in the last decade.

Assuming this fix gets in, I'll still consider whether it's better we change the mappings we have (there's more than just this one!). However, @matthiasblaesing makes a good point that doing this in Java forces an array copy which we currently don't need - it's definitely more convenient on the Java side to keep the old behaviour.

[matthiasblaesing]

I confirmed the PR and merged it into master. Whether or not the signature should be changed in GStreamer-Java should be discussed there. But the argument, that it worked for the past and no bugs were raised for this, holds for me, so I would not change the gstreamer-java side right now.

[neilcsmith-net]

Thanks! Agree that the signature change discussion goes back to us, and am inclined not to, assuming that fixing this implies that this will remain a supported mapping in JNA for the foreseeable future? ie. any decision we make is influenced by whether this should be considered building on shaky ground. 😄

[matthiasblaesing]

I added a unittest that covers the "full varargs" case, so my answer at this point in time: Yes it will stay.

[matthiasblaesing]

A snapshot was published to:

https://oss.sonatype.org/content/repositories/snapshots/net/java/dev/jna/jna/4.3.1-SNAPSHOT/jna-4.3.1-20170207.055900-2.jar
https://oss.sonatype.org/content/repositories/snapshots/net/java/dev/jna/jna-platform/4.3.1-SNAPSHOT/jna-platform-4.3.1-20170207.055946-2.jar