[Metal] Descriptor set layout bindings out-of-bounds access and unsorted bindings invariant violation
- Dominant language
- C++
- Stars
- 20.5k
- Forks
- 2.3k
- Avg merge
- 2d 13h
- Merged PRs (30d)
- 90
Description
**Describe the bug**
There are two related descriptor set handling issues in the Metal backend when handling sparse/high binding indices and unsorted bindings:
1. **Out-of-bounds indexing in `MetalDriver::updateDescriptorSetBuffer`**:
In `filament/backend/src/metal/MetalDriver.mm`:
```cpp
ShaderStageFlags stageFlags = descriptorSet->layout->getBindings()[binding].stageFlags;
```
`descriptorSet->layout->getBindings()` returns `mLayout.descriptors`, which is an array with size equal to the descriptor count (e.g. 3 bindings). Indexing directly by `[binding]` assumes contiguous 0-based binding indices matching array indices. When a descriptor set has sparse or high binding indices (e.g. binding 32 or 63), `getBindings()[binding]` accesses out of bounds, causing memory corruption or crash.
In contrast, `MetalDriver::updateDescriptorSetTexture` correctly looks up the binding via `std::find_if`:
```cpp
auto const& bindings = descriptorSet->layout->getBindings();
auto found = std::find_if(bindings.begin(), bindings.end(),
[binding](const auto& b) { return b.binding == binding; });
```
2. **Binding sort invariant trapped inside `#if FILAMENT_METAL_DEBUG_LOG == 1`**:
In `filament/backend/src/metal/MetalHandles.mm`:
```cpp
// Important! The bindings must be sorted by binding number. This has already been done inside
// createDescriptorSetLayout.
```
However, in `filament/backend/src/metal/MetalDriver.mm`:
```cpp
void MetalDriver::createDescriptorSetLayoutR(...) {
#if FILAMENT_METAL_DEBUG_LOG == 1
...
std::sort(info.descriptors.begin(), info.descriptors.end(),
[](const auto& a, const auto& b) { return a.binding < b.binding; });
...
#endif
construct_handle(dslh, std::move(info));
}
```
`std::sort` was accidentally placed inside the `FILAMENT_METAL_DEBUG_LOG == 1` block. In standard release and debug builds, `FILAMENT_METAL_DEBUG_LOG` is 0, so bindings are never sorted. This violates the assumption in `MetalDescriptorSetLayout::getArgumentEncoderSlow` and can cause mismatched texture type indexing and broken argument encoder cache keys.
**To Reproduce**
Steps to reproduce the behavior:
1. Create a `DescriptorSetLayout` with sparse/high binding indices, e.g. binding 0 and binding 32.
2. Create a descriptor set from the layout.
3. Call `updateDescriptorSetBuffer` on binding 32.
4. Observe out-of-bounds vector access in `updateDescriptorSetBuffer`.
**Expected behavior**
- `updateDescriptorSetBuffer` should locate the binding descriptor via lookup (`std::find_if`) rather than directly indexing `getBindings()[binding]`.
- `MetalDescriptorSetLayout` or `createDescriptorSetLayoutR` should unconditionally sort `descriptors` by binding number regardless of debug logging macros.
**Desktop (please complete the following information):**
- OS: macOS / iOS
- Backend: Metal
**Additional context**
Discovered while auditing descriptor set binding index limits and writing cross-backend tests.
Contributor guide
Assessment
This issue has not been assessed yet.