8380391: Update java.smartcardio finalizer to use Cleaner by bchristi-git · Pull Request #30683 · openjdk/jdk

bchristi-git · 2026-04-10T18:41:27Z

This is a pull request to convert the finalizer in sun.security.smartcardio.CardImpl to use Cleaner instead. The relevant state is refactored into a Context object, in the standard fashion.

This change uses the recommended try/finally/reachabilityFence() technique to prevent races between the program thread and the Cleaner thread, per the Reference.reachabilityFence() API Note:

there is a race between the program thread running the method, and the cleanup thread running the Cleaner or finalizer. The cleanup thread could free a resource, followed by the program thread (still running the method) attempting to access the now-already-freed resource. Use of reachabilityFence can prevent this race by ensuring that the object remains strongly reachable.

See Reference.reachabilityFence() and java.lang.ref - Memory Visibility for details / background info.

The test creates a Card object, and allows it to be collected. This confirms that the new cleaning action does not hold onto the Owner ("this") object, per the Cleaner class API Note ("it is important that the object implementing the cleaning action does not hold references to the object").

I've tried to make the test similar to nearby JavaCard tests. I tried it on Windows using the latest JavaCard Simulator.

I confirm that I make this contribution in accordance with the OpenJDK Interim AI Policy.

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-8380391: Update java.smartcardio finalizer to use Cleaner (Bug - P4)

Reviewers

Mikhail Yankelevich (@myankelev - Committer) 🔄 Re-review required (review applies to 1009913b)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 30683

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2026-04-10T18:43:11Z

👋 Welcome back bchristi! 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 · 2026-04-10T18:44:34Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2026-04-10T18:45:59Z

@bchristi-git The following labels will be automatically applied to this pull request:

core-libs
security

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

mlbridge · 2026-04-10T18:53:02Z

Webrevs

AlanBateman · 2026-04-14T10:10:12Z

This is a pull request to convert the finalizer in sun.security.smartcardio.CardImpl to use Cleaner instead.

Has there been any static analysis done to get some sense as to how common it is to not disconnect a connection to a card or close a card channel? Asking because it would be nice if we could just remove the undocumented use of the finalizer. Related is that the example in the package example should be using try-finally to ensure that disconnect is called (I'm sure there has been discussed about having Card and CardChannel implement AutoClosable but the smartcard API is a bit awkward in that's a standalone technology, not part of Java SE but happens to be bundled with the JDK).

bchristi-git · 2026-04-15T21:33:04Z

Has there been any static analysis done to get some sense as to how common it is to not disconnect a connection to a card or close a card channel?

I'll look into it, Alan. Thanks.

bchristi-git · 2026-05-08T22:01:58Z

Has there been any static analysis done to get some sense as to how common it is to not disconnect a connection to a card or close a card channel? Asking because it would be nice if we could just remove the undocumented use of the finalizer.

Static analysis turned up several artifacts/libraries where a Card object returned by CardTerminal.connect() is not disconnect()ed. 😕

Related is that the example in the package example should be using try-finally to ensure that disconnect is called

I updated the example to use try-finally.

viktorklang-ora · 2026-05-21T11:01:43Z

+            checkSecurity("exclusive");
+            checkState();


What's the reason that these checks are done within the try-finally whereas the openLogicalChannel()-checks are done outside of the try-finally?

I think maybe github is rendering diffs in a confusing way?
In all cases, I add a new, outer try/finally block (for the reachabilityFence()) around the existing code.
openLogicalChannel() has the checks within this new try/finally block, as do the other methods.
Try enabling "Hide whitespace" (in the "gear" menu) and hopefully that will look better.

viktorklang-ora · 2026-05-21T11:02:02Z

-        } catch (PCSCException e) {
-            handleError(e);
-            throw new CardException("endExclusive() failed", e);
+            checkState();


Same comment as https://github.com/openjdk/jdk/pull/30683/changes#r3280643716

viktorklang-ora · 2026-05-21T11:02:13Z

+            checkSecurity("transmitControl");
+            checkState();
+            checkExclusive();


Same comment as https://github.com/openjdk/jdk/pull/30683/changes#r3280643716

viktorklang-ora · 2026-05-21T11:02:39Z

+            if (reset) {
+                checkSecurity("reset");
+            }
+            if (context.state != State.OK) {
+                return;
+            }
+            checkExclusive();


Same comment as https://github.com/openjdk/jdk/pull/30683/changes#r3280643716

viktorklang-ora

Did another review pass over this PR, after comments are answered/possibly addressed, this looks good to go from me

bchristi-git added 2 commits April 1, 2026 17:04

convert CardImpl finalizer to use Cleaner

ee4339f

add test case

48ca793

openjdk Bot added security security-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Apr 10, 2026

openjdk Bot added the rfr Pull request is ready for review label Apr 10, 2026

Marcono1234 reviewed Apr 12, 2026

View reviewed changes

Comment thread src/java.smartcardio/share/classes/sun/security/smartcardio/CardImpl.java Outdated

viktorklang-ora reviewed Apr 13, 2026

View reviewed changes

Comment thread src/java.smartcardio/share/classes/sun/security/smartcardio/CardImpl.java Outdated

Marcono1234 reviewed Apr 14, 2026

View reviewed changes

Comment thread src/java.smartcardio/share/classes/sun/security/smartcardio/CardImpl.java

myankelev reviewed Apr 15, 2026

View reviewed changes

bchristi-git added 5 commits April 15, 2026 14:54

fix imports and copyright years

ceae990

add missed clean() calls, update comments

9e1c066

minor comment update

4788c32

update example code in javax.smartcardio package doc to use try-finally

95937ea

Merge branch 'master' into smartcardio

fce7e3f

viktorklang-ora reviewed May 10, 2026

View reviewed changes

Comment thread src/java.smartcardio/share/classes/javax/smartcardio/package-info.java Outdated

myankelev reviewed May 11, 2026

View reviewed changes

Comment thread test/jdk/sun/security/smartcardio/TestCleaner.java

bchristi-git added 2 commits May 11, 2026 11:24

update copyright year

46a53d9

Merge branch 'master' into smartcardio

1009913

myankelev approved these changes May 13, 2026

View reviewed changes

viktorklang-ora reviewed May 21, 2026

View reviewed changes

Comment thread src/java.smartcardio/share/classes/sun/security/smartcardio/CardImpl.java Outdated

viktorklang-ora reviewed May 21, 2026

View reviewed changes

Context class should be final

cb73a2f

Conversation

bchristi-git commented Apr 10, 2026 • edited by openjdk Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper Bot commented Apr 10, 2026

Uh oh!

openjdk Bot commented Apr 10, 2026

Uh oh!

openjdk Bot commented Apr 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge Bot commented Apr 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlanBateman commented Apr 14, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bchristi-git commented Apr 15, 2026

Uh oh!

bchristi-git commented May 8, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

viktorklang-ora May 21, 2026

Choose a reason for hiding this comment

Uh oh!

bchristi-git May 21, 2026

Choose a reason for hiding this comment

Uh oh!

viktorklang-ora May 21, 2026

Choose a reason for hiding this comment

Uh oh!

viktorklang-ora May 21, 2026

Choose a reason for hiding this comment

Uh oh!

viktorklang-ora May 21, 2026

Choose a reason for hiding this comment

Uh oh!

viktorklang-ora left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

bchristi-git commented Apr 10, 2026 •

edited by openjdk Bot

Loading

openjdk Bot commented Apr 10, 2026 •

edited

Loading

mlbridge Bot commented Apr 10, 2026 •

edited

Loading