MidiInWinMM :: closePort might return without calling LeaveCriticalSection

[tucano42]

During a code review I noticed that the method MidiInWinMM :: closePort looks like it might possibly fail to release an acquired critical section:

void MidiInWinMM :: closePort( void )
{
  if ( connected_ ) {
    WinMidiData *data = static_cast<WinMidiData *> (apiData_);
    EnterCriticalSection( &(data->_mutex) );
    midiInReset( data->inHandle );
    midiInStop( data->inHandle );

    for ( size_t i=0; i < data->sysexBuffer.size(); ++i ) {
      int result = midiInUnprepareHeader(data->inHandle, data->sysexBuffer[i], sizeof(MIDIHDR));
      delete [] data->sysexBuffer[i]->lpData;
      delete [] data->sysexBuffer[i];
      if ( result != MMSYSERR_NOERROR ) {
        midiInClose( data->inHandle );
        data->inHandle = 0;
        errorString_ = "MidiInWinMM::openPort: error closing Windows MM MIDI input port (midiInUnprepareHeader).";
        error( RtMidiError::DRIVER_ERROR, errorString_ );
        return;
      }
    }

    midiInClose( data->inHandle );
    data->inHandle = 0;
    connected_ = false;
    LeaveCriticalSection( &(data->_mutex) );
  }
}

At the end of the for loop, return might be called, and this would exit the function without calling LeaveCriticalSection on the last line. Is this intentional, or a bug? If it is a bug, using a Locker class that will call LeaveCriticalSection in its destructor would fix the issue.

[tucano42]

Here is the complete fix. With this, the critical section will be unlocked for all code paths that can exit the method:

class LockCriticalSection
{
public:
  LockCriticalSection(CRITICAL_SECTION& sect)
  : sect_(sect)
  {
    EnterCriticalSection(&sect_);
  }

  ~LockCriticalSection()
  {
    LeaveCriticalSection(&sect_);
  }
private:
  CRITICAL_SECTION& sect_;
};

void MidiInWinMM :: closePort( void )
{
  if ( connected_ ) {
    WinMidiData *data = static_cast<WinMidiData *> (apiData_);
    LockCriticalSection lock( data->_mutex );
    midiInReset( data->inHandle );
    midiInStop( data->inHandle );

    for ( size_t i=0; i < data->sysexBuffer.size(); ++i ) {
      int result = midiInUnprepareHeader(data->inHandle, data->sysexBuffer[i], sizeof(MIDIHDR));
      delete [] data->sysexBuffer[i]->lpData;
      delete [] data->sysexBuffer[i];
      if ( result != MMSYSERR_NOERROR ) {
        midiInClose( data->inHandle );
        data->inHandle = 0;
        errorString_ = "MidiInWinMM::openPort: error closing Windows MM MIDI input port (midiInUnprepareHeader).";
        error( RtMidiError::DRIVER_ERROR, errorString_ );
        return;
      }
    }

    midiInClose( data->inHandle );
    data->inHandle = 0;
    connected_ = false;
  }
}

[Paul-Dempsey]

I think I may have been observing the subsequent deadlock that will occur by not leaving the cs here. Shouldn't it also close the port and do the rest of the cleanup (e.g. set connected false)? In other words, perhaps the fix can simply be replacing the early return with a break.