godotengine / godotengine/godot
GDShader preprocessor evaluates weird functions in `#if` condition expressions
- Dominant language
- C++
- Stars
- 117k
- Forks
- 26.8k
- PR merge metrics
- PR metrics pending
Description
### Tested versions
- Reproducible in v4.3.stable.flathub [77dcf97d82cbfe4e4615475fa52ca03da645dbd8]
### System information
Godot v4.3.stable (77dcf97d8) - Freedesktop SDK 23.08 (Flatpak runtime) - X11 - Vulkan (Forward+) - integrated Intel(R) HD Graphics 5500 (BDW GT2) - Intel(R) Core(TM) i5-5300U CPU @ 2.30GHz (4 Threads)
### Issue description
In GDShader `#if` condition expressions, you can use several operators (more than I expected it to allow) and even functions (e.g. math like `sqrt`, `sin`), which will be evaluated in the preprocessor. Which ones are undocumented.
At first I expected those to at least match the GDShader functions and operator syntax. But I found it's not always the case (e.g. ternary `? :` doesn't work, `inversesqrt(...)` doesn't either).
After some more tests (see https://github.com/godotengine/godot/issues/96253#issuecomment-2318882070 for background), I found it's accepting functions in `@GlobalScope` as well as some constructors like `bool` etc. It allows even functions very weird to allow, like `randf`, `instance_from_id` and `rid_from_int64`.
***Allowing even stuff like `bytes_to_var_with_objects` seems like it could be particularly dangerous (ACE risk?).***
But I don't know for sure to which extent it allows functions with side-effects and whether it affects the editor.
In any case, I'm surprised things like functions work at all. I was expecting that dealing with integers and booleans would be enough for a preprocessor. In fact, ***if the intention is to match C/GLSL, then GDShader is doing way more than it should*** in the preprocessor `#if` directives. ***Not even C deals with float or boolean constants in the preprocessor, let alone math functions. GLSL doesn't either.*** They do handle integers without the `u` suffix (so no `uint` support).
C does handle arbitrary names, treating them like `0` (even `true` is treated like `0`), but GLSL doesn't allow them on `#if`.
GDShader allows most literals (bool, int_decimal, int_hex, uint_decimal, float), except for uint_hex like `0x0u`.
Comments from @pirey0:
> 7 - function calls in #if directives:
> Really interesting stuff! Looks like it could be very useful and at the same time very dangerous. Again probably a topic for a rendering meeting (unless this was already discussed in the past.)
> I brought up this Issue in the 2024-09-03 Rendering Meeting, these are the key points:
> - Aim for glsl-like behavior, so that shaders can be ported over very easily
> - Do not bloat the preprocessor codebase
> - May need to scope back some of the eval() stuff in #if directives
IMO, GDShader should match GLSL, and be as safe as possible:
- only deterministic behavior must be allowed
- no side-effects
- no preprocessor functions, only macros
- no dealing with float at all; no boolean literals either
- there's no need to extrapolate C/GLSL behavior
- keep current GLSL-like (not C-like) behavior of not expanding undefined macro names as `0`
- so even though `true` isn't a keyword, it won't expand to `0` by default; raise "undefined macro" error instead
- only integer literals should be allowed, as well as macros that evaluate to integers;
- like expected in C and GLSL, have 0 mean false and non-zero mean true when on the context of boolean operators (like `&&` and `||`) and the final `#if` expression boolean result
- no arbitrary expressions, only allow:
- int32 literals (decimal and hex) without `u` suffix
- parentheses
- macro expansions (simple and function-like calls)
- logical and arithmetic operators to deal with integers as said above
- the `defined(macro_name)` (also `defined macro_name`) preprocessor keyword is the only "function" allowed
A lot of these behaviors are explicitly defined in the [GLSL ES 3.00 spec](https://registry.khronos.org/OpenGL/specs/es/3.0/GLSL_ES_Specification_3.00.pdf) pages 13~14.
---
Moved from [#96253 (sub-issue 7).](https://github.com/godotengine/godot/issues/96253#issuecomment-2318882070)
### Steps to reproduce
`bug-global-fn-evaluation.gdshader`
```glsl
shader_type spatial;
#if (sin(0) + typeof(print(rid_allocate_id())) - typeof(print(bytes_to_var_with_objects([]))))
// can typing here have editor side-effects? it prints to console at least...
// bytes_to_var_with_objects = arbitrary code execution risk? no idea
code, etc
#endif
#if randf() < 0.5
I have heard of non deterministic compilation but this is crazy
// 50% chance of this code being included every time I type
#endif
$ // error on purpose to log preprocessor output
```
### Minimal reproduction project (MRP)
N/A
Contributor guide
Research direction
Start with the `bug-global-fn-evaluation.gdshader` reproduction and compare its `#if` behavior with the referenced GLSL ES specification. Determine which literals, macros, operators, and calls are accepted, then verify that unsupported functions and side effects are rejected while the intended integer-only behavior remains.
Written by the indexing model from the issue text.
Assessment
- Domain
- compilers, computer-graphics
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100