From 9e5cfa8b96e9948ec2aff5060dc64cafc8d9fd11 Mon Sep 17 00:00:00 2001 From: baldurk Date: Wed, 22 Nov 2017 12:21:46 +0000 Subject: [PATCH] Add safety to satisfy Coverity on some errors that are likely impossible * Reported by Coverity Scan --- .../Windows/Dialogs/EnvironmentEditor.cpp | 7 +- qrenderdoc/Windows/Dialogs/RemoteManager.cpp | 6 +- renderdoc/common/shader_cache.h | 121 ++++++++++-------- renderdoc/common/wrapped_pool.h | 3 +- renderdoc/driver/d3d11/d3d11_replay.cpp | 8 +- renderdoc/driver/d3d12/d3d12_debug.cpp | 2 +- renderdoc/driver/d3d12/d3d12_debug.h | 2 +- renderdoc/driver/d3d12/d3d12_replay.cpp | 8 +- renderdoc/driver/gl/gl_replay.cpp | 8 +- .../driver/shaders/dxbc/dxbc_inspect.cpp | 10 +- renderdoc/driver/vulkan/vk_debug.cpp | 12 +- renderdoc/driver/vulkan/vk_replay.cpp | 9 +- .../driver/vulkan/wrappers/vk_queue_funcs.cpp | 32 +++-- .../vulkan/wrappers/vk_resource_funcs.cpp | 4 +- renderdoc/replay/capture_file.cpp | 4 + 15 files changed, 142 insertions(+), 94 deletions(-) diff --git a/qrenderdoc/Windows/Dialogs/EnvironmentEditor.cpp b/qrenderdoc/Windows/Dialogs/EnvironmentEditor.cpp index f7ed756b0..a514c849e 100644 --- a/qrenderdoc/Windows/Dialogs/EnvironmentEditor.cpp +++ b/qrenderdoc/Windows/Dialogs/EnvironmentEditor.cpp @@ -170,7 +170,12 @@ void EnvironmentEditor::on_deleteButton_clicked() if(!sel) return; - delete ui->variables->takeTopLevelItem(ui->variables->indexOfTopLevelItem(sel)); + int idx = ui->variables->indexOfTopLevelItem(sel); + + if(idx >= 0) + delete ui->variables->takeTopLevelItem(idx); + else + qCritical() << "Can't find item to delete"; on_name_textChanged(ui->name->text()); } diff --git a/qrenderdoc/Windows/Dialogs/RemoteManager.cpp b/qrenderdoc/Windows/Dialogs/RemoteManager.cpp index 1eae99a32..7d2437e45 100644 --- a/qrenderdoc/Windows/Dialogs/RemoteManager.cpp +++ b/qrenderdoc/Windows/Dialogs/RemoteManager.cpp @@ -640,8 +640,10 @@ void RemoteManager::on_deleteHost_clicked() RemoteHost *host = getRemoteHost(item); + int itemIdx = ui->hosts->indexOfTopLevelItem(item); + // don't delete running instances on a host - if(item->parent() != ui->hosts->invisibleRootItem() || !host) + if(item->parent() != ui->hosts->invisibleRootItem() || itemIdx < 0 || !host) return; QString hostname = item->text(0); @@ -664,7 +666,7 @@ void RemoteManager::on_deleteHost_clicked() item->clear(); - queueDelete(ui->hosts->takeTopLevelItem(ui->hosts->indexOfTopLevelItem(item))); + queueDelete(ui->hosts->takeTopLevelItem(itemIdx)); ui->hosts->clearSelection(); diff --git a/renderdoc/common/shader_cache.h b/renderdoc/common/shader_cache.h index b22225163..583ed290c 100644 --- a/renderdoc/common/shader_cache.h +++ b/renderdoc/common/shader_cache.h @@ -68,65 +68,76 @@ bool LoadShaderCache(const char *filename, const uint32_t magicNumber, const uin { uint32_t numentries = header[2]; - byte *ptr = cache + sizeof(uint32_t) * 3; - - int64_t bufsize = (int64_t)cachelen - sizeof(uint32_t) * 3; - - for(uint32_t i = 0; i < numentries; i++) + // assume at least 16 bytes for any cache entry. 8 bytes for hash and length, and 8 bytes + // data. + if(numentries > cachelen / 16LLU) { - if((size_t)bufsize < sizeof(uint32_t)) - { - RDCERR("Invalid shader cache - truncated, not enough data for shader hash"); - ret = false; - break; - } - - uint32_t hash = *(uint32_t *)ptr; - ptr += sizeof(uint32_t); - bufsize -= sizeof(uint32_t); - - if((size_t)bufsize < sizeof(uint32_t)) - { - RDCERR("Invalid shader cache - truncated, not enough data for shader length"); - ret = false; - break; - } - - uint32_t len = *(uint32_t *)ptr; - ptr += sizeof(uint32_t); - bufsize -= sizeof(uint32_t); - - if(bufsize < len) - { - RDCERR("Invalid shader cache - truncated, not enough data for shader buffer"); - ret = false; - break; - } - - byte *data = ptr; - ptr += len; - bufsize -= len; - - ResultType result; - bool created = callbacks.Create(len, data, &result); - - if(!created) - { - RDCERR("Couldn't create blob of size %u from shadercache", len); - ret = false; - break; - } - - resultCache[hash] = result; - } - - if(ret == true && bufsize != 0) - { - RDCERR("Invalid shader cache - trailing data"); + RDCERR("Invalid shader cache - more entries %u than are feasible in a %llu byte cache", + numentries, cachelen); ret = false; } + else + { + byte *ptr = cache + sizeof(uint32_t) * 3; - RDCDEBUG("Successfully loaded %d shaders from shader cache", resultCache.size()); + int64_t bufsize = (int64_t)cachelen - sizeof(uint32_t) * 3; + + for(uint32_t i = 0; i < numentries; i++) + { + if((size_t)bufsize < sizeof(uint32_t)) + { + RDCERR("Invalid shader cache - truncated, not enough data for shader hash"); + ret = false; + break; + } + + uint32_t hash = *(uint32_t *)ptr; + ptr += sizeof(uint32_t); + bufsize -= sizeof(uint32_t); + + if((size_t)bufsize < sizeof(uint32_t)) + { + RDCERR("Invalid shader cache - truncated, not enough data for shader length"); + ret = false; + break; + } + + uint32_t len = *(uint32_t *)ptr; + ptr += sizeof(uint32_t); + bufsize -= sizeof(uint32_t); + + if(bufsize < len) + { + RDCERR("Invalid shader cache - truncated, not enough data for shader buffer"); + ret = false; + break; + } + + byte *data = ptr; + ptr += len; + bufsize -= len; + + ResultType result; + bool created = callbacks.Create(len, data, &result); + + if(!created) + { + RDCERR("Couldn't create blob of size %u from shadercache", len); + ret = false; + break; + } + + resultCache[hash] = result; + } + + if(ret == true && bufsize != 0) + { + RDCERR("Invalid shader cache - trailing data"); + ret = false; + } + + RDCDEBUG("Successfully loaded %d shaders from shader cache", resultCache.size()); + } } delete[] cache; diff --git a/renderdoc/common/wrapped_pool.h b/renderdoc/common/wrapped_pool.h index 0887c9d06..f94e13f9f 100644 --- a/renderdoc/common/wrapped_pool.h +++ b/renderdoc/common/wrapped_pool.h @@ -225,7 +225,8 @@ private: allocated[idx] = false; #if ENABLED(RDOC_DEVEL) - memset(p, 0xfe, DebugClear ? AllocByteSize : 0); + if(DebugClear) + memset(p, 0xfe, AllocByteSize); #endif } diff --git a/renderdoc/driver/d3d11/d3d11_replay.cpp b/renderdoc/driver/d3d11/d3d11_replay.cpp index afdd84b82..b32375e45 100644 --- a/renderdoc/driver/d3d11/d3d11_replay.cpp +++ b/renderdoc/driver/d3d11/d3d11_replay.cpp @@ -2033,8 +2033,12 @@ void D3D11_ProcessStructured(RDCFile *rdc, SDFile &output) { WrappedID3D11Device device(NULL, NULL); - device.SetStructuredExport( - rdc->GetSectionProperties(rdc->SectionIndex(SectionType::FrameCapture)).version); + int sectionIdx = rdc->SectionIndex(SectionType::FrameCapture); + + if(sectionIdx < 0) + return; + + device.SetStructuredExport(rdc->GetSectionProperties(sectionIdx).version); device.ReadLogInitialisation(rdc, true); device.GetStructuredFile().swap(output); diff --git a/renderdoc/driver/d3d12/d3d12_debug.cpp b/renderdoc/driver/d3d12/d3d12_debug.cpp index 9f1f09609..e9a8fde06 100644 --- a/renderdoc/driver/d3d12/d3d12_debug.cpp +++ b/renderdoc/driver/d3d12/d3d12_debug.cpp @@ -1663,7 +1663,7 @@ D3D12RootSignature D3D12DebugManager::GetRootSig(const void *data, size_t dataSi return ret; } -ID3DBlob *D3D12DebugManager::MakeRootSig(const std::vector params, +ID3DBlob *D3D12DebugManager::MakeRootSig(const std::vector ¶ms, D3D12_ROOT_SIGNATURE_FLAGS Flags, UINT NumStaticSamplers, const D3D12_STATIC_SAMPLER_DESC *StaticSamplers) { diff --git a/renderdoc/driver/d3d12/d3d12_debug.h b/renderdoc/driver/d3d12/d3d12_debug.h index abed92911..2803ba477 100644 --- a/renderdoc/driver/d3d12/d3d12_debug.h +++ b/renderdoc/driver/d3d12/d3d12_debug.h @@ -115,7 +115,7 @@ public: void FreeRTV(D3D12_CPU_DESCRIPTOR_HANDLE handle); static D3D12RootSignature GetRootSig(const void *data, size_t dataSize); - static ID3DBlob *MakeRootSig(const std::vector params, + static ID3DBlob *MakeRootSig(const std::vector ¶ms, D3D12_ROOT_SIGNATURE_FLAGS Flags = D3D12_ROOT_SIGNATURE_FLAG_NONE, UINT NumStaticSamplers = 0, const D3D12_STATIC_SAMPLER_DESC *StaticSamplers = NULL); diff --git a/renderdoc/driver/d3d12/d3d12_replay.cpp b/renderdoc/driver/d3d12/d3d12_replay.cpp index d2c46ca0a..625966c05 100644 --- a/renderdoc/driver/d3d12/d3d12_replay.cpp +++ b/renderdoc/driver/d3d12/d3d12_replay.cpp @@ -1797,8 +1797,12 @@ void D3D12_ProcessStructured(RDCFile *rdc, SDFile &output) { WrappedID3D12Device device(NULL, NULL); - device.SetStructuredExport( - rdc->GetSectionProperties(rdc->SectionIndex(SectionType::FrameCapture)).version); + int sectionIdx = rdc->SectionIndex(SectionType::FrameCapture); + + if(sectionIdx < 0) + return; + + device.SetStructuredExport(rdc->GetSectionProperties(sectionIdx).version); device.ReadLogInitialisation(rdc, true); device.GetStructuredFile().swap(output); diff --git a/renderdoc/driver/gl/gl_replay.cpp b/renderdoc/driver/gl/gl_replay.cpp index b6cbc68e2..a4a194fe0 100644 --- a/renderdoc/driver/gl/gl_replay.cpp +++ b/renderdoc/driver/gl/gl_replay.cpp @@ -3323,8 +3323,12 @@ void GL_ProcessStructured(RDCFile *rdc, SDFile &output) GLDummyPlatform dummy; WrappedOpenGL device(empty, dummy); - device.SetStructuredExport( - rdc->GetSectionProperties(rdc->SectionIndex(SectionType::FrameCapture)).version); + int sectionIdx = rdc->SectionIndex(SectionType::FrameCapture); + + if(sectionIdx < 0) + return; + + device.SetStructuredExport(rdc->GetSectionProperties(sectionIdx).version); device.ReadLogInitialisation(rdc, true); device.GetStructuredFile().swap(output); diff --git a/renderdoc/driver/shaders/dxbc/dxbc_inspect.cpp b/renderdoc/driver/shaders/dxbc/dxbc_inspect.cpp index e8ea128ce..f98340dc4 100644 --- a/renderdoc/driver/shaders/dxbc/dxbc_inspect.cpp +++ b/renderdoc/driver/shaders/dxbc/dxbc_inspect.cpp @@ -914,14 +914,16 @@ DXBCFile::DXBCFile(const void *ByteCode, size_t ByteCodeLength) RDCASSERT(sig && sig->empty()); - SIGNElement *el = (SIGNElement *)(sign + 1); - SIGNElement7 *el7 = (SIGNElement7 *)el; - SIGNElement1 *el1 = (SIGNElement1 *)el; + SIGNElement *el0 = (SIGNElement *)(sign + 1); + SIGNElement7 *el7 = (SIGNElement7 *)el0; + SIGNElement1 *el1 = (SIGNElement1 *)el0; for(uint32_t signIdx = 0; signIdx < sign->numElems; signIdx++) { SigParameter desc; + const SIGNElement *el = el0; + if(*fourcc == FOURCC_ISG1 || *fourcc == FOURCC_OSG1) { desc.stream = el1->stream; @@ -1021,7 +1023,7 @@ DXBCFile::DXBCFile(const void *ByteCode, size_t ByteCodeLength) sig->push_back(desc); - el++; + el0++; el1++; el7++; } diff --git a/renderdoc/driver/vulkan/vk_debug.cpp b/renderdoc/driver/vulkan/vk_debug.cpp index 6583858f2..4ce938464 100644 --- a/renderdoc/driver/vulkan/vk_debug.cpp +++ b/renderdoc/driver/vulkan/vk_debug.cpp @@ -974,17 +974,17 @@ VulkanDebugManager::VulkanDebugManager(WrappedVulkan *driver, VkDevice dev) m_pDriver->vkDestroyRenderPass(dev, rp, NULL); } - - attDesc.samples = VK_SAMPLE_COUNT_1_BIT; - msaa.rasterizationSamples = VK_SAMPLE_COUNT_1_BIT; - - msaa.sampleShadingEnable = false; - msaa.minSampleShading = 0.0f; } // restore pipeline state to normal cb.attachmentCount = 1; + attDesc.samples = VK_SAMPLE_COUNT_1_BIT; + msaa.rasterizationSamples = VK_SAMPLE_COUNT_1_BIT; + + msaa.sampleShadingEnable = false; + msaa.minSampleShading = 0.0f; + pipeInfo.renderPass = RGBA8sRGBRP; dyn.dynamicStateCount = ARRAY_COUNT(dynstates); dyn.pDynamicStates = dynstates; diff --git a/renderdoc/driver/vulkan/vk_replay.cpp b/renderdoc/driver/vulkan/vk_replay.cpp index f7e9185a9..8a0b18131 100644 --- a/renderdoc/driver/vulkan/vk_replay.cpp +++ b/renderdoc/driver/vulkan/vk_replay.cpp @@ -5398,8 +5398,13 @@ static VulkanDriverRegistration VkDriverRegistration; void Vulkan_ProcessStructured(RDCFile *rdc, SDFile &output) { WrappedVulkan vulkan; - vulkan.SetStructuredExport( - rdc->GetSectionProperties(rdc->SectionIndex(SectionType::FrameCapture)).version); + + int sectionIdx = rdc->SectionIndex(SectionType::FrameCapture); + + if(sectionIdx < 0) + return; + + vulkan.SetStructuredExport(rdc->GetSectionProperties(sectionIdx).version); vulkan.ReadLogInitialisation(rdc, true); vulkan.GetStructuredFile().swap(output); diff --git a/renderdoc/driver/vulkan/wrappers/vk_queue_funcs.cpp b/renderdoc/driver/vulkan/wrappers/vk_queue_funcs.cpp index 4cdb565ff..b68607695 100644 --- a/renderdoc/driver/vulkan/wrappers/vk_queue_funcs.cpp +++ b/renderdoc/driver/vulkan/wrappers/vk_queue_funcs.cpp @@ -286,22 +286,24 @@ bool WrappedVulkan::Serialise_vkQueueSubmit(SerialiserType &ser, VkQueue queue, m_ImageLayouts); } - submitInfo.commandBufferCount = (uint32_t)rerecordedCmds.size(); - submitInfo.pCommandBuffers = &rerecordedCmds[0]; + VkSubmitInfo rerecordedSubmit = submitInfo; + + rerecordedSubmit.commandBufferCount = (uint32_t)rerecordedCmds.size(); + rerecordedSubmit.pCommandBuffers = &rerecordedCmds[0]; #if ENABLED(SINGLE_FLUSH_VALIDATE) - submitInfo.commandBufferCount = 1; - for(uint32_t i = 0; i < submitInfo.commandBufferCount; i++) + rerecordedSubmit.commandBufferCount = 1; + for(uint32_t i = 0; i < rerecordedSubmit.commandBufferCount; i++) { - ObjDisp(queue)->QueueSubmit(Unwrap(queue), 1, &submitInfo, VK_NULL_HANDLE); - submitInfo.pCommandBuffers++; + ObjDisp(queue)->QueueSubmit(Unwrap(queue), 1, &rerecordedSubmit, VK_NULL_HANDLE); + rerecordedSubmit.pCommandBuffers++; FlushQ(); } #else // don't submit the fence, since we have nothing to wait on it being signalled, and we // might not have it correctly in the unsignalled state. - ObjDisp(queue)->QueueSubmit(Unwrap(queue), 1, &submitInfo, VK_NULL_HANDLE); + ObjDisp(queue)->QueueSubmit(Unwrap(queue), 1, &rerecordedSubmit, VK_NULL_HANDLE); #endif } else if(m_LastEventID > startEID && m_LastEventID < m_RootEventID) @@ -358,22 +360,24 @@ bool WrappedVulkan::Serialise_vkQueueSubmit(SerialiserType &ser, VkQueue queue, RDCASSERT(trimmedCmds.size() > 0); - submitInfo.commandBufferCount = (uint32_t)trimmedCmds.size(); - submitInfo.pCommandBuffers = &trimmedCmds[0]; + VkSubmitInfo trimmedSubmit = submitInfo; + + trimmedSubmit.commandBufferCount = (uint32_t)trimmedCmds.size(); + trimmedSubmit.pCommandBuffers = &trimmedCmds[0]; #if ENABLED(SINGLE_FLUSH_VALIDATE) - submitInfo.commandBufferCount = 1; - for(uint32_t i = 0; i < submitInfo.commandBufferCount; i++) + trimmedSubmit.commandBufferCount = 1; + for(uint32_t i = 0; i < trimmedSubmit.commandBufferCount; i++) { - ObjDisp(queue)->QueueSubmit(Unwrap(queue), 1, &submitInfo, VK_NULL_HANDLE); - submitInfo.pCommandBuffers++; + ObjDisp(queue)->QueueSubmit(Unwrap(queue), 1, &trimmedSubmit, VK_NULL_HANDLE); + trimmedSubmit.pCommandBuffers++; FlushQ(); } #else // don't submit the fence, since we have nothing to wait on it being signalled, and we // might not have it correctly in the unsignalled state. - ObjDisp(queue)->QueueSubmit(Unwrap(queue), 1, &submitInfo, VK_NULL_HANDLE); + ObjDisp(queue)->QueueSubmit(Unwrap(queue), 1, &trimmedSubmit, VK_NULL_HANDLE); #endif for(uint32_t i = 0; i < trimmedCmdIds.size(); i++) diff --git a/renderdoc/driver/vulkan/wrappers/vk_resource_funcs.cpp b/renderdoc/driver/vulkan/wrappers/vk_resource_funcs.cpp index f1cfca669..4dd807a7d 100644 --- a/renderdoc/driver/vulkan/wrappers/vk_resource_funcs.cpp +++ b/renderdoc/driver/vulkan/wrappers/vk_resource_funcs.cpp @@ -1413,9 +1413,11 @@ VkResult WrappedVulkan::vkCreateImage(VkDevice device, const VkImageCreateInfo * reqs[i].formatProperties.imageGranularity.depth); int a = 0; - for(; a < NUM_VK_IMAGE_ASPECTS; a++) + for(a = 0; a < NUM_VK_IMAGE_ASPECTS; a++) + { if(reqs[i].formatProperties.aspectMask & (1 << a)) break; + } record->sparseInfo->pages[a] = new pair[numpages]; } diff --git a/renderdoc/replay/capture_file.cpp b/renderdoc/replay/capture_file.cpp index e9749e700..ff40f0d6d 100644 --- a/renderdoc/replay/capture_file.cpp +++ b/renderdoc/replay/capture_file.cpp @@ -665,6 +665,10 @@ bool CaptureFile::InitResolver(float *progress, volatile bool *killSignal) *progress = 0.001f; int idx = m_RDC->SectionIndex(SectionType::ResolveDatabase); + + if(idx < 0) + return false; + StreamReader *reader = m_RDC->ReadSection(idx); std::vector buf;