layers: Check an offset names an instruction before decoding

[apollo-2006]

Fixes the crash in #13134.

FindShaderSource is called with an offset that, for one caller, is not one GPU-AV wrote. The shared memory data race check recovers it from the previous contents of a shadow slot, so when an application indexes its shared memory out of bounds the slot is out of range too and the word read back was never a packed offset. It can still land inside the module without naming an instruction, and decoding it underflows remaining_words in Instruction::Describe() and indexes an empty operand list.

Per the discussion on the issue, the check lives in spirv_logging.cpp and Describe() asserts the invariant rather than checking it. The FindOpStructFromBDA path is left alone: it only ever receives the offset from the error record header, which was a valid instruction boundary in 2826 of 2826 reports I measured.

Verified by forcing instruction_position_offset to a mid instruction offset inside the module, on a build with -D_GLIBCXX_ASSERTIONS, with only these two files differing:

	result
without	exit=134, back() [with _Tp = OperandKind]
with	exit=0, "(specific instruction not recorded)", 5292/5292 backend tests pass

--gtest_filter='*SharedMemoryDataRace*' 71 passed, 6 skipped, 0 failed, and *GpuAVBufferDeviceAddress* 86 passed, 0 failed.

No test here. The out of bounds shared memory access that produces the bad offset also produces false data race reports, and a test for that belongs with the change that detects and reports it, which is the follow up discussed on the issue.

[ci-tester-lunarg]

Author apollo-2006 not on autobuild list. Waiting for curator authorization before starting CI build.

[spencer-lunarg]

1. Did we not have a test for this
2. Instead of returning "(specific instruction not recorded)" lets return a bool and the caller can see if it is false to know it needs to print a specific error message (in this case the shared memory array is likely being accessed OOB)
3. instead of making IsInstructionBoundary reloop the entire SPIR-V again (this is a hot code path here), lets have the

    if (instruction_position_offset < kModuleStartingOffset || instruction_position_offset >= instructions.size()) {

check inside FindShaderSource

then instead of looping you can have GetDebugLineOffset set instruction_position_offset to a value like static constexpr uint32_t kInvalidValue = vvl::kNoIndex32; and then inside FindShaderSource it can go

} else if (instruction_position_offset == invalid) {
    // the offset is OOB and the caller needs to handle this case
    ss << "(instruction offset is larger than SPIR-V)";
    return false;
} else if (instruction_position_offset != 0) {
    spirv::Instruction target_inst(instructions.data() + instruction_position_offset);
    if (target_inst.Length() == 0) {
        ss << "(instruction offset is malformed)";
       return false;
     }
}

this both will save the extra loop, but also provide a more accurate "what is wrong" error message rather then just a general "(specific instruction not recorded)"

[apollo-2006]

2 and 3 are done. GetDebugLineOffset already walks every boundary, so it now reports back rather than adding a second loop: landing exactly on the offset means a boundary, overshooting means it is not one. Exact, no extra pass. It also needed a length == 0 guard, that loop can spin forever on a malformed module. Moving the range check into FindShaderSource let the data race caller drop its own size check.

Need clarity on point 1 though. The word read back from outside the shadow array is whatever LDS holds, and it is usually zero, which is indistinguishable from the "not recorded" marker. Five runs of the out of bounds test, three detections each:

word read back	offset	detectable
0x00000000, usually two of the three	0	no, same as "not recorded"
0x3fc00000, 0x3f590000	130048, 128400	yes

The out of bounds message printed in one run out of five, so the test cannot assert it and users would rarely see it.

In the shader the condition is exact, start + idx >= shadow length. That is the check you pushed back on, but it does not have to skip anything, it can report the out of bounds access as its own error, which surfaces the application bug instead of hiding it.

Which do you want? The host side is built and verified against the crash; the shader side report is a small change I have not written yet.

[spencer-lunarg]

For the test, you don't need to test the specific error, just make sure it prints "some error" (just do m_errorMonitor->SetDesiredError("A data race was detected");) and just make a note in the TEST_DESCRIPTION the test is designed to not crash on the error message (and reference your issue link)

[ci-tester-lunarg]

Author apollo-2006 not on autobuild list. Waiting for curator authorization before starting CI build.

[apollo-2006]

Pushed. Test does what you said, asserts an error prints and the description says it is there to prove building the message does not crash, with the issue link. Passes 6 runs out of 6 here.

Rest of the review is in the same commit: FindShaderSource returns the bool, the range check moved into it so the data race caller could drop its own size check, and boundary detection reuses the walk GetDebugLineOffset already does instead of a second pass. That walk needed a guard too, a zero length instruction made it spin forever.

Suites here: *SharedMemoryDataRace* 78 ran, 72 passed, 6 skipped, 0 failed; *GpuAVBufferDeviceAddress* 86 passed; *GpuAVDescriptorIndexing* 101 passed, that last one because descriptor_validator.cpp is the third caller.

[ci-tester-lunarg]

Author apollo-2006 not on autobuild list. Waiting for curator authorization before starting CI build.

[spencer-lunarg]

sorry, 100% my fault, but since this is a static function, lets just something like

struct DebugLineInfo {
    uint last_line_offset;
    bool valid_offset;
};

static DebugLineInfo GetDebugLineOffset(const std::vector<uint32_t>& instructions, uint32_t instruction_position_offset)

const DebugLineInfo debug_line_info = GetDebugLineOffset(instructions, instruction_position_offset);
const uint32_t last_line_offset = debug_line_info.last_line_offset;

instead of doing half the values returned and half a constant ref

[spencer-lunarg]

Suggested change

	strm << "The shared memory array is likely being accessed out of bounds, which would "
	strm << "Unable to detect source code, most likely because it did an OOB access on the shared memory array.\nNote: The shared memory data race report is not unreliable now."

[ci-tester-lunarg]

Author apollo-2006 not on autobuild list. Waiting for curator authorization before starting CI build.

[apollo-2006]

Done, GetDebugLineOffset returns a DebugLineInfo now and the caller reads one value. Dropped the containers/limits.h include with it, that was only there for the sentinel.

[spencer-lunarg]

lets just do

    if (opcode == spv::OpEntryPoint) {
        ss << " " << string_SpvExecutionModel(Word(1)) << " %" << Word(2) << " [Unknown]";
    } else if (opcode == spv::OpNop) { 
        ss << "OpNop found, which means something else has gone wrong";
    }

[ci-tester-lunarg]

Author apollo-2006 not on autobuild list. Waiting for curator authorization before starting CI build.

[apollo-2006]

All four taken. One word changed in your suggestion: "is not unreliable now" reads as a double negative, so it went in as "is now unreliable". Say the word if you meant it the other way.

[ci-tester-lunarg]

CI Vulkan-ValidationLayers build queued with queue ID 125329.

[ci-tester-lunarg]

CI Vulkan-ValidationLayers build # 24579 running.

[ci-tester-lunarg]

CI Vulkan-ValidationLayers build # 24579 failed.

[ci-tester-lunarg]

Author apollo-2006 not on autobuild list. Waiting for curator authorization before starting CI build.

[ci-tester-lunarg]

CI Vulkan-ValidationLayers build queued with queue ID 125358.

[ci-tester-lunarg]

CI Vulkan-ValidationLayers build # 24580 running.

[ci-tester-lunarg]

CI Vulkan-ValidationLayers build # 24580 failed.

[ci-tester-lunarg]

Author apollo-2006 not on autobuild list. Waiting for curator authorization before starting CI build.