Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add safety check when setting several rendering effect quality #93617

Merged
merged 1 commit into from
Jun 26, 2024

Conversation

jsjtxietian
Copy link
Contributor

Partially fixes #92960 (since I can't reproduce the crash), but nevertheless some protection is needed.

Copy link
Contributor

@BastiaanOlij BastiaanOlij left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems straight forward enough, it's a shame these enums don't have a _MAX entry or it would be nicer to use ERR_FAIL_INDEX.
I guess this could be simplified to for instance ERR_FAIL_INDEX(p_quality, RS::SubSurfaceScatteringQuality::SUB_SURFACE_SCATTERING_QUALITY_HIGH + 1) but I'm personally ok with ERR_FAIL_COND.

@akien-mga akien-mga added this to the 4.3 milestone Jun 26, 2024
@qarmin
Copy link
Contributor

qarmin commented Jun 26, 2024

I don't think that invalid p_quality is problem here, but ss_effects which is null

servers/rendering/renderer_rd/forward_clustered/render_forward_clustered.cpp:3596:39: runtime error: member call on null pointer of type 'struct SSEffects'
servers/rendering/renderer_rd/effects/ss_effects.cpp:1344:24: runtime error: member access within null pointer of type 'struct SSEffects'
================================================================
handle_crash: Program crashed with signal 11
Engine version: Godot Engine v4.3.beta.custom_build (5833f597865c773fae3ee09fc4e31d4a243f812d)
Dumping the backtrace. Please include this when reporting the bug to the project developer.
[1] ./godot.linuxbsd.editor.dev.x86_64.san(+0x354756c1) [0x558e747c96c1] (/home/runner/work/GodotBuilds/GodotBuilds/godot/platform/linuxbsd/crash_handler_linuxbsd.cpp:61)
[2] /lib/x86_64-linux-gnu/libc.so.6(+0x42520) [0x7fc095242520] (??:0)
[3] RendererRD::SSEffects::ssr_set_roughness_quality(RenderingServer::EnvironmentSSRRoughnessQuality) (/home/runner/work/GodotBuilds/GodotBuilds/godot/servers/rendering/renderer_rd/effects/ss_effects.cpp:1344)
[4] RendererSceneRenderImplementation::RenderForwardClustered::environment_set_ssr_roughness_quality(RenderingServer::EnvironmentSSRRoughnessQuality) (/home/runner/work/GodotBuilds/GodotBuilds/godot/servers/rendering/renderer_rd/forward_clustered/render_forward_clustered.cpp:3597)
[5] RendererSceneCull::environment_set_ssr_roughness_quality(RenderingServer::EnvironmentSSRRoughnessQuality) (/home/runner/work/GodotBuilds/GodotBuilds/godot/servers/rendering/renderer_scene_cull.h:1309)

@BastiaanOlij
Copy link
Contributor

I don't think that invalid p_quality is problem here, but ss_effects which is null

Totally missed that but yes :) Its good to check both even if just to keep the automated testing happy

@akien-mga akien-mga merged commit ba3bb44 into godotengine:master Jun 26, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@jsjtxietian jsjtxietian deleted the protect-enum branch June 26, 2024 13:12
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.

Executing RenderingServer.environment_set_ssr_roughness_quality function crashes Godot
4 participants