Skip to content

Fix UB in void* pointer arithmetic and memory leak in VIF init - #1476

Open
StormBytePP wants to merge 1 commit into
Netflix:masterfrom
StormBytePP:fix_UB_pointer_arith
Open

StormBytePP wants to merge 1 commit into
Netflix:masterfrom
StormBytePP:fix_UB_pointer_arith

Conversation

@StormBytePP

Copy link
Copy Markdown
Contributor

Fix undefined behavior in void* pointer arithmetic and a potential memory leak in the failure path of vif_init().

Changes

  • Replaced all data += N operations (which are UB on void* per C standard) with the portable and well-defined idiom data = (char *)data + N.
  • In the fail: label, we now free the original allocation pointer (s->public.buf.data) instead of the advanced data pointer, preventing a memory leak when vmaf_feature_name_dict_from_provided_features() fails.

Why

  • The original code relied on a GNU extension (treating void* as char* for arithmetic), which is not guaranteed by the C standard and can break under strict conformance or aggressive optimizers.
  • The leak was present even before this change; the UB fix simply made it more visible.

The memory layout and runtime behavior remain identical — only the correctness and portability are improved.

No functional change, only standard corrections.

StormBytePP added a commit to StormBytePP/vmaf that referenced this pull request Mar 29, 2026
StormBytePP added a commit to StormBytePP/vmaf that referenced this pull request Mar 29, 2026
@kylophone

Copy link
Copy Markdown
Collaborator

Maybe just change the original type instead of casting N times?

@StormBytePP

StormBytePP commented Apr 9, 2026

Copy link
Copy Markdown
Contributor Author

Maybe just change the original type instead of casting N times?

I wanted to maintain the original format so the changes introduced are minimal.

Edit: There are some fields which still require explicit casts if original type is changed (at least in mingw) like:

../src/feature/integer_vif.c:636:23: error: assignment to 'uint16_t *' {aka 'short unsigned int *'} from incompatible pointer type 'char *' [-Wincompatible-pointer-types]
  636 |     s->public.buf.mu1 = data; data += h * s->public.buf.stride_16;
      |                       ^
../src/feature/integer_vif.c:637:23: error: assignment to 'uint16_t *' {aka 'short unsigned int *'} from incompatible pointer type 'char *' [-Wincompatible-pointer-types]
  637 |     s->public.buf.mu2 = data; data += h * s->public.buf.stride_16;

So I reverted because I think it is more readable to have all casted uniformly instead of sometimes and sometimes not. What do you think?

@StormBytePP
StormBytePP force-pushed the fix_UB_pointer_arith branch from e1dba8e to d3ef638 Compare April 9, 2026 18:06
Signed-off-by: David C. Manuelda <StormByte@gmail.com>
@StormBytePP
StormBytePP force-pushed the fix_UB_pointer_arith branch from d3ef638 to 8384c37 Compare April 9, 2026 18:11
StormBytePP added a commit to StormBytePP/vmaf that referenced this pull request Aug 9, 2026
Signed-off-by: David C. Manuelda <StormByte@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants