Skip to content

fix: work around koffi leak - #114

Merged
TimoGlastra merged 5 commits into
mainfrom
TimoGlastra/koffi-decode-leaks-audit
Oct 28, 2026
Merged

TimoGlastra merged 5 commits into
mainfrom
TimoGlastra/koffi-decode-leaks-audit

Conversation

@TimoGlastra

Copy link
Copy Markdown
Contributor

koffi.decode(ptr, type, len) leaks about 20 bytes on every call (Koromix/koffi#280). Each Askar call that returns bytes or a handle list went through it.

Bytes and handle lists are now decoded through a struct whose pointer member is counted by its len member. koffi decodes that without creating an array type, so nothing leaks.

@TimoGlastra
TimoGlastra added this pull request to stack #113 October 27, 2026 19:00
@TimoGlastra
TimoGlastra requested a review from a team as a code owner October 27, 2026 19:00
@changeset-bot

changeset-bot Bot commented Oct 27, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 410d6c4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@openwallet-foundation/askar-nodejs Patch
@openwallet-foundation/askar-react-native Patch
@openwallet-foundation/askar-shared Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@genaris genaris left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The approach looks correct: decodeCounted decodes a copy, so the original struct keeps the pointer for askar_buffer_free. Can you add a comment that each counted struct must have the same layout as its source struct?

The tests always use a non-null pointer. What does koffi return for a counted pointer that is null, with len 0? An early if (len === 0) return new Uint8Array(0) prevents this case and does not copy the struct.

@TimoGlastra
TimoGlastra force-pushed the TimoGlastra/koffi-decode-leaks-audit branch from 8db0d82 to 417924d Compare October 28, 2026 10:55
@TimoGlastra
TimoGlastra enabled stacked auto-merge (squash) October 28, 2026 10:55
@TimoGlastra
TimoGlastra disabled stacked auto-merge October 28, 2026 11:24
Build the packages with tsdown, like identity-common, into a single
build/index.mjs with index.d.mts types per package. The exports maps
only have types and default conditions, plus askar-shared/installBinary
for the install scripts. The Node.js package finds its native directory
through its own package.json instead of __dirname.

Signed-off-by: TimoGlastra <timo@animo.id>
Signed-off-by: TimoGlastra <timo@animo.id>
Signed-off-by: TimoGlastra <timo@animo.id>
Signed-off-by: TimoGlastra <timo@animo.id>
Signed-off-by: TimoGlastra <timo@animo.id>
@TimoGlastra
TimoGlastra force-pushed the TimoGlastra/koffi-decode-leaks-audit branch from 417924d to 410d6c4 Compare October 28, 2026 11:24
Base automatically changed from TimoGlastra/esm-only to main October 28, 2026 11:28
@TimoGlastra
TimoGlastra merged commit 8643f17 into main Oct 28, 2026
11 of 12 checks passed
@TimoGlastra
TimoGlastra deleted the TimoGlastra/koffi-decode-leaks-audit branch October 28, 2026 11:28
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