From 801f47a31ae97fd891d8242b7265f9ab7e97efd9 Mon Sep 17 00:00:00 2001 From: baldurk Date: Thu, 2 Feb 2017 12:28:34 +0000 Subject: [PATCH] Fix crashes with redundant changes or 0 refcount binds. Refs #23, #503 * Two potential crashes here - one from the previous fix to #503, we would double-release a refcount on an object that was set for write. First we'd decrement the refcount when it was 'unbound', then again trying to unbind it for write because the slot wasn't NULL'd. This was just plain broken. * The second more obscure one was when binding for read. If the external object had no other refcount than the one in the binding slot, then changing its binding would implicitly destroy it. However if the code was setting the same object back again (ie. with the pointer they had not reference to) then the refcount would drop to 0 then should be incremented again to 1 when it's re-bound. However because the count bounces off 0, the object is destroyed between being unbound and re-bound causing pure virtual calls and other crashes when we try to access it. * The fix is first to check if we're binding something to its own slot and skip it. Second we need to keep the new objects ref'd at all times during the binding (in case we are e.g. performing an array bind which moves the 1-refcount object from slot 2 to slot 3. It would be unbound when processing slot 2, hit refcount 0, and then be added again when processing slot 3). --- renderdoc/driver/d3d11/d3d11_renderstate.h | 131 +++++++++++---------- 1 file changed, 67 insertions(+), 64 deletions(-) diff --git a/renderdoc/driver/d3d11/d3d11_renderstate.h b/renderdoc/driver/d3d11/d3d11_renderstate.h index 8e06e4dac..0b3c821d7 100644 --- a/renderdoc/driver/d3d11/d3d11_renderstate.h +++ b/renderdoc/driver/d3d11/d3d11_renderstate.h @@ -386,87 +386,90 @@ struct D3D11RenderState void TakeRef(ID3D11DeviceChild *p); void ReleaseRef(ID3D11DeviceChild *p); - template - void ChangeRefRead(T **stateArray, T *const *newArray, size_t offset, size_t num) - { - for(size_t i = 0; i < num; i++) - { - T *old = stateArray[offset + i]; - ReleaseRef(old); - - if(newArray[i] == NULL) - { - stateArray[offset + i] = newArray[i]; - } - else - { - if(IsBoundForWrite(newArray[i])) - { - // RDCDEBUG("Resource %d was bound for write, forcing to NULL", offset+i); - stateArray[offset + i] = NULL; - } - else - { - stateArray[offset + i] = newArray[i]; - } - } - TakeRef(stateArray[offset + i]); - } - } - - template - void ChangeRefWrite(T **stateArray, T *const *newArray, size_t offset, size_t num) - { - for(size_t i = 0; i < num; i++) - { - T *old = stateArray[offset + i]; - ReleaseRef(old); - - if(newArray[i]) - { - UnbindForRead(newArray[i]); - // when binding something for write, all other write slots are NULL'd too - UnbindForWrite(newArray[i]); - } - stateArray[offset + i] = newArray[i]; - TakeRef(stateArray[offset + i]); - } - } - template void ChangeRefRead(T *&stateItem, T *newItem) { + // don't do anything for redundant changes. This prevents the object from bouncing off refcount + // 0 during the changeover if it's only bound once, has no external refcount. + if(stateItem == newItem) + return; + + // release the old item, which may destroy it but we won't use it again as we know is not the + // same as the new item. ReleaseRef(stateItem); + + // assign the new item, but don't ref it yet stateItem = newItem; - if(newItem == NULL) + // if the item is bound for writing anywhere, we instead bind NULL + if(IsBoundForWrite(newItem)) { - stateItem = newItem; - } - else - { - if(IsBoundForWrite(newItem)) - { - // RDCDEBUG("Resource was bound for write, forcing to NULL"); - stateItem = NULL; - } - else - { - stateItem = newItem; - } + // RDCDEBUG("Resource was bound for write, forcing to NULL"); + stateItem = NULL; } - TakeRef(newItem); + // finally we take the ref on the new item + TakeRef(stateItem); + } + + template + void ChangeRefRead(T **stateArray, T *const *newArray, size_t offset, size_t num) + { + // addref the whole array so none of it can be destroyed during processing + for(size_t i = 0; i < num; i++) + if(newArray[i]) + newArray[i]->AddRef(); + + for(size_t i = 0; i < num; i++) + ChangeRefRead(stateArray[offset + i], newArray[i]); + + // release the ref we added above + for(size_t i = 0; i < num; i++) + if(newArray[i]) + newArray[i]->Release(); } template void ChangeRefWrite(T *&stateItem, T *newItem) { + // don't do anything for redundant changes. This prevents the object from bouncing off refcount + // 0 during the changeover if it's only bound once, has no external refcount. + if(stateItem == newItem) + return; + + // release the old item, which may destroy it but we won't use it again as we know is not the + // same as the new item. We NULL it out so that it doesn't get unbound again below ReleaseRef(stateItem); - stateItem = newItem; + stateItem = NULL; + + // if we're not binding NULL, then unbind any other conflicting uses if(newItem) + { UnbindForRead(newItem); - TakeRef(newItem); + // when binding something for write, all other write slots are NULL'd too + UnbindForWrite(newItem); + } + + // now bind the new item and ref it + stateItem = newItem; + TakeRef(stateItem); + } + + template + void ChangeRefWrite(T **stateArray, T *const *newArray, size_t offset, size_t num) + { + // addref the whole array so none of it can be destroyed during processing + for(size_t i = 0; i < num; i++) + if(newArray[i]) + newArray[i]->AddRef(); + + for(size_t i = 0; i < num; i++) + ChangeRefWrite(stateArray[offset + i], newArray[i]); + + // release the ref we added above + for(size_t i = 0; i < num; i++) + if(newArray[i]) + newArray[i]->Release(); } template