From 0768f5ad2febea80e4766b25547d42fa12b7bd9a Mon Sep 17 00:00:00 2001 From: Dalton Messmer <33463986+messmerd@users.noreply.github.com> Date: Sun, 3 Sep 2023 17:29:31 -0400 Subject: [PATCH] Fix a few memory issues found with ASan (#6843) * Fix LADSPA effects memory leak * Fix buffer overflow in PianoView * Avoid using invalid iterators in AutomationClip * Fix memory leaks in SimpleTextFloat * Handle potential case where QMap::lowerBound(...) returns end iterator * Implement suggestions from review --- plugins/LadspaEffect/caps/Descriptor.h | 3 +- src/core/AutomationClip.cpp | 27 +++---- src/gui/instrument/PianoView.cpp | 103 ++++++++++++------------- src/gui/widgets/SimpleTextFloat.cpp | 4 +- 4 files changed, 62 insertions(+), 75 deletions(-) diff --git a/plugins/LadspaEffect/caps/Descriptor.h b/plugins/LadspaEffect/caps/Descriptor.h index 12c5d1c8846..c3e1c325e73 100644 --- a/plugins/LadspaEffect/caps/Descriptor.h +++ b/plugins/LadspaEffect/caps/Descriptor.h @@ -53,7 +53,7 @@ class DescriptorStub PortCount = 0; } - ~DescriptorStub() + virtual ~DescriptorStub() { if (PortCount) { @@ -87,6 +87,7 @@ class Descriptor public: Descriptor() { setup(); } + ~Descriptor() override = default; void setup(); void autogen() diff --git a/src/core/AutomationClip.cpp b/src/core/AutomationClip.cpp index 906cb148c82..3b36f6b49b7 100644 --- a/src/core/AutomationClip.cpp +++ b/src/core/AutomationClip.cpp @@ -1106,16 +1106,16 @@ void AutomationClip::generateTangents(timeMap::iterator it, int numToGenerate) { QMutexLocker m(&m_clipMutex); - if( m_timeMap.size() < 2 && numToGenerate > 0 ) + for (int i = 0; i < numToGenerate && it != m_timeMap.end(); ++i, ++it) { - it.value().setInTangent(0); - it.value().setOutTangent(0); - return; - } - - for( int i = 0; i < numToGenerate; i++ ) - { - if( it == m_timeMap.begin() ) + if (it + 1 == m_timeMap.end()) + { + // Previously, the last value's tangent was always set to 0. That logic was kept for both tangents + // of the last node + it.value().setInTangent(0); + it.value().setOutTangent(0); + } + else if (it == m_timeMap.begin()) { // On the first node there's no curve behind it, so we will only calculate the outTangent // and inTangent will be set to 0. @@ -1123,14 +1123,6 @@ void AutomationClip::generateTangents(timeMap::iterator it, int numToGenerate) it.value().setInTangent(0); it.value().setOutTangent(tangent); } - else if( it+1 == m_timeMap.end() ) - { - // Previously, the last value's tangent was always set to 0. That logic was kept for both tangents - // of the last node - it.value().setInTangent(0); - it.value().setOutTangent(0); - return; - } else { // When we are in a node that is in the middle of two other nodes, we need to check if we @@ -1159,7 +1151,6 @@ void AutomationClip::generateTangents(timeMap::iterator it, int numToGenerate) it.value().setOutTangent(outTangent); } } - it++; } } diff --git a/src/gui/instrument/PianoView.cpp b/src/gui/instrument/PianoView.cpp index 20db2e8e83f..d20cbcac52e 100644 --- a/src/gui/instrument/PianoView.cpp +++ b/src/gui/instrument/PianoView.cpp @@ -322,70 +322,65 @@ void PianoView::modelChanged() -// gets the key from the given mouse-position +// Gets the key from the given mouse position /*! \brief Get the key from the mouse position in the piano display * - * First we determine it roughly by the position of the point given in - * white key widths from our start. We then add in any black keys that - * might have been skipped over (they take a key number, but no 'white - * key' space). We then add in our starting key number. - * - * We then determine whether it was a black key that was pressed by - * checking whether it was within the vertical range of black keys. - * Black keys sit exactly between white keys on this keyboard, so - * we then shift the note down or up if we were in the left or right - * half of the white note. We only do this, of course, if the white - * note has a black key on that side, so to speak. - * - * This function returns const because there is a linear mapping from - * the point given to the key returned that never changes. - * - * \param _p The point that the mouse was pressed. + * \param p The point that the mouse was pressed. */ -int PianoView::getKeyFromMouse( const QPoint & _p ) const +int PianoView::getKeyFromMouse(const QPoint& p) const { - int offset = _p.x() % PW_WHITE_KEY_WIDTH; - if( offset < 0 ) offset += PW_WHITE_KEY_WIDTH; - int key_num = ( _p.x() - offset) / PW_WHITE_KEY_WIDTH; + // The left-most key visible in the piano display is always white + const int startingWhiteKey = m_pianoScroll->value(); - for( int i = 0; i <= key_num; ++i ) - { - if ( Piano::isBlackKey( m_startKey+i ) ) - { - ++key_num; - } - } - for( int i = 0; i >= key_num; --i ) - { - if ( Piano::isBlackKey( m_startKey+i ) ) - { - --key_num; - } - } + // Adjust the mouse x position as if x == 0 was the left side of the lowest key + const int adjX = p.x() + (startingWhiteKey * PW_WHITE_KEY_WIDTH); + + // Can early return for notes too low + if (adjX <= 0) { return 0; } + + // Now we can calculate the key number (in only white keys) and the octave + const int whiteKey = adjX / PW_WHITE_KEY_WIDTH; + const int octave = whiteKey / Piano::WhiteKeysPerOctave; - key_num += m_startKey; + // Calculate for full octaves + int key = octave * KeysPerOctave; - // is it a black key? - if( _p.y() < PIANO_BASE + PW_BLACK_KEY_HEIGHT ) + // Adjust for white notes in the current octave + // (WhiteKeys maps each white key to the number of notes to their left in the octave) + key += static_cast(WhiteKeys[whiteKey % Piano::WhiteKeysPerOctave]); + + // Might be a black key, which would require further adjustment + if (p.y() < PIANO_BASE + PW_BLACK_KEY_HEIGHT) { - // then do extra checking whether the mouse-cursor is over - // a black key - if( key_num > 0 && Piano::isBlackKey( key_num-1 ) && - offset <= ( PW_WHITE_KEY_WIDTH / 2 ) - - ( PW_BLACK_KEY_WIDTH / 2 ) ) + // Maps white keys to neighboring black keys + static constexpr std::array neighboringKeyMap { + std::pair{ 0, 1 }, // C --> no B#; C# + std::pair{ 1, 1 }, // D --> C#; D# + std::pair{ 1, 0 }, // E --> D#; no E# + std::pair{ 0, 1 }, // F --> no E#; F# + std::pair{ 1, 1 }, // G --> F#; G# + std::pair{ 1, 1 }, // A --> G#; A# + std::pair{ 1, 0 }, // B --> A#; no B# + }; + + const auto neighboringBlackKeys = neighboringKeyMap[whiteKey % Piano::WhiteKeysPerOctave]; + const int offset = adjX - (whiteKey * PW_WHITE_KEY_WIDTH); // mouse X offset from white key + + if (offset < PW_BLACK_KEY_WIDTH / 2) { - --key_num; + // At the location of a (possibly non-existent) black key on the left side + key -= neighboringBlackKeys.first; } - if( key_num < NumKeys - 1 && Piano::isBlackKey( key_num+1 ) && - offset >= ( PW_WHITE_KEY_WIDTH - - PW_BLACK_KEY_WIDTH / 2 ) ) + else if (offset > PW_WHITE_KEY_WIDTH - (PW_BLACK_KEY_WIDTH / 2)) { - ++key_num; + // At the location of a (possibly non-existent) black key on the right side + key += neighboringBlackKeys.second; } + + // For white keys in between black keys, no further adjustment is needed } - // some range-checking-stuff - return qBound( 0, key_num, NumKeys - 1 ); + return std::clamp(key, 0, NumKeys - 1); } @@ -396,12 +391,12 @@ int PianoView::getKeyFromMouse( const QPoint & _p ) const * * We need to update our start key position based on the new position. * - * \param _new_pos the new key position. + * \param newPos the new key position, counting only white keys. */ -void PianoView::pianoScrolled(int new_pos) +void PianoView::pianoScrolled(int newPos) { - m_startKey = static_cast(new_pos / Piano::WhiteKeysPerOctave) - + WhiteKeys[new_pos % Piano::WhiteKeysPerOctave]; + m_startKey = static_cast(newPos / Piano::WhiteKeysPerOctave) + + WhiteKeys[newPos % Piano::WhiteKeysPerOctave]; update(); } diff --git a/src/gui/widgets/SimpleTextFloat.cpp b/src/gui/widgets/SimpleTextFloat.cpp index df438423e27..e37753229ac 100644 --- a/src/gui/widgets/SimpleTextFloat.cpp +++ b/src/gui/widgets/SimpleTextFloat.cpp @@ -46,11 +46,11 @@ SimpleTextFloat::SimpleTextFloat() : m_textLabel = new QLabel(this); layout->addWidget(m_textLabel); - m_showTimer = new QTimer(); + m_showTimer = new QTimer(this); m_showTimer->setSingleShot(true); QObject::connect(m_showTimer, &QTimer::timeout, this, &SimpleTextFloat::show); - m_hideTimer = new QTimer(); + m_hideTimer = new QTimer(this); m_hideTimer->setSingleShot(true); QObject::connect(m_hideTimer, &QTimer::timeout, this, &SimpleTextFloat::hide); }