fix(mediaplayer): RIST SSRF address gate + req_append format attribute (#997)

## Summary
This hardens the media player's native core against two issues found in
a security review of the demux/transport layer.

**1. RIST transport SSRF gate.** Every other network transport (HTTP,
HLS, RTSP, RTMP) reaches its socket through `basis_io`, which refuses a
non-global-unicast target — loopback, RFC1918, link-local, CGNAT, ULA —
by re-checking the *resolved* address at connect time. The RIST path
handed the host straight to librist, which owns its own UDP sockets and
resolves independently, so that gate never ran. Because any peer can
push a media URL to every client via the world sync protocol, a
`rist://` URL could open an outbound flow to a private or loopback
address, including via DNS rebinding (public at validation, private by
the time librist connects).

The fix adds `basis_io_resolve_checked()` to the SSRF module: it
resolves the host once, applies the same address policy
`basis_io_connect` uses, and returns the vetted numeric literal.
`basis_rist_open` pins librist to that literal (`peer_cfg->address` +
`address_family`, carrying the URL port across via `physical_port` —
librist's manual-socket path reads the port from that field, not the
address string) so it cannot re-resolve to a different address between
the check and its connect, closing the rebind window. Unresolvable or
blocked hosts fail closed.

Only RIST needed this. The Android JNI and Windows WinHTTP sources have
the same "an external library owns the socket" shape, but they can't be
pinned to a resolved IP without breaking TLS SNI and
certificate-hostname validation, so their host-string check — backed by
the connect-time re-check on the `basis_io` paths — is correct as-is.
RIST is Main-Profile PSK-AES (no hostname certificate), so pinning to a
literal breaks nothing. This is gated behind the opt-in
`-DBASIS_WITH_RIST=ON` build; the shipped Windows x64 and Android arm64
binaries carry it.

**2. `req_append` printf format attribute.** The RTSP request builder
forwards its format string to `vsnprintf` but carried no
`format(printf)` attribute, so `-Wformat-security` (already enabled in
the build) was inert for its call sites — a future format/argument
mismatch on this remote-facing path would have compiled clean. Tagged it
via a `BASIS_PRINTF_FMT` macro that expands to nothing on compilers
without the attribute. No behaviour change; all current call sites
already pass matching literals.

All four platform plugins were rebuilt (Windows x64/ARM64, Linux x86_64,
Android arm64-v8a), and the testing guide gained a RIST host-SSRF lane.

## 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
- [x] Android
- [ ] iOS
- [ ] macOS

Input / control mode coverage:

- [x] Tested in VR (note headset under **Notes**)
- [x] Tested in desktop / non-VR mode
- [ ] Tested with phone controls (mobile touch input)
- [ ] 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 native C/C++ plugin code (`Native~/`) with no Unity C# surface,
so the Unity-specific required checks (transform access, Addressables,
GetComponent, BasisEventDriver, jobification, properties, camera driver,
BasisDebug, scene discovery, hot-path allocation/logging/collection) do
not apply — ticked as N/A per the instruction above.

**Tested** on Windows (Unity Editor) and Android (Quest Pro, built
player). All four plugins compile clean (only the pre-existing MSVC
C4996 CRT-deprecation warnings; no format-mismatch warning, confirming
the printf attribute is active). Playback confirmed against a live test
rig, verified both visually and through the player's diagnostic CSV
(drop/skip/format-error/audio counters all zero):

- **Editor:** RIST plain + AES, RTSP (two sources), and progressive MP4
with forward/backward seeks — all clean.
- **Quest Pro (built APK):** RIST plain, RTSP, and progressive MP4 with
forward/backward seeks — all clean; the native lib in the APK was
verified byte-identical to the committed build.

Linux and Windows ARM64 are RIST-off stub builds, so the RIST change
compiles out there; they were rebuilt and link-checked but not run in a
player (no runtime behaviour change reaches them beyond the printf
attribute).

Reachability note for the RIST fix: a plain `rist://<private>` URL is
refused by the managed (C#) gate before native runs, since a `rist://`
host is the entry URL, so the native guard is a DNS-rebind backstop
rather than a first line of defence. Confirming the refusal end-to-end
needs a rebind fixture (a name answering public then private); the
device passes above confirm the fix didn't regress normal RIST playback,
which was the live risk in pinning to a resolved address.

Author: dooly123

Basis/Packages/com.basis.mediaplayer/Native~/protocol/basis_io.c

@@ -234,6 +234,43 @@ int basis_io_host_is_blocked(const char* host) {
     return blocked;
 }
 
+int basis_io_resolve_checked(const char* host, char* out_ip, int out_cap, int* out_family) {
+    if (!host || !host[0] || !out_ip || out_cap <= 0) return -1;
+
+    /* Same bracket handling as basis_io_host_is_blocked: a URL authority hands an
+     * IPv6 literal over in brackets, but getaddrinfo wants it bare. */
+    char bare[256];
+    size_t hl = strlen(host);
+    if (host[0] == '[') {
+        if (hl < 3 || host[hl - 1] != ']' || hl - 2 >= sizeof(bare)) return -1;
+        memcpy(bare, host + 1, hl - 2);
+        bare[hl - 2] = 0;
+        host = bare;
+    }
+
+    struct addrinfo hints, *res = NULL, *ai;
+    memset(&hints, 0, sizeof(hints));
+    hints.ai_family = AF_UNSPEC;
+    hints.ai_socktype = SOCK_DGRAM;
+    if (getaddrinfo(host, NULL, &hints, &res) != 0 || !res) return -1;
+
+    /* Pick the first allowed address, exactly as basis_io_connect does: skip a
+     * non-global one rather than blocking the whole name, since pinning to the
+     * chosen literal means the skipped address can never be reached. */
+    int allow_local = local_allowed();
+    int rc = -1;
+    for (ai = res; ai; ai = ai->ai_next) {
+        if (!allow_local && sockaddr_is_blocked(ai->ai_addr)) continue;
+        if (getnameinfo(ai->ai_addr, (socklen_t)ai->ai_addrlen, out_ip, (socklen_t)out_cap,
+                        NULL, 0, NI_NUMERICHOST) != 0) continue;
+        if (out_family) *out_family = ai->ai_family;
+        rc = 0;
+        break;
+    }
+    freeaddrinfo(res);
+    return rc;
+}
+
 basis_io_t* basis_io_connect(const char* host, int port, int timeout_ms) {
     if (!host || port <= 0) return NULL;
 

Basis/Packages/com.basis.mediaplayer/Native~/protocol/basis_io.h

@@ -78,6 +78,18 @@ int basis_io_poll_read(basis_io_t** ios, int n, int timeout_ms);
  * targets are attempted, not which address a later lookup returns. */
 int basis_io_host_is_blocked(const char* host);
 
+/* Resolve `host` to a single vetted numeric address literal for a caller that owns
+ * its own sockets (e.g. librist) and would otherwise re-resolve the name at connect
+ * time. Applies the same non-global-unicast guard basis_io_connect uses to the
+ * resolved addresses and writes the first allowed one, in numeric form, to `out_ip`
+ * (at most out_cap bytes incl. the terminator) plus its address family to
+ * *out_family. Returns 0 on success; -1 if the host is empty, unresolvable, or every
+ * resolved address is blocked (fail-closed). Pinning the caller to this literal
+ * closes the DNS-rebind window that basis_io_host_is_blocked alone leaves open.
+ * Honours the same BASIS_MEDIA_ALLOW_LOCAL escape hatch; accepts a bare or
+ * bracketed IPv6 literal like basis_io_host_is_blocked. */
+int basis_io_resolve_checked(const char* host, char* out_ip, int out_cap, int* out_family);
+
 /* Process-wide one-time init/teardown (WSAStartup on Windows; no-op elsewhere). */
 void basis_io_global_init(void);
 void basis_io_global_shutdown(void);

Basis/Packages/com.basis.mediaplayer/Native~/protocol/basis_rist.c

@@ -19,6 +19,7 @@
 #include <librist/receiver.h>
 #include <librist/peer.h>
 #include <librist/logging.h>
+#include "basis_io.h"       /* basis_io_resolve_checked (SSRF guard) */
 #include <stdlib.h>
 #include <string.h>
 #include <stdio.h>
@@ -119,6 +120,20 @@ void* basis_rist_open(const basis_url_t* parts, basis_media_sink_t* sink) {
     c->sink = sink;
     if (ring_init(&c->ring, BASIS_RIST_RING_BYTES) != 0) { free(c); return NULL; }
 
+    /* librist owns its own sockets and re-resolves the host at connect time, so the
+     * basis_io SSRF gate never runs on this path. Resolve and vet the host here, then
+     * pin librist to the validated literal below so it cannot resolve to a different
+     * (private) address between this check and its connect. Fail closed on a blocked
+     * or unresolvable host, the same policy basis_io_connect applies. */
+    char vetted_ip[64];
+    int vetted_family = 0;
+    if (basis_io_resolve_checked(parts->host, vetted_ip, (int)sizeof(vetted_ip), &vetted_family) != 0) {
+        if (sink && sink->on_error)
+            sink->on_error(sink->user, "RIST: refused blocked or unresolvable host.");
+        basis_rist_close(c);
+        return NULL;
+    }
+
     /* Reconstruct the rist:// URL for librist's parser. It wants
      * "rist://host:port[?query]" with NO path — a trailing '/' makes it mis-parse the
      * host/port entirely. parts->path holds the query (with any leading '/'), so pass
@@ -154,6 +169,16 @@ void* basis_rist_open(const basis_url_t* parts, basis_media_sink_t* sink) {
     }
     peer_cfg->initiate_conn = 1;  /* caller: open an outbound flow to the broadcaster */
 
+    /* Pin librist to the address vetted above. Setting address_family routes librist
+     * onto its manual-sockdata path, which treats peer_cfg->address as a literal (no
+     * re-resolution, closing the window between the SSRF check and librist's connect)
+     * but takes the port from peer_cfg->physical_port rather than the address string —
+     * and rist_parse_address2 never populates physical_port. Carry the URL port across
+     * explicitly, or librist resolves the literal against port 0 and sends nowhere. */
+    snprintf(peer_cfg->address, sizeof(peer_cfg->address), "%s", vetted_ip);
+    peer_cfg->address_family = vetted_family;
+    peer_cfg->physical_port = (uint16_t)parts->port;
+
     struct rist_peer* peer = NULL;
     int peer_rc = rist_peer_create(c->receiver, &peer, peer_cfg);
     rist_peer_config_free2(&peer_cfg);

Basis/Packages/com.basis.mediaplayer/Native~/protocol/basis_rtsp.c

@@ -37,6 +37,15 @@
   #include <time.h>
 #endif
 
+/* -Wformat-security only checks call sites of functions it knows are printf-like,
+ * i.e. libc calls and locals carrying this attribute. Tag our vsnprintf wrapper so
+ * the checker covers it too; expands to nothing where the attribute is unsupported. */
+#if defined(__GNUC__) || defined(__clang__)
+#  define BASIS_PRINTF_FMT(fmt_idx, va_idx) __attribute__((format(printf, fmt_idx, va_idx)))
+#else
+#  define BASIS_PRINTF_FMT(fmt_idx, va_idx)
+#endif
+
 /* UDP transport tuning. The no-data deadlines are deliberately snappy: a
  * false fallback lands on TCP-interleaved, which works wherever UDP does, so
  * over-triggering costs nothing observable while under-triggering stalls the
@@ -114,6 +123,7 @@ typedef struct {
  * would then write out of bounds. The field sizes today keep the total under
  * 2 KiB, so this is a guard on the arithmetic rather than a fix for a reachable
  * overflow; it stops being safe the moment any of the inputs grows. */
+static int req_append(char* req, size_t cap, int* n, const char* fmt, ...) BASIS_PRINTF_FMT(4, 5);
 static int req_append(char* req, size_t cap, int* n, const char* fmt, ...) {
     if (*n < 0 || (size_t)*n >= cap) return -1;
     va_list ap;

Basis/Packages/com.basis.mediaplayer/Plugins/Android/arm64-v8a/libbasis_media_native.so

@@ -445,24 +445,39 @@ https→http case the row above requires to be refused. An `http`→`https` **up
 is worth checking separately; only the downgrade is refused. A self-redirect must terminate rather
 than spin.
 
-**Android's OS-extractor leg is inside this matrix, and the fixture extension decides whether you
-are testing it.** On Android a URL that is not `.m3u8`, `.m2ts` or `.mts` goes to `AMediaExtractor`
-first, and only falls through to the JNI source plus the portable demuxers if the extractor declines
-it. The extractor reads through the JNI source rather than fetching the URL itself, so the hop loop
-covers both legs — but they are different code, and a `.ts` fixture only ever exercises the fallback.
-Run at least the loopback, DNS-to-loopback and downgrade rows against an `.mp4` fixture on Android
-so the extractor leg is the one under test, and record which extension each row used. The client-side
-gate is a second reason to be careful here: `BasisMediaUrlRouter.IsDirectlyPlayable` requires the URI
-path to end in a media extension once the query is stripped, so an extensionless redirect fixture is
-rejected in C# and never reaches native at all.
-
-**Android extractor playback and seek** — the extractor's byte source is the JNI HTTP source, so any
-change to either wants a plain regression pass behind it: play a large progressive `.mp4` over
-https, seek forwards and backwards several times, and confirm the position tracks and audio stays in
-sync. A source that cannot be re-requested by range is declined up front and falls through to the
-portable demuxers, so a fixture served without `Accept-Ranges` should still play — just not through
-the extractor. Worth confirming both outcomes rather than only the happy one, since a regression that
-silently pushes everything down the fallback path looks identical from the sofa.
+**RIST host SSRF** (opt-in `-DBASIS_WITH_RIST=ON` build only) — librist opens and resolves its own
+UDP sockets, so the transport sits outside the `basis_io` connect-time guard the other lanes share.
+`basis_rist_open` closes that by resolving and vetting the host itself and pinning librist to the
+validated address literal. The subtlety when testing it: a `rist://` host is the **entry** URL, so
+the C# gate (`BasisMediaPlayerSecurity`) already refuses a literal private target or a hostname that
+resolves to a private address before native runs — a plain `rist://192.168.x.x` never reaches
+`basis_rist_open` at all, unlike the HLS/redirect lanes where the private target hides in a
+sub-resource the C# gate never sees. The native guard is therefore a rebind backstop, exercised only
+by a target that passes the C# check but is private by the time native resolves: a DNS-rebinding
+fixture whose name answers a public address first and a private one on the next lookup. Point librist
+at it and watch the private listener — it must see **no UDP at all**. `BASIS_MEDIA_ALLOW_LOCAL` is
+not a way in here: it does not relax the C# gate, so it cannot carry a private literal through to
+native. Where no rebind fixture is available, exercise `basis_io_resolve_checked` directly against
+the loopback/RFC1918/link-local set and record that the stream lane was skipped. This whole lane
+only applies to the RIST-enabled build: in the default build `rist://` still passes the scheme
+allowlist (it is listed there), reaches native, and is declined by the stub `basis_rist_open` with
+a clear "RIST is not built into this plugin — rebuild with `-DBASIS_WITH_RIST=ON`" error on the sink;
+playback fails and the SSRF guard above does not exist in that build.
+
+**The client-side extension gate shapes which redirect fixtures reach native.**
+`BasisMediaUrlRouter.IsDirectlyPlayable` requires the URL path to end in a media extension once the
+query is stripped, so an extensionless redirect fixture is rejected in C# and never reaches the
+native source at all — give the fixture a real media extension on its final path when exercising the
+redirect rows. On Android there is no OS-extractor leg: MP4/WebM demux through the same portable path
+as Windows, fed by the JNI HTTP source, so the extension you pick selects the container under test,
+not a separate code path, and the SSRF hop loop is shared across all of them.
+
+**Android progressive playback and seek** — MP4/WebM demux through the portable path fed by the JNI
+HTTP source, so any change to either wants a plain regression pass behind it: play a large
+progressive `.mp4` over https, seek forwards and backwards several times, and confirm the position
+tracks and audio stays in sync. Use a byte-range server (`206` + a valid `Content-Range`) for the
+seek path; the no-`Accept-Ranges` and trailing-moov cases carry their own expectations in the
+**Trailing-moov progressive MP4** row above.
 
 **A/V sync judgement** — use real footage with **visible speech**; synthetic patterns hide sync
 drift, and Big Buck Bunny has no dialogue at all. A CC-BY Blender open