Repository navigation
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
@microsoft-github-policy-service agree company="Broadcom" |
There was a problem hiding this comment.
🟡 Changes recommended
Several malformed metadata, cast, function-type, and switch inputs can still reach null dereferences or assertions.
6 open findings
Validate types in legacy function record branch · New Validate type pointer before dereferencing metadata kind · New Guard null operand before upgrading bitcast expression · New Reject non-integer switch condition types before casting · New Fix misspelled constant expression diagnostic · New Document robustness fix in upcoming release notes · New
What changed in this PR
Hardens malformed bitcode and DXIL handling to return diagnostics instead of crashing.
Changes:
- Adds defensive type, value, cast, and diagnostic checks.
- Makes DXIL validation paths tolerate missing functions or symbols.
- Adds three malformed-bitcode regression fixtures.
| File | Description |
|---|---|
lib/Bitcode/Reader/BitcodeReader.cpp |
Adds malformed-input validation. |
lib/DXIL/DxilResourceBase.cpp |
Validates resource symbols and types. |
lib/DXIL/DxilUtilDbgInfoAndMisc.cpp |
Supports diagnostics without a function. |
lib/DxilValidation/DxilValidationUtils.cpp |
Guards debug-function lookup. |
tools/clang/test/DXC/invalid-bitcode.test |
Verifies malformed-input diagnostics. |
tools/clang/test/DXC/Inputs/invalid-bitcode-function-return-type.bc |
Tests invalid function return types. |
tools/clang/test/DXC/Inputs/invalid-bitcode-constexpr-cast.bc |
Tests invalid constant casts. |
tools/clang/test/DXC/Inputs/invalid-bitcode-null-type.bc |
Tests null type handling. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
d0a71dd to
8d14fff
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
🟡 Changes recommended
Some malformed records can still assert, and the new cast test has a stale diagnostic expectation.
3 open findings
6 resolved since last review
Reject non-integer switch condition types before casting Guard null operand before upgrading bitcast expression Validate type pointer before dereferencing metadata kind Validate types in legacy function record branch Document robustness fix in upcoming release notes Fix misspelled constant expression diagnostic
🧠 Review effort: Balanced
8d14fff to
c0fee21
Compare
c0fee21 to
5d173db
Compare
5d173db to
86fe000
Compare
Add error checks so that malformed bitcode or DXIL is rejected with a diagnostic instead of dereferencing a null pointer: - BitcodeReader: check for null types and values returned by getTypeByID/getValueFwdRef in metadata (including the selective named-metadata parser used by IDxcPdbUtils and METADATA_OLD_FN_NODE, which also must reference a function-local value), constants and switch parsing, validate function return and argument types (for both FUNCTION and FUNCTION_OLD type records), switch condition types and record sizes (for both switch record formats), constant cast operands and cast instructions, and replace an assert on function-local metadata with an error. - AutoUpgrade: when upgrading a bitcast between pointers in different address spaces, go through an integer vector for pointer vectors, and do not upgrade casts whose source and destination shapes differ. - report_fatal_error_handler: tolerate a missing diagnostic handler. - DxilResourceBase::GetHLSLType: fail on a missing symbol or type, and DxilMDHelper::LoadDxilResourceBase: reject a non-constant resource symbol before it is used. - EmitWarningOrErrorOnFunction and ValidationContext::EmitFnFormatError: tolerate a null function. Add Bitcode/invalid-bitcode.test with eighteen malformed bitcode inputs, and Bitcode/upgrade-addrspace-bitcast.test and Bitcode/switch-case-range.test with well-formed inputs that must still be read correctly. Three inputs were found by bit-flipping a small valid module; the rest were generated directly to reach records the writer no longer emits, the selective metadata parser, switch and cast instructions, and address space bitcasts. Before this change, all of the malformed inputs except an old-format switch on a float condition, which is silently accepted as invalid IR, crash the reader or end in an uncaught exception with no diagnostic, as do the address space bitcasts that should be upgraded. The tests use llvm-dis, which exits cleanly on a read error, so add it to the check-clang dependencies. Assisted-by: Claude Code
86fe000 to
c4176e2
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Chris B (llvm-beanz)
left a comment
There was a problem hiding this comment.
I have two concerns about this PR that I want to raise:
- This PR seems to violate our AI tool policy. I believe this to be the case because the description indicates that the author has not configured a development environment correctly to actually test and verify the change.
- The inclusion of a bunch of binary files as test collateral is a security issue that gives me strong smells of XZ Utils. We need test cases that are not completely opaque to reviewers.
Yes, my local setup didn't have TAEF, so I only ran the lit suites, with SPIR-V disabled.
Would a small bitcode generator, written by AI, work for you? My original work is based on fuzzing binary shaders to identify a single crash and then finiding & fixing similar issues in the neighbourhood. Also, the Copilot review bot raised a lot of issues that were not about my changes but about existing surrounding code that had other vulns. This PR is the first in a series, so is it OK to address only those comments that point out problems in my changes? |


Malformed bitcode or DXIL can currently crash the bitcode reader, or terminate it with an uncaught exception, instead of returning a diagnostic. This adds the missing checks so that such input is rejected with an error.
Testing: added DXC/invalid-bitcode.test with three malformed bitcode inputs, found by flipping bits in a small valid module. Without this change each one ends in an uncaught exception with no diagnostic. With it, each reports the expected error. The LitDXILValidation, DXC, CodeGenDXIL and HLSLFileCheckLit lit suites pass on Windows (MSVC, x64). The one exception is batch_compile.test, which I couldn't run locally. I have not run the TAEF unit tests.
Assisted-by: Claude Code (test inputs and test file)