`FormantOscillator::Init()` leaves `ps_inc_` uninitialized (undefined behavior on first `Process()`)

[shakfu]

Summary

FormantOscillator::Init() does not initialize the member ps_inc_, but FormantOscillator::Process() reads it on the very first call (before SetPhaseShift() is ever invoked). Reading an uninitialized float is undefined behavior. In practice the first output sample's phase is corrupted by whatever garbage ps_inc_ holds, which can collapse the oscillator to a stuck, near-DC output on some toolchains while appearing to work on others.

Affected versions

Present in the latest release (V1.0.0) and on main. Source/Synthesis/formantosc.cpp has not been functionally changed since commit e68d5dde (2021-02-05, "Folder reorganization #127"), so every tagged release carries this.

Details

In Source/Synthesis/formantosc.h, ps_inc_ is declared with no in-class initializer:

float phase_shift_;
float ps_inc_;

In Source/Synthesis/formantosc.cpp, Init() initializes every other member but omits ps_inc_:

void FormantOscillator::Init(float sample_rate)
{
    carrier_phase_ = 0.0f;
    formant_phase_ = 0.0f;
    next_sample_   = 0.0f;

    carrier_frequency_ = 0.0f;
    formant_frequency_ = 100.f;
    phase_shift_       = 0.0f;
    // ps_inc_ is never set here

    sample_rate_ = sample_rate;
}

Process() then reads ps_inc_ on the first sample, before any SetPhaseShift() call could have set it:

float FormantOscillator::Process()
{
    ...
    // on a carrier reset, ps_inc_ is read:
    float before = Sine(formant_phase_at_reset + phase_shift_
                        + (ps_inc_ * (1.0f - reset_time)));
    float after  = Sine(0.0f + phase_shift_ + ps_inc_);
    ...
    phase_shift_ += ps_inc_;   // <-- uninitialized read corrupts phase_shift_
    ps_inc_ = 0.f;             // only zeroed *after* the first use
    ...
}

ps_inc_ is only ever assigned in SetPhaseShift():

void FormantOscillator::SetPhaseShift(float ps)
{
    ps_inc_ = ps - phase_shift_;
}

So any usage that does not call SetPhaseShift() before the first Process() reads uninitialized memory. The very first phase_shift_ += ps_inc_; permanently offsets the running phase by a garbage amount, which is why the symptom is a persistently wrong (often stuck/near-constant) output rather than a single bad sample.

Reproduction

#include "daisysp.h"
#include <cstdio>
#include <cmath>

using namespace daisysp;

int main()
{
    FormantOscillator fo;
    fo.Init(48000.0f);
    fo.SetCarrierFreq(220.0f);
    fo.SetFormantFreq(800.0f);

    float peak = 0.0f;
    for (int i = 0; i < 1024; ++i)
        peak = std::fmax(peak, std::fabs(fo.Process()));

    std::printf("peak = %f\n", peak);   // expected ~1.0; can be ~0.0 depending on build/allocation
    return 0;
}

The result is build- and allocation-dependent: with zeroed memory the peak is ~1.0, but with non-zero garbage in ps_inc_ the oscillator can produce a near-constant low-amplitude output (we observed ~0.0079). Building with a memory sanitizer (e.g. -fsanitize=memory on Clang, or MSan/Valgrind) flags the uninitialized read directly.

We hit this as a CI flake: the same test passed on macOS but failed on Linux because the uninitialized ps_inc_ happened to be non-zero there.

Suggested fix

Initialize ps_inc_ in Init():

void FormantOscillator::Init(float sample_rate)
{
    carrier_phase_ = 0.0f;
    formant_phase_ = 0.0f;
    next_sample_   = 0.0f;

    carrier_frequency_ = 0.0f;
    formant_frequency_ = 100.f;
    phase_shift_       = 0.0f;
    ps_inc_            = 0.0f;   // add this

    sample_rate_ = sample_rate;
}

An in-class initializer in formantosc.h (float ps_inc_ = 0.0f;) would also work and additionally guards against Process() being called before Init().

[stephenhensley]

Nice catch!!

I'll be getting around to doing some maintenance on this repo sometime this summer, and can patch this when I do.
If you'd like to open a PR with your proposed fix, you're more than welcome :)