Skip to content

Commit bc8ba23

Browse files
authored
fix(mediaplayer): native C/C++ security hardening (parser/protocol memory safety + fuzzing) (#968)
## Summary Security hardening for the media player's native C/C++ (`Native~/`), from a memory-safety review of the parser/protocol layer. That code parses attacker-controlled bytes in-process — a peer-broadcast URL is parsed by every client in a room — so the findings here are remotely reachable. This closes five higher-severity and five medium issues, and extends the fuzz harness to two parsers that had no coverage. One commit per concern so each fix can be reviewed independently. **Out-of-bounds write / SSRF / DoS (higher severity)** - **RTSP Basic-auth base64 OOB stack write** — a `rtsp://user:pass@host` URL could encode a 255-byte `user:pass` into a 256-byte field (340 bytes out), an 85-byte overwrite of adjacent stack fields. Size the field for the true maximum and bound the encode loop. - **HLS sub-resource SSRF** — the managed URL check only validates the top-level playlist; variant/segment/`EXT-X-MAP` URIs from the playlist body were fetched with no host check, so a hostile playlist could steer a fetch at `169.254.169.254`/RFC1918/loopback. Every HLS fetch now goes through a resolved-address block (fail-closed, `http(s)`-only), with policy rejection distinguished from transient failure so a blocked reload/segment terminates deterministically instead of skipping or busy-looping. The non-global-unicast IPv4 classifier is also aligned to the full IANA special-use set in both the native and managed guards. - **MP4 box-tree recursion** — `parse_box_tree` recursed with no depth limit; a small crafted `moov` overflowed the demux-thread stack. Capped at 32. - **MPEG-TS PES accumulation** — a stream that sets PUSI once and streams continuation packets forever grew the PES buffer unbounded, with a signed-overflow in the growth math. 8 MiB cap, 64-bit math. - **Android Vulkan output-texture UAF** — the framebuffer/image-view were destroyed on the main thread while the render thread could be mid-present, on a mid-stream resolution change. The destroy is deferred to the render thread under the lock, with one consistent target extent for the framebuffer and render area. **Medium** - Render-event liveness registry so a late `OnRenderEvent` can't use-after-free an engine freed by `basis_media_close`. - RTMP `_result` AMF parsing bounds every read (was reading up to 8 bytes past the buffer). - RTSP RTP reassembly buffers capped (16 MiB) with the overflow fixed; a cap hit drops the whole AU/frame rather than a truncated one. - Windows Media Foundation allocations checked (a large declared sample turned an alloc failure into a null-deref); COM buffer unlock/HRESULT hygiene; software-fallback size bounded against overflow. - Android `AMediaCodec_getInputBuffer` null-checked, sizes compared as `size_t`, and the codec queue status propagated so a dropped frame isn't reported as accepted. **Fuzzing** — new `fuzz_url`, `fuzz_hls`, `fuzz_rtsp` and `fuzz_rtmp` targets under ASan/UBSan, wired into `build.sh`; the URL/HLS/RTSP/RTMP parsers were all previously unfuzzed. HLS uses an in-memory HTTP provider; RTSP/RTMP `#include` the real `.c` and provide a `basis_io` stub (byte-serving the socket-read paths), driving `parse_sdp`/`depkt_video`/`depkt_audio`/`amf_find_stream_id`/FLV tag parsers directly. The RTSP target earned its keep — it found a signed left-shift UB in the RTP timestamp read (`rtp[4] << 24` on a byte ≥ 128) on its first run, fixed here and pinned as a replay repro. Deeper full-session RTSP/RTMP coverage (a scripted handshake, or an injected transport vtable that would also enable RTSP/RTMP unit tests) is tracked as a backlog follow-up. **Docs / tooling** — also retires the self-hosted `tools/media-test-streams` Docker stack and its TESTING.md guidance. The public endpoints already listed cover the common lanes, the CI conformance gate (`tools/media-conformance`) already proves the demuxers on every supported container/codec, and the remaining lanes (live RTSP/RTMP/RIST, split-stream, captions, LPCM 7.1, `localhost`) are cheaper to serve from your own files/server than to maintain a bundled stack. TESTING.md now lists the supported container/codec/transport inputs to bring your own of, framed around what CI can't touch (real decode + present, A/V sync, live transports). ## Required checks All boxes below must be ticked before this PR can merge. If a check is genuinely N/A, tick it anyway and explain under **Notes**. <!-- required-checks-start --> <!-- Tick the boxes in place — do not edit the line text. The pr-checklist workflow parses this block; per-PR context goes under Notes. --> - [x] **Tested** — I built and ran this locally. The change works in the editor and (where relevant) in a built player. - [x] **Transform access is combined and limited** — In hot paths, transform reads/writes go through `TransformAccessArray` or are otherwise batched. I have not added per-frame `transform.position` / `transform.rotation` / `transform.localPosition` calls inside loops. Whenever I need both position and rotation, I use the combined APIs — `SetPositionAndRotation` / `SetLocalPositionAndRotation` for writes, `GetPositionAndRotation` / `GetLocalPositionAndRotation` for reads — instead of two separate property accesses; the combined call does one local-to-world matrix traversal instead of two. - [x] **Addressables used for asset/memory loading** — Any new asset loads go through Addressables. No new `Resources.Load`, no direct asset references that pull large content into memory on scene load. - [x] **No new `GetComponent` / `AddComponent` where avoidable** — Where unavoidable, the result is cached on a field, and any `GetComponent<T>` is replaced with `TryGetComponent<T>(out var x)` — bare `GetComponent` will be denied. `TryGetComponent` is the modern API (Unity 2019.2+) and skips the Editor-only GC allocation `GetComponent` causes when a component is missing: Unity wraps the `null` return in a managed "fake null" object so its overloaded `==` operator can still detect destroyed C++ objects, and constructing that wrapper allocates; `TryGetComponent` returns a `bool` plus `out` parameter and never builds the wrapper. None of these calls run inside `Update`, `LateUpdate`, `FixedUpdate`, jobs, or other per-frame code paths. - [x] **Per-frame work is scheduled through `BasisEventDriver`** — Any new per-frame work hooks into `BasisEventDriver` rather than adding standalone `Update` / `LateUpdate` / `FixedUpdate` callbacks on a MonoBehaviour. - [x] **Anything added to `BasisEventDriver` is bulletproof, or guarded by `try`/`catch`** — `BasisEventDriver` runs the single per-frame tick that drives the whole framework (network apply, local player sim, blendshapes, JigglePhysics, nameplates, and more) as one sequential chain. An unhandled exception anywhere in that chain aborts the rest of the tick, so every step after the throwing one is silently skipped for that frame. New work added to the driver must either be guaranteed not to throw, or be wrapped in a `try`/`catch` that contains the failure and surfaces it through `BasisDebug` — logged once / rate-limited, never every frame (see the existing `HVRBasisBuiltInAddresses.Simulate()` guard for the pattern). Expect this to be scrutinized closely in review. - [x] **Considered jobification** — I asked whether this work can be moved to a Unity Job (Burst-compiled where possible). If it can, it is. If it cannot, the reason is in **Notes**. - [x] **No needless `{ get; set; }` properties or access lockdowns** — Public fields are fine; Basis is meant to be read and modified freely, so don't wall things off `private`/`internal` without a real reason. Don't wrap a field in `{ get; set; }` when the accessors do nothing — property accessors have a real performance cost vs direct field access, and the lead maintainer prefers plain fields (or a method / setter-only property when only the setter needs logic) over a noop-getter pair. For `.Instance` singletons, callers reassigning `Type.Instance` is allowed; if that would break your code, log a warning or throw — don't block the assignment. Locking down access is not your call. - [x] **Camera access goes through `BasisLocalCameraDriver`** — Code that needs the local camera (transform, projection, rig data, etc.) pulls it from `BasisLocalCameraDriver` rather than looking one up itself. Don't roll a separate camera discovery path. - [x] **Logging uses `BasisDebug`** — All new logging calls go through `BasisDebug.Log` / `BasisDebug.LogWarning` / `BasisDebug.LogError` (with an appropriate `LogTag`) instead of `UnityEngine.Debug.Log` / `Debug.LogWarning` / `Debug.LogError`. `BasisDebug` routes through Basis's tagged, color-coded logger and respects the project-wide `LoggingDisabled` toggle so logging can be killed at runtime; bare `Debug.Log` calls bypass that and will be denied. - [x] **No scene-wide discovery for dependencies** — New code is architected so it does not need `FindObjectOfType` / `FindObjectsOfType` / `GameObject.Find` / `FindGameObjectsWithTag` to locate what it depends on. References are wired in — registered through an existing manager/driver, injected at init, or passed in by the caller — rather than discovered by scanning the scene at runtime. If a scene scan is genuinely unavoidable, justify it under **Notes**. - [x] **No allocations in hot paths** — Per-frame code (Update / LateUpdate / FixedUpdate, simulation loops, jobs, anything called once per frame or more) does not allocate. No `new` on reference types, no LINQ, no `string` concatenation/interpolation, no boxing, no `foreach` over interface-typed collections. Allocate once at init and reuse the buffer. - [x] **No debugging in hot paths** — No log calls of any kind on per-frame paths, including `BasisDebug`. Hot-path logging floods the console and incurs cost on every frame regardless of whether the message is filtered out downstream. If a hot-path log is needed while iterating, gate it behind `#if UNITY_EDITOR` and remove (or leave gated) before merge. - [x] **Hot-path collection access is optimized** — Cache `.Count` (lists) / `.Length` (arrays) into a local `int` before the loop instead of re-reading the property each iteration. Prefer `T[]` (with a separate length int when the array is over-sized) over `List<T>` where the data is hot — Unity's mono BCL doesn't expose `CollectionsMarshal.AsSpan(List<T>)`, so a list can't be fed into `Span<T>` / unsafe paths cleanly. Where the perf justifies it, drop into `Span<T>` / `ref` locals / `Unsafe.As` / `unsafe` pointer code to skip bounds checks and copies, and call out the invariants you're relying on under **Notes** so reviewers can sanity-check them. <!-- required-checks-end --> ## Testing details Tick the platforms you actually tested on. Leave the rest unticked — these are informational and do not block merge. - [x] Windows - [ ] Linux - [ ] Android - [ ] iOS - [ ] macOS Input / control mode coverage: - [ ] Tested in VR (note headset under **Notes**) - [ ] Tested in desktop / non-VR mode - [ ] Tested with phone controls (mobile touch input) - [x] N/A — change does not touch player/XR/input code Where applicable, confirm these flows still work after your changes: - [ ] Hot-switching (desktop ↔ VR mode swap at runtime) - [ ] Avatar swapping - [ ] Server swapping (joining / leaving / changing servers) - [x] N/A — change does not touch any of the above ## Notes This is almost entirely a native-plugin change under `Native~/`. The one C# file it touches is `BasisMediaPlayerSecurity.cs` — the SSRF address classifier, extended (in lockstep with the native `basis_io.c` guard) to the full IANA "not globally reachable" special-use set. It has no transform/component/hot-path/per-frame code, so the Unity-specific required checks (transform access, Addressables, `GetComponent`, `BasisEventDriver`, per-frame allocation/logging, camera driver, scene discovery, jobification, collection access) still don't apply and are ticked as N/A per the template. **Verification** is via the repo's own native gates, run locally against the real shipping source rather than a mock: `tools/media-fuzz` (all 10 targets build + run clean under AddressSanitizer + UndefinedBehaviorSanitizer, and the pinned crash-repro replay gate passes — including the two shift-UB repros the new RTSP/RTMP fuzzers surfaced), and `tools/media-conformance` (demux output still matches ffprobe on all synthetic fixtures, so the caps don't change valid-input playback). Portable C and `basis_win_decode.cpp` compile clean, and both native binaries are rebuilt from this source. One behavioural change for testers: the native transports (HLS/RTSP/RTMP) now enforce the non-global-unicast address block themselves, so a `localhost` URL on those transports is refused even in the Editor unless `BASIS_MEDIA_ALLOW_LOCAL` relaxes the native re-check (it does not relax the top-level C# gate — a top-level RFC1918 URL stays refused regardless). Documented in `TESTING.md`. **Known residuals (deliberate, tracked as follow-ups — not oversights):** - **SSRF pre-check depth.** The host check closes literal-internal and statically-private URLs (the common vector). It does *not* close active DNS rebinding or an allowed URL that redirects to an internal host, because the platform HTTP stacks (WinHTTP/JNI) re-resolve and follow redirects. Fully closing those is a connect-by-pinned-IP change with per-redirect re-validation and connected-peer verification at the provider boundary — it needs on-device testing (a naive "reject all 3xx" breaks legitimate CDN redirects), so it's scoped as its own follow-up. - **Render-event ABA.** The liveness registry closes the use-after-*free* of a closed engine. The narrow pointer-reuse edge (a delayed event matching a *new* engine at the same address) is benign for the C# binding's `RENDER_UPDATE`-only, idempotent usage; fully closing it needs a generation-stamped handle in the event payload (a C# ABI change) — follow-up. - **RTSP/RTMP fuzz depth.** The new targets drive the buffer-taking parsers directly (where the found bugs lived); full end-to-end session fuzzing (scripted handshake) is a follow-up.
2 parents 1ac5a40 + 4e5315e commit bc8ba23

32 files changed

Lines changed: 929 additions & 853 deletions

Basis/Packages/com.basis.mediaplayer/Native~/android/basis_android_decode.c

Lines changed: 17 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -508,6 +508,7 @@ static void feed_extractor_sample(basis_decoder_t* d, AMediaCodec* codec, int tr
508508
if (ii < 0) return;
509509
size_t cap = 0;
510510
uint8_t* buf = AMediaCodec_getInputBuffer(codec, ii, &cap);
511+
if (!buf) { AMediaCodec_queueInputBuffer(codec, ii, 0, 0, 0, 0); return; }
511512
ssize_t sz = AMediaExtractor_readSampleData(d->extractor, buf, cap);
512513
if (sz < 0) {
513514
AMediaCodec_queueInputBuffer(codec, ii, 0, 0, 0, AMEDIACODEC_BUFFER_FLAG_END_OF_STREAM);
@@ -858,17 +859,23 @@ int basis_decoder_set_audio_format(basis_decoder_t* d, basis_codec_t codec,
858859

859860
int basis_decoder_submit_video(basis_decoder_t* d, const uint8_t* annexb, int len, int64_t pts_us, int key) {
860861
(void)key;
861-
if (!d || !d->vcodec) return -1;
862+
if (!d || !d->vcodec || !annexb || len <= 0) return -1;
863+
int rc = -1;
862864
ssize_t ii = AMediaCodec_dequeueInputBuffer(d->vcodec, 2000);
863865
if (ii >= 0) {
864866
size_t cap = 0;
865-
uint8_t* buf = AMediaCodec_getInputBuffer(d->vcodec, ii, &cap);
866-
int n = len < (int)cap ? len : (int)cap;
867-
memcpy(buf, annexb, n);
868-
AMediaCodec_queueInputBuffer(d->vcodec, ii, 0, n, pts_us, 0);
867+
uint8_t* buf = AMediaCodec_getInputBuffer(d->vcodec, ii, &cap); /* size_t: no sign-cast */
868+
if (buf && (size_t)len <= cap) {
869+
memcpy(buf, annexb, (size_t)len);
870+
rc = (AMediaCodec_queueInputBuffer(d->vcodec, ii, 0, (size_t)len, pts_us, 0) == AMEDIA_OK) ? 0 : -1;
871+
} else {
872+
/* NULL buffer or the AU doesn't fit: never queue a partial frame — it
873+
* decodes to corruption. Release the slot empty and report the drop. */
874+
AMediaCodec_queueInputBuffer(d->vcodec, ii, 0, 0, pts_us, 0);
875+
}
869876
}
870877
drain_video_output(d);
871-
return 0;
878+
return rc;
872879
}
873880

874881
/* Source-order -> WAVE-order channel map for the Blu-ray HDMV LPCM
@@ -928,21 +935,22 @@ int basis_decoder_submit_audio(basis_decoder_t* d, const uint8_t* data, int len,
928935
if (!d || !data || len <= 0) return -1;
929936
if (d->ac == BASIS_CODEC_LPCM) { submit_lpcm(d, data, len, pts_us); return 0; }
930937
if (!d->acodec) return -1;
938+
int rc = -1;
931939
ssize_t ii = AMediaCodec_dequeueInputBuffer(d->acodec, 2000);
932940
if (ii >= 0) {
933941
size_t cap = 0;
934942
uint8_t* buf = AMediaCodec_getInputBuffer(d->acodec, ii, &cap);
935-
if ((size_t)len <= cap) {
943+
if (buf && (size_t)len <= cap) {
936944
memcpy(buf, data, (size_t)len);
937-
AMediaCodec_queueInputBuffer(d->acodec, ii, 0, len, pts_us, 0);
945+
rc = (AMediaCodec_queueInputBuffer(d->acodec, ii, 0, len, pts_us, 0) == AMEDIA_OK) ? 0 : -1;
938946
} else {
939947
/* Never feed a partial frame — it decodes to an error + silence.
940948
* max-input-size should prevent this; return the buffer empty if not. */
941949
AMediaCodec_queueInputBuffer(d->acodec, ii, 0, 0, pts_us, 0);
942950
}
943951
}
944952
drain_audio_output(d);
945-
return 0;
953+
return rc;
946954
}
947955

948956
/* ---- render thread + accessors ----------------------------------------- */

Basis/Packages/com.basis.mediaplayer/Native~/android/basis_android_vk.c

Lines changed: 45 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -96,10 +96,12 @@ struct basis_vk_present {
9696
* the YCbCr->RGB render pass, and rebuild that when Unity rotates the
9797
* underlying VkImage (rare). */
9898
void* unityNativeTex;
99-
VkImage cachedUnityImage; /* the VkImage AccessTexture returned for unityNativeTex */
99+
int unityDirty; /* handle changed on the main thread; the render thread drops the fbo */
100+
VkImage cachedUnityImage; /* the VkImage AccessTexture returned for unityNativeTex (render thread) */
100101
VkImageView unityImageView;
101102
VkFramebuffer fbo;
102-
int unityW, unityH;
103+
int fboW, fboH; /* extent the fbo was built with — render-thread-owned, distinct from unityW/H */
104+
int unityW, unityH; /* C#-registered RenderTexture size; written by the setter under v->lock */
103105
int unityFormat; /* VkFormat returned by AccessTexture (UNORM or SRGB) */
104106

105107
uint64_t frameCounter;
@@ -407,7 +409,7 @@ static void destroy_unity_fbo(basis_vk_present* v) {
407409
* or rotated under us). The image itself is OWNED BY UNITY — we never destroy
408410
* it, only the view and framebuffer we created on top. */
409411
static int ensure_unity_fbo(basis_vk_present* v, VkImage image, VkFormat format, int w, int h) {
410-
if (v->fbo && v->cachedUnityImage == image && v->unityW == w && v->unityH == h) return 0;
412+
if (v->fbo && v->cachedUnityImage == image && v->fboW == w && v->fboH == h) return 0;
411413
destroy_unity_fbo(v);
412414

413415
VkImageViewCreateInfo vci = { VK_STRUCTURE_TYPE_IMAGE_VIEW_CREATE_INFO };
@@ -434,24 +436,28 @@ static int ensure_unity_fbo(basis_vk_present* v, VkImage image, VkFormat format,
434436
if (vkCreateFramebuffer(v->device, &fci, NULL, &v->fbo) != VK_SUCCESS) return -1;
435437

436438
v->cachedUnityImage = image;
437-
v->unityW = w;
438-
v->unityH = h;
439+
v->fboW = w;
440+
v->fboH = h;
439441
v->unityFormat = (int)format;
440442
return 0;
441443
}
442444

443445
void basis_vk_set_output_texture(basis_vk_present* v, void* native_texture, int w, int h) {
444446
if (!v) return;
445-
/* If the handle changed, drop the cached framebuffer; next render_update will
446-
* rebuild against the new Unity image. We intentionally don't touch
447-
* cachedUnityImage here because AccessTexture may legitimately return the
448-
* same VkImage for a recreated RenderTexture if Unity reused the slot. */
449-
if (v->unityNativeTex != native_texture) {
450-
destroy_unity_fbo(v);
447+
/* Runs on the main thread while the render thread may be mid-present using the
448+
* framebuffer/image-view. Don't destroy them here — just record the change
449+
* under the lock and let render_update drop+rebuild the fbo on its own thread
450+
* (unityDirty). We intentionally don't touch cachedUnityImage because
451+
* AccessTexture may legitimately return the same VkImage for a recreated
452+
* RenderTexture if Unity reused the slot. */
453+
pthread_mutex_lock(&v->lock);
454+
if (v->unityNativeTex != native_texture || v->unityW != w || v->unityH != h) {
451455
v->unityNativeTex = native_texture;
456+
v->unityDirty = 1; /* a same-handle resize still needs the fbo rebuilt */
452457
}
453458
v->unityW = w;
454459
v->unityH = h;
460+
pthread_mutex_unlock(&v->lock);
455461
}
456462

457463
/* ---- plugin-owned submission objects ------------------------------------ */
@@ -494,17 +500,31 @@ static int ensure_cmd_objects(basis_vk_present* v) {
494500

495501
int basis_vk_render_update(basis_vk_present* v) {
496502
if (!v || !v->device || !v->getAHBProps) return 0;
497-
/* Without a registered Unity output texture we have nowhere to render. C# is
498-
* expected to call basis_media_set_output_texture once TryGetVideoSize
499-
* returns a non-zero size; until then the demuxer's AHBs sit in v->pending. */
500-
if (!v->unityNativeTex) return 0;
501503

504+
/* All Unity-registration state is read as one locked snapshot — no unlocked
505+
* peek at unityNativeTex first. Without a registered output texture there is
506+
* nowhere to render (C# calls basis_media_set_output_texture once
507+
* TryGetVideoSize is non-zero; until then the demuxer's AHBs sit in pending). */
502508
AHardwareBuffer* ahb = NULL; int w, h; float uv[4];
509+
void* unityTex; int unityDirty; int regW, regH;
503510
pthread_mutex_lock(&v->lock);
504-
ahb = v->pending; v->pending = NULL; w = v->w; h = v->h;
511+
unityTex = v->unityNativeTex; regW = v->unityW; regH = v->unityH;
512+
unityDirty = v->unityDirty;
513+
w = v->w; h = v->h;
505514
uv[0] = v->uv[0]; uv[1] = v->uv[1]; uv[2] = v->uv[2]; uv[3] = v->uv[3];
515+
/* Only detach the pending frame when there's a texture to render it into —
516+
* otherwise it stays queued (the producer replaces + releases it) instead of
517+
* being dropped here with its AHB reference leaked. */
518+
if (unityTex) {
519+
ahb = v->pending; v->pending = NULL;
520+
if (ahb) v->unityDirty = 0; /* consume the dirty flag only when this pass rebuilds+renders */
521+
}
506522
pthread_mutex_unlock(&v->lock);
523+
if (!unityTex) return 0;
507524
if (!ahb) return 0;
525+
/* Handle changed on the main thread: drop the old framebuffer/image-view here,
526+
* on the render thread, so nothing is destroyed under a present in flight. */
527+
if (unityDirty) destroy_unity_fbo(v);
508528

509529
/* AHB format + memory properties (drives the ycbcr conversion + allocation) */
510530
VkAndroidHardwareBufferFormatPropertiesANDROID fmtProps = { VK_STRUCTURE_TYPE_ANDROID_HARDWARE_BUFFER_FORMAT_PROPERTIES_ANDROID };
@@ -520,16 +540,20 @@ int basis_vk_render_update(basis_vk_present* v) {
520540
* it. The render pass takes the attachment from UNDEFINED and returns it
521541
* SHADER_READ_ONLY, so no layout coordination with Unity is needed. */
522542
uint64_t unityImageU64 = 0; int unityFormat = 0, unityW = 0, unityH = 0;
523-
if (!basis_gfx_vk_access_texture(v->unityNativeTex,
543+
if (!basis_gfx_vk_access_texture(unityTex,
524544
&unityImageU64, &unityFormat, &unityW, &unityH)) {
525545
AHardwareBuffer_release(ahb);
526546
return 0;
527547
}
528548
VkImage unityImage = (VkImage)(uintptr_t)unityImageU64;
529549

530-
if (ensure_unity_fbo(v, unityImage, (VkFormat)unityFormat,
531-
unityW > 0 ? unityW : w,
532-
unityH > 0 ? unityH : h) != 0) {
550+
/* One target extent for both the framebuffer and the render area, or the
551+
* render pass area can disagree with the framebuffer it renders into. Prefer
552+
* the RenderTexture's actual extent (AccessTexture), then the registered size,
553+
* then the source AHB. */
554+
int targetW = unityW > 0 ? unityW : (regW > 0 ? regW : w);
555+
int targetH = unityH > 0 ? unityH : (regH > 0 ? regH : h);
556+
if (ensure_unity_fbo(v, unityImage, (VkFormat)unityFormat, targetW, targetH) != 0) {
533557
AHardwareBuffer_release(ahb);
534558
return 0;
535559
}
@@ -572,7 +596,7 @@ int basis_vk_render_update(basis_vk_present* v) {
572596
vkCmdPipelineBarrier(cmd, VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT, VK_PIPELINE_STAGE_FRAGMENT_SHADER_BIT,
573597
0, 0, NULL, 0, NULL, 1, &toRead);
574598

575-
int rw = v->unityW, rh = v->unityH;
599+
int rw = targetW, rh = targetH;
576600
VkRenderPassBeginInfo rp = { VK_STRUCTURE_TYPE_RENDER_PASS_BEGIN_INFO };
577601
rp.renderPass = v->renderPass; rp.framebuffer = v->fbo;
578602
rp.renderArea.extent.width = (uint32_t)rw; rp.renderArea.extent.height = (uint32_t)rh;

Basis/Packages/com.basis.mediaplayer/Native~/basis_media_core.c

Lines changed: 80 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -227,6 +227,62 @@ int basis_engine_is_paused(basis_media_engine_t* e) { return e ? e->paused : 0;
227227
int basis_engine_is_running(basis_media_engine_t* e) { return e ? e->running : 0; }
228228
int basis_engine_is_paced(basis_media_engine_t* e) { return e ? e->paced : 0; }
229229

230+
/* ---- render-event liveness registry ------------------------------------
231+
* OnRenderEvent (Unity render thread) is handed the engine pointer and can fire
232+
* concurrently with basis_media_close on the main thread. C# quiesces render
233+
* events before closing, but a stale event must be a safe no-op, not a
234+
* use-after-free. Every open engine is registered here; basis_engine_render_event
235+
* dispatches under g_registry_lock only while the engine is still registered, and
236+
* close removes it under the same lock — waiting out any in-flight event — before
237+
* it frees the decoder and engine.
238+
*
239+
* Engines are keyed by pointer, so this stops a dispatch against a freed engine but
240+
* not the narrow ABA case where a delayed event's pointer matches a *new* engine
241+
* that reused the freed address. For the shipping C# binding that is benign: it
242+
* issues only RENDER_UPDATE (idempotent — republishes the current frame). But
243+
* RENDER_RELEASE is part of the public render-event ABI, and a caller that delivers
244+
* one across a close+reopen could tear down the reused engine's decoder — so this
245+
* registry's ABA-safety is only as strong as "no RELEASE is delivered after close."
246+
* Closing the window fully needs a generation-stamped handle in the event payload
247+
* (a C# ABI change) — deliberately out of scope here. */
248+
#define BASIS_MAX_ENGINES 64
249+
static basis_mutex_t g_registry_lock;
250+
static int g_registry_ready;
251+
static basis_media_engine_t* g_engines[BASIS_MAX_ENGINES];
252+
253+
/* opens run on Unity's main thread, so first-use init needs no extra guard. */
254+
static void registry_ensure(void) {
255+
if (!g_registry_ready) { mutex_init(&g_registry_lock); g_registry_ready = 1; }
256+
}
257+
static int registry_add(basis_media_engine_t* e) {
258+
registry_ensure();
259+
int ok = 0;
260+
mutex_lock(&g_registry_lock);
261+
for (int i = 0; i < BASIS_MAX_ENGINES; ++i) if (!g_engines[i]) { g_engines[i] = e; ok = 1; break; }
262+
mutex_unlock(&g_registry_lock);
263+
return ok; /* 0 => registry full */
264+
}
265+
static void registry_remove(basis_media_engine_t* e) {
266+
if (!g_registry_ready) return;
267+
mutex_lock(&g_registry_lock);
268+
for (int i = 0; i < BASIS_MAX_ENGINES; ++i) if (g_engines[i] == e) { g_engines[i] = NULL; break; }
269+
mutex_unlock(&g_registry_lock);
270+
}
271+
272+
void basis_engine_render_event(basis_media_engine_t* e, int event_id) {
273+
if (!e || !g_registry_ready) return;
274+
mutex_lock(&g_registry_lock);
275+
int live = 0;
276+
for (int i = 0; i < BASIS_MAX_ENGINES; ++i) if (g_engines[i] == e) { live = 1; break; }
277+
/* Dispatch under the lock so registry_remove (in close) blocks until this
278+
* returns — the decoder can't be freed while a render event is using it. */
279+
if (live && e->decoder) {
280+
if (event_id == BASIS_RENDER_UPDATE) basis_decoder_render_update(e->decoder);
281+
else if (event_id == BASIS_RENDER_RELEASE) basis_decoder_render_release(e->decoder);
282+
}
283+
mutex_unlock(&g_registry_lock);
284+
}
285+
230286
/* Real-time delivery pacing. Blocks the demux thread so an access unit is handed to the
231287
* decoder no more than BASIS_PACE_LEAD_US ahead of a fixed 1x clock anchored to the first
232288
* AU — stalling the socket read (TCP backpressure) so a faster-than-real-time source can't
@@ -1177,6 +1233,22 @@ static basis_media_engine_t* open_impl(const char* url, const char* audio_url, i
11771233
}
11781234
if (has_audio) e->audio_thread_started = 1;
11791235

1236+
/* Live now: the pointer is about to reach C#, which may issue render events.
1237+
* Registered last so no partially-built engine is ever visible to a dispatch.
1238+
* If the registry is full (too many concurrent players), fail cleanly rather
1239+
* than hand back an engine whose render events would be silently ignored. */
1240+
if (!registry_add(e)) {
1241+
e->running = 0;
1242+
thread_join(e);
1243+
audio_thread_join(e);
1244+
basis_decoder_destroy(e->decoder);
1245+
basis_io_global_shutdown();
1246+
basis_caption_destroy(e->captions);
1247+
mutex_destroy(&e->submit_lock);
1248+
mutex_destroy(&e->lock);
1249+
free(e);
1250+
return NULL;
1251+
}
11801252
return e;
11811253
}
11821254

@@ -1195,8 +1267,14 @@ BASIS_API basis_media_engine_t* BASIS_CALL basis_media_open_dual(const char* vid
11951267
BASIS_API void BASIS_CALL basis_media_close(basis_media_engine_t* e) {
11961268
if (!e) return;
11971269

1198-
/* Stop the demux threads first so nothing submits while we tear down. Both
1199-
* legs observe the same running flag; join both before freeing the decoder. */
1270+
/* Deregister first, before anything is torn down: this blocks until any
1271+
* in-flight render event returns and makes every later one a no-op, so no
1272+
* render callback can touch the decoder while the demux threads are still
1273+
* exiting or the decoder is being freed. */
1274+
registry_remove(e);
1275+
1276+
/* Stop the demux threads so nothing submits while we tear down. Both legs
1277+
* observe the same running flag; join both before freeing the decoder. */
12001278
e->running = 0;
12011279
thread_join(e);
12021280
audio_thread_join(e);

Basis/Packages/com.basis.mediaplayer/Native~/basis_media_internal.h

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -228,6 +228,12 @@ void basis_engine_set_state(basis_media_engine_t* engine, basis_media_sta
228228
void basis_engine_set_error(basis_media_engine_t* engine, const char* message);
229229
basis_decoder_t* basis_engine_get_decoder(basis_media_engine_t* engine);
230230

231+
/* Render-thread entry point (the Unity plugin's OnRenderEvent forwards here). The
232+
* engine pointer comes from Unity and can arrive after basis_media_close has freed
233+
* it; this checks a liveness registry under a lock and no-ops on a stale engine,
234+
* so a late render event can't use-after-free. event_id is a BASIS_RENDER_* value. */
235+
void basis_engine_render_event(basis_media_engine_t* engine, int event_id);
236+
231237
/* Consulted by the platform backend: paused freezes video publishing and mutes
232238
* audio reads; running going to 0 tells decode/demux loops to unwind. */
233239
int basis_engine_is_paused(basis_media_engine_t* engine);

0 commit comments

Comments
 (0)