Validate undefined Ids and builtin set ids in release builds by idubinov · Pull Request #3817 · KhronosGroup/SPIRV-LLVM-Translator

idubinov · 2026-06-29T15:45:23Z

Malformed input runs an unchecked static_cast on objects leading to type confusion/null-pointer deref and out-of-bound reads that can leak heap memory into the translated output.

This change adds checks and rejection of module, closing the leak and the type-confusion.

idubinov · 2026-06-30T19:02:56Z

@aobolensk @maarquitos14 @MrSidims @vmaksimo

MrSidims

@idubinov I believe the PR should be re-opened.

Please humanize the description. May be it's just me. but I've re-read it several times and didn't understood it. Meanwhile guarding getEntry result seem to fix a potential and real problem.

MrSidims · 2026-07-07T21:59:41Z

+    if (!BM->getErrorLog().checkError(
+            LenExpr && isConstantOpCode(LenExpr->getOpCode()),
+            SPIRVEC_InvalidModule,
+            "OpTypeArray length must reference an integer constant"))


this should be an assertion imho

this runtime check prevents OOB data read

I also fail to see how this can result into OOB data read. A test case showing it would be helpful.

MrSidims · 2026-07-07T21:59:45Z

+    getErrorLog().checkError(false, SPIRVEC_InvalidModule,
+                             "OpTypeArray length must reference a constant");


this should be an assertion imho

this check should be runtime check because it prevents potential uncontrollable data read

@idubinov do you mean it could happen within the translator? I don't see any calls to getArrayLength() that could be dangerous. Am I missing anything?

MrSidims · 2026-07-07T22:02:07Z

+  if (!P) {
+    I.M.getErrorLog().checkError(false, SPIRVEC_InvalidModule,
+                                 "operand references an undefined Id");
+    I.M.setInvalid();
+  }


The check belongs to getEntry implementation

Disagree. The main idea of this code is to label the module invalid in case of try to get incorrect Entry. The getter should not change state of class. I also think that both asserts inside getEntry function were placed by mistake - it is normal behavior for getter to return nullptr in case of no item found

It's not a getter.

@idubinov is it really meaningful to mark the module invalid? Are we not terminating the process with I.M.getErrorLog().checkError(...)?

Disagree. The main idea of this code is to label the module invalid in case of try to get incorrect Entry. The getter should not change state of class. I also think that both asserts inside getEntry function were placed by mistake - it is normal behavior for getter to return nullptr in case of no item found

This contradicts your other patch in https://github.com/KhronosGroup/SPIRV-LLVM-Translator/pull/3798/changes#r3544542206 which adds getEntry error out.

@maarquitos14 , it can be aborted, asserted, or ignored, so, in general, the app is not ends in case of check fail
SPIRVError.h:156

Theoretically, yes, but what I've seen is that it's set to SPIRVDbgErrorHandlingKinds::Exit, and there is no mechanism to change that value. I'm not sure if any downstream tool would ever use that, otherwise we could even think of removing the other two options, since they're not used anywhere. Obviously, that would be a different patch.

@maarquitos14 SPIRVDbgErrorHandlingKinds::Exit is set in this project. but was added for translator to be able to be used by "some custom clang toolchains" (#561). So, I expect that in some custom toolchains the SPIRVDbgError may have different value and checkError may have different behavior

Thanks for the explanation and the reference.

idubinov · 2026-07-08T09:24:54Z

@MrSidims ,

May be it's just me

Me too

maarquitos14 · 2026-07-08T13:04:57Z


 SPIRVExtInstSetKind SPIRVModuleImpl::getBuiltinSet(SPIRVId SetId) const {
  auto Loc = IdToInstSetMap.find(SetId);
-  assert(Loc != IdToInstSetMap.end() && "Invalid builtin set id");


I'm happy to keep the assert too for debugging purposes if this is something that should not happen. To be clear, I mean keeping the assert additionally to the new changes you added. The new changes should be absolutely kept.

maarquitos14 · 2026-07-08T13:18:50Z

+  if (!P) {
+    I.M.getErrorLog().checkError(false, SPIRVEC_InvalidModule,
+                                 "operand references an undefined Id");
+    I.M.setInvalid();
+  }


@idubinov is it really meaningful to mark the module invalid? Are we not terminating the process with I.M.getErrorLog().checkError(...)?

maarquitos14 · 2026-07-08T13:22:21Z

+    getErrorLog().checkError(false, SPIRVEC_InvalidModule,
+                             "OpTypeArray length must reference a constant");


@idubinov do you mean it could happen within the translator? I don't see any calls to getArrayLength() that could be dangerous. Am I missing anything?

maarquitos14 · 2026-07-08T13:26:22Z

+    if (!BM->getErrorLog().checkError(
+            LenExpr && isConstantOpCode(LenExpr->getOpCode()),
+            SPIRVEC_InvalidModule,
+            "OpTypeArray length must reference an integer constant"))


I also fail to see how this can result into OOB data read. A test case showing it would be helpful.

idubinov added 2 commits June 29, 2026 10:39

Validate undefined ids

0d75448

Replace assert with check

bcc4910

idubinov marked this pull request as draft June 29, 2026 15:45

idubinov changed the title ~~SPIR-V reader: validate undefined Ids and builtin set ids in release builds~~ Validate undefined Ids and builtin set ids in release builds Jun 29, 2026

idubinov closed this Jun 29, 2026

idubinov reopened this Jun 29, 2026

Merge branch 'main' into t_ASSERT_E2_id_lookup

49d66db

idubinov marked this pull request as ready for review June 30, 2026 09:16

remove unused

ed6fb44

idubinov closed this Jul 7, 2026

MrSidims reviewed Jul 7, 2026

View reviewed changes

idubinov reopened this Jul 8, 2026

MrSidims requested review from maarquitos14, svenvh and vmaksimo July 8, 2026 09:41

maarquitos14 reviewed Jul 8, 2026

View reviewed changes

idubinov closed this Jul 9, 2026

		getErrorLog().checkError(false, SPIRVEC_InvalidModule,
		"OpTypeArray length must reference a constant");

Uh oh!

Conversation

idubinov commented Jun 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

idubinov commented Jun 30, 2026

Uh oh!

MrSidims left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

idubinov Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

idubinov commented Jul 8, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

idubinov commented Jun 29, 2026 •

edited

Loading

MrSidims left a comment •

edited

Loading

idubinov Jul 8, 2026 •

edited

Loading