Skip to content

fix: bind VBlob data views to their arena - #236

Merged
lopcode merged 3 commits into
lopcode:mainfrom
coldmans:codex/fix-vblob-arena-scope
Sep 10, 2026
Merged

fix: bind VBlob data views to their arena#236
lopcode merged 3 commits into
lopcode:mainfrom
coldmans:codex/fix-vblob-arena-scope

Conversation

@coldmans

@coldmans coldmans commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

This fixes the arena lifetime and thread-access guarantees of buffers returned by VBlob.asArenaScopedByteBuffer().

getUnsafeDataAddress() currently uses asSlice() to set the bounds of the pointer returned by vips_blob_get(). The pointer retains its global scope, so the returned buffer's scope remains alive after the owning arena closes and releases the native memory. Buffers created with Arena.ofConfined() also appear accessible from other threads.

The following reproduces the lifetime mismatch without reading freed memory:

ByteBuffer buffer;
try (var arena = Arena.ofConfined()) {
    buffer = VBlob.newFromBytes(arena, new byte[]{1, 2, 3})
            .asArenaScopedByteBuffer();
}
System.out.println(MemorySegment.ofBuffer(buffer).scope().isAlive());
// Before: true; after this fix: false

Changes

  • Use reinterpret(length, arena, null) to bind the data view to the blob's arena and set its bounds. This preserves the existing cleanup ownership without copying data or registering another deallocator.
  • Add six native regression tests covering access after arena closure, segment scope, confined/shared arena access, independent copies, and blobs produced by PNG encoding. Tests check scope metadata before attempting access after closure, so they fail safely on the unpatched implementation without reading freed memory.
  • Add a scope check to the existing VBlobByteBufferSample so the existing sample CI also catches the regression. The native tests live in integrationTest, keeping the default unit tests runnable before libvips is installed.

Validation

Tested against base commit 1ed5dd596fb48ab8c7b69b21a28f05fc3090851e on Linux ARM64 with Temurin 25, libvips 8.18.6, and JUnit Platform Console 1.11.3.

  • Compiled all core sources with javac --release 22, then ran the six new tests and the existing SenseCheckTest: 2/7 passed and 5/7 failed before the fix; 7/7 passed after it.
  • The updated VBlobByteBufferSample fails its scope assertion before the fix and passes afterward.
  • git diff --check passes. I have not run the full Gradle build or tested other OS/JDK combinations locally.

With the project's toolchain and libvips installed, the new regression tests can be run with:

./gradlew :core:integrationTest --tests app.photofox.vipsffm.VBlobArenaTest

Compatibility

This enforces the documented arena lifetime contract; it does not reduce native allocations or change when native memory is released. Code that previously accessed a confined arena's buffer from another thread will now be subject to the arena's thread-access checks. Such callers should use a shared arena or an independent copy.

Copilot AI lite review requested due to automatic review settings September 10, 2026 05:59

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@coldmans coldmans changed the title fix: VBlob 데이터 뷰를 Arena 수명에 연결 fix: bind VBlob data views to their arena Sep 10, 2026
@lopcode
lopcode enabled auto-merge (squash) September 10, 2026 06:39
@lopcode

lopcode commented Sep 10, 2026

Copy link
Copy Markdown
Owner

Thank you for your contribution, and for including tests for validation. I realised there were a couple of other places with the same bug, so I included them in this PR.

@lopcode
lopcode merged commit 52bcbf6 into lopcode:main Sep 10, 2026
7 checks passed
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.

3 participants