8307145: windowsaccessbridge.dll erroneously includes private methods in its C API #13734

TheShermanTanker · 2023-04-29T19:26:00Z

In windowsaccessbridge(-64).dll the following methods are exported in a def file to C API which in turn call their real implementation, which are instance methods of the class WinAccessBridge:

addJavaEventNotification
removeJavaEventNotification
addAccessibilityEventNotification
removeAccessibilityEventNotification

However, they are nowhere to be seen in the actual C interface, in AccessBridgeWindowsEntryPoints.cpp. Your guess is as good as mine as to how on earth MSVC is still capable of compiling and linking this without any errors whatsoever, but in any case, this is a severe oversight and should be properly defined in the C API lest this happy accident within MSVC is fixed by Microsoft in the future

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-8307145: windowsaccessbridge.dll erroneously includes private methods in its C API (Bug - P3)

Reviewers

Daniel Jeliński (@djelinski - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13734

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-04-29T19:26:56Z

👋 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 · 2023-04-29T19:28:24Z

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

client

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 · 2023-04-29T19:46:57Z

Webrevs

01: Full - Incremental (835513ce)
00: Full (ef810f7f)

prrace

These don't seem to be part of the public API, so this is probably deliberate and not a bug.
https://docs.oracle.com/javase/accessbridge/2.0.2/api.htm#jab-api-specification

NikitkoCent · 2023-05-03T13:12:51Z

These don't seem to be part of the public API, so this is probably deliberate and not a bug.
https://docs.oracle.com/javase/accessbridge/2.0.2/api.htm#jab-api-specification

@prrace should non-public API methods be removed from .DEF then?

TheShermanTanker · 2023-05-03T13:20:15Z

I should think so, it doesn't make sense to export a function that doesn't even exist at all. And to make matters worse Microsoft Visual C still does export functions with those names (you can easily verify this with dumpbin) which is even more confusing. It may be a cursed Microsoft extension that has simply gone under the radar, and is somehow exporting the C++ class instance methods as static, or even exporting dummy functions, neither of which is what we want it to do!

prrace · 2023-05-03T17:27:53Z

So perhaps something internal in the various accessbridge dll's needs it. How will you test this ?
Who here has JAWS ? If you don't have JAWS and therefore can't use it to verify the A11Y test suite then
I don't think you can test this change in a way that makes me comfortable in doing anything here.

savoptik · 2023-05-03T19:57:41Z

@prrace Maybe try using free NVDA for testing?

TheShermanTanker · 2023-05-04T07:05:04Z

The problem as I see it is that the .def is exporting something that simply does not exist at all, even if it is a non-public API and the symbols are simply exported from the DLL for internal use by other accessbridge code, then these methods should still be placed in the extern "C" block in the .cpp (but maybe not in the corresponding header since they aren't public). Conversely if it is never meant to be exported but the accessbridge DLLs somehow need the .def files to export phantom, nonexistent symbols to work properly, then something is seriously wrong with them and we should probably take a look at what on earth is going on there

djelinski · 2023-05-05T14:57:27Z

FWIW, this is what dumpbin displays for these entries:

          1    2 0000351C addAccessibilityEventNotification = ?addAccessibilityEventNotification@WinAccessBridge@@QEAAX_J@Z (public: void __cdecl WinAccessBridge::addAccessibilityEventNotification(__int64))
          2    4 00003628 addJavaEventNotification = ?addJavaEventNotification@WinAccessBridge@@QEAAX_J@Z (public: void __cdecl WinAccessBridge::addJavaEventNotification(__int64))
          3   42 00006D94 removeAccessibilityEventNotification = ?removeAccessibilityEventNotification@WinAccessBridge@@QEAAX_J@Z (public: void __cdecl WinAccessBridge::removeAccessibilityEventNotification(__int64))
          4   44 00006EA0 removeJavaEventNotification = ?removeJavaEventNotification@WinAccessBridge@@QEAAX_J@Z (public: void __cdecl WinAccessBridge::removeJavaEventNotification(__int64))

There's no way to get a pointer to an instance of WinAccessBridge using the supported interface, and calling these methods with garbage value for this would likely crash.

TheShermanTanker · 2023-05-05T15:04:36Z

FWIW, this is what dumpbin displays for these entries:

          1    2 0000351C addAccessibilityEventNotification = ?addAccessibilityEventNotification@WinAccessBridge@@QEAAX_J@Z (public: void __cdecl WinAccessBridge::addAccessibilityEventNotification(__int64))
          2    4 00003628 addJavaEventNotification = ?addJavaEventNotification@WinAccessBridge@@QEAAX_J@Z (public: void __cdecl WinAccessBridge::addJavaEventNotification(__int64))
          3   42 00006D94 removeAccessibilityEventNotification = ?removeAccessibilityEventNotification@WinAccessBridge@@QEAAX_J@Z (public: void __cdecl WinAccessBridge::removeAccessibilityEventNotification(__int64))
          4   44 00006EA0 removeJavaEventNotification = ?removeJavaEventNotification@WinAccessBridge@@QEAAX_J@Z (public: void __cdecl WinAccessBridge::removeJavaEventNotification(__int64))

There's no way to get a pointer to an instance of WinAccessBridge using the supported interface, and calling these methods with garbage value for this would likely crash.

Dear lord, the Microsoft linker is truly cursed, through and through

@prrace What sort of testing would need to be done for such a change?

prrace · 2023-05-05T17:35:38Z

FWIW, this is what dumpbin displays for these entries:
          1    2 0000351C addAccessibilityEventNotification = ?addAccessibilityEventNotification@WinAccessBridge@@QEAAX_J@Z (public: void __cdecl WinAccessBridge::addAccessibilityEventNotification(__int64))
          2    4 00003628 addJavaEventNotification = ?addJavaEventNotification@WinAccessBridge@@QEAAX_J@Z (public: void __cdecl WinAccessBridge::addJavaEventNotification(__int64))
          3   42 00006D94 removeAccessibilityEventNotification = ?removeAccessibilityEventNotification@WinAccessBridge@@QEAAX_J@Z (public: void __cdecl WinAccessBridge::removeAccessibilityEventNotification(__int64))
          4   44 00006EA0 removeJavaEventNotification = ?removeJavaEventNotification@WinAccessBridge@@QEAAX_J@Z (public: void __cdecl WinAccessBridge::removeJavaEventNotification(__int64))
There's no way to get a pointer to an instance of WinAccessBridge using the supported interface, and calling these methods with garbage value for this would likely crash.
Dear lord, the Microsoft linker is truly cursed, through and through

@prrace What sort of testing would need to be done for such a change?

@azuev-java and @kumarabhi006 can help with review and testing of this PR.

bridgekeeper · 2023-06-02T19:30:02Z

@TheShermanTanker This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

TheShermanTanker · 2023-06-06T15:33:18Z

Bumping

TheShermanTanker · 2023-06-08T07:43:18Z

Anyone?

aivanov-jdk · 2023-06-08T11:19:45Z

I'll take a look… hopefully next week.

TheShermanTanker · 2023-06-08T13:01:22Z

Thanks aivanov, no rush haha

TheShermanTanker · 2023-06-15T06:02:23Z

Bumping

aivanov-jdk · 2023-06-15T15:04:57Z

These don't seem to be part of the public API, so this is probably deliberate and not a bug.
https://docs.oracle.com/javase/accessbridge/2.0.2/api.htm#jab-api-specification

@prrace should non-public API methods be removed from .DEF then?

I think it's the best solution. These four functions are not part of the public API to AccessBridge, they're missing from AccessBridgeCalls.c and AccessBridgeCalls.h.

They should not be exported. If the functions aren't loaded by its name, which is implemented in AccessBridgeCalls.c, or otherwise linked to from other modules (.exe or .dll), they don't need to be exported. These functions are part of the WinAccessBridge class, none of them cannot be called directly.

Did you try removing them from the .def file? Does anything break?

TheShermanTanker · 2023-06-15T17:46:21Z

@aivanov-jdk Compiles and works fine without the exports in .def

TheShermanTanker · 2023-06-15T17:46:50Z

I don't know about the more extensive test suite Phil alludes to, though

TheShermanTanker · 2023-06-20T07:11:37Z

Bumping

aivanov-jdk · 2023-06-20T20:08:05Z

I don't know about the more extensive test suite Phil alludes to, though

Neither do I know about more extensive testing. I presume these four entry points were not used externally. Yet “compiles” may not be enough. What does your “works” mean? Did you run it with an AccessBridge client?

TheShermanTanker · 2023-06-21T05:27:09Z

@aivanov-jdk The client does indeed function on my end without issue, but I don't know whether it will do so on other devices as well

TheShermanTanker · 2023-06-21T05:47:26Z

JTReg sure does have a sense of humour, immediately crashing and leaving that ugly cross on the tests the moment I say that

djelinski

LGTM. These methods are not usable outside of the windowsaccessbridge dll. Caller would need a working instance of WinAccessBridge class to pass in this parameter, and there's no way to get one outside of the DLL.

openjdk · 2023-06-21T06:33:07Z

@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:

8307145: windowsaccessbridge.dll erroneously includes private methods in its C API

Reviewed-by: djelinski

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 774 new commits pushed to the master branch:

47d00a4: 8310265: (process) jspawnhelper should not use argv[0]
e022e87: 8310053: VarHandle and slice handle derived from layout are lacking alignment check
45eaf5e: 8298443: Remove expired flags in JDK 22
28415ad: 8310225: Reduce inclusion of oopMapCache.hpp and generateOopMap.hpp
4c3efb3: 8309034: NoClassDefFoundError when initializing Long$LongCache
1120106: 8310458: Fix build failure caused by JDK-8310049
09174e0: 8310049: Refactor Charset tests to use JUnit
99d2a9a: 8310330: HttpClient: debugging interestOps/readyOps could cause exceptions and smaller cleanup
31b6fd7: 8309258: RISC-V: Add riscv_hwprobe syscall
4a9cc8a: 8309266: C2: assert(final_con == (jlong)final_int) failed: final value should be integer
... and 764 more: https://git.openjdk.org/jdk/compare/6d6d00b69cea47ccbe05a844db0fb6c384045caa...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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

TheShermanTanker · 2023-06-21T06:37:23Z

Right, thanks Daniel

TheShermanTanker · 2023-06-21T06:37:41Z

I will change the title of the PR first

TheShermanTanker · 2023-06-21T06:41:54Z

/integrate

TheShermanTanker · 2023-06-21T06:42:07Z

Thanks all for reviewing this change!

openjdk · 2023-06-21T06:43:05Z

Going to push as commit 3faba07.
Since your change was applied there have been 775 commits pushed to the master branch:

886ac1c: 8308606: C2 SuperWord: remove alignment checks when not required
47d00a4: 8310265: (process) jspawnhelper should not use argv[0]
e022e87: 8310053: VarHandle and slice handle derived from layout are lacking alignment check
45eaf5e: 8298443: Remove expired flags in JDK 22
28415ad: 8310225: Reduce inclusion of oopMapCache.hpp and generateOopMap.hpp
4c3efb3: 8309034: NoClassDefFoundError when initializing Long$LongCache
1120106: 8310458: Fix build failure caused by JDK-8310049
09174e0: 8310049: Refactor Charset tests to use JUnit
99d2a9a: 8310330: HttpClient: debugging interestOps/readyOps could cause exceptions and smaller cleanup
31b6fd7: 8309258: RISC-V: Add riscv_hwprobe syscall
... and 765 more: https://git.openjdk.org/jdk/compare/6d6d00b69cea47ccbe05a844db0fb6c384045caa...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-06-21T06:43:11Z

@TheShermanTanker Pushed as commit 3faba07.

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

aivanov-jdk · 2023-06-21T10:08:31Z

@aivanov-jdk The client does indeed function on my end without issue, but I don't know whether it will do so on other devices as well

That's good to know.

I would've approved it, if I'd had a slightest chance to read your reply.

TheShermanTanker · 2023-06-21T10:48:10Z

Ah, sorry about that. I've had several changes starved of reviews recently and lying around for almost a month, and was feeling fairly frustrated and impatient :(

windowsaccessbridge.dll is missing 4 critical methods in its C API

127e80d

Declare missing API in header as well

ef810f7

openjdk bot added the rfr Pull request is ready for review label Apr 29, 2023

openjdk bot added the client client-libs-dev@openjdk.org label Apr 29, 2023

prrace suggested changes May 2, 2023

View reviewed changes

TheShermanTanker added 3 commits June 21, 2023 13:24

Revert AccessBridgeWindowsEntryPoints.cpp

55f3949

Remove from WinAccessBridge.DEF

655460f

Revert AccessBridgeWindowsEntryPoints.h

835513c

djelinski approved these changes Jun 21, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 21, 2023

TheShermanTanker changed the title ~~8307145: windowsaccessbridge.dll is missing 4 critical methods in its C API~~ 8307145: windowsaccessbridge.dll erroneously includes private methods in its C API Jun 21, 2023

openjdk bot added the integrated Pull request has been integrated label Jun 21, 2023

openjdk bot closed this Jun 21, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 21, 2023

TheShermanTanker deleted the patch-7 branch June 21, 2023 06:43

8307145: windowsaccessbridge.dll erroneously includes private methods in its C API #13734

8307145: windowsaccessbridge.dll erroneously includes private methods in its C API #13734

Conversation

TheShermanTanker commented Apr 29, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Apr 29, 2023

openjdk bot commented Apr 29, 2023

mlbridge bot commented Apr 29, 2023 • edited Loading

Webrevs

prrace left a comment

Choose a reason for hiding this comment

NikitkoCent commented May 3, 2023

TheShermanTanker commented May 3, 2023

prrace commented May 3, 2023

savoptik commented May 3, 2023

TheShermanTanker commented May 4, 2023

djelinski commented May 5, 2023

TheShermanTanker commented May 5, 2023

prrace commented May 5, 2023

bridgekeeper bot commented Jun 2, 2023

TheShermanTanker commented Jun 6, 2023

TheShermanTanker commented Jun 8, 2023

aivanov-jdk commented Jun 8, 2023

TheShermanTanker commented Jun 8, 2023

TheShermanTanker commented Jun 15, 2023

aivanov-jdk commented Jun 15, 2023

TheShermanTanker commented Jun 15, 2023

TheShermanTanker commented Jun 15, 2023

TheShermanTanker commented Jun 20, 2023

aivanov-jdk commented Jun 20, 2023

TheShermanTanker commented Jun 21, 2023

TheShermanTanker commented Jun 21, 2023

djelinski left a comment

Choose a reason for hiding this comment

openjdk bot commented Jun 21, 2023 • edited Loading

TheShermanTanker commented Jun 21, 2023

TheShermanTanker commented Jun 21, 2023

TheShermanTanker commented Jun 21, 2023

TheShermanTanker commented Jun 21, 2023

openjdk bot commented Jun 21, 2023

openjdk bot commented Jun 21, 2023

aivanov-jdk commented Jun 21, 2023

TheShermanTanker commented Jun 21, 2023

TheShermanTanker commented Apr 29, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Apr 29, 2023 •

edited

Loading

openjdk bot commented Jun 21, 2023 •

edited

Loading