Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UTFStringRef.set() throws IllegalArgumentException #159

Closed
Robmaister opened this issue Jan 26, 2018 · 5 comments · Fixed by #276
Closed

UTFStringRef.set() throws IllegalArgumentException #159

Robmaister opened this issue Jan 26, 2018 · 5 comments · Fixed by #276
Milestone

Comments

@Robmaister
Copy link

I'm trying to wrap a library that has some structs that expose bare char* strings. When I try to set one of these values using UTFStringRef.set, I get the following exception:

Exception in thread "main" java.lang.IllegalArgumentException: negative size: -4
        at jnr.ffi.provider.jffi.TransientNativeMemory.allocate(TransientNativeMemory.java:46)
        at jnr.ffi.provider.jffi.NativeMemoryManager.allocateDirect(NativeMemoryManager.java:41)
        at jnr.ffi.Struct$UTFStringRef.set(Struct.java:2364)
        at test.TestProject.setValue(TestProject.java:207)
        at test.SimpleExample.main(SimpleExample.java:8)

I can include some code, but unless I'm mistaken, there's a relatively straightforward bug here:

https://github.com/jnr/jnr-ffi/blob/master/src/main/java/jnr/ffi/Struct.java#L2364

valueHolder = getRuntime().getMemoryManager().allocateDirect(length() * 4);
valueHolder.putString(0, value, length() * 4, charset);

I believe that both instances of length() here were intended to be value.length() instead. By default, the constructor sets the value of length() to Integer.MAX_VALUE, and it would make sense to allocate a buffer of the same size as the incoming string.

Also along the same lines, does String.length() in Java include the size of a null terminator? Should it perhaps be (value.length() + 1) * 4?

If I'm completely off track here let me know and I'll include some code with what I'm trying to accomplish

@Robmaister
Copy link
Author

Due to the incompatibility of JNR with Android, I'm going to have to stop using JNR and write my bindings directly with JNI. I should have probably checked that before starting to write my bindings.

Anyways, the immediate solution I came up with was to drop back to Pointer fields and manually marshal strings back and forth via getString(0) and putString(0) (with a null check on the result of get() from the struct field).

@headius
Copy link
Member

headius commented May 16, 2018

Thanks for the workaround info, and I'm sorry we did not see this issue! Did you ever figure out what was causing the problem in JNR?

@Robmaister
Copy link
Author

No worries! I'm not really fantastic about keeping up with my own repos haha

If I remember correctly changing length() to value.length() would have fixed it. I ended up having to focus on other stuff and now work elsewhere, so I can't really go back and try running it again with a patched version.

If you want to create a reproducible example, create a struct with a string field and try to set it on the Java side before passing that struct over to the native side.

At a higher level, we were better off re-implementing the protocol we were trying to port with Android's socket library for better integration into the API (handler/looper stuff) so I doubt whoever is continuing my work would be working with the same bindings I wrote.

@charleskorn
Copy link
Contributor

I am still able to reproduce this issue - and I agree with @Robmaister's assessment:

If I remember correctly changing length() to value.length() would have fixed it.

This looks like it would resolve the issue.

charleskorn added a commit to charleskorn/jnr-ffi that referenced this issue Oct 23, 2021
charleskorn added a commit to batect/docker-client that referenced this issue Oct 23, 2021
@headius headius added this to the 2.2.7 milestone Oct 26, 2021
@headius
Copy link
Member

headius commented Oct 26, 2021

Thanks for waking this one up with a fix!

@headius headius modified the milestones: 2.2.7, 2.2.8, 2.2.9 Oct 26, 2021
charleskorn added a commit to batect/docker-client that referenced this issue Nov 3, 2021
We need jnr/jnr-ffi#159 to be fixed first.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants