Otherwise the CS might later try to unbind a resource that has already been freed.
Signed-off-by: Zebediah Figura z.figura12@gmail.com --- dlls/wined3d/device.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/dlls/wined3d/device.c b/dlls/wined3d/device.c index 92259e3ea5b..0d92177cd0c 100644 --- a/dlls/wined3d/device.c +++ b/dlls/wined3d/device.c @@ -1142,6 +1142,7 @@ void wined3d_device_uninit_3d(struct wined3d_device *device) wined3d_texture_decref(texture); }
+ wined3d_cs_emit_reset_state(device->cs); state_unbind_resources(&device->state); for (i = 0; i < device->adapter->d3d_info.limits.max_rt_count; ++i) {
On Sun, 15 Sep 2019 at 20:09, Zebediah Figura z.figura12@gmail.com wrote:
Otherwise the CS might later try to unbind a resource that has already been freed.
How can you get there? Generally speaking destruction goes through the CS as well.
On 09/16/2019 02:44 AM, Henri Verbeet wrote:
On Sun, 15 Sep 2019 at 20:09, Zebediah Figura z.figura12@gmail.com wrote:
Otherwise the CS might later try to unbind a resource that has already been freed.
How can you get there? Generally speaking destruction goes through the CS as well.
It's been a while since I wrote the patch, so I have to remind myself why I did...
The primary reason is that destruction itself goes through the CS, but the CS state is still holding pointers to those resources, so a later call to something like wined3d_cs_exec_set_index_buffer() would try to decrease the bind_count of an already freed resource.
The specific way this manifests is that a ddraw test will call wined3d_device_set_index_buffer(), then later the device gets destroyed and calls ddraw_set_cooperative_level() as a result. That first destroys the swapchain, calling wined3d_device_uninit_3d(), and then restores a stateblock, which sets the index buffer to NULL, and causes the CS thread to perform the use-after-free mentioned above.
It's not a problem currently because the stateblock that ddraw is capturing (as well as the device->stateblock_state directly) is holding an extra reference to the index buffer. In my patches to move the stateblock tracking I didn't preserve this reference in the client-side primary stateblock, and so it doesn't get captured either. I could do so (and, if we want to get rid of wined3d_device_get_*() functions—which I'm assuming we want to do?—I should), but it also seems wrong to the naïve reader that the CS is relying on something that's not the actual device state to keep alive things it's referencing.
On the other hand, I also don't know why state_unbind_resources specifically is called there [as opposed to nothing at all, or state_cleanup()]. Could you by chance provide an explanation?
ἔρρωσο
On Thu, 19 Sep 2019 at 08:34, Zebediah Figura z.figura12@gmail.com wrote:
The primary reason is that destruction itself goes through the CS, but the CS state is still holding pointers to those resources, so a later call to something like wined3d_cs_exec_set_index_buffer() would try to decrease the bind_count of an already freed resource.
The specific way this manifests is that a ddraw test will call wined3d_device_set_index_buffer(), then later the device gets destroyed and calls ddraw_set_cooperative_level() as a result. That first destroys the swapchain, calling wined3d_device_uninit_3d(), and then restores a stateblock, which sets the index buffer to NULL, and causes the CS thread to perform the use-after-free mentioned above.
It's not a problem currently because the stateblock that ddraw is capturing (as well as the device->stateblock_state directly) is holding an extra reference to the index buffer. In my patches to move the stateblock tracking I didn't preserve this reference in the client-side primary stateblock, and so it doesn't get captured either. I could do so (and, if we want to get rid of wined3d_device_get_*() functions—which I'm assuming we want to do?—I should), but it also seems wrong to the naïve reader that the CS is relying on something that's not the actual device state to keep alive things it's referencing.
That makes sense.
On the other hand, I also don't know why state_unbind_resources specifically is called there [as opposed to nothing at all, or state_cleanup()]. Could you by chance provide an explanation?
I'm not sure there's an especially good reason. In fact, when you add the wined3d_cs_emit_reset_state() call, you may need state_cleanup() instead of state_unbind_resources(). The code in question is fairly messy and in need of some cleanups and restructuring. You've probably also noticed that there's a fair amount of duplication with e.g. wined3d_device_reset(). Unfortunately it's also fairly sensitive code. Things need to be torn down in the correct order, since e.g. context acquisition depends on having a swapchain, and depending on the D3D version and the application, either the device or the swapchain may be released first.
-
Henri Verbeet -
Zebediah Figura