Skip to content

ffi: refresh cached string buffers on every call - #65051

Open
trivikr wants to merge 2 commits into
nodejs:mainfrom
trivikr:ffi-temp-string-buffer-reuse
Open

ffi: refresh cached string buffers on every call#65051
trivikr wants to merge 2 commits into
nodejs:mainfrom
trivikr:ffi-temp-string-buffer-reuse

Conversation

@trivikr

@trivikr trivikr commented Aug 5, 2026

Copy link
Copy Markdown
Member

Fixes: #65050

Native code can mutate temporary string storage during an FFI call. Rewrite cached buffers on every conversion so a later call with the same JavaScript string receives a fresh copy of its UTF-8 bytes.


Assisted-by: codex:gpt-5.6-sol

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/ffi

@nodejs-github-bot nodejs-github-bot added ffi Issues and PRs related to experimental Foreign Function Interface support. needs-ci PRs that need a full CI run. labels Aug 5, 2026
@codecov

codecov Bot commented Aug 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.31%. Comparing base (a576f1c) to head (8ef6d08).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #65051   +/-   ##
=======================================
  Coverage   90.30%   90.31%           
=======================================
  Files         760      760           
  Lines      248526   248521    -5     
  Branches    46886    46896   +10     
=======================================
+ Hits       224439   224445    +6     
+ Misses      15515    15502   -13     
- Partials     8572     8574    +2     
Files with missing lines Coverage Δ
lib/internal/ffi/fast-api.js 95.54% <ø> (+0.76%) ⬆️

... and 28 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@trivikr trivikr added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 7, 2026
MILLERMARRU

This comment was marked as low quality.

@trivikr
trivikr force-pushed the ffi-temp-string-buffer-reuse branch from a88fe4c to 8f76513 Compare August 9, 2026 23:22

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@trivikr trivikr added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 10, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 10, 2026
@nodejs-github-bot

This comment was marked as outdated.

@trivikr trivikr added request-ci Add this label to start a Jenkins CI on a PR. needs-ci PRs that need a full CI run. and removed needs-ci PRs that need a full CI run. request-ci Add this label to start a Jenkins CI on a PR. labels Aug 10, 2026
@nodejs-github-bot

This comment was marked as resolved.

@trivikr

trivikr commented Aug 10, 2026

Copy link
Copy Markdown
Member Author

Converted to draft as test-ffi-fast-buffer is failing on ci.nodejs.org/job/node-test-commit-aix/64314

---
duration_ms: 1355.201
exitcode: 1
severity: fail
stack: |-
  Test failure: 'fast FFI refreshes cached temporary string buffers'
  Location: test/ffi/test-ffi-fast-buffer.js:99:1
  AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:

  '' !== 'yello'

      at TestContext.<anonymous> (/home/iojs/build/workspace/node-test-commit-aix/nodes/aix72-power9/test/ffi/test-ffi-fast-buffer.js:108:12)
      at Test.runInAsyncScope (node:async_hooks:227:14)
      at Test.run (node:internal/test_runner/test:1397:25)
      at Test.processPendingSubtests (node:internal/test_runner/test:969:18)
      at Test.postRun (node:internal/test_runner/test:1537:19)
      at Test.run (node:internal/test_runner/test:1462:12)
      at process.processTicksAndRejections (node:internal/process/task_queues:104:5)
      at async Test.processPendingSubtests (node:internal/test_runner/test:969:7) {
    generatedMessage: true,
    code: 'ERR_ASSERTION',
    actual: '',
    expected: 'yello',
    operator: 'strictEqual',
    diff: 'simple'
  }

  (node:38469944) internal/test/binding: These APIs are for internal testing only. Do not use them.
  (Use `node --trace-warnings ...` to show where the warning was created)
  (node:38469944) ExperimentalWarning: FFI is an experimental feature and might change at any time
...

@trivikr
trivikr marked this pull request as draft August 10, 2026 20:51
@trivikr
trivikr marked this pull request as ready for review August 11, 2026 01:43
@trivikr

trivikr commented Aug 11, 2026

Copy link
Copy Markdown
Member Author

There was a bug in test.

AIX does not support Node’s PPC64 fast-call trampoline, so it exposes the test’s invalid assumption: the pointer returned by overwrite_string() points into temporary storage owned by Node’s generic FFI invocation. That storage is only valid during the native call. AIX frees/reuses the storage immediately, yielding ''.

Fast-path platforms retain the cached Buffer, so "yello" remains readable.

It was fixed in 89cc17f by observing the mutation during the native call instead of returning a pointer to temporary storage.

I'll re-request review after the CI is successful.

Native code can mutate temporary string storage during an FFI call.
Rewrite cached buffers on every conversion so a later call with the
same JavaScript string receives a fresh copy of its UTF-8 bytes.

Signed-off-by: Kamat, Trivikram <16024985+trivikr@users.noreply.github.com>
Assisted-by: codex:gpt-5.6-sol
@trivikr
trivikr force-pushed the ffi-temp-string-buffer-reuse branch from 89cc17f to d225103 Compare August 11, 2026 01:46
@trivikr trivikr removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 11, 2026
@trivikr
trivikr force-pushed the ffi-temp-string-buffer-reuse branch from d225103 to 8ef6d08 Compare August 11, 2026 01:52
@trivikr

trivikr commented Aug 11, 2026

Copy link
Copy Markdown
Member Author

The ffi contract explicitly says JavaScript strings are copied to temporary NUL-terminated UTF-8 storage “for the duration of the call”

string values are copied to temporary NUL-terminated UTF-8 strings for the duration of the call.

The fast path happens to retain reusable buffers longer, but that is an implementation detail, not a lifetime guarantee.
The test should return the observed byte by value as done in 8ef6d08 and not the pointer.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ffi Issues and PRs related to experimental Foreign Function Interface support. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ffi reuses mutated temporary string buffer without restoring its contents

5 participants