Skip to content

Commit 42e0389

Browse files
saddamr3esaddamr3e
authored
key memberRemapper by member list to fix buffer_reference oob write (KhronosGroup#4326)
memberRemapper was keyed through glslangTypeToIdMap, whose operator[] hands back id 0 for structs never registered from a visited symbol, so distinct buffer_reference blocks shared one remapper vector. Converting a nested block resized that shared vector smaller and the outer block's next member write went out of bounds. Key it by the member-list pointer so each block gets its own slot.
1 parent 998f38c commit 42e0389

4 files changed

Lines changed: 98 additions & 20 deletions

File tree

‎SPIRV/GlslangToSpv.cpp‎

Lines changed: 11 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -358,10 +358,9 @@ class TGlslangToSpvTraverser : public glslang::TIntermTraverser {
358358
// rather than a pointer
359359
std::unordered_map<std::string, spv::Function*> functionMap;
360360
std::unordered_map<const glslang::TTypeList*, spv::Id> structMap[glslang::ElpCount][glslang::ElmCount];
361-
// for mapping glslang block indices to spv indices (e.g., due to hidden members):
362-
std::unordered_map<long long, std::vector<int>> memberRemapper;
363-
// for mapping glslang symbol struct to symbol Id
364-
std::unordered_map<const glslang::TTypeList*, long long> glslangTypeToIdMap;
361+
// for mapping glslang block indices to spv indices (e.g., due to hidden members),
362+
// keyed by the block's member list so distinct block types never share a slot:
363+
std::unordered_map<const glslang::TTypeList*, std::vector<int>> memberRemapper;
365364
std::stack<bool> breakForLoop; // false means break for switch
366365
std::unordered_map<std::string, const glslang::TIntermSymbol*> counterOriginator;
367366
// Map pointee types for EbtReference to their forward pointers
@@ -2337,8 +2336,6 @@ void TGlslangToSpvTraverser::visitSymbol(glslang::TIntermSymbol* symbol)
23372336
}
23382337

23392338
SpecConstantOpModeGuard spec_constant_op_mode_setter(&builder);
2340-
if (symbol->getType().isStruct())
2341-
glslangTypeToIdMap[symbol->getType().getStruct()] = symbol->getId();
23422339

23432340
if (symbol->getType().getQualifier().isSpecConstant())
23442341
spec_constant_op_mode_setter.turnOnSpecConstantOpMode();
@@ -2544,12 +2541,6 @@ void TGlslangToSpvTraverser::recordDescHeapAccessChainInfo(glslang::TIntermBinar
25442541
bool TGlslangToSpvTraverser::visitBinary(glslang::TVisit /* visit */, glslang::TIntermBinary* node)
25452542
{
25462543
builder.setDebugSourceLocation(node->getLoc().line, node->getLoc().getFilename());
2547-
if (node->getLeft()->getAsSymbolNode() != nullptr && node->getLeft()->getType().isStruct()) {
2548-
glslangTypeToIdMap[node->getLeft()->getType().getStruct()] = node->getLeft()->getAsSymbolNode()->getId();
2549-
}
2550-
if (node->getRight()->getAsSymbolNode() != nullptr && node->getRight()->getType().isStruct()) {
2551-
glslangTypeToIdMap[node->getRight()->getType().getStruct()] = node->getRight()->getAsSymbolNode()->getId();
2552-
}
25532544

25542545
SpecConstantOpModeGuard spec_constant_op_mode_setter(&builder);
25552546
if (node->getType().getQualifier().isSpecConstant())
@@ -2663,9 +2654,9 @@ bool TGlslangToSpvTraverser::visitBinary(glslang::TVisit /* visit */, glslang::T
26632654
{
26642655
// This may be, e.g., an anonymous block-member selection, which generally need
26652656
// index remapping due to hidden members in anonymous blocks.
2666-
long long glslangId = glslangTypeToIdMap[node->getLeft()->getType().getStruct()];
2667-
if (memberRemapper.find(glslangId) != memberRemapper.end()) {
2668-
std::vector<int>& remapper = memberRemapper[glslangId];
2657+
const glslang::TTypeList* glslangMembers = node->getLeft()->getType().getStruct();
2658+
if (memberRemapper.find(glslangMembers) != memberRemapper.end()) {
2659+
std::vector<int>& remapper = memberRemapper[glslangMembers];
26692660
assert(remapper.size() > 0);
26702661
spvIndex = remapper[glslangIndex];
26712662
}
@@ -5939,7 +5930,7 @@ spv::Id TGlslangToSpvTraverser::convertGlslangToSpvType(const glslang::TType& ty
59395930

59405931
// else, we haven't seen it...
59415932
if (type.getBasicType() == glslang::EbtBlock)
5942-
memberRemapper[glslangTypeToIdMap[glslangMembers]].resize(glslangMembers->size());
5933+
memberRemapper[glslangMembers].resize(glslangMembers->size());
59435934
spvType = convertGlslangStructToSpvType(type, glslangMembers, explicitLayout, qualifier);
59445935
}
59455936
break;
@@ -6353,15 +6344,15 @@ spv::Id TGlslangToSpvTraverser::convertGlslangStructToSpvType(const glslang::TTy
63536344
if (glslangMember.type->hiddenMember()) {
63546345
++memberDelta;
63556346
if (type.getBasicType() == glslang::EbtBlock)
6356-
memberRemapper[glslangTypeToIdMap[glslangMembers]][i] = -1;
6347+
memberRemapper[glslangMembers][i] = -1;
63576348
} else {
63586349
if (type.getBasicType() == glslang::EbtBlock) {
63596350
if (filterMember(*glslangMember.type)) {
63606351
memberDelta++;
6361-
memberRemapper[glslangTypeToIdMap[glslangMembers]][i] = -1;
6352+
memberRemapper[glslangMembers][i] = -1;
63626353
continue;
63636354
}
6364-
memberRemapper[glslangTypeToIdMap[glslangMembers]][i] = i - memberDelta;
6355+
memberRemapper[glslangMembers][i] = i - memberDelta;
63656356
}
63666357
// modify just this child's view of the qualifier
63676358
glslang::TQualifier memberQualifier = glslangMember.type->getQualifier();
@@ -6892,7 +6883,7 @@ void TGlslangToSpvTraverser::decorateStructType(const glslang::TType& type,
68926883
glslang::TType& glslangMember = *(*glslangMembers)[i].type;
68936884
int member = i;
68946885
if (type.getBasicType() == glslang::EbtBlock) {
6895-
member = memberRemapper[glslangTypeToIdMap[glslangMembers]][i];
6886+
member = memberRemapper[glslangMembers][i];
68966887
if (filterMember(glslangMember))
68976888
continue;
68986889
}

‎Test/baseResults/spv.bufferReferenceNestedStruct.comp.out‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
spv.bufferReferenceNestedStruct.comp
2+
// Module Version 10000
3+
// Generated by (magic number): 8000b
4+
// Id's are bound by 21
5+
6+
Capability Shader
7+
Capability PhysicalStorageBufferAddressesEXT
8+
Extension "SPV_KHR_physical_storage_buffer"
9+
1: ExtInstImport "GLSL.std.450"
10+
MemoryModel PhysicalStorageBuffer64EXT GLSL450
11+
EntryPoint GLCompute 4 "main"
12+
ExecutionMode 4 LocalSize 16 16 1
13+
Source GLSL 450
14+
SourceExtension "GL_EXT_buffer_reference"
15+
Name 4 "main"
16+
Name 7 "PushConstants"
17+
MemberName 7(PushConstants) 0 "args"
18+
Name 9 "GridInfo"
19+
MemberName 9(GridInfo) 0 "probes"
20+
Name 12 "ProbeInfo"
21+
MemberName 12(ProbeInfo) 0 "validity"
22+
Name 14 "Args"
23+
MemberName 14(Args) 0 "gridInfo"
24+
MemberName 14(Args) 1 "colorSpace"
25+
Name 16 ""
26+
Decorate 7(PushConstants) Block
27+
MemberDecorate 7(PushConstants) 0 Offset 0
28+
MemberDecorate 9(GridInfo) 0 Offset 0
29+
Decorate 11 ArrayStride 4
30+
Decorate 12(ProbeInfo) Block
31+
MemberDecorate 12(ProbeInfo) 0 Offset 0
32+
Decorate 14(Args) Block
33+
MemberDecorate 14(Args) 0 Offset 0
34+
MemberDecorate 14(Args) 1 Offset 8
35+
Decorate 20 BuiltIn WorkgroupSize
36+
2: TypeVoid
37+
3: TypeFunction 2
38+
TypeForwardPointer 6 PhysicalStorageBufferEXT
39+
7(PushConstants): TypeStruct 6
40+
TypeForwardPointer 8 PhysicalStorageBufferEXT
41+
9(GridInfo): TypeStruct 8
42+
10: TypeFloat 32
43+
11: TypeRuntimeArray 10(float)
44+
12(ProbeInfo): TypeStruct 11
45+
8: TypePointer PhysicalStorageBufferEXT 12(ProbeInfo)
46+
13: TypeInt 32 0
47+
14(Args): TypeStruct 9(GridInfo) 13(int)
48+
6: TypePointer PhysicalStorageBufferEXT 14(Args)
49+
15: TypePointer PushConstant 7(PushConstants)
50+
16: 15(ptr) Variable PushConstant
51+
17: TypeVector 13(int) 3
52+
18: 13(int) Constant 16
53+
19: 13(int) Constant 1
54+
20: 17(ivec3) ConstantComposite 18 18 19
55+
4(main): 2 Function None 3
56+
5: Label
57+
Return
58+
FunctionEnd

‎Test/spv.bufferReferenceNestedStruct.comp‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
#version 450
2+
#extension GL_EXT_buffer_reference : require
3+
4+
layout(buffer_reference, std430) buffer ProbeInfo
5+
{
6+
float validity[];
7+
};
8+
9+
struct GridInfo
10+
{
11+
ProbeInfo probes;
12+
};
13+
14+
layout(buffer_reference, std430) buffer Args
15+
{
16+
GridInfo gridInfo;
17+
uint colorSpace;
18+
};
19+
20+
layout(push_constant, std430) uniform PushConstants
21+
{
22+
Args args;
23+
};
24+
25+
layout(local_size_x = 16, local_size_y = 16) in;
26+
void main()
27+
{
28+
}

‎gtests/Spv.FromFile.cpp‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -403,6 +403,7 @@ INSTANTIATE_TEST_SUITE_P(
403403
"spv.bufferhandleRuntimeArray.frag",
404404
"spv.bufferhandleUvec2.frag",
405405
"spv.bufferhandle_Error.frag",
406+
"spv.bufferReferenceNestedStruct.comp",
406407
"spv.builtInXFB.vert",
407408
"spv.conditionalDemote.frag",
408409
"spv.conditionalDiscard.frag",

0 commit comments

Comments
 (0)