8289275: Remove incorrect __declspec(dllimport) attributes from pointers in jdk.crypto.cryptoki #9353

TheShermanTanker · 2022-07-02T02:12:15Z

Several instances of function pointers in jdk.crypto.cryptoki are marked with the dllimport attribute, which should only be applied to symbol declarations, of which a typedef'd function pointer is not. This would only be useful if a function pointer defined in the linked dll is desired to be imported, not if the pointer itself is created locally and used to store a function address. In addition to being incorrect, at least on the versions of Visual C++ the JDK supports today, it is also redundant; Typically they are used to avoid an indirect stub that jumps to the proper entry in the import address table, but usage of these typedefs involves loading the address of a function and directly (Usually through GetProcAddress, even in other cases it would simply be set to the address of a function anyway) assigning it to the pointer before immediately dispatching when called, which bypasses this procedure entirely and makes the attribute pointless.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8289275: Remove incorrect __declspec(dllimport) attributes from pointers in jdk.crypto.cryptoki

Reviewers

Valerie Peng (@valeriepeng - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9353/head:pull/9353
$ git checkout pull/9353

Update a local copy of the PR:
$ git checkout pull/9353
$ git pull https://git.openjdk.org/jdk pull/9353/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9353

View PR using the GUI difftool:
$ git pr show -t 9353

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9353.diff

bridgekeeper · 2022-07-02T02:13:31Z

👋 Welcome back jwaters! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2022-07-02T02:16:32Z

@TheShermanTanker The following label will be automatically applied to this pull request:

security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2022-07-02T02:21:09Z

Webrevs

valeriepeng · 2022-07-07T21:57:42Z

pkcs11.h is a standard header file from OASIS. Best to leave it alone. Just my .02.

TheShermanTanker · 2022-07-09T06:05:09Z

pkcs11.h is a standard header file from OASIS. Best to leave it alone. Just my .02.

The only change to pkcs11.h is in a comment though, the other changes are outside the ANSI C headers from the standard. Should I just revert the comment in that case?

TheShermanTanker · 2022-07-10T08:18:28Z

Tested with Microsoft (R) C/C++ Optimizing Compiler Version 19.31.31104 while observing FPTR_VersionCheck used in the method JNIEXPORT jboolean JNICALL Java_sun_security_pkcs11_Secmod_nssVersionCheck (j2secmod.c, line 40 and line 54), no differences in the generated assembly snippets:

Without __declspec(dllimport):

   0:	48 83 ec 38          	sub    $0x38,%rsp
   4:	48 8d 0d 00 00 00 00 	lea    0x0(%rip),%rcx
   b:	ff 54 24 20          	call   *0x20(%rsp)
   f:	33 c0                	xor    %eax,%eax
  11:	48 83 c4 38          	add    $0x38,%rsp
  15:	c3                   	ret

With __declspec(dllimport):

   0:	48 83 ec 38          	sub    $0x38,%rsp
   4:	48 8d 0d 00 00 00 00 	lea    0x0(%rip),%rcx
   b:	ff 54 24 20          	call   *0x20(%rsp)
   f:	33 c0                	xor    %eax,%eax
  11:	48 83 c4 38          	add    $0x38,%rsp
  15:	c3                   	ret

valeriepeng · 2022-07-13T16:40:48Z

pkcs11.h is a standard header file from OASIS. Best to leave it alone. Just my .02.

The only change to pkcs11.h is in a comment though, the other changes are outside the ANSI C headers from the standard. Should I just revert the comment in that case?

Sounds good to me.
Will take a look at the rest of changes in a day or two.
Thanks!

TheShermanTanker · 2022-07-14T02:24:55Z

pkcs11.h is a standard header file from OASIS. Best to leave it alone. Just my .02.

The only change to pkcs11.h is in a comment though, the other changes are outside the ANSI C headers from the standard. Should I just revert the comment in that case?

Sounds good to me. Will take a look at the rest of changes in a day or two. Thanks!

Alright, will do. I also do want to look at p11_md.h to see if my changes are appropriate in the meantime, as I have a feeling it can be accomplished somewhat more elegantly

Thanks for the review!

openjdk · 2022-07-19T23:59:59Z

@TheShermanTanker This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8289275: Remove incorrect __declspec(dllimport) attributes from pointers in jdk.crypto.cryptoki

Reviewed-by: valeriep

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 82 new commits pushed to the master branch:

4358864: 8290511: compiler/vectorapi/TestMaskedMacroLogicVector.java fails IR verification
7b418f9: 8287916: Address the inconsistency between the constant array and pool size
41ef2b2: 8285407: Improve Xalan supports
d991ec4: 8282676: Improve subject handling
b7b917a: 8284370: Improve zlib usage
a37465b: 8283190: Improve MIDI processing
632d2d2: 8281866: Enhance MethodHandle invocations
84b4e9b: 8281859: Improve class compilation
e7f8003: 8272249: Better properties of loaded Properties
cdc1582: 8277608: Address IP Addressing
... and 72 more: https://git.openjdk.org/jdk/compare/dbab827bee5297c19b10f0923a962fe6c0ac5cbd...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@valeriepeng) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

TheShermanTanker · 2022-07-20T02:39:58Z

/integrate

openjdk · 2022-07-20T02:41:05Z

@TheShermanTanker
Your change (at version 3babccb) is now ready to be sponsored by a Committer.

TheShermanTanker · 2022-07-21T02:06:17Z

@valeriepeng Might need you to sponsor this, thanks for the review!

valeriepeng · 2022-07-21T19:10:22Z

/sponsor

openjdk · 2022-07-21T19:11:52Z

Going to push as commit 0dda3c1.
Since your change was applied there have been 126 commits pushed to the master branch:

620c8a0: 8289643: File descriptor leak with ProcessBuilder.startPipeline
7ec0132: 8286844: com/sun/jdi/RedefineCrossEvent.java failed with 1 threads completed while VM suspended
80bd8c3: 8290504: Close streams returned by ModuleReader::list
15f4b30: 8290115: ArrayCopyObject JMH has wrong package
4c1cd66: 8288368: simplify code in ValueTaglet, remove redundant code
6346c33: 8290826: validate-source failures after JDK-8290016
604a115: 8290016: IGV: Fix graph panning when mouse dragged outside of window
59e495e: 8290704: x86: TemplateTable::_new should not call eden_allocate() without contiguous allocs enabled
799a2c8: 8276561: URL$DefaultFactory::PREFIX should be static final
52cc6cd: 8288723: Avoid redundant ConcurrentHashMap.get call in java.time
... and 116 more: https://git.openjdk.org/jdk/compare/dbab827bee5297c19b10f0923a962fe6c0ac5cbd...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-07-21T19:12:08Z

@valeriepeng @TheShermanTanker Pushed as commit 0dda3c1.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Remove __declspec(dllimport) from pointers

f299e7c

openjdk bot changed the title ~~8289275~~ 8289275: Remove incorrect __declspec(dllimport) attributes from pointers in jdk.crypto.cryptoki Jul 2, 2022

openjdk bot added the rfr Pull request is ready for review label Jul 2, 2022

openjdk bot added the security security-dev@openjdk.org label Jul 2, 2022

Merge branch 'master' into typedefs

f6a94f4

TheShermanTanker added 2 commits July 14, 2022 10:37

Merge branch 'master' into typedefs

f011550

Revert changes to pkcs11.h

3babccb

valeriepeng approved these changes Jul 19, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jul 20, 2022

openjdk bot added the sponsor Pull request is ready to be sponsored label Jul 20, 2022

openjdk bot added the integrated Pull request has been integrated label Jul 21, 2022

openjdk bot closed this Jul 21, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Jul 21, 2022

TheShermanTanker deleted the typedefs branch July 21, 2022 19:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8289275: Remove incorrect __declspec(dllimport) attributes from pointers in jdk.crypto.cryptoki #9353

8289275: Remove incorrect __declspec(dllimport) attributes from pointers in jdk.crypto.cryptoki #9353

TheShermanTanker commented Jul 2, 2022 •

edited by openjdk bot

bridgekeeper bot commented Jul 2, 2022

openjdk bot commented Jul 2, 2022

mlbridge bot commented Jul 2, 2022 •

edited

valeriepeng commented Jul 7, 2022

TheShermanTanker commented Jul 9, 2022

TheShermanTanker commented Jul 10, 2022 •

edited

valeriepeng commented Jul 13, 2022

TheShermanTanker commented Jul 14, 2022

openjdk bot commented Jul 19, 2022 •

edited

TheShermanTanker commented Jul 20, 2022

openjdk bot commented Jul 20, 2022

TheShermanTanker commented Jul 21, 2022

valeriepeng commented Jul 21, 2022

openjdk bot commented Jul 21, 2022

openjdk bot commented Jul 21, 2022

8289275: Remove incorrect __declspec(dllimport) attributes from pointers in jdk.crypto.cryptoki #9353

8289275: Remove incorrect __declspec(dllimport) attributes from pointers in jdk.crypto.cryptoki #9353

Conversation

TheShermanTanker commented Jul 2, 2022 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jul 2, 2022

openjdk bot commented Jul 2, 2022

mlbridge bot commented Jul 2, 2022 • edited

Webrevs

valeriepeng commented Jul 7, 2022

TheShermanTanker commented Jul 9, 2022

TheShermanTanker commented Jul 10, 2022 • edited

valeriepeng commented Jul 13, 2022

TheShermanTanker commented Jul 14, 2022

openjdk bot commented Jul 19, 2022 • edited

TheShermanTanker commented Jul 20, 2022

openjdk bot commented Jul 20, 2022

TheShermanTanker commented Jul 21, 2022

valeriepeng commented Jul 21, 2022

openjdk bot commented Jul 21, 2022

openjdk bot commented Jul 21, 2022

TheShermanTanker commented Jul 2, 2022 •

edited by openjdk bot

mlbridge bot commented Jul 2, 2022 •

edited

TheShermanTanker commented Jul 10, 2022 •

edited

openjdk bot commented Jul 19, 2022 •

edited