unom/punktfunk
5
0
Fork 0
Code Issues Pull Requests 2 Actions 2 Packages Projects Releases 36 Wiki Activity

fix(encode/pyrowave): pin the NT-handle import contract to consume-on-success-only

Browse Source 
import_plane closed the shared NT handle on every pyrowave_image_create
failure, believing pyrowave only consumes it on success. The vendored
truth was messier: Granite's allocator closed by-reference handles
UNCONDITIONALLY after the first vkAllocateMemory — success AND failure —
while failures before the allocator (validation, vkCreateImage, no
memory type) left the handle open, and both classes surface as the same
error code. So the call site could not know whether to close, and an
allocate-stage failure double-closed a possibly-recycled handle value
(audit WP5.3). The filed fix (DuplicateHandle + close the original
unconditionally) traded the double close for a guaranteed leak on every
pre-allocate failure and left the ambiguity in place.

Patch 0006 fixes the callee instead: the allocator never closes a
caller's handle, and pyrowave_image_create consumes it exactly at its
success return — which is what pyrowave.h ("take ownership and close the
HANDLE on import") documents, and what Granite's semaphore import
already did, so import_fence was correct as written and the two paths
now share one contract. The close-on-failure in import_plane is thereby
correct on EVERY path, including a vkBindImageMemory failure after a
successful allocate. Bonus fix recorded in the patch: the allocator's
block-recycling retry loop used to re-run vkAllocateMemory with
import_info still pointing at the just-closed handle.

Both-platform gates green; pyrowave_win_smoke (.173, 34/34) re-validates
the live import path against the rebuilt vendored C++.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
enricobuehler
2026-07-25 16:51:00 +02:00
co-authored by Claude Fable 5
parent d6d4408c8e
commit e0e845b7df
4 changed files with 124 additions and 14 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/pyrowave-sys/patches/0006-external-memory-consume-on-success-only.patch
+88
View File
@@ -0,0 +1,88 @@
NT-handle import: consume on SUCCESS only — PUNKTFUNK LOCAL PATCH.
Not upstream. Granite's DeviceAllocator::internal_allocate closed the imported
by-reference NT handle (OPAQUE_WIN32 / D3D11_TEXTURE / D3D12_RESOURCE — the
predicate excludes fd and KMT types) UNCONDITIONALLY after the FIRST
vkAllocateMemory — on success AND on failure — while every failure before the
allocator (c-shim validation, vkCreateImage, find_memory_type) left the handle
open. Both classes surface to pyrowave_image_create's caller as the same error
code, so the caller could not know whether to close: punktfunk's import_plane
closed on every failure and double-closed whenever the failure was at or after
the allocate — and a recycled handle value makes that close an unrelated live
handle's. The unconditional consume also broke the allocator's own
block-recycling retry loop, which re-ran vkAllocateMemory with import_info
still pointing at the just-closed handle.
Fixed by moving the consume to the API commit point: pyrowave_image_create
closes the by-reference handle exactly on its success return. This pins
pyrowave.h's documented contract ("take ownership and close the HANDLE on
import") to mean on SUCCESS, matching pyrowave_sync_object_create (Granite's
semaphore import already closes only after a successful vkImportSemaphore*).
NT-handle memory imports never transfer ownership at the Vulkan level (the
implementation holds its own reference), so the post-create close is legal.
Behavior note: Granite code that imports by-reference memory WITHOUT going
through pyrowave_image_create — in this vendored subset only wsi_dxgi.cpp's
interop swapchain, which the pyrowave C API never reaches — now keeps the
caller's handle open on allocate-stage failure instead of consuming it. That
matches the semaphore contract; those callers own their close-on-failure.
Correctness re-validated by pyrowave_win_smoke on real hardware after the
change.
diff --git a/crates/pyrowave-sys/vendor/pyrowave/Granite/vulkan/memory_allocator.cpp b/crates/pyrowave-sys/vendor/pyrowave/Granite/vulkan/memory_allocator.cpp
index f33bdac3..fd035b66 100644
--- a/crates/pyrowave-sys/vendor/pyrowave/Granite/vulkan/memory_allocator.cpp
+++ b/crates/pyrowave-sys/vendor/pyrowave/Granite/vulkan/memory_allocator.cpp
@@ -732,16 +732,15 @@ bool DeviceAllocator::internal_allocate(
res = table->vkAllocateMemory(device->get_device(), &info, nullptr, &device_memory);
}
- // If we're importing, make sure we consume the native handle.
- if (external && bool(*external) &&
- ExternalHandle::memory_handle_type_imports_by_reference(external->memory_handle_type))
- {
-#ifdef _WIN32
- ::CloseHandle(external->handle);
-#else
- ::close(external->handle);
-#endif
- }
+ // PUNKTFUNK (patch 0006): the consume of by-reference native handles used to happen HERE,
+ // unconditionally — success AND failure of the first vkAllocateMemory. That made the caller's
+ // failure contract ambiguous (an allocate-stage failure had already closed the handle while a
+ // create/find_memory_type failure had not), and the block-recycling retry loop below re-ran
+ // vkAllocateMemory with import_info still pointing at the just-closed handle. The consume now
+ // lives at the API commit point (pyrowave_c.cpp: pyrowave_image_create's success return), so
+ // the allocator never closes a caller's handle and retries import a still-open handle.
+ // (fd-type imports were never affected: memory_handle_type_imports_by_reference excludes
+ // OPAQUE_FD/DMA_BUF, whose ownership vkAllocateMemory itself transfers on success.)
if (res == VK_SUCCESS)
{
diff --git a/crates/pyrowave-sys/vendor/pyrowave/pyrowave_c.cpp b/crates/pyrowave-sys/vendor/pyrowave/pyrowave_c.cpp
index eb917e64..985cd0a9 100644
--- a/crates/pyrowave-sys/vendor/pyrowave/pyrowave_c.cpp
+++ b/crates/pyrowave-sys/vendor/pyrowave/pyrowave_c.cpp
@@ -592,6 +592,21 @@ pyrowave_result pyrowave_image_create(const pyrowave_image_create_info *info, py
if (!img)
return PYROWAVE_ERROR_FAILED_EXTERNAL_HANDLE;
+ // PUNKTFUNK (patch 0006): consume the imported by-reference NT handle exactly here, at the
+ // API's success boundary — the allocator no longer closes it (see memory_allocator.cpp).
+ // This pins pyrowave.h's documented contract ("take ownership and close the HANDLE on
+ // import") to mean ON SUCCESS, matching pyrowave_sync_object_create's semantics: on ANY
+ // failure return the caller still owns the handle and can close it unconditionally.
+ // By-reference NT-handle imports never transfer ownership at the Vulkan level (the
+ // implementation keeps its own reference), so a post-create close here is always legal.
+#ifdef _WIN32
+ if (info->external_handle &&
+ ExternalHandle::memory_handle_type_imports_by_reference(info->handle_type))
+ {
+ ::CloseHandle(reinterpret_cast<HANDLE>(info->external_handle));
+ }
+#endif
+
auto *image = new pyrowave_image_opaque();
image->device = &device;
image->img = std::move(img);