Skip to content

fix: make the examples report their failures, plus six correctness fixes - #10

Open
jeanpierrefortune wants to merge 8 commits into
develop-2025-04-18from
develop-2025-04-18-fix-win
Open

fix: make the examples report their failures, plus six correctness fixes#10
jeanpierrefortune wants to merge 8 commits into
develop-2025-04-18from
develop-2025-04-18-fix-win

Conversation

@jeanpierrefortune

@jeanpierrefortune jeanpierrefortune commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Eight changes to the examples, found while running the whole PC/SC suite on Windows with SpringCard Puck and Identive readers. Two of them are about error reporting and were a prerequisite to diagnosing everything else: before them, a failing example printed nothing at all.

The library fixes these examples exposed are in separate PRs on keyple-card-calypso-cpp-lib, keyple-card-calypso-crypto-legacysam-cpp-lib, keyple-service-cpp-lib, keyple-plugin-pcsc-cpp-lib, keyple-util-cpp-lib and keypop-calypso-card-cpp-api.

One commit per subject.

Error reporting

  • 8d20977 report the cause in the reader observer callbacks. Two format strings passed one argument more than they had placeholders, so the logger threw from inside the error path and the cause of an observation error was never printed.
  • 9119e5e catch and report the exceptions escaping the examples. Each main becomes a runExample() called from a main that catches, logs the message and returns 1. An escaping exception used to reach abort() with exit code 3, no message, and the buffered log was lost whenever output was redirected.

Correctness

  • 58b2810 do not finalize card processing on an UNAVAILABLE event. The guard excluded only CARD_REMOVED, so UNAVAILABLE restarted a monitoring job on a reader being unregistered. The guard keeps the log from showing a card removal sequence starting on a reader that just vanished.
  • dcadda4 select the Innovatron protocol by the name the plugin registers. UseCase3_Rev1Selection activated the protocol under the name of the deprecated PcscSupportedContactlessProtocol, whose Innovatron constant carries a _CARD suffix. See the note below on how this interacts with the plugin.
  • 8e5f8e2 take the PIN key references from the shared constants. UseCase6_VerifyPin and UseCase9_ChangePin each declared their own copies, which had drifted away from common/CalypsoConstants. Details below.
  • 15c890d check findReader for null before dereferencing the reader. findReader returns null when no reader name matches the regex, where the Java original throws. Every caller then called getName() on that null pointer, so a missing or renamed reader ended in an access violation with no diagnostic at all. The seven call sites now throw an IllegalStateException naming the regex that matched nothing.

Housekeeping

  • 40a83d5 point the Calypso examples at the current test card AID. Thirteen files including the shared CalypsoConstants. The Service and PC/SC examples keep the previous AID because the cards available for those tests carry the old application, and the four stub files keep it because the simulated card is that application.
  • 755260a migrate the examples off the deprecated PC/SC protocol classes. 19 contactless and 9 contact call sites move to PcscCardCommunicationProtocol. Both moves are behaviour-neutral: the contactless name is the same string in both classes, and the three contact rules share the same ATR pattern. One call site keeps the deprecated class on purpose, UseCase2_ProtocolBasedSelection, because MIFARE_CLASSIC does not exist in the current one.

This migration depends on the export fix in the keyple-plugin-pcsc-cpp-lib PR: without it, PcscCardCommunicationProtocol does not link on Windows.

On the PIN key references

CalypsoConstants was until now used by nobody but its own translation unit, which is how the two example copies drifted apart from it unnoticed. Both examples now take all four references, and the PIN values, from that one file.

One of the four was wrong there. The verification ciphering key carried KVC 79h where the card under test expects 74h, the value both examples shipped. An encrypted Verify PIN answered 63C2, read as a wrong PIN, although the same PIN succeeded in plain form on the line above. With 74h in the shared file, UseCase6_VerifyPin runs its five steps and the attempt counter goes 3, 3, 2, 2, 3 as expected.

The modification ciphering key keeps the 21h/79h of the shared file. It is worth saying plainly that this pair does not work either: UseCase9_ChangePin answers 6988, incorrect cryptogram, with 21h/79h exactly as it does with the 21h/74h the examples shipped. So neither shipped value matches the key this card expects, and the right one is still unknown. The untried candidate is KIF 30h with KVC 74h, the key that works for verification. That question is separate from the de-duplication done here, and this PR does not claim to answer it.

Note on the Innovatron commit

When this was found, the plugin keyed its default rules only from PcscCardCommunicationProtocol, without the _CARD suffix, so the example failed with "The card protocol INNOVATRON_B_PRIME_CARD is not supported". The PC/SC plugin has since started registering the deprecated names alongside the current ones, so the old name resolves again and the example is no longer broken by this.

The commit is kept because it moves the example onto the non-deprecated name, consistently with the migration commit above. It is now a migration rather than a fix, and can be dropped if you prefer.

A format string this base corrects differently

This base changes the %% of the UseCase12 and UseCase13 failure messages to %s. Logger::printf treats a bare % as the placeholder and %% as an escaped percent, so %s consumes the argument and then prints a literal s. The restructuring commit uses % %, three placeholders for three arguments, which is why it conflicts there. Worth a separate look on your side.

Validation

The whole suite was run on hardware. Every example either completes or exits 1 with a message naming its cause, the one exception being UseCase9_ChangePin and its open 6988 described above. The stub examples and the 761 unit tests of the dependency chain pass.

@jeanpierrefortune
jeanpierrefortune marked this pull request as draft September 11, 2026 13:36
Two format strings passed one argument more than they had placeholders, so
the logger threw from inside the error path, and the cause of an observation
error was never printed.
Each main is now a runExample() called from a main that catches, logs the
message and returns 1. An escaping exception used to reach abort() with exit
code 3, no message, and the buffered log lost whenever output was
redirected.
Thirteen files including the shared CalypsoConstants. The Service and PC/SC
examples keep the previous AID because the cards available for those tests
carry the old application, and the four stub files keep it because the
simulated card IS that application.
The guard excluded only CARD_REMOVED, so UNAVAILABLE restarted a monitoring
job on a reader being unregistered. This does NOT affect reader
reconnection detection, which was a PC/SC plugin matter, and the executor
now rejects such a post-shutdown job anyway; the guard keeps the log from
showing a card removal sequence starting on a reader that just vanished.
UseCase3 activated the protocol under the name of the deprecated
PcscSupportedContactlessProtocol, whose Innovatron constant carries a
_CARD suffix, while the plugin keys its default rules from
PcscCardCommunicationProtocol, without the suffix. The activation failed
with "The card protocol INNOVATRON_B_PRIME_CARD is not supported". The
new rule also matches more ATRs than the old one.
19 contactless ISO_14443_4 and 9 contact ISO_7816_3_T0 call sites moved to
PcscCardCommunicationProtocol. Both moves are behaviour-neutral: the
contactless name is the same string in both classes, and the three contact
rules share the same ATR pattern. One call site keeps the deprecated class
on purpose, UseCase2_ProtocolBasedSelection, because MIFARE_CLASSIC does
not exist in the current one.
@jeanpierrefortune
jeanpierrefortune force-pushed the develop-2025-04-18-fix-win branch from cb50bcf to 8f39fb9 Compare September 11, 2026 14:03
UseCase6 and UseCase9 each declared their own copies of the PIN key
references and PIN values, and those copies had drifted away from
common/CalypsoConstants, which was until now used by nobody but its own
translation unit. Both examples now take the four references from it.

One of the four was wrong there: the verification ciphering key carried
KVC 79h where the card under test expects 74h, the value both examples
shipped. An encrypted Verify PIN answered 63C2, read as a wrong PIN,
although the same PIN succeeded in plain form on the line above. With 74h
in the shared file, UseCase6 runs its five steps and the attempt counter
goes 3, 3, 2, 2, 3 as expected.

The modification ciphering key keeps the 21h/79h of the shared file. Note
that Change PIN still answers 6988, incorrect cryptogram, with that pair
as well as with the 21h/74h the examples shipped, so neither value matches
the key this card expects and the right one is still unknown. That is a
separate question from the de-duplication done here.
findReader returns null when no reader name matches the regex, where the
Java original throws. Every caller in the examples then called getName()
on that null pointer, so a missing or renamed reader ended in an access
violation with no diagnostic at all, the try/catch wrapper seeing no
exception to report.

Guard the seven call sites with an IllegalStateException naming the regex
that matched nothing. The Calypso helper covers the twelve Calypso
examples at once, for the card reader and the SAM reader alike. In
UseCase4 the test also covers a reader found but not observable, since it
applies to the result of the cast to ObservableCardReader.
@jeanpierrefortune
jeanpierrefortune force-pushed the develop-2025-04-18-fix-win branch from 8f39fb9 to 15c890d Compare September 11, 2026 15:06
@sonarqubecloud

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
29.1% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

@jeanpierrefortune
jeanpierrefortune marked this pull request as ready for review September 11, 2026 15:14
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.

1 participant