vkd3d-shader: Return a valid pointer when count=0 in param allocator (ubsan).
After compiling and linking with -fsanitize=undefined
the following
error pops up in many tests:
vkd3d_shader_main.c:2024:12: runtime error: member access within null pointer of type 'struct vkd3d_shader_param_node'
This happens in the scenario where shader_param_allocator_get()
gets
called with count = 0
but no allocation has been made yet, so
allocator->current
is NULL.
In this case the result of the function, given by:
params = &allocator->current->param[allocator->index * allocator->stride];
is an invalid non-NULL pointer.
We can't just return NULL on count == 0
, because NULL is interpreted
as a memory allocation failure on the callers. So we force allocation
of the next node even if count = 0
when allocator->current
is NULL.
Thanks @vitorhnn for showing me ubsan!
Merge request reports
Activity
The fix is probably fine, although I wouldn't mind just making an initial allocation in shader_param_allocator_init() either.
Why are we calling shader_param_allocator_get() with a 0 count? Why is that valid? It's not too hard to figure out, but presumably you already did, and it's useful information.
+ assert(allocator->current);
Don't add asserts.
Why are we calling shader_param_allocator_get() with a 0 count? Why is that valid? It's not too hard to figure out, but presumably you already did, and it's useful information.
Currently shader_sm1_read_instruction() and shader_sm4_read_instruction() both call vsir_program_get_src_params() and vsir_program_get_dst_params() for every instruction read, regardless of the number of src or dst parameters. I am not sure if these are the only sources though.
My guess is that it should be valid since a malloc(0) is valid (granted, the implementation is allowed to return NULL) albeit some callers like shader_instruction_array_clone_instruction() perform checks to avoid passing
count = 0
.Don't add asserts.
Okay, I will try to remember that policy from now on, I understand the purpose is to lower the chance of regressions because of failing asserts.
How do you feel about using conditional
WARN
s instead of asserts? Maybe something likewarn_if()
to just use one line.Granted the assert() I put here is not so useful.
added 1 commit
- 02280e10 - vkd3d-shader: Return a valid pointer when count=0 in param allocator (ubsan).
Why are we calling shader_param_allocator_get() with a 0 count? Why is that valid? It's not too hard to figure out, but presumably you already did, and it's useful information.
Currently shader_sm1_read_instruction() and shader_sm4_read_instruction() both call vsir_program_get_src_params() and vsir_program_get_dst_params() for every instruction read, regardless of the number of src or dst parameters. I am not sure if these are the only sources though.
My guess is that it should be valid since a malloc(0) is valid (granted, the implementation is allowed to return NULL) albeit some callers like shader_instruction_array_clone_instruction() perform checks to avoid passing
count = 0
.Right, but we'd like to see that information in the commit message. I.e., what I'd like to read when looking at the commit log would be something along these lines: "Functions like shader_sm4_read_instruction() may call vsir_program_get_src_params() or vsir_program_get_dst_params() with 0 counts for various DCL_ instructions, as well as things like NOP, ELSE, and SYNC. We could avoid calling the functions in question with 0 counts, but it doesn't seem worth the effort."
added 1 commit
- e5239885 - vkd3d-shader: Return a valid pointer when count=0 in param allocator (ubsan).
added 62 commits
-
e5239885...28d267b7 - 61 commits from branch
wine:master
- 5b719128 - vkd3d-shader: Return a valid pointer when count=0 in param allocator (ubsan).
-
e5239885...28d267b7 - 61 commits from branch