Skip to content
Snippets Groups Projects

vkd3d-shader: Return a valid pointer when count=0 in param allocator (ubsan).

Merged Francisco Casas requested to merge fcasas/vkd3d:fix_allocator_count0 into master
2 unresolved threads

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

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Elizabeth Figura approved this merge request

    approved this merge request

  • Giovanni Mascellani approved this merge request

    approved this merge request

    • 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 WARNs instead of asserts? Maybe something like warn_if() to just use one line.

      Granted the assert() I put here is not so useful.

    • Please register or sign in to reply
  • added 1 commit

    • 02280e10 - vkd3d-shader: Return a valid pointer when count=0 in param allocator (ubsan).

    Compare with previous version

    • 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."

    • I included that to the commit message since seems to be already well written.

    • Please register or sign in to reply
  • added 1 commit

    • e5239885 - vkd3d-shader: Return a valid pointer when count=0 in param allocator (ubsan).

    Compare with previous version

  • Henri Verbeet approved this merge request

    approved this merge request

  • 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).

    Compare with previous version

  • Alexandre Julliard approved this merge request

    approved this merge request

Please register or sign in to reply
Loading