KhronosGroup / SPIRV-Reflect

SPIRV-Reflect is a lightweight library that provides a C/C++ reflection API for SPIR-V shader bytecode in Vulkan applications.
Apache License 2.0
671 stars 147 forks source link

Infinite loop in ParseDescriptorBlockVariableUsage when parsing #77

Open rocketz opened 4 years ago

rocketz commented 4 years ago

Hi! I get an infinite loop in ParseDescriptorBlockVariableUsage when trying to call spvReflectCreateShaderModule() on the code below. It seems that it's the array passed to "FailFunc" that upsets it..

` struct S_cbPerObjectBones { float4 v4Bones[4095]; };

shared cbuffer _cbPerObjectBones : register(b4) { S_cbPerObjectBones cbPerObjectBones; };

struct v2f { float4 HPosition: SV_POSITION; float4 Position: POSITION; };

struct a2v_StandardWeighted { float4 Position: POSITION0; int4 Indices: BLENDINDICES; };

float3x4 FailFunc(int4 i, float4 v4Bones[4095]) { float4 mRow1 = v4Bones[i.x+0]; return float3x4(mRow1, mRow1, mRow1); }

v2f Test_VS(a2v_StandardWeighted input) { v2f OUT=(v2f)0; float4 inputPosition = float4(input.Position.xyz, 1); int4 i = input.Indices; float3x4 mMatrix = FailFunc(i, cbPerObjectBones.v4Bones); float4 v4Position = float4(mul(mMatrix, inputPosition), 1); OUT.Position = v4Position; return OUT; } ` Compiled with dxc Version: dxcompiler.dll: 1.5 - 1.5.0.2428 (HEAD, bd57c441)

dxc.exe -T vs_6_0 -spirv -E Test_VS -Fo reflectbug.spirv reflectbug.hlsl

darksylinc commented 4 years ago

I've hit the same bug:

  1. 100000011PixelShader_ps.glsl.zip
  2. spirv.spirv.zip (text)
  3. latestSpirv.bin.zip (binary)

Like OP, it looks like spirv-reflect does not like an array passed to a function in

void LTC_Evaluate( float3 N, float3 V, float3 P, float3x3 Minv, float4 points[4], bool twoSided )

when that array comes from a UBO.

Callstack is an infinite recursion:

1   ParseDescriptorBlockVariableUsage            spirv_reflect.c             2236 0x7fffa9f99cb9 
2   ParseDescriptorBlockVariableUsage            spirv_reflect.c             2236 0x7fffa9f99ce1 
3   ParseDescriptorBlockVariableUsage            spirv_reflect.c             2236 0x7fffa9f99ce1 
4   ParseDescriptorBlockVariableUsage            spirv_reflect.c             2236 0x7fffa9f99ce1 
5   ParseDescriptorBlockVariableUsage            spirv_reflect.c             2263 0x7fffa9f99dbe 
6   ParseDescriptorBlockVariableUsage            spirv_reflect.c             2236 0x7fffa9f99ce1 
7   ParseDescriptorBlockVariableUsage            spirv_reflect.c             2263 0x7fffa9f99dbe 
8   ParseDescriptorBlocks                        spirv_reflect.c             2310 0x7fffa9f99ef5 
9   spvReflectCreateShaderModule                 spirv_reflect.c             3229 0x7fffa9f9c227 
10  Ogre::VulkanProgram::compile                 OgreVulkanProgram.cpp       556  0x7fffa9f44bba 
11  Ogre::VulkanProgram::loadFromSource          OgreVulkanProgram.cpp       278  0x7fffa9f435bb 
12  Ogre::HighLevelGpuProgram::loadHighLevelImpl OgreHighLevelGpuProgram.cpp 299  0x7ffff690b1bc 
13  Ogre::HighLevelGpuProgram::loadHighLevel     OgreHighLevelGpuProgram.cpp 161  0x7ffff690a783 
14  Ogre::HighLevelGpuProgram::loadImpl          OgreHighLevelGpuProgram.cpp 89   0x7ffff690a302 
15  Ogre::Resource::load                         OgreResource.cpp            241  0x7ffff6b1b9af 
16  Ogre::Hlms::compileShaderCode                OgreHlms.cpp                2101 0x7ffff6916fdd 
17  Ogre::Hlms::compileShaderCode                OgreHlms.cpp                2275 0x7ffff69183c1 
18  Ogre::Hlms::createShaderCacheEntry           OgreHlms.cpp                2334 0x7ffff6918a92 
19  Ogre::HlmsPbs::createShaderCacheEntry        OgreHlmsPbs.cpp             515  0x7ffff74dc7b0 
20  Ogre::Hlms::getMaterial                      OgreHlms.cpp                3243 0x7ffff691dd34 
... <More>              
darksylinc commented 4 years ago

Suggested potential fix:

diff --git a/spirv_reflect.c b/spirv_reflect.c
index ae0b78b..7b1260c 100644
--- a/spirv_reflect.c
+++ b/spirv_reflect.c
@@ -2232,6 +2232,10 @@ static SpvReflectResult ParseDescriptorBlockVariableUsage(
         // Next access index
         index_index += 1;
       }
+
+      if( index_index >= p_access_chain->index_count && p_type->op == SpvOpTypeArray )
+          return SPV_REFLECT_RESULT_SUCCESS;
+
       // Parse current var again with a type override and advanced index index
       SpvReflectResult result = ParseDescriptorBlockVariableUsage(
         p_parser,
chaoticbob commented 4 years ago

Thanks for the filing this issue. Will investigate.

cdwfs commented 3 years ago

I can still repro the stack overflow using @rocketz's shader source, compiling with dxc from the 1.2.154.0 SDK (Version: dxcompiler.dll: 1.6 - 1.5.0.2802 (bf5d5854)). If I compile with the original command:

dxc.exe -T vs_6_0 -spirv -E Test_VS -Fo array_from_ubo.spv array_from_ubo.hlsl

and run spirv-reflect on the output in the debugger, I get the infinite loop as described. Interestingly, if I add the -O0 flag when compiling the shader:

dxc.exe -O0 -T vs_6_0 -spirv -E Test_VS -Fo array_from_ubo_with_O0.spv array_from_ubo.hlsl

I get a SPV_REFLECT_RESULT_ERROR_SPIRV_INVALID_BLOCK_MEMBER_REFERENCE error at parse time instead, which seems suspicious. Shader source and both SPIR-V files here: array_from_ubo.zip

nickyc95 commented 3 years ago

I've hit this same issue today.

If you pass through a struct with the array in (rather than the array directly) then it exhibits the same behaviour.

float3x4 FailFunc(int4 i, S_cbPerObjectBones bones)
{
  float4 mRow1 = bones.v4Bones[i.x+0];
  return float3x4(mRow1, mRow1, mRow1);
}
hevrard commented 3 years ago

We hit a similar issue in AGI (https://github.com/google/agi).

The fix suggested in https://github.com/KhronosGroup/SPIRV-Reflect/issues/77#issuecomment-670252132 seems to work fine for us.

hevrard commented 3 years ago

@cdwfs , @chaoticbob , what do you think of the fix suggested by @darksylinc ?

chaoticbob commented 3 years ago

https://github.com/KhronosGroup/SPIRV-Reflect/commit/64b0cc260585ef4759a2a6f9923fc60cfc6f61c4 addressed the infinite loop that @rocketz ran into.

@darksylinc Would you mind checking the SPIR-V binary you uploaded? Both spirv-dis and spirv-reflect thinks it's an invalid SPIR-V binary. I also wasn't able to compile the GLSL source. Here's what I'm seeing:

$ glslangValidator.exe -V -S frag -e main -o 100000011PixelShader_ps.spv 100000011PixelShader_ps.glsl
100000011PixelShader_ps.glsl
ERROR: 100000011PixelShader_ps.glsl:489: '' :  syntax error, unexpected IDENTIFIER
ERROR: 1 compilation errors.  No code generated.

@hevrard The fix in the commit should have accounted for what @darksylinc suggested. I'll report back once I get a test case to make sure it's working.

chaoticbob commented 3 years ago

Just checked against shaders binaries that @cdwfs provided and didn't get an infinite loop or SPV_REFLECT_RESULT_ERROR_SPIRV_INVALID_BLOCK_MEMBER_REFERENCE .

hevrard commented 3 years ago

I confirm the latest version works fine for AGI. Thanks!

chaoticbob commented 3 years ago

Thanks, @hevrard.