diff --git a/CMakeLists.txt b/CMakeLists.txt index a0f0352b..dbe4308c 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -54,7 +54,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 00000000..e5d59d4f Binary files /dev/null and b/Test/remap.invalid-spirv-1.spv differ diff --git a/Test/remap.invalid-spirv-2.spv b/Test/remap.invalid-spirv-2.spv new file mode 100644 index 00000000..df8c96d8 Binary files /dev/null and b/Test/remap.invalid-spirv-2.spv differ 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 #