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

Fix PageantConnector struct class visibility #86

Merged
merged 1 commit into from
Oct 13, 2021
Merged

Fix PageantConnector struct class visibility #86

merged 1 commit into from
Oct 13, 2021

Conversation

ST-DDT
Copy link
Contributor

@ST-DDT ST-DDT commented Oct 13, 2021

The visibility of the struct classes need to be public otherwise the following exception will be thrown:

Caused by: java.lang.IllegalAccessException: Class com.sun.jna.Structure can not access a member of class com.jcraft.jsch.PageantConnector$COPYDATASTRUCT64 with modifiers "public"
	at sun.reflect.Reflection.ensureMemberAccess(Reflection.java:102)
	at java.lang.reflect.AccessibleObject.slowCheckMemberAccess(AccessibleObject.java:296)
	at java.lang.reflect.AccessibleObject.checkAccess(AccessibleObject.java:288)
	at java.lang.reflect.Field.get(Field.java:390)
	at com.sun.jna.Structure.getFieldValue(Structure.java:650)
	... 101 more

protected seems to work as well, but this might be due to the test setup.

However, this causes these internal data classes to become accessible from the outside.
As a workaround it is also possible to move the data classes inside the private User32 interface.
Should we do this instead?

See also: https://stackoverflow.com/questions/66214743/instantiating-subclass-of-jna-structure-throws-illegalaccessexception

I have verified the code with gitlab, jgit and KeePass (as Pageant SSHAgent):

#85 (comment)

The visibility of the struct classes need to be public otherwise the following exception will be thrown:

````txt
Caused by: java.lang.IllegalAccessException: Class com.sun.jna.Structure can not access a member of class com.jcraft.jsch.PageantConnector$COPYDATASTRUCT64 with modifiers "public"
	at sun.reflect.Reflection.ensureMemberAccess(Reflection.java:102)
	at java.lang.reflect.AccessibleObject.slowCheckMemberAccess(AccessibleObject.java:296)
	at java.lang.reflect.AccessibleObject.checkAccess(AccessibleObject.java:288)
	at java.lang.reflect.Field.get(Field.java:390)
	at com.sun.jna.Structure.getFieldValue(Structure.java:650)
	... 101 more
````

`protected` seems to work as well, but this might be due to the test setup.
@norrisjeremy
Copy link
Contributor

I think it should be fine to just make them public. I wonder if we should change the visibility of the User32 interface as well?

@norrisjeremy
Copy link
Contributor

I also wonder if this means that some changes might be necessary to module-info.java in order to allow JNA to perform reflection operations into these classes?

@norrisjeremy
Copy link
Contributor

I also wonder if this means that some changes might be necessary to module-info.java in order to allow JNA to perform reflection operations into these classes?

Nevermind, the base exports com.jcraft.jsch; should allow reflective access after the visibility of these classes is changed public.

Copy link
Contributor

@norrisjeremy norrisjeremy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also go ahead and change the visibility of the User32 interface to public as well.

@ST-DDT
Copy link
Contributor Author

ST-DDT commented Oct 13, 2021

I think it should be fine to just make them public. I wonder if we should change the visibility of the User32 interface as well?

This doesn't seem to be necessary (at least for my tests).

I also wonder if this means that some changes might be necessary to module-info.java in order to allow JNA to perform reflection operations into these classes?

I don't know. Is there an easy way to create a test project that actually uses the modules instead of only defining one?

@norrisjeremy
Copy link
Contributor

I think it should be fine to just make them public. I wonder if we should change the visibility of the User32 interface as well?

This doesn't seem to be necessary (at least for my tests).

I think it probably wouldn't hurt to go ahead and change it to public visibility, and that would hopefully cover any sort of corner cases that we aren't detecting. Would you mind doing that in this PR?

I also wonder if this means that some changes might be necessary to module-info.java in order to allow JNA to perform reflection operations into these classes?

I don't know. Is there an easy way to create a test project that actually uses the modules instead of only defining one?

I think after looking at it further, we should be fine: the exports com.jcraft.jsch; should allow reflective access to these classes once their visibility is declared public.

@mwiede mwiede merged commit 6472500 into mwiede:master Oct 13, 2021
@ST-DDT ST-DDT deleted the patch-1 branch October 13, 2021 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants