Skip to content

Fix re-spirv null pointer crash on invalid SPIR-V parsing#113708

Merged
akien-mga merged 1 commit intogodotengine:masterfrom
mxtherfxcker:patch
Dec 9, 2025
Merged

Fix re-spirv null pointer crash on invalid SPIR-V parsing#113708
akien-mga merged 1 commit intogodotengine:masterfrom
mxtherfxcker:patch

Conversation

@mxtherfxcker
Copy link
Contributor

@mxtherfxcker mxtherfxcker commented Dec 7, 2025

Description

Fixes a crash in re-spirv when parsing invalid or unsupported SPIR-V constructs (e.g., mediump/lowp keywords).
This fix does not resolve common issues with precision qualifiers and etc., as their processing is not implemented in re-spirv.

Problem

The respv::Shader constructor was not checking the result of parse() method. When parse() failed due to invalid SPIR-V, the Shader object remained uninitialized, but was then passed to Optimizer::run(), causing a null pointer dereference crash.

Related Issues

Closes #113665
Related to #111452 (re-spirv integration)
Similar to #113684 (closed by me)

Bugsquad edit: Fixes: #113675

@DarioSamo
Copy link
Contributor

DarioSamo commented Dec 7, 2025

Hi, thanks for the report.

Can you try using the version from this commit instead as well as updating the hash on the thirdparty/README.md to b3aaf5151bf64c73f7236486be57c80cf196476c if it works? It shouldn't be necessary to add a new method either, the empty() method seemed to be not implemented by mistake and it should be enough.

renderbag/re-spirv@b3aaf51

@mxtherfxcker
Copy link
Contributor Author

Hi, thanks for the report.

Can you try using the version from this commit instead as well as updating the hash on the thirdparty/README.md to b3aaf5151bf64c73f7236486be57c80cf196476c if it works? It shouldn't be necessary to add a new method either, the empty() method seemed to be not implemented by mistake and it should be enough.

renderbag/re-spirv@b3aaf51

Okay, thank you for clarifying that. I will test your current implementation now.

@mxtherfxcker
Copy link
Contributor Author

Hi, thanks for the report.

Can you try using the version from this commit instead as well as updating the hash on the thirdparty/README.md to b3aaf5151bf64c73f7236486be57c80cf196476c if it works? It shouldn't be necessary to add a new method either, the empty() method seemed to be not implemented by mistake and it should be enough.

renderbag/re-spirv@b3aaf51

Yes, it works. I can update the pr - add only the file from your commit and update the readme. Or should I keep the current implementation?

@DarioSamo
Copy link
Contributor

You should update that and keep the change to the driver that adds an if branch, but you should be able to just use the empty() method alone. I can do a review directly in the code afterwards if anything else needs to be addressed.

@mxtherfxcker mxtherfxcker force-pushed the patch branch 2 times, most recently from a3371d6 to 6fbd52a Compare December 8, 2025 08:21
@DarioSamo
Copy link
Contributor

The changes to the third party folder must be reverted and updated with the files from the commit I linked before.

@DarioSamo
Copy link
Contributor

Alternatively if you want, I can just merge this PR into mine since mine hasn't been merged yet and it also updates re-spirv to a newer version. #113582

@mxtherfxcker
Copy link
Contributor Author

Alternatively if you want, I can just merge this PR into mine since mine hasn't been merged yet and it also updates re-spirv to a newer version. #113582

Yes, if it's not too much trouble.

@DarioSamo
Copy link
Contributor

Yes, if it's not too much trouble.

Oops, mine was merged. That said, I should be perfectly able to update your PR myself, so I'll do that.

@DarioSamo
Copy link
Contributor

I've updated your branch, let me know if it resolves your issue still and I'll approve it.

@DarioSamo DarioSamo force-pushed the patch branch 2 times, most recently from abd4468 to c167cd0 Compare December 8, 2025 19:13
- Fix behavior regression from decoration change.
- Empty shader fix.
- Add image query ops.

Co-authored-by: DarioSamo <dariosamo@gmail.com>
@mxtherfxcker
Copy link
Contributor Author

I've updated your branch, let me know if it resolves your issue still and I'll approve it.

Oh, thanks! Yes, it solves the problem completely.

@akien-mga akien-mga merged commit 03b0a0b into godotengine:master Dec 9, 2025
20 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Regression on Vulkan support for OpImageQueryLod mediump crashes the engine in 4.6dev6 in vulkan

4 participants