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

Enhance bindings for Crypt32 and fix bindings for array attributes #1252

Merged

Conversation

matthiasblaesing
Copy link
Member

The implementations of the rgAttribute, rgCTLEntry and rgExtension
attributes are not in line with the observed behaviour of the API. At
least in part they are documented to be pointers to arrays of pointers
to structures. This is not correct, as segfaults were observed and
testing shows, that they are in fact pointers to arrays of structures.

In addition it was observed, that the bindings of CTL_INFO#getRgCTLEntry
and CTL_INFO#getRgExtension method names were inverted.

The bindings for CERT_EXTENSIONS#getRgExtension and
CTL_INFO#getRgExtension are untested as but assumed to be implemented
identically to the tested bindings.

The functions

  • CertEnumCertificatesInStore
  • CertEnumCTLsInStore
  • CertEnumCRLsInStore
  • CryptQueryObject

from c.s.j.p.win32.Crypt32 were bound to be able to excercise the
accessors.

@matthiasblaesing
Copy link
Member Author

This changeset changes method signatures, but you'd need to be blind to use the methods in the way they were originally bound. They are obviously broken and thus from my POV don't form an api break.

@matthiasblaesing matthiasblaesing force-pushed the fix-crypto32 branch 2 times, most recently from e9a76b9 to 6120932 Compare August 31, 2020 18:40
Copy link
Contributor

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

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

Looks good to me. A few typos and questions more for my education than concern that they are wrong.

The implementations of the rgAttribute, rgCTLEntry and rgExtension
attributes are not in line with the observed behaviour of the API. At
least in part they are documented to be pointers to arrays of pointers
to structures. This is not correct, as segfaults were observed and
testing shows, that they are in fact pointers to arrays of structures.

In addition it was observed, that the bindings of CTL_INFO#getRgCTLEntry
and CTL_INFO#getRgExtension method names were inverted.

The bindings for CERT_EXTENSIONS#getRgExtension and 
CTL_INFO#getRgExtension are untested as but assumed to be implemented
identically to the tested bindings.

The functions 

- CertEnumCertificatesInStore
- CertEnumCTLsInStore
- CertEnumCRLsInStore
- CryptQueryObject

from `c.s.j.p.win32.Crypt32` were bound to be able to excercise the
accessors.
@matthiasblaesing
Copy link
Member Author

I updated the PR - @dbwiddis ok to merge from your perspective?

@dbwiddis
Copy link
Contributor

dbwiddis commented Sep 5, 2020

Looks good from my point of view. I have not yet had time to actually test it, but I have looked closely at the code!

@matthiasblaesing
Copy link
Member Author

@dbwiddis thank you for your review. I'll merge this in now, if you recognize a problem later (preferably before the next release) just speak up and I'll revisit.

@matthiasblaesing matthiasblaesing merged commit 0a0e44d into java-native-access:master Sep 8, 2020
@matthiasblaesing matthiasblaesing deleted the fix-crypto32 branch November 8, 2020 19:57
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.

2 participants