Commit b320866
NativeEngine: fix use-after-free in async texture upload mip loop (#1758)
## Problem
`NativeEngine::LoadTextureFromImage` (and the single-mip-per-face branch
of
`LoadCubeTextureFromImages`) drove the mip-upload loop with
`for (uint8_t mip = 0; mip < image->m_numMips; ++mip)`, re-reading
`image->m_numMips` for the exit condition on every iteration.
On the **last** iteration the mip is submitted to bgfx via
`bgfx::makeRef(imageMip.m_data, imageMip.m_size, releaseFn, image)`,
where
`releaseFn` calls `bimg::imageFree(image)`. bgfx invokes that release
once it
has consumed the memory — which happens on the render thread inside
`bgfx::frame()`, potentially **immediately** after
`Update2D`/`UpdateCube`
submits the command. The loop then evaluates `mip < image->m_numMips` by
dereferencing `image` *after it may already have been freed*. If the
stale
`m_numMips` read happens to be greater than `mip`, the loop re-enters
`bimg::imageGetRawData(*image, ...)` on the freed container; its garbage
`m_format` indexes `s_imageBlockInfo` out of bounds → an intermittent
access
violation.
This is a **use-after-free (CWE-416)**, reproduced as a rare (~2–4%)
crash on
texture-loading validation tests (e.g. an NME particle test that loads a
ramp
texture while the render loop is active).
### Not a bgfx threading bug
bgfx resource creation/update **are** mutex-guarded
(`m_resourceApiLock`, which
`bgfx::frame()` also holds), so calling them from the decode worker
thread is
safe and supported. The defect is purely that BabylonNative keeps
dereferencing `image` after handing its ownership to bgfx's asynchronous
release queue.
## Fix
Cache `image->m_numMips` in the loop initializer
(`for (uint8_t mip = 0, numMips = image->m_numMips; mip < numMips;
++mip)`) so
the loop bound is no longer re-read from `image` after the freeing
submit.
This restores **verbatim** the change made in
[`e8ee5f7`](e8ee5f7)
(#1628) — see Regression history below.
## Regression history
This is a **re-fix of a previously-fixed bug**. The exact same race was
fixed
in April by commit
[`e8ee5f7`](e8ee5f7)
("Fixed race condition when passing data to bgfx. Issue #1398", #1628),
which
changed both loops to `for (uint8_t mip = 0, numMips = image->m_numMips;
...)`.
That fix was inadvertently reverted by commit
[`6bb8014`](6bb8014)
("Reworked threading model.", #1652), which restored the original
`for (uint8_t mip = 0; mip < image->m_numMips; ++mip)` form in both
`LoadTextureFromImage` and `LoadCubeTextureFromImages`, reintroducing
the
use-after-free. This PR re-applies the original fix unchanged.
## Validation
- Stress-tested an affected texture-loading validation test **3000×
headless**
(30 × 100, 5-way concurrent for extra scheduler pressure): **0 crashes**
(baseline crashes ~8% / ~1-in-12). At that baseline rate P(0 in 3000) is
effectively zero, so the use-after-free is eliminated.
- Particle validation subset: no regressions.
Co-authored-by: Branimir Karadzic <branimirkaradzic@gmail.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>1 parent 53a0432 commit b320866
1 file changed
Lines changed: 2 additions & 2 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
323 | 323 | | |
324 | 324 | | |
325 | 325 | | |
326 | | - | |
| 326 | + | |
327 | 327 | | |
328 | 328 | | |
329 | 329 | | |
| |||
392 | 392 | | |
393 | 393 | | |
394 | 394 | | |
395 | | - | |
| 395 | + | |
396 | 396 | | |
397 | 397 | | |
398 | 398 | | |
| |||
0 commit comments