[Bug 59593] New: d3d12_desc_mark_as_modified silently drops dirty descriptors and races with flush

[WineHQ Bugzilla]

http://bugs.winehq.org/show_bug.cgi?id=59593

            Bug ID: 59593
           Summary: d3d12_desc_mark_as_modified silently drops dirty
                    descriptors and races with flush
           Product: vkd3d
           Version: 1.19
          Hardware: x86-64
                OS: Linux
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: vkd3d
          Assignee: [e-mail]
          Reporter: [e-mail]
      Distribution: ---

Hi team,

I was exploring the vkd3d descriptor management code and found what appear to
be correctness issues in d3d12_desc_mark_as_modified. I wanted to bring them to
your attention.

Bug #1: Silent Descriptor Drops

In d3d12_desc_mark_as_modified:

```c
if (!vkd3d_atomic_compare_exchange_u32(&dst->next, 0, (head << 1) | 1))
    return;  // Descriptor dirty but not added to list
```

If the CAS fails because dst->next is already non-zero (descriptor already
dirty), the function returns without adding the descriptor to the dirty list.
However, the descriptor needs to be flushed. This can result in stale Vulkan
descriptors and visual corruption.

Bug #2: Race Between Mark and Flush

During flush, d3d12_desc_flush_vk_heap_updates_locked sets dirty_list_head =
UINT_MAX. Marking threads may read this value and compute:

```c
vkd3d_atomic_store_u32(&dst->next, (head << 1) | 1);  // head = UINT_MAX
```

This creates corrupted next pointers, leading to lost descriptors or crashes.

Bug #3: Memory Leak

When Bug #1 occurs, the object reference obtained via d3d12_desc_get_object_ref
during the copy is never released, causing reference count leaks and gradual
memory exhaustion.

Hope this will be helpful...

-- 
Do not reply to this email, post in Bugzilla using the
above URL to reply.
You are receiving this mail because:
You are watching all bug changes.

[Bug 59593] d3d12_desc_mark_as_modified silently drops dirty descriptors and races with flush

[WineHQ Bugzilla]

http://bugs.winehq.org/show_bug.cgi?id=59593

--- Comment #1 from Henri Verbeet <[e-mail]> ---
Hi, thanks for the report. I'm afraid I'm not the original author of this code,
so apologies in advance if I'm asking questions that seem obvious.

(In reply to Arun from comment #0)
> Bug #1: Silent Descriptor Drops
>
> In d3d12_desc_mark_as_modified:
>
> ```c
> if (!vkd3d_atomic_compare_exchange_u32(&dst->next, 0, (head << 1) | 1))
>     return;  // Descriptor dirty but not added to list
> ```
>
> If the CAS fails because dst->next is already non-zero (descriptor already
> dirty), the function returns without adding the descriptor to the dirty
> list. However, the descriptor needs to be flushed. This can result in stale
> Vulkan descriptors and visual corruption.

Doesn't "next" being non-zero imply the descriptor is (effectively) already on
the "descriptor_heap->dirty_list_head" list, so it will get flushed the next
time d3d12_desc_flush_vk_heap_updates_locked() runs?
> Bug #2: Race Between Mark and Flush
>
> During flush, d3d12_desc_flush_vk_heap_updates_locked sets dirty_list_head =
> UINT_MAX. Marking threads may read this value and compute:
>
> ```c
> vkd3d_atomic_store_u32(&dst->next, (head << 1) | 1);  // head = UINT_MAX
> ```
>
> This creates corrupted next pointers, leading to lost descriptors or crashes.

Did you mean the vkd3d_atomic_exchange_u32() call in
d3d12_desc_mark_as_modified() here? vkd3d doesn't currently have a
vkd3d_atomic_store_u32() function.

Assuming that's the case, I'm not sure I see the issue? "dst->next" will get
set to UINT_MAX, but that's fine; UINT_MAX is used as the end-of-list marker,
and d3d12_desc_flush_vk_heap_updates_locked() checks for that. (One of the less
obvious parts is perhaps that the "next = (int)next >> 1;" line depends on the
int cast to preserve UINT_MAX values.)
> Bug #3: Memory Leak
>
> When Bug #1 occurs, the object reference obtained via
> d3d12_desc_get_object_ref during the copy is never released, causing
> reference count leaks and gradual memory exhaustion.

I think that reference is supposed to be released by the next
d3d12_desc_replace() call. (Which may be called from d3d12_desc_destroy() when
the entire heap is released.)

-- 
Do not reply to this email, post in Bugzilla using the
above URL to reply.
You are receiving this mail because:
You are watching all bug changes.