Обзор дефектов кода музыкального софта. Часть 2. Audacity


    Цикл статей про обзор дефектов кода музыкально софта продолжается. Вторым претендентом для анализа выбран аудиоредактор Audacity. Это программа очень популярна и широко используется, как любителями, так и профессионалами в музыкальной индустрии. В этой статье описание фрагментов кода будет дополнительно сопровождаться популярными мемами. Скучно не будет!

    Введение


    Audacity — свободный многоплатформенный аудиоредактор звуковых файлов, ориентированный на работу с несколькими дорожками. Программа распространяется с открытым исходным кодом и работает под управлением таких операционных систем, как Microsoft Windows, Linux, Mac OS X, FreeBSD и других.

    Audacity активно пользуется сторонними библиотеками, поэтому из анализа был исключён каталог lib-src, в котором содержалось ещё около тысячи файлов с исходным кодом разных библиотек. В статью вошли только наиболее интересные ошибки. Для просмотра полного отчёта авторы могут самостоятельно проверить проект, запросив в поддержке временный ключ.

    PVS-Studio — это инструмент для выявления ошибок в исходном коде программ, написанных на языках С, C++ и C#. Работает в среде Windows и Linux.

    Copy-Paste — он везде!




    V523 The 'then' statement is equivalent to the 'else' statement. AButton.cpp 297

    AButton::AButtonState AButton::GetState()
    {
      ....
          if (mIsClicking) {
            state = mButtonIsDown ? AButtonOver : AButtonDown; //ok
          }
          else {
            state = mButtonIsDown ? AButtonDown : AButtonOver; //ok
          }
        }
      }
      else {
        if (mToggle) {
          state = mButtonIsDown ? AButtonDown : AButtonUp; // <= fail
        }
        else {
          state = mButtonIsDown ? AButtonDown : AButtonUp; // <= fail
        }
      }
      return state;
    }

    Приведённый фрагмент кода — идеальный претендент для копирования: достаточно поменять условное выражение и пару констант. К сожалению, автор не довёл дело до конца и оставил в коде две идентичных ветви кода.

    Ещё несколько странных мест:

    • V523 The 'then' statement is equivalent to the 'else' statement. ASlider.cpp 394
    • V523 The 'then' statement is equivalent to the 'else' statement. ExpandingToolBar.cpp 297
    • V523 The 'then' statement is equivalent to the 'else' statement. Ruler.cpp 2422

    Другой пример:

    V501 There are identical sub-expressions 'buffer[remaining — WindowSizeInt — 2]' to the left and to the right of the '-' operator. VoiceKey.cpp 309

    sampleCount VoiceKey::OnBackward (
       const WaveTrack & t, sampleCount end, sampleCount len)
    {
      ....
      int atrend = sgn(buffer[remaining - 2]-buffer[remaining - 1]);
      int ztrend = sgn(buffer[remaining - WindowSizeInt - 2] -
                       buffer[remaining - WindowSizeInt - 2]);
      ....
    }

    При втором вызове функции sgn() в неё передаётся разность одинаковых значений. Скорее всего, хотели получить разницу между соседними элементами буфера, но забыли изменить двойку на единицу после копирования фрагмента строки.

    Неправильное использование функций




    V530 The return value of function 'remove' is required to be utilized. OverlayPanel.cpp 31

    bool OverlayPanel::RemoveOverlay(Overlay *pOverlay)
    {
      const size_t oldSize = mOverlays.size();
      std::remove(mOverlays.begin(), mOverlays.end(), pOverlay);
      return oldSize != mOverlays.size();
    }

    Неправильное использование функции std::remove() так распространено, что такой пример приведён в документации к этой диагностике. Поэтому, чтобы снова не копировать описание из документации, я просто приведу исправленный вариант:

    bool OverlayPanel::RemoveOverlay(Overlay *pOverlay)
    {
      const size_t oldSize = mOverlays.size();
      mOverlays.erase(std::remove(mOverlays.begin(), mOverlays.end(),
        pOverlay), mOverlays.end());
      return oldSize != mOverlays.size();
    }



    V530 The return value of function 'Left' is required to be utilized. ASlider.cpp 973

    wxString LWSlider::GetTip(float value) const
    {
      wxString label;
    
      if (mTipTemplate.IsEmpty())
      {
        wxString val;
    
        switch(mStyle)
        {
        case FRAC_SLIDER:
          val.Printf(wxT("%.2f"), value);
          break;
    
        case DB_SLIDER:
          val.Printf(wxT("%+.1f dB"), value);
          if (val.Right(1) == wxT("0"))
          {
            val.Left(val.Length() - 2);        // <=
          }
          break;
      ....
    }

    Вот как выглядит прототип функции Left():

    wxString Left (size_t count) const

    Очевидно, что строка val не изменится. Скорее всего, изменённую строку хотели сохранить обратно в val, но не прочли документацию к функции.

    Страшный сон пользователей ПК




    V590 Consider inspecting this expression. The expression is excessive or contains a misprint. ExtImportPrefs.cpp 600

    void ExtImportPrefs::OnDelRule(wxCommandEvent& WXUNUSED(event))
    {
      ....
      int msgres = wxMessageBox (_("...."), wxYES_NO, RuleTable);
      if (msgres == wxNO || msgres != wxYES)
        return;
      ....
    }

    Многие пользователи компьютерных программ когда-нибудь нажимали «не туда» и пытались отменить действие… Так вот найденная ошибка в Audacity заключается в том, что условие, проверяющее нажатую кнопку в диалоговом окне, не зависит от того, нажали на «No» или нет :D

    Вот как выглядит таблица истинности для приведённого фрагмента кода:



    Все подобные ошибки в условиях собраны в статье "Логические выражения в C/C++. Как ошибаются профессионалы".

    «while» или «if»?




    V612 An unconditional 'return' within a loop. Equalization.cpp 379

    bool EffectEqualization::ValidateUI()
    {
      while (mDisallowCustom && mCurveName.IsSameAs(wxT("unnamed")))
      {
        wxMessageBox(_("...."),
           _("EQ Curve needs a different name"),
           wxOK | wxCENTRE,
           mUIParent);
        return false;
      }
      ....
    }

    Вот такой цикл написал автор, который выполняется 1 или 0 итераций. Если тут нет ошибки, то нагляднее будет переписать на использование оператора if.

    Использование std::unique_ptr




    V522 Dereferencing of the null pointer 'mInputStream' might take place. FileIO.cpp 65

    std::unique_ptr<wxInputStream> mInputStream;
    std::unique_ptr<wxOutputStream> mOutputStream;
    
    wxInputStream & FileIO::Read(void *buf, size_t size)
    {
       if (mInputStream == NULL) {
          return *mInputStream;
       }
    
       return mInputStream->Read(buf, size);
    }
    
    wxOutputStream & FileIO::Write(const void *buf, size_t size)
    {
       if (mOutputStream == NULL) {
          return *mOutputStream;
       }
    
       return mOutputStream->Write(buf, size);
    }

    Очень странный код обнаружил анализатор. Разыменование указателей происходит в любом случае, независимо от того, равен он нулю или нет.

    V607 Ownerless expression. LoadEffects.cpp 340

    void BuiltinEffectsModule::DeleteInstance(IdentInterface *instance)
    {
       // Releases the resource.
       std::unique_ptr < Effect > {
          dynamic_cast<Effect *>(instance)
       };
    }

    Пример очень интересного применения unique_ptr. Этот «однострочник» (без учёта форматирования) служит для того, чтобы создать unique_ptr и тут же его уничтожить, освободив при этом указатель instance.

    Разное




    V779 Unreachable code detected. It is possible that an error is present. ToolBar.cpp 706

    void ToolBar::MakeRecoloredImage( teBmps eBmpOut, teBmps eBmpIn )
    {
      // Don't recolour the buttons...
      MakeMacRecoloredImage( eBmpOut, eBmpIn );
      return;
      wxImage * pSrc = &theTheme.Image( eBmpIn );
      ....
    }

    Анализатор обнаружил недостижимый код из-за безусловного оператора return в коде.

    V610 Undefined behavior. Check the shift operator '<<'. The left operand '-1' is negative. ExportFFmpeg.cpp 229

    #define AV_VERSION_INT(a, b, c) (a<<16 | b<<8 | c)
    
    ExportFFmpeg::ExportFFmpeg() : ExportPlugin()
    {
      ....
      int canmeta = ExportFFmpegOptions::fmts[newfmt].canmetadata;
      if (canmeta && (canmeta == AV_VERSION_INT(-1,-1,-1)  // <=
                   || canmeta <= avfver))
      {
        SetCanMetaData(true,fmtindex);
      }
      ....
    }

    Намерено делают сдвиг отрицательного числа, что может приводить к неочевидным проблемам.

    V595 The 'clip' pointer was utilized before it was verified against nullptr. Check lines: 4094, 4095. Project.cpp 4094

    void AudacityProject::AddImportedTracks(....)
    {
      ....
      WaveClip* clip = ((WaveTrack*)newTrack)->GetClipByIndex(0);
      BlockArray &blocks = clip->GetSequence()->GetBlockArray();
      if (clip && blocks.size())
      {
        ....
      }
      ....
    }

    В этом условии проверять указатель clip уже поздно, он был разыменован строкой выше.

    Ещё несколько опасных мест:

    • V595 The 'outputMeterFloats' pointer was utilized before it was verified against nullptr. Check lines: 5246, 5255. AudioIO.cpp 5246
    • V595 The 'buffer2' pointer was utilized before it was verified against nullptr. Check lines: 404, 409. Compressor.cpp 404
    • V595 The 'p' pointer was utilized before it was verified against nullptr. Check lines: 946, 974. ControlToolBar.cpp 946
    • V595 The 'mParent' pointer was utilized before it was verified against nullptr. Check lines: 1890, 1903. LV2Effect.cpp 1890

    V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: true. TimeTrack.cpp 296

    void TimeTrack::WriteXML(XMLWriter &xmlFile) const
    {
      ....
      // MB: so why don't we just call Invalidate()? :)
      mRuler->SetFlip(GetHeight() > 75 ? true : true);
      ....
    }

    Похоже один из разработчиков догадался, что такой код не имеет смысла, но вместо исправления кода, решил его прокомментировать.

    V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!j->hasFixedBinCount' and 'j->hasFixedBinCount'. LoadVamp.cpp 169

    wxArrayString VampEffectsModule::FindPlugins(....)
    {
      ....
      if (.... ||
          !j->hasFixedBinCount ||
          (j->hasFixedBinCount && j->binCount > 1))
     {
       ++output;
       continue;
     }
     ....
    }

    Условие является избыточным. Его можно упростить до такого варианта:

    !j->hasFixedBinCount || j->binCount > 1

    И ещё один пример такого кода:

    • V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!j->hasFixedBinCount' and 'j->hasFixedBinCount'. LoadVamp.cpp 297

    Заключение


    Вряд ли такие ошибки будут заметны конечному слушателю, но вот пользователям Audacity это может доставлять большие неудобства.

    Другие обзоры:
    Если вы знаете интересный софт для работы с музыкой и хотите увидеть его в обзоре, то присылайте названия мне на почту.

    Попробовать анализатор PVS-Studio на своём проекте очень легко, достаточно перейти на страницу загрузки.


    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. Code Defects in Music Software. Part 2. Audacity

    Прочитали статью и есть вопрос?
    Часто к нашим статьям задают одни и те же вопросы. Ответы на них мы собрали здесь: Ответы на вопросы читателей статей про PVS-Studio, версия 2015. Пожалуйста, ознакомьтесь со списком.
    PVS-Studio 352,34
    Ищем ошибки в C, C++ и C# на Windows и Linux
    Поделиться публикацией
    Похожие публикации
    Комментарии 21
    • +2
      Многие пользователи компьютерных программ когда-нибудь нажимали «не туда» и пытались отменить действие… Так вот найденная ошибка в Audacity заключается в том, что условие, проверяющее нажатую кнопку в диалоговом окне, не зависит от того, нажали на «No» или нет :D

      Таки зависит, посмотрите на вашу же таблицу еще раз:


      таблица

      • 0
        Таблица верная. И ошибка в коде есть. Суть в том, что результат условия зависит только от того, нажали YES или нет. Добавление дополнительных кнопок также не будет влиять на результат.
        • +2

          Учитывая что других кнопок в том окне нет в принципе (wxYES_NO) — ошибка сводится к тому что слева и справа от || написано одно и то же.

          • +2
            Я согласен, что «нажать на wxNO» = «не нажать на wxYES», но ошибку надо исправить, пока не добавили ещё кнопку. Или хуже того, не скопировали всё это в другое место.
            • +4
              Ну, такое. Код, не рассчитанный на расширение != код с ошибкой.
              • –1
                Ошибки в перечислениях, свичах, каскадах словий и т.п. допускаются как раз из-за таких расчётов.
      • 0
        Ardour?
        • 0
          Спасибо, добавил себе в список.
          • 0
            Поддерживаю, хотя и переживаю за глаза единорога :) не поэтому ли уже пришлось очки одеть? Судя по тому как работают Ardour, JACK и все эти Catia/Claudia/LADISH, Cadence и открытые драйвера внешних многоканальных звуковых карт (а именно — изредка и вопреки здравому смыслу), единорог может заплакать.
            • –2
              В очках отражается энергия звука) Но вы правы насчёт качества кода. Выбор проектов не типичен для нас, они довольно маленькие и я изначально сомневался в возможности работы с ними. Даже думал об объединении нескольких проектов в статью, чтобы хоть как-то затронуть музыкальный софт, но реальность такова, что материал стабильно набирается с каждого проекта.
          • +5
            То, как работает std::remove() — это один из моментов, когда становится стыдно за дизайн языка С++.
            • +2
              Не сказал бы что так уж плохо все. В моей практике очень часто встречалась задача удалить некоторый набор данных из вектора а потом вставить в конец что-то новое. Если бы std::remove() сразу выполнял операцию erase то на один realloc могло быть больше в этом случае…
              • +5
                В том-то и дело. С++ ставит эффективность в приоритет и возможность делать вот подобные Вашей штуки — его суть. Но это не должно называться «remove». От операции с таким простым названием ожидаешь простого, интуитивного поведения. И она должна именно что удалять с изменением размера контейнера. А то, что понадобилось Вам, должно называться как-то типа «remove_no_erase» и вызываться лишь в редких случаях, только полностью осмысленно.
                • +1
                  Тогда уж лучше remove_no_resize :)
                  • 0
                    Ну или так. Главное — чтобы не появлялось желание вызвать её первым делом без чтения документации, потому что «ну, интуитивно же понятно что она должна делать», как это происходит с remove.
            • 0
              Спасибо за статью, но тут явно избыток мемов. Две-три штуки хватило бы.
              Похоже один из разработчиков догадался, что такой код не имеет смысла, но вместо исправления кода, решил его прокомментировать.

              // Иногда мне кажется, что компилятор игнорирует все мои комментарии
              • 0
                Спасибо за материал. Побольше бы разборов на тему работы со звуком
                • –2
                  Благодарю за интерес :-) В списке ещё есть проекты. Наверное, в последней статье в заключении я напишу, что проекты закончились и если больше не предложат, то завершу цикл.
                • 0
                  Интересно, а если исправить все-все найденные ошибки, программа останется работоспособной? Никто не проверял?
                  • –2
                    Останется) А вот прохождение тестов не гаранитируется. Ещё ошибки умеют маскировать более серьёзные проблемы, которые могут быть вскрыты после доработок.
                  • 0
                    trollface

                    Вы проверяете достаточно неплохой код. А как на счёт чего нибудь более грязного? Например Grafx2.
                    http://pulkomandy.tk/projects/GrafX2
                    https://gitlab.com/GrafX2/grafX2


                    SDL2 и SFML кстати незаслуженно обошли.

                    Только полноправные пользователи могут оставлять комментарии. Войдите, пожалуйста.

                    Самое читаемое