From 23c137612bea1e319ecdfb894c020b6651f4909a Mon Sep 17 00:00:00 2001 From: Jan Zielinski Date: Wed, 5 Feb 2020 13:57:55 +0100 Subject: [PATCH] gallium/swr: Fix various asserts and security issues To improve the robustness of the code, we want to better detect issues in testing (using asserts) and use more secure techniques. Reviewed-by: Krzysztof Raszkowski Tested-by: Marge Bot Part-of: --- src/gallium/auxiliary/gallivm/lp_bld_init.c | 3 ++- .../swr/rasterizer/common/rdtsc_buckets.cpp | 6 ++++-- .../drivers/swr/rasterizer/common/swr_assert.h | 6 ------ src/gallium/drivers/swr/rasterizer/core/api.cpp | 4 ++-- src/gallium/drivers/swr/rasterizer/core/arena.h | 2 +- .../drivers/swr/rasterizer/core/tessellator.h | 1 + .../drivers/swr/rasterizer/core/threads.cpp | 9 +++++---- .../swr/rasterizer/jitter/builder_mem.cpp | 3 ++- .../swr/rasterizer/jitter/builder_misc.cpp | 1 + .../drivers/swr/rasterizer/jitter/fetch_jit.cpp | 15 ++++++++------- .../jitter/functionpasses/lower_x86.cpp | 12 ++++++++++-- src/gallium/drivers/swr/swr_query.cpp | 2 +- src/gallium/drivers/swr/swr_shader.cpp | 17 ++++++++++++++--- src/gallium/drivers/swr/swr_state.cpp | 1 + 14 files changed, 52 insertions(+), 30 deletions(-) diff --git a/src/gallium/auxiliary/gallivm/lp_bld_init.c b/src/gallium/auxiliary/gallivm/lp_bld_init.c index efc0096a744..6206d79cc54 100644 --- a/src/gallium/auxiliary/gallivm/lp_bld_init.c +++ b/src/gallium/auxiliary/gallivm/lp_bld_init.c @@ -450,7 +450,7 @@ lp_build_init(void) */ lp_native_vector_width = 128; } - + lp_native_vector_width = debug_get_num_option("LP_NATIVE_VECTOR_WIDTH", lp_native_vector_width); @@ -513,6 +513,7 @@ gallivm_create(const char *name, LLVMContextRef context) } } + assert(gallivm != NULL); return gallivm; } diff --git a/src/gallium/drivers/swr/rasterizer/common/rdtsc_buckets.cpp b/src/gallium/drivers/swr/rasterizer/common/rdtsc_buckets.cpp index e19a2d11045..2404ae7590d 100644 --- a/src/gallium/drivers/swr/rasterizer/common/rdtsc_buckets.cpp +++ b/src/gallium/drivers/swr/rasterizer/common/rdtsc_buckets.cpp @@ -103,9 +103,10 @@ void BucketManager::PrintBucket( BUCKET_DESC& desc = mBuckets[bucket.id]; // construct hierarchy visualization + std::string str = arrows[level]; + str += desc.name; char hier[80]; - strcpy(hier, arrows[level]); - strcat(hier, desc.name.c_str()); + strcpy_s(hier, sizeof(hier), str.c_str()); // print out fprintf(f, @@ -156,6 +157,7 @@ void BucketManager::PrintReport(const std::string& filename) { { FILE* f = fopen(filename.c_str(), "w"); + assert(f); mThreadMutex.lock(); for (const BUCKET_THREAD& thread : mThreads) diff --git a/src/gallium/drivers/swr/rasterizer/common/swr_assert.h b/src/gallium/drivers/swr/rasterizer/common/swr_assert.h index 7e90e21e12f..f6bf83ea5a8 100644 --- a/src/gallium/drivers/swr/rasterizer/common/swr_assert.h +++ b/src/gallium/drivers/swr/rasterizer/common/swr_assert.h @@ -164,12 +164,6 @@ void SwrTrace( #define SWR_ASSERT(e, ...) _SWR_ASSERT(true, e, ##__VA_ARGS__) #define SWR_ASSUME_ASSERT(e, ...) SWR_ASSERT(e, ##__VA_ARGS__) #define SWR_TRACE(_fmtstr, ...) _SWR_TRACE(_fmtstr, ##__VA_ARGS__) - -#if defined(assert) -#undef assert -#endif -#define assert(exp) SWR_ASSERT(exp) - #endif // SWR_ENABLE_ASSERTS #if SWR_ENABLE_REL_ASSERTS diff --git a/src/gallium/drivers/swr/rasterizer/core/api.cpp b/src/gallium/drivers/swr/rasterizer/core/api.cpp index 5405bf2d8ba..04f03c78f42 100644 --- a/src/gallium/drivers/swr/rasterizer/core/api.cpp +++ b/src/gallium/drivers/swr/rasterizer/core/api.cpp @@ -478,7 +478,7 @@ void SWR_API SwrSaveState(HANDLE hContext, void* pOutputStateBlock, size_t memSi { SWR_CONTEXT* pContext = GetContext(hContext); auto pSrc = GetDrawState(pContext); - SWR_ASSERT(pOutputStateBlock && memSize >= sizeof(*pSrc)); + assert(pOutputStateBlock && memSize >= sizeof(*pSrc)); memcpy(pOutputStateBlock, pSrc, sizeof(*pSrc)); } @@ -487,7 +487,7 @@ void SWR_API SwrRestoreState(HANDLE hContext, const void* pStateBlock, size_t me { SWR_CONTEXT* pContext = GetContext(hContext); auto pDst = GetDrawState(pContext); - SWR_ASSERT(pStateBlock && memSize >= sizeof(*pDst)); + assert(pStateBlock && memSize >= sizeof(*pDst)); memcpy(pDst, pStateBlock, sizeof(*pDst)); } diff --git a/src/gallium/drivers/swr/rasterizer/core/arena.h b/src/gallium/drivers/swr/rasterizer/core/arena.h index a3cfdb47818..831617c213f 100644 --- a/src/gallium/drivers/swr/rasterizer/core/arena.h +++ b/src/gallium/drivers/swr/rasterizer/core/arena.h @@ -110,7 +110,7 @@ struct CachingAllocatorT : DefaultAllocator if (pBlock) { - SWR_ASSUME_ASSERT(pPrevBlock && pPrevBlock->pNext == pBlock); + assert(pPrevBlock && pPrevBlock->pNext == pBlock); pPrevBlock->pNext = pBlock->pNext; pBlock->pNext = nullptr; diff --git a/src/gallium/drivers/swr/rasterizer/core/tessellator.h b/src/gallium/drivers/swr/rasterizer/core/tessellator.h index 66b6d5e102a..26e6d542b28 100644 --- a/src/gallium/drivers/swr/rasterizer/core/tessellator.h +++ b/src/gallium/drivers/swr/rasterizer/core/tessellator.h @@ -113,6 +113,7 @@ namespace Tessellator default: SWR_INVALID("Invalid Tessellation Domain: %d", Domain); + assert(false); } NumDomainPoints = (uint32_t)SUPER::GetPointCount(); diff --git a/src/gallium/drivers/swr/rasterizer/core/threads.cpp b/src/gallium/drivers/swr/rasterizer/core/threads.cpp index 113a31ee0c6..c75fb568431 100644 --- a/src/gallium/drivers/swr/rasterizer/core/threads.cpp +++ b/src/gallium/drivers/swr/rasterizer/core/threads.cpp @@ -1006,6 +1006,7 @@ void CreateThreadPool(SWR_CONTEXT* pContext, THREAD_POOL* pPool) CPUNumaNodes nodes; uint32_t numThreadsPerProcGroup = 0; CalculateProcessorTopology(nodes, numThreadsPerProcGroup); + assert(numThreadsPerProcGroup > 0); // Assumption, for asymmetric topologies, multi-threaded cores will appear // in the list before single-threaded cores. This appears to be true for @@ -1191,7 +1192,7 @@ void CreateThreadPool(SWR_CONTEXT* pContext, THREAD_POOL* pPool) pContext->NumWorkerThreads = pPool->numThreads; pPool->pThreadData = new (std::nothrow) THREAD_DATA[pPool->numThreads]; - SWR_ASSERT(pPool->pThreadData); + assert(pPool->pThreadData); memset(pPool->pThreadData, 0, sizeof(THREAD_DATA) * pPool->numThreads); pPool->numaMask = 0; @@ -1203,7 +1204,7 @@ void CreateThreadPool(SWR_CONTEXT* pContext, THREAD_POOL* pPool) pContext->workerPrivateState.pfnInitWorkerData = nullptr; pContext->workerPrivateState.pfnFinishWorkerData = nullptr; } - + // initialize contents of SWR_WORKER_DATA size_t perWorkerSize = AlignUpPow2(pContext->workerPrivateState.perWorkerPrivateStateSize, 64); @@ -1231,7 +1232,7 @@ void CreateThreadPool(SWR_CONTEXT* pContext, THREAD_POOL* pPool) } pPool->pThreads = new (std::nothrow) THREAD_PTR[pPool->numThreads]; - SWR_ASSERT(pPool->pThreads); + assert(pPool->pThreads); if (pContext->threadInfo.MAX_WORKER_THREADS) { @@ -1300,7 +1301,7 @@ void CreateThreadPool(SWR_CONTEXT* pContext, THREAD_POOL* pPool) if (numRemovedThreads) { --numRemovedThreads; - SWR_REL_ASSERT(numReservedThreads); + assert(numReservedThreads); --numReservedThreads; pPool->pApiThreadData[numReservedThreads].workerId = 0xFFFFFFFFU; pPool->pApiThreadData[numReservedThreads].procGroupId = core.procGroup; diff --git a/src/gallium/drivers/swr/rasterizer/jitter/builder_mem.cpp b/src/gallium/drivers/swr/rasterizer/jitter/builder_mem.cpp index 2d8240187c5..1418d65e2d0 100644 --- a/src/gallium/drivers/swr/rasterizer/jitter/builder_mem.cpp +++ b/src/gallium/drivers/swr/rasterizer/jitter/builder_mem.cpp @@ -632,6 +632,7 @@ namespace SwrJit break; } + assert(vConstMask && "Invalid info.numComps value"); vGatherOutput[swizzleIndex] = BITCAST(PSHUFB(BITCAST(vGatherInput, v32x8Ty), vConstMask), vGatherTy); // after pshufb for x channel @@ -654,7 +655,7 @@ namespace SwrJit // if (vSrc->getType() != mSimdFP32Ty) // { // vSrc = BITCAST(vSrc, mSimdFP32Ty); -// } +// } SWR_ASSERT(vSrc->getType()->getVectorElementType()->isFloatTy()); VSCATTERPS(pDst, vMask, vOffsets, vSrc, C(1)); return; diff --git a/src/gallium/drivers/swr/rasterizer/jitter/builder_misc.cpp b/src/gallium/drivers/swr/rasterizer/jitter/builder_misc.cpp index 183ba880c0f..4f6b0b46a60 100644 --- a/src/gallium/drivers/swr/rasterizer/jitter/builder_misc.cpp +++ b/src/gallium/drivers/swr/rasterizer/jitter/builder_misc.cpp @@ -553,6 +553,7 @@ namespace SwrJit else { Constant* cB = dyn_cast(b); + assert(cB != nullptr); // number of 8 bit elements in b uint32_t numElms = cast(cB->getType())->getNumElements(); // output vector diff --git a/src/gallium/drivers/swr/rasterizer/jitter/fetch_jit.cpp b/src/gallium/drivers/swr/rasterizer/jitter/fetch_jit.cpp index 72704e94e4c..dea509b0e7e 100644 --- a/src/gallium/drivers/swr/rasterizer/jitter/fetch_jit.cpp +++ b/src/gallium/drivers/swr/rasterizer/jitter/fetch_jit.cpp @@ -209,8 +209,8 @@ Function* FetchJit::Create(const FETCH_COMPILE_STATE& fetchState) : vIndices = GetSimdValid32bitIndices(indices, pLastIndex); break; // incoming type is already 32bit int default: - SWR_INVALID("Unsupported index type"); vIndices = nullptr; + assert(false && "Unsupported index type"); break; } @@ -1392,11 +1392,11 @@ void FetchJit::Shuffle8bpcGatherd16(Shuffle8bpcArgs& args) conversionFactor = VIMMED1((float)(1.0)); break; case CONVERT_USCALED: - SWR_INVALID("Type should not be sign extended!"); + assert(false && "Type should not be sign extended!"); conversionFactor = nullptr; break; default: - SWR_ASSERT(conversionType == CONVERT_NONE); + assert(conversionType == CONVERT_NONE); conversionFactor = nullptr; break; } @@ -1466,11 +1466,11 @@ void FetchJit::Shuffle8bpcGatherd16(Shuffle8bpcArgs& args) conversionFactor = VIMMED1((float)(1.0)); break; case CONVERT_SSCALED: - SWR_INVALID("Type should not be zero extended!"); + assert(false && "Type should not be zero extended!"); conversionFactor = nullptr; break; default: - SWR_ASSERT(conversionType == CONVERT_NONE); + assert(conversionType == CONVERT_NONE); conversionFactor = nullptr; break; } @@ -1511,6 +1511,7 @@ void FetchJit::Shuffle8bpcGatherd16(Shuffle8bpcArgs& args) 3, -1, -1, -1, 7, -1, -1, -1, 11, -1, -1, -1, 15, -1, -1, -1}); break; default: + assert(false && "Invalid component"); vConstMask = nullptr; break; } @@ -1762,11 +1763,11 @@ void FetchJit::Shuffle16bpcGather16(Shuffle16bpcArgs& args) conversionFactor = VIMMED1((float)(1.0)); break; case CONVERT_USCALED: - SWR_INVALID("Type should not be sign extended!"); + assert(false && "Type should not be sign extended!"); conversionFactor = nullptr; break; default: - SWR_ASSERT(conversionType == CONVERT_NONE); + assert(conversionType == CONVERT_NONE); conversionFactor = nullptr; break; } diff --git a/src/gallium/drivers/swr/rasterizer/jitter/functionpasses/lower_x86.cpp b/src/gallium/drivers/swr/rasterizer/jitter/functionpasses/lower_x86.cpp index 2fadca340db..fc5157e89c9 100644 --- a/src/gallium/drivers/swr/rasterizer/jitter/functionpasses/lower_x86.cpp +++ b/src/gallium/drivers/swr/rasterizer/jitter/functionpasses/lower_x86.cpp @@ -224,13 +224,16 @@ namespace SwrJit TargetWidth* pWidth, Type** pTy) { + assert(pCallInst); Type* pVecTy = pCallInst->getType(); // Check for intrinsic specific types // VCVTPD2PS type comes from src, not dst if (intrinName.equals("meta.intrinsic.VCVTPD2PS")) { - pVecTy = pCallInst->getOperand(0)->getType(); + Value* pOp = pCallInst->getOperand(0); + assert(pOp); + pVecTy = pOp->getType(); } if (!pVecTy->isVectorTy()) @@ -307,7 +310,9 @@ namespace SwrJit Instruction* ProcessIntrinsicAdvanced(CallInst* pCallInst) { - Function* pFunc = pCallInst->getCalledFunction(); + Function* pFunc = pCallInst->getCalledFunction(); + assert(pFunc); + auto& intrinsic = intrinsicMap2[mTarget][pFunc->getName()]; TargetWidth vecWidth; Type* pElemTy; @@ -367,6 +372,7 @@ namespace SwrJit Instruction* ProcessIntrinsic(CallInst* pCallInst) { Function* pFunc = pCallInst->getCalledFunction(); + assert(pFunc); // Forward to the advanced support if found if (intrinsicMap2[mTarget].find(pFunc->getName()) != intrinsicMap2[mTarget].end()) @@ -731,7 +737,9 @@ namespace SwrJit auto B = pThis->B; auto vf32Src = pCallInst->getOperand(0); + assert(vf32Src); auto i8Round = pCallInst->getOperand(1); + assert(i8Round); auto pfnFunc = Intrinsic::getDeclaration(B->JM()->mpCurrentModule, Intrinsic::x86_avx_round_ps_256); diff --git a/src/gallium/drivers/swr/swr_query.cpp b/src/gallium/drivers/swr/swr_query.cpp index 8c49a2ebb29..005b64fb090 100644 --- a/src/gallium/drivers/swr/swr_query.cpp +++ b/src/gallium/drivers/swr/swr_query.cpp @@ -46,9 +46,9 @@ swr_create_query(struct pipe_context *pipe, unsigned type, unsigned index) assert(index < MAX_SO_STREAMS); pq = (struct swr_query *) AlignedMalloc(sizeof(struct swr_query), 64); - memset(pq, 0, sizeof(*pq)); if (pq) { + memset(pq, 0, sizeof(*pq)); pq->type = type; pq->index = index; } diff --git a/src/gallium/drivers/swr/swr_shader.cpp b/src/gallium/drivers/swr/swr_shader.cpp index b8f2a10e04d..fcbdcd54369 100644 --- a/src/gallium/drivers/swr/swr_shader.cpp +++ b/src/gallium/drivers/swr/swr_shader.cpp @@ -1510,6 +1510,7 @@ BuilderSWR::CompileGS(struct swr_context *ctx, swr_jit_gs_key &key) struct lp_build_sampler_soa *sampler = swr_sampler_soa_create(key.sampler, PIPE_SHADER_GEOMETRY); + assert(sampler != nullptr); struct lp_bld_tgsi_system_values system_values; memset(&system_values, 0, sizeof(system_values)); @@ -1523,6 +1524,7 @@ BuilderSWR::CompileGS(struct swr_context *ctx, swr_jit_gs_key &key) ubyte semantic_idx = info->input_semantic_index[slot]; unsigned vs_slot = locate_linkage(semantic_name, semantic_idx, &ctx->vs->info.base); + assert(vs_slot < PIPE_MAX_SHADER_OUTPUTS); vs_slot += VERTEX_ATTRIB_START_SLOT; @@ -1736,6 +1738,7 @@ BuilderSWR::CompileTES(struct swr_context *ctx, swr_jit_tes_key &key) struct lp_build_sampler_soa *sampler = swr_sampler_soa_create(key.sampler, PIPE_SHADER_TESS_EVAL); + assert(sampler != nullptr); struct lp_bld_tgsi_system_values system_values; memset(&system_values, 0, sizeof(system_values)); @@ -1830,6 +1833,7 @@ BuilderSWR::CompileTES(struct swr_context *ctx, swr_jit_tes_key &key) // Where in TCS output is my attribute? // TESS_TODO: revisit after implement pass-through TCS unsigned tcs_slot = locate_linkage(semantic_name, semantic_idx, pPrevShader); + assert(tcs_slot < PIPE_MAX_SHADER_OUTPUTS); // Skip tessellation levels - these go to the tessellator, not TES switch (semantic_name) { @@ -2035,6 +2039,7 @@ BuilderSWR::CompileTCS(struct swr_context *ctx, swr_jit_tcs_key &key) struct lp_build_sampler_soa *sampler = swr_sampler_soa_create(key.sampler, PIPE_SHADER_TESS_CTRL); + assert(sampler != nullptr); struct lp_bld_tgsi_system_values system_values; memset(&system_values, 0, sizeof(system_values)); @@ -2069,6 +2074,7 @@ BuilderSWR::CompileTCS(struct swr_context *ctx, swr_jit_tcs_key &key) unsigned vs_slot = locate_linkage(semantic_name, semantic_idx, &ctx->vs->info.base); + assert(vs_slot < PIPE_MAX_SHADER_OUTPUTS); vs_slot += VERTEX_ATTRIB_START_SLOT; @@ -2311,6 +2317,7 @@ BuilderSWR::CompileVS(struct swr_context *ctx, swr_jit_vs_key &key) struct lp_build_sampler_soa *sampler = swr_sampler_soa_create(key.sampler, PIPE_SHADER_VERTEX); + assert(sampler != nullptr); struct lp_bld_tgsi_system_values system_values; memset(&system_values, 0, sizeof(system_values)); @@ -2397,6 +2404,7 @@ BuilderSWR::CompileVS(struct swr_context *ctx, swr_jit_vs_key &key) } } } + assert(cv < PIPE_MAX_SHADER_OUTPUTS); LLVMValueRef cx = LLVMBuildLoad(gallivm->builder, outputs[cv][0], ""); LLVMValueRef cy = LLVMBuildLoad(gallivm->builder, outputs[cv][1], ""); LLVMValueRef cz = LLVMBuildLoad(gallivm->builder, outputs[cv][2], ""); @@ -2419,6 +2427,7 @@ BuilderSWR::CompileVS(struct swr_context *ctx, swr_jit_vs_key &key) if ((pLastFE->clipdist_writemask & clip_mask & (1 << val)) || ((pLastFE->culldist_writemask << pLastFE->num_written_clipdistance) & (1 << val))) { unsigned cv = locate_linkage(TGSI_SEMANTIC_CLIPDIST, val < 4 ? 0 : 1, pLastFE); + assert(cv < PIPE_MAX_SHADER_OUTPUTS); if (val < 4) { LLVMValueRef dist = LLVMBuildLoad(gallivm->builder, outputs[cv][val], ""); WriteVS(unwrap(dist), pVsCtx, vtxOutput, VERTEX_CLIPCULL_DIST_LO_SLOT, val); @@ -2711,7 +2720,7 @@ BuilderSWR::CompileFS(struct swr_context *ctx, swr_jit_fs_key &key) linkedAttrib = pPrevShader->num_outputs + extraAttribs - 1; swr_fs->pointSpriteMask |= (1 << linkedAttrib); extraAttribs++; - } else if (linkedAttrib == 0xFFFFFFFF) { + } else if (linkedAttrib + 1 == 0xFFFFFFFF) { inputs[attrib][0] = wrap(VIMMED1(0.0f)); inputs[attrib][1] = wrap(VIMMED1(0.0f)); inputs[attrib][2] = wrap(VIMMED1(0.0f)); @@ -2733,15 +2742,16 @@ BuilderSWR::CompileFS(struct swr_context *ctx, swr_jit_fs_key &key) Value *offset = NULL; if (semantic_name == TGSI_SEMANTIC_COLOR && key.light_twoside) { bcolorAttrib = locate_linkage( - TGSI_SEMANTIC_BCOLOR, semantic_idx, pPrevShader) - 1; + TGSI_SEMANTIC_BCOLOR, semantic_idx, pPrevShader); /* Neither front nor back colors were available. Nothing to load. */ if (bcolorAttrib == 0xFFFFFFFF && linkedAttrib == 0xFFFFFFFF) continue; /* If there is no front color, just always use the back color. */ - if (linkedAttrib == 0xFFFFFFFF) + if (linkedAttrib + 1 == 0xFFFFFFFF) linkedAttrib = bcolorAttrib; if (bcolorAttrib != 0xFFFFFFFF) { + bcolorAttrib -= 1; if (interpMode == TGSI_INTERPOLATE_CONSTANT) { swr_fs->constantMask |= 1 << bcolorAttrib; } else if (interpMode == TGSI_INTERPOLATE_COLOR) { @@ -2797,6 +2807,7 @@ BuilderSWR::CompileFS(struct swr_context *ctx, swr_jit_fs_key &key) } sampler = swr_sampler_soa_create(key.sampler, PIPE_SHADER_FRAGMENT); + assert(sampler != nullptr); struct lp_bld_tgsi_system_values system_values; memset(&system_values, 0, sizeof(system_values)); diff --git a/src/gallium/drivers/swr/swr_state.cpp b/src/gallium/drivers/swr/swr_state.cpp index 34e36a8025a..625f0050ad5 100644 --- a/src/gallium/drivers/swr/swr_state.cpp +++ b/src/gallium/drivers/swr/swr_state.cpp @@ -71,6 +71,7 @@ swr_create_blend_state(struct pipe_context *pipe, const struct pipe_blend_state *blend) { struct swr_blend_state *state = CALLOC_STRUCT(swr_blend_state); + assert(state != nullptr); memcpy(&state->pipe, blend, sizeof(*blend)); -- 2.30.2