Skip to content

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

Open
StormBytePP wants to merge 1 commit intoNetflix:masterfrom
StormBytePP:fix_UB_pointer_arith
Open

Fix UB in void* pointer arithmetic and memory leak in VIF init#1476
StormBytePP wants to merge 1 commit intoNetflix: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
Copy link
Copy Markdown
Contributor Author

StormBytePP commented Apr 9, 2026

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
lusoris pushed a commit to lusoris/vmaf that referenced this pull request Apr 18, 2026
…tflix#1476)

Mirror upstream PR Netflix#1476 wholesale — two bugs in the integer VIF init
function:

1. Pointer arithmetic on void * is a GCC extension and UB under strict
   ISO C. All `data += N` lines now go through `(char *)data + N` so the
   arithmetic is well-defined byte-stride.
2. The fail: cleanup label freed the local `data` pointer, which has
   been advanced through the arena by the carve-out loop above. The
   original allocation pointer is `s->public.buf.data`; freeing the
   advanced cursor either leaks or invokes UB on aligned_free's
   header-walk.

Bundled per ADR-0108 (six deep-dive deliverables in same PR):
- rebase-notes entry 0015 documents both invariants for next sync.
- CHANGELOG row added under `### Fixed`.
- Reproducer: meson test -C libvmaf/build (all 27 pass).
- No ADR — pure upstream port, no fork-local design choice.

Re-test:

  ninja -C libvmaf/build && meson test -C libvmaf/build

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
lusoris pushed a commit to lusoris/vmaf that referenced this pull request Apr 19, 2026
Port the leak-fix half of upstream Netflix PR Netflix#1476. `init()` in
`libvmaf/src/feature/integer_vif.c` carves one `aligned_malloc` into
the VifBuffer sub-pointers by walking a `uint8_t *data` cursor forward
through the allocation. When
`vmaf_feature_name_dict_from_provided_features` returned NULL, the
`fail:` handler called `aligned_free(data)` on the advanced cursor
rather than the `aligned_malloc` return, leaking the whole block and
passing a garbage pointer to `free`. Fail path now frees
`s->public.buf.data`, the saved base pointer assigned at init-time.

The companion void*→uint8_t* UB portability half of upstream PR Netflix#1476
is already on master via commit `b0a4ac3a` (rebase-notes 0022 §e), so
this PR was rewritten to carry only the leak-fix delta. 2-line change.

- rebase-notes.md: new entry 0023 with `rg` static-check reproducer
- CHANGELOG.md: Fixed entry cross-linking to Netflix PR Netflix#1476

Validation:
- meson test -C build → 29/29 OK
- No SIMD / GPU impact (cold-path fail handler, same `goto fail` site)

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
lusoris pushed a commit to lusoris/vmaf that referenced this pull request Apr 19, 2026
Port the leak-fix half of upstream Netflix PR Netflix#1476. `init()` in
`libvmaf/src/feature/integer_vif.c` carves one `aligned_malloc` into
the VifBuffer sub-pointers by walking a `uint8_t *data` cursor forward
through the allocation. When
`vmaf_feature_name_dict_from_provided_features` returned NULL, the
`fail:` handler called `aligned_free(data)` on the advanced cursor
rather than the `aligned_malloc` return, leaking the whole block and
passing a garbage pointer to `free`. Fail path now frees
`s->public.buf.data`, the saved base pointer assigned at init-time.

The companion void*→uint8_t* UB portability half of upstream PR Netflix#1476
is already on master via commit `b0a4ac3a` (rebase-notes 0022 §e), so
this PR was rewritten to carry only the leak-fix delta. 2-line change.

- rebase-notes.md: new entry 0023 with `rg` static-check reproducer
- CHANGELOG.md: Fixed entry cross-linking to Netflix PR Netflix#1476

Validation:
- meson test -C build → 29/29 OK
- No SIMD / GPU impact (cold-path fail handler, same `goto fail` site)

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
lusoris pushed a commit to lusoris/vmaf that referenced this pull request Apr 19, 2026
Port the leak-fix half of upstream Netflix PR Netflix#1476. `init()` in
`libvmaf/src/feature/integer_vif.c` carves one `aligned_malloc` into
the VifBuffer sub-pointers by walking a `uint8_t *data` cursor forward
through the allocation. When
`vmaf_feature_name_dict_from_provided_features` returned NULL, the
`fail:` handler called `aligned_free(data)` on the advanced cursor
rather than the `aligned_malloc` return, leaking the whole block and
passing a garbage pointer to `free`. Fail path now frees
`s->public.buf.data`, the saved base pointer assigned at init-time.

The companion void*→uint8_t* UB portability half of upstream PR Netflix#1476
is already on master via commit `b0a4ac3a` (rebase-notes 0022 §e), so
this PR was rewritten to carry only the leak-fix delta. 2-line change.

- rebase-notes.md: new entry 0023 with `rg` static-check reproducer
- CHANGELOG.md: Fixed entry cross-linking to Netflix PR Netflix#1476

Validation:
- meson test -C build → 29/29 OK
- No SIMD / GPU impact (cold-path fail handler, same `goto fail` site)

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
lusoris added a commit to lusoris/vmaf that referenced this pull request Apr 19, 2026
Port the leak-fix half of upstream Netflix PR Netflix#1476. `init()` in
`libvmaf/src/feature/integer_vif.c` carves one `aligned_malloc` into
the VifBuffer sub-pointers by walking a `uint8_t *data` cursor forward
through the allocation. When
`vmaf_feature_name_dict_from_provided_features` returned NULL, the
`fail:` handler called `aligned_free(data)` on the advanced cursor
rather than the `aligned_malloc` return, leaking the whole block and
passing a garbage pointer to `free`. Fail path now frees
`s->public.buf.data`, the saved base pointer assigned at init-time.

The companion void*→uint8_t* UB portability half of upstream PR Netflix#1476
is already on master via commit `b0a4ac3a` (rebase-notes 0022 §e), so
this PR was rewritten to carry only the leak-fix delta. 2-line change.

- rebase-notes.md: new entry 0023 with `rg` static-check reproducer
- CHANGELOG.md: Fixed entry cross-linking to Netflix PR Netflix#1476

Validation:
- meson test -C build → 29/29 OK
- No SIMD / GPU impact (cold-path fail handler, same `goto fail` site)

Co-authored-by: Lusoris <lusoris@pm.me>
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.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