Skip to content

fix: free leaked vorbis_info/vorbis_comment on recursive error unwind#126

Open
SongTonyLi wants to merge 1 commit intoxiph:mainfrom
SongTonyLi:fix/issue-125-bisect-forward-serialno-leak
Open

fix: free leaked vorbis_info/vorbis_comment on recursive error unwind#126
SongTonyLi wants to merge 1 commit intoxiph:mainfrom
SongTonyLi:fix/issue-125-bisect-forward-serialno-leak

Conversation

@SongTonyLi
Copy link
Copy Markdown

@SongTonyLi SongTonyLi commented May 6, 2026

Summary

Fixes #125.

_bisect_forward_serialno() parses intermediate link headers into local temporaries (vorbis_info, vorbis_comment, next_serialno_list) via _fetch_headers(). When a deeper recursive call fails, the early return at line 582 exits without freeing those locals. Ownership was never transferred to vf->vi[m+1]/vf->vc[m+1], so ov_clear() can't reclaim them. This leaks ~27 KB per invocation on crafted multi-stream Ogg files.

Root cause

Call chain: ov_open_callbacks() -> _ov_open2() -> _open_seekable2() -> _bisect_forward_serialno().

I audited all six return sites in _bisect_forward_serialno() (lines 548, 551, 569, 572, 582, 598). Five are safe: three fire before _fetch_headers() allocates anything, one fires when _fetch_headers() itself fails (bail_header cleans up), and the success path transfers ownership then frees the list. Line 582 is the sole leak edge: it fires after a successful _fetch_headers() filled the locals but before ownership transfers at lines 590-591.

Forward instrumentation confirmed this: at m=0 (serialno 8738), the successful parse yields live heap pointers in vi.codec_setup, vc.vendor, vc.user_comments, and next_serialno_list. The deeper m=1 parse then fails with ret=-133, and unwind at m=0 shows those same pointers leaked.

Fix

Two-site patch in lib/vorbisfile.c:

  1. _bisect_forward_serialno() recursive error path: adds vorbis_info_clear(&vi), vorbis_comment_clear(&vc), and _ogg_free(next_serialno_list) before returning the error code.
  2. _fetch_headers() bail path: frees *serialno_list and zeroes *serialno_n when present, before clearing vi/vc.

Why this approach

I looked at three possible fix sites:

  • Caller-side cleanup in _bisect_forward_serialno() (went with this one): the function owns the locals until explicit transfer, so this is the only place that can actually reach them on recursive failure.
  • _fetch_headers() bail cleanup only: doesn't work because bail only runs when _fetch_headers() itself fails, not when a later recursive call fails after a successful header parse.
  • ov_clear() / _ov_open2() teardown: doesn't work either since the leaked objects are stack locals that never got assigned to vf->vi[m+1]/vf->vc[m+1], so ov_clear() has no way to reach them.

Verification

Tested with an ASan+LeakSanitizer-instrumented build:

  • Before: SUMMARY: AddressSanitizer: 26929 byte(s) leaked in 89 allocation(s).
  • After: no LeakSanitizer findings, clean exit

Reproducer:

ASAN_OPTIONS=detect_leaks=1 /src/harness_bin /testcase/poc.ogg

Fixes xiph#125.

_bisect_forward_serialno() parses intermediate link headers into local
temporaries (vorbis_info, vorbis_comment, next_serialno_list) via
_fetch_headers(). When a deeper recursive call fails, the early return
exits without freeing those locals — leaking ~27 KB per invocation on
crafted multi-stream Ogg files.

Two-site fix in lib/vorbisfile.c:
1. _bisect_forward_serialno() recursive error path: add
   vorbis_info_clear/vorbis_comment_clear/_ogg_free before return.
2. _fetch_headers() bail path: free *serialno_list when present.
@SongTonyLi SongTonyLi marked this pull request as ready for review May 6, 2026 14:03
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.

Memory leak in header parsing via _vorbis_unpack_comment during multi-stream file open

1 participant