WinMM: midi_out_winmm::outHandle is never initialized, so the destructor can call midiOutClose on an indeterminate handle #253

Filing this separately as offered in the observer-polling issue #251

Remainder Generated by Claude Opus 5

Pete
Microsoft

Repo: celtera/libremidi
Version: v5.4.2 / master @ cac4d84bf6bac8e8e807228b272640a213fdebbf
Platform: Windows, WINDOWS_MM backend
File: include/libremidi/backends/winmm/midi_out.hpp

The defect

outHandle has no initializer:

private:
  HMIDIOUT outHandle; // Handle to Midi Output Device
  std::vector<char> buffer;

and the constructor does not set it either:

midi_out_winmm(output_configuration&& conf, winmm_output_configuration&& apiconf)
    : configuration{std::move(conf), std::move(apiconf)}
{
  this->client_open_ = stdx::error{};
}

It is only ever assigned by midiOutOpen inside do_open. But the destructor reads it
unconditionally:

~midi_out_winmm() override
{
  midi_out_winmm::close_port();
  ...
}

stdx::error close_port() override
{
  if (this->outHandle)              // <-- reads an indeterminate value
    midiOutClose(this->outHandle);  // <-- and may close it

  this->outHandle = nullptr;
  return stdx::error{};
}

So on any path where do_open is not reached, close_port() reads an uninitialized pointer and
may hand it to midiOutClose. Reading an indeterminate value of pointer type is undefined
behavior in C++, independently of what WinMM then does with it.

How it is reached

Three paths, easiest first.

1. Construct and destroy without opening — no failure required.
midi_out::midi_out builds the backend eagerly
(midi_out.cpp#L88-L95),
so this is enough:

{
  libremidi::midi_out out{{}, libremidi::winmm_output_configuration{}};
  // never call open_port
}   // ~midi_out_winmm -> close_port() -> midiOutClose(indeterminate)

This is a completely ordinary thing for an application to do: construct the output, enumerate,
decide there is no suitable device (or let the user cancel the dialog), destroy it.

2. open_port fails its name lookup. When neither the display_name nor the port_name pass
matches, open_port returns std::errc::invalid_argument without ever calling do_open. This is
the normal outcome when a remembered device is no longer present — a saved session referring to a
controller that is not plugged in today.

3. midiOutOpen itself fails (MMSYSERR_ALLOCATED, device removed mid-open, etc.). WinMM does
not guarantee it writes to the handle on failure, and do_open does not reset it, so it stays
indeterminate and the destructor reads it.

Why it is worth fixing rather than shrugging at

In the common case midiOutClose will validate the handle and return MMSYSERR_INVALHANDLE,
which close_port discards — it returns stdx::error{} unconditionally, so nothing surfaces to
the caller either way.

The case that concerns me is stale handle reuse. midi_out_winmm is heap-allocated through
std::make_unique, so outHandle contains whatever was previously in that allocation. A block
recycled from a destroyed midi_out_winmm holds that object's old HMIDIOUT value — and WinMM
reuses handle values after a close. Open a port, close it, construct a new midi_out that lands in
the same block and is never opened, and the destructor can close a handle that now belongs to a
different, live port. The symptom is "one of my MIDI outputs silently stopped working", with no
error anywhere and no obvious connection to the object that caused it. That is exactly the shape of
bug that generates unreproducible reports.

Reliably reproducible in debug builds: the MSVC debug CRT fills fresh allocations with 0xCD,
so outHandle is deterministically 0xCDCDCDCDCDCDCDCD — non-null, so the if always passes and
midiOutClose is always called with it. In release builds it depends on what the allocator hands
back, which is why this has been quiet. If you want to see it, a debug build plus a breakpoint on
midiOutClose in the destructor of a never-opened midi_out shows it immediately.

Fix

private:
  HMIDIOUT outHandle{}; // Handle to Midi Output Device, null when the port is closed
  std::vector<char> buffer;

Worth doing at the same time, for path 3 — reset on open failure so a later close_port cannot act
on a handle midiOutOpen may or may not have written:

[[nodiscard]] stdx::error do_open(unsigned int portNumber)
{
  MMRESULT result = midiOutOpen(&this->outHandle, portNumber, 0, 0, CALLBACK_NULL);
  if (result != MMSYSERR_NOERROR)
  {
    this->outHandle = nullptr;
    libremidi_handle_error(configuration, "error creating Windows MM MIDI output port.");
    return from_mmerr(result);
  }

  return stdx::error{};
}

Note: the input backend already does all of this

midi_in_winmm gets it right, which is what makes this look like a simple oversight on the output
side rather than a deliberate difference:

HMIDIIN inHandle{}; // Handle to Midi Input Device, null when the port is closed

its do_open resets to nullptr on midiInOpen failure, and its close_port carries a comment
that is about precisely this hazard:

// Both midi_in::close_port() and ~midi_in_winmm() end up here, and the port
// may never have been opened at all - in which case _mutex may not even
// have been initialized, so this check has to come before the locking.

For what it is worth, I grepped every backend header in the tree for handle members declared
without an initializer, and midi_out_winmm::outHandle is the only one. So this looks like a
one-line fix and the rest of the codebase is already consistent.