projectM-visualizer / projectM-visualizer/projectm

[DEV BUG] Custom waveform sample count issues, including possible out-of-bound reads

Open
#1,028 2 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
C++
Stars
4.5k
Forks
485
Avg merge
3d 7h
Merged PRs (30d)
5

Description

While analysing some unexpected preset behaviour with @OfficialIncubo suspecting a bug in expression evaluation, the actual issue at hand turned out to be something else and was based on how samples and other parameters are handled in custom waveforms. There are essentially two separate issues here, which both seem to be actual bugs/issues present in the original Milkdrop 2 code:

  1. The maximum sample count for custom waveforms should be 480 for oscilloscope waves and 512 for spectrum waves. This is calculated properly first, but then always clamped to a max of 512 samples after evaluation the waveform per-frame expression. Exceeding the 480 samples in oscilloscope waveforms will always result in negative sample indexes being used, and thus accessing arbitrary memory before the actual waveform data.
  2. The value of the sep parameter for custom waveforms is not bound-checked in any way, thus it's passed it in as-is from the preset file. Since this parameter is used to calculate sample indexes and thus memory offsets in C arrays, this can lead to out-of-bounds memory access in both directions, with the potential of crashing Milkdrop (and probably all derivatives as well, including projectM) if the value get too large in both positive and negative directions.

The first issue - the max sample count - has two possible, obvious fixes:

  1. Properly clamp it to 480 or 512 values depending on the spectrum parameter. This will break a few presets always expecting 512 samples though, such as Zylot & Krash - Snowflake Halo (Ice Cube mix). This is what projectM, BeatDrop and some other forks currently do.
  2. Allow 512 samples in oscilloscope waves and make sure there are enough samples in the buffer to satisfy the demand. this can be done by either zeroing the up to 32 additional samples, causing a potential flat wave at the end, or by other mean like mirroring the last samples from the buffer required to fill the remaining gap, resulting in a more visually appealing waveform.

The second issue is probably an easy fix, as clamping sep to a range that doesn't throw off the calculated sample offsets (while also taking both the max and actual sample counts into account) would prevent an out-of-bounds read. Since sep affects bot smoothing and the actual sample selection form the buffer, some care must be taken to not break presets using it.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in src/vis_milk2/milkdropfs.cpp at the sample-count calculation around lines 2602-2605 and 2631-2632, then inspect sep handling around lines 2643-2650. Determine the intended oscilloscope and spectrum limits and safe sep range, then verify that custom waveforms cannot produce out-of-bounds sample indexes without unnecessarily breaking existing presets.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
computer-graphics
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.