From 8004d3652870b4dccf5fb1a1f266544a0a2a50e6 Mon Sep 17 00:00:00 2001 From: LoopDawg Date: Sun, 17 Sep 2017 10:38:52 -0600 Subject: [PATCH] Remapper: make remapper robust against non-exiting error handlers Remapper errors are generally fatal: there has been some unexpected situation while parsing the SPV binary, and there is no reasonable way to carry on. The errorHandler() function is called in this case, which by default exits, but it is possible to submit a handler which does not. In that case the remapper would carry on in a bad state. This change ensures a graceful termination of the remap() function. While a try {} catch {} construct would be the ideal and safe way to do this, that's off limits for certain environments, so this tries to do the same thing with explicit code, to catch all the bailout paths. --- CMakeLists.txt | 2 +- SPIRV/SPVRemapper.cpp | 165 ++++++++++++++++++--- SPIRV/SPVRemapper.h | 12 +- Test/baseResults/remap.invalid-spirv-1.out | 1 + Test/baseResults/remap.invalid-spirv-2.out | 1 + Test/remap.invalid-spirv-1.spv | Bin 0 -> 219 bytes Test/remap.invalid-spirv-2.spv | Bin 0 -> 212 bytes Test/runtests | 13 ++ 8 files changed, 173 insertions(+), 21 deletions(-) create mode 100644 Test/baseResults/remap.invalid-spirv-1.out create mode 100644 Test/baseResults/remap.invalid-spirv-2.out create mode 100644 Test/remap.invalid-spirv-1.spv create mode 100644 Test/remap.invalid-spirv-2.spv diff --git a/CMakeLists.txt b/CMakeLists.txt index 9bc94b01..14d019e2 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -52,7 +52,7 @@ endif(WIN32) if(${CMAKE_CXX_COMPILER_ID} MATCHES "GNU") add_compile_options(-Wall -Wmaybe-uninitialized -Wuninitialized -Wunused -Wunused-local-typedefs - -Wunused-parameter -Wunused-value -Wunused-variable -Wunused-but-set-parameter -Wunused-but-set-variable) + -Wunused-parameter -Wunused-value -Wunused-variable -Wunused-but-set-parameter -Wunused-but-set-variable -fno-exceptions) add_compile_options(-Wno-reorder) # disable this from -Wall, since it happens all over. elseif(${CMAKE_CXX_COMPILER_ID} MATCHES "Clang") add_compile_options(-Wall -Wuninitialized -Wunused -Wunused-local-typedefs diff --git a/SPIRV/SPVRemapper.cpp b/SPIRV/SPVRemapper.cpp index bc7f4788..4d96df6e 100755 --- a/SPIRV/SPVRemapper.cpp +++ b/SPIRV/SPVRemapper.cpp @@ -135,6 +135,9 @@ namespace spv { const unsigned typeStart = idPos(id); const spv::Op opCode = asOpCode(typeStart); + if (errorLatch) + return 0; + switch (opCode) { case spv::OpTypeInt: // fall through... case spv::OpTypeFloat: return (spv[typeStart+2]+31)/32; @@ -148,8 +151,10 @@ namespace spv { unsigned spirvbin_t::idTypeSizeInWords(spv::Id id) const { const auto tid_it = idTypeSizeMap.find(id); - if (tid_it == idTypeSizeMap.end()) + if (tid_it == idTypeSizeMap.end()) { error("type size for ID not found"); + return 0; + } return tid_it->second; } @@ -253,19 +258,31 @@ namespace spv { { assert(id != spv::NoResult && newId != spv::NoResult); + if (id > bound()) { + error(std::string("ID out of range: ") + std::to_string(id)); + return spirvbin_t::unused; + } + if (id >= idMapL.size()) idMapL.resize(id+1, unused); if (newId != unmapped && newId != unused) { - if (isOldIdUnused(id)) + if (isOldIdUnused(id)) { error(std::string("ID unused in module: ") + std::to_string(id)); + return spirvbin_t::unused; + } - if (!isOldIdUnmapped(id)) + if (!isOldIdUnmapped(id)) { error(std::string("ID already mapped: ") + std::to_string(id) + " -> " - + std::to_string(localId(id))); + + std::to_string(localId(id))); - if (isNewIdMapped(newId)) + return spirvbin_t::unused; + } + + if (isNewIdMapped(newId)) { error(std::string("ID already used in module: ") + std::to_string(newId)); + return spirvbin_t::unused; + } msg(4, 4, std::string("map: ") + std::to_string(id) + " -> " + std::to_string(newId)); setMapped(newId); @@ -299,6 +316,10 @@ namespace spv { process(inst_fn_nop, // ignore instructions [this](spv::Id& id) { id = localId(id); + + if (errorLatch) + return; + assert(id != unused && id != unmapped); } ); @@ -317,14 +338,22 @@ namespace spv { continue; // Find a new mapping for any used but unmapped IDs - if (isOldIdUnmapped(id)) + if (isOldIdUnmapped(id)) { localId(id, unusedId = nextUnusedId(unusedId)); + if (errorLatch) + return; + } - if (isOldIdUnmapped(id)) + if (isOldIdUnmapped(id)) { error(std::string("old ID not mapped: ") + std::to_string(id)); + return; + } // Track max bound maxBound = std::max(maxBound, localId(id) + 1); + + if (errorLatch) + return; } bound(maxBound); // reset header ID bound to as big as it now needs to be @@ -406,6 +435,9 @@ namespace spv { if (typeId != spv::NoResult) { const unsigned idTypeSize = typeSizeInWords(typeId); + if (errorLatch) + return false; + if (idTypeSize != 0) idTypeSizeMap[resultId] = idTypeSize; } @@ -421,17 +453,26 @@ namespace spv { } else if (opCode == spv::Op::OpEntryPoint) { entryPoint = asId(start + 2); } else if (opCode == spv::Op::OpFunction) { - if (fnStart != 0) + if (fnStart != 0) { error("nested function found"); + return false; + } + fnStart = start; fnRes = asId(start + 2); } else if (opCode == spv::Op::OpFunctionEnd) { assert(fnRes != spv::NoResult); - if (fnStart == 0) + if (fnStart == 0) { error("function end without function start"); + return false; + } + fnPos[fnRes] = range_t(fnStart, start + asWordCount(start)); fnStart = 0; } else if (isConstOp(opCode)) { + if (errorLatch) + return false; + assert(asId(start + 2) != spv::NoResult); typeConstPos.insert(start); } else if (isTypeOp(opCode)) { @@ -451,18 +492,24 @@ namespace spv { { msg(2, 2, std::string("validating: ")); - if (spv.size() < header_size) + if (spv.size() < header_size) { error("file too short: "); + return; + } - if (magic() != spv::MagicNumber) + if (magic() != spv::MagicNumber) { error("bad magic number"); + return; + } // field 1 = version // field 2 = generator magic // field 3 = result bound - if (schemaNum() != 0) + if (schemaNum() != 0) { error("bad schema, must be 0"); + return; + } } int spirvbin_t::processInstruction(unsigned word, instfn_t instFn, idfn_t idFn) @@ -472,8 +519,10 @@ namespace spv { const int nextInst = word++ + wordCount; spv::Op opCode = asOpCode(instructionStart); - if (nextInst > int(spv.size())) + if (nextInst > int(spv.size())) { error("spir instruction terminated too early"); + return -1; + } // Base for computing number of operands; will be updated as more is learned unsigned numOperands = wordCount - 1; @@ -555,6 +604,9 @@ namespace spv { const unsigned literalSize = idTypeSizeInWords(idBuffer[literalSizePos]); const unsigned numLiteralIdPairs = (nextInst-word) / (1+literalSize); + if (errorLatch) + return -1; + for (unsigned arg=0; arg 0) @@ -932,6 +1010,9 @@ namespace spv { }, op_fn_nop); + if (errorLatch) + return; + // Chase replacements to their origins, in case there is a chain such as: // 2 = store 1 // 3 = load 2 @@ -965,6 +1046,9 @@ namespace spv { } ); + if (errorLatch) + return; + strip(); // strip out data we decided to eliminate } @@ -1008,6 +1092,9 @@ namespace spv { fn->second.first, fn->second.second); + if (errorLatch) + return; + fn = fnPos.erase(fn); } else ++fn; } @@ -1040,6 +1127,9 @@ namespace spv { [&](spv::Id& id) { if (varUseCount[id]) ++varUseCount[id]; } ); + if (errorLatch) + return; + // Remove single-use function variables + associated decorations and names process( [&](spv::Op opCode, unsigned start) { @@ -1081,6 +1171,9 @@ namespace spv { [&](spv::Id& id) { if (isType[id]) ++typeUseCount[id]; } ); + if (errorLatch) + return; + // Remove single reference types for (const auto typeStart : typeConstPos) { const spv::Id typeId = asTypeConstId(typeStart); @@ -1090,6 +1183,9 @@ namespace spv { stripInst(typeStart); } } + + if (errorLatch) + return; } } @@ -1168,8 +1264,10 @@ namespace spv { unsigned spirvbin_t::idPos(spv::Id id) const { const auto tid_it = idPosR.find(id); - if (tid_it == idPosR.end()) + if (tid_it == idPosR.end()) { error("ID not found"); + return 0; + } return tid_it->second; } @@ -1268,8 +1366,14 @@ namespace spv { const spv::Id resId = asTypeConstId(typeStart); const std::uint32_t hashval = hashType(typeStart); - if (isOldIdUnmapped(resId)) + if (errorLatch) + return; + + if (isOldIdUnmapped(resId)) { localId(resId, nextUnusedId(hashval % softTypeIdLimit + firstMappedID)); + if (errorLatch) + return; + } } } @@ -1315,24 +1419,49 @@ namespace spv { msg(3, 4, std::string("ID bound: ") + std::to_string(bound())); if (options & STRIP) stripDebug(); + if (errorLatch) return; + strip(); // strip out data we decided to eliminate + if (errorLatch) return; + if (options & OPT_LOADSTORE) optLoadStore(); + if (errorLatch) return; + if (options & OPT_FWD_LS) forwardLoadStores(); + if (errorLatch) return; + if (options & DCE_FUNCS) dceFuncs(); + if (errorLatch) return; + if (options & DCE_VARS) dceVars(); + if (errorLatch) return; + if (options & DCE_TYPES) dceTypes(); + if (errorLatch) return; strip(); // strip out data we decided to eliminate + if (errorLatch) return; + stripDeadRefs(); // remove references to things we DCEed + if (errorLatch) return; + // after the last strip, we must clean any debug info referring to now-deleted data if (options & MAP_TYPES) mapTypeConst(); + if (errorLatch) return; + if (options & MAP_NAMES) mapNames(); + if (errorLatch) return; + if (options & MAP_FUNCS) mapFnBodies(); + if (errorLatch) return; if (options & MAP_ALL) { mapRemainder(); // map any unmapped IDs + if (errorLatch) return; + applyMap(); // Now remap each shader to the new IDs we've come up with + if (errorLatch) return; } } diff --git a/SPIRV/SPVRemapper.h b/SPIRV/SPVRemapper.h index f9f369a3..97e3f31f 100755 --- a/SPIRV/SPVRemapper.h +++ b/SPIRV/SPVRemapper.h @@ -39,6 +39,7 @@ #include #include #include +#include namespace spv { @@ -111,7 +112,9 @@ namespace spv { class spirvbin_t : public spirvbin_base_t { public: - spirvbin_t(int verbose = 0) : entryPoint(spv::NoResult), largestNewId(0), verbose(verbose) { } + spirvbin_t(int verbose = 0) : entryPoint(spv::NoResult), largestNewId(0), verbose(verbose), errorLatch(false) + { } + virtual ~spirvbin_t() { } // remap on an existing binary in memory @@ -165,7 +168,7 @@ private: typedef std::unordered_map typesize_map_t; // handle error - void error(const std::string& txt) const { errorHandler(txt); } + void error(const std::string& txt) const { errorLatch = true; errorHandler(txt); } bool isConstOp(spv::Op opCode) const; bool isTypeOp(spv::Op opCode) const; @@ -286,6 +289,11 @@ private: std::uint32_t options; int verbose; // verbosity level + // Error latch: this is set if the error handler is ever executed. It would be better to + // use a try/catch block and throw, but that's not desired for certain environments, so + // this is the alternative. + mutable bool errorLatch; + static errorfn_t errorHandler; static logfn_t logHandler; }; diff --git a/Test/baseResults/remap.invalid-spirv-1.out b/Test/baseResults/remap.invalid-spirv-1.out new file mode 100644 index 00000000..91b4b6d9 --- /dev/null +++ b/Test/baseResults/remap.invalid-spirv-1.out @@ -0,0 +1 @@ +ID out of range: 4160749568 diff --git a/Test/baseResults/remap.invalid-spirv-2.out b/Test/baseResults/remap.invalid-spirv-2.out new file mode 100644 index 00000000..808b9b87 --- /dev/null +++ b/Test/baseResults/remap.invalid-spirv-2.out @@ -0,0 +1 @@ +ID not found diff --git a/Test/remap.invalid-spirv-1.spv b/Test/remap.invalid-spirv-1.spv new file mode 100644 index 0000000000000000000000000000000000000000..e5d59d4fbf88b8ea5f91a9fa540e3d9f38c5a82c GIT binary patch literal 219 zcmYk0OA3H65JS_SwJrn^441$TK5|A=@-xEQ^YldvnR=lB;NVcjxN|XPd?E2+ed1G1t5qqnv?q4*)||5V KKa1boT3HXnG78-Q literal 0 HcmV?d00001 diff --git a/Test/remap.invalid-spirv-2.spv b/Test/remap.invalid-spirv-2.spv new file mode 100644 index 0000000000000000000000000000000000000000..df8c96d8aa1578b7496475a0dd855f03b88a209c GIT binary patch literal 212 zcmYj~OA3H65JTg)>Ov6l1TNi+OYeUn6%ovniUy|5yqUCFItC?)q@czp>XZu7Dtmf- zAK9SXPFh_|nN2DlmfOFY&6pJtu)BM!^u0M5B0p98MSlNO*^))IiyZ9m%^p4c-@`Ab G7S|1^mI}K7 literal 0 HcmV?d00001 diff --git a/Test/runtests b/Test/runtests index 3cceb430..5e873f50 100755 --- a/Test/runtests +++ b/Test/runtests @@ -3,6 +3,7 @@ TARGETDIR=localResults BASEDIR=baseResults EXE=../build/install/bin/glslangValidator +REMAPEXE=../build/install/bin/spirv-remap HASERROR=0 mkdir -p localResults @@ -141,6 +142,7 @@ diff -b $BASEDIR/hlsl.dashI.vert.out $TARGETDIR/hlsl.dashI.vert.out || HASERROR= # # Testing -D and -U # +echo "Testing -D and -U" $EXE -DUNDEFED -UIN_SHADER -DFOO=200 -i -l -UUNDEFED -DMUL=FOO*2 glsl.-D-U.frag > $TARGETDIR/glsl.-D-U.frag.out diff -b $BASEDIR/glsl.-D-U.frag.out $TARGETDIR/glsl.-D-U.frag.out || HASERROR=1 $EXE -D -e main -V -i -DUNDEFED -UIN_SHADER -DFOO=200 -UUNDEFED hlsl.-D-U.frag > $TARGETDIR/hlsl.-D-U.frag.out @@ -149,6 +151,7 @@ diff -b $BASEDIR/hlsl.-D-U.frag.out $TARGETDIR/hlsl.-D-U.frag.out || HASERROR=1 # # Test --client and --target-env # +echo "Testing --client and --target-env" $EXE --client vulkan100 spv.targetVulkan.vert || HASERROR=1 $EXE --client opengl100 spv.targetOpenGL.vert || HASERROR=1 $EXE --target-env vulkan1.0 spv.targetVulkan.vert || HASERROR=1 @@ -159,6 +162,7 @@ $EXE -G100 spv.targetOpenGL.vert || HASERROR=1 # # Testing GLSL entry point rename # +echo "Testing GLSL entry point rename" $EXE -H -e foo --source-entrypoint main glsl.entryPointRename.vert > $TARGETDIR/glsl.entryPointRename.vert.out diff -b $BASEDIR/glsl.entryPointRename.vert.out $TARGETDIR/glsl.entryPointRename.vert.out || HASERROR=1 $EXE -H -e foo --source-entrypoint bar glsl.entryPointRename.vert > $TARGETDIR/glsl.entryPointRename.vert.bad.out @@ -166,6 +170,15 @@ diff -b $BASEDIR/glsl.entryPointRename.vert.bad.out $TARGETDIR/glsl.entryPointRe $EXE -H -e foo --source-entrypoint main glsl.entryPointRename2.vert > $TARGETDIR/glsl.entryPointRename2.vert.out diff -b $BASEDIR/glsl.entryPointRename2.vert.out $TARGETDIR/glsl.entryPointRename2.vert.out || HASERROR=1 +# +# Testing remapper error handling +# +echo "Testing remapper error handling" +$REMAPEXE --do-everything -i remap.invalid-spirv-1.spv -o $TARGETDIR > $TARGETDIR/remap.invalid-spirv-1.out && HASERROR=1 +diff -b $BASEDIR/remap.invalid-spirv-1.out $TARGETDIR/remap.invalid-spirv-1.out || HASERROR=1 +$REMAPEXE --do-everything -i remap.invalid-spirv-2.spv -o $TARGETDIR > $TARGETDIR/remap.invalid-spirv-2.out && HASERROR=1 +diff -b $BASEDIR/remap.invalid-spirv-2.out $TARGETDIR/remap.invalid-spirv-2.out || HASERROR=1 + # # Final checking #