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


    Я продолжаю обзор кода музыкальных приложений, и перед нами первый представитель коммерческого программного обеспечения. В комментариях к предыдущим статьям я заметил популярность программы Cubase и решил почитать о ней. Это продукт компании Steinberg, у которой есть несколько программ с закрытым исходным кодом. Случайно на их сайте я нашёл SDK для сторонних разработчиков, и, изучив его, обнаружил множество интересных ошибок.

    Введение


    Steinberg GmbH (Steinberg Media Technologies GmbH) – немецкая компания, основанная в Гамбурге, разрабатывающая музыкальное программное обеспечение и оборудование. В большей степени она производит программное обеспечение для записи, аранжировки и редактирования музыки для использования как на цифровых аудио рабочих станциях, так и на синтезаторах с программным обеспечением VSTi формата. Steinberg — дочерняя компания Yamaha Corporation, Steinberg является полной собственностью Yamaha Corporation.



    Даже для небольшого количества исходников из SDK будет мало одной обзорной статьи, поэтому для просмотра полного отчёта авторы могут самостоятельно проверить проект, запросив в поддержке временный ключ для оценки возможностей статического анализатора PVS-Studio. Это инструмент для выявления ошибок в исходном коде программ, написанных на языках С, C++ и C#. Работает в средах Windows и Linux.

    Оператор «запятая» (,)


    Оператор «запятая» (,) используется для выполнения стоящих по обе стороны от него выражений в порядке слева направо и получения значения правого выражения. Чаще всего оператор применяется в выражении изменения счётчика для цикла for. Иногда его удобно использовать в макросах отладки и тестирования. Но чаще всего этим оператором злоупотребляют и используют неправильно.

    V521 Such expressions using the ',' operator are dangerous. Make sure the expression 'i < temp, i < numParams' is correct. mdaBaseProcessor.cpp 309

    tresult PLUGIN_API BaseProcessor::setState (IBStream* state)
    {
      ....
      // read each parameter
      for (uint32 i = 0; i < temp, i < numParams; i++)
      {
        state->read (&params[i], sizeof (ParamValue));
        SWAP64_BE(params[i])
      }
      ....
    }

    Небольшой пример неправильного использования оператора «запятая». Непонятно, что хотел сказать этим автор кода. Код вроде выглядит безобидным, поэтому перейдём к следующему примеру.

    V521 Such expressions using the ',' operator are dangerous. Make sure the expression is correct. mdaBaseProcessor.cpp 142

    bool BaseProcessor::bypassProcessing (ProcessData& data)
    {
      ....
      for (int32 bus = 0; bus < data.numInputs,   // <=
                          bus < data.numOutputs; bus++)
      {
        ....
        if (data.numInputs <= bus ||
            data.inputs[bus].numChannels <= channel)
        {
          memset(data.outputs[bus].channelBuffers32[channel], ....);
          data.outputs[bus].silenceFlags |= (uint64)1 << channel;
        }
        else
        {
          ....
        }
        ....
      }
      ....
    }

    Вот тут уже допущена серьёзная ошибка. В цикле обращаются к массивам data.inputs и data.outputs, но условное выражение написано с ошибкой. Выражение bus < data.numInputs хоть и вычисляется, но на результат не влияет. Следовательно, возможно обращение к памяти за пределом массива data.inputs.

    Я специально привёл два примера, чтобы показать, что один из программистов злоупотребляет использованием этим оператором и допускает ошибки.

    Разные ошибки


    V567 Undefined behavior. The 'p' variable is modified while being used twice between sequence points. mdaAmbienceProcessor.cpp 151

    void AmbienceProcessor::doProcessing (ProcessData& data)
    {
      ....
      ++p  &= 1023;
      ++d1 &= 1023;
      ++d2 &= 1023;
      ++d3 &= 1023;
      ++d4 &= 1023;
      ....
    }

    Анализатор обнаружил выражения, которые приводят к неопределенному поведению программы. Переменные неоднократно используются между двумя точками следования, при этом их значения изменяются. В итоге невозможно предсказать результат работы такого выражения. Всего найдено около 11 таких мест.

    V595 The 'inputBitmap' pointer was utilized before it was verified against nullptr. Check lines: 409, 410. cbitmapfilter.cpp 409

    bool run (bool replace) override
    {
      CBitmap* inputBitmap = getInputBitmap ();
      uint32_t radius = static_cast<uint32_t>(static_cast<double>(
        .... * inputBitmap->getPlatformBitmap()->getScaleFactor());
      if (inputBitmap == nullptr || radius == UINT_MAX)
        return false;
      ....
    }

    Указатель inputBitmap сравнивается с nullptr сразу после использования. Программист хотел проверить указатель inputBitmap и переменную radius в одном условии, но так сделать невозможно, т.к. одно значение вычисляется с помощью другого. Необходимо проверять каждую переменную по отдельности.

    V1004 The 'module' pointer was used unsafely after it was verified against nullptr. Check lines: 76, 84. audiohost.cpp 84

    void App::startAudioClient (....)
    {
      std::string error;
      module = VST3::Hosting::Module::create (path, error);
      if (!module)
      {
        std::string reason = "Could not create Module for file:";
        reason += path;
        reason += "\nError: ";
        reason += error;
        // EditorHost::IPlatform::instance ().kill (-1, reason);
      }
      auto factory = module->getFactory ();
      ....
    }

    Ранее, если указатель module был равен нулю, выполнение функции прерывалось с помощью вызова kill(). Сейчас вызов этой функции закомментирован, поэтому появился риск разыменования нулевого указателя.

    V766 An item with the same key '0xff9b' has already been added. x11frame.cpp 51

    using VirtMap = std::unordered_map<guint, uint16_t>;
    const VirtMap keyMap = {
      {GDK_KEY_BackSpace, VKEY_BACK},
      {GDK_KEY_Tab, VKEY_TAB},
      {GDK_KEY_ISO_Left_Tab, VKEY_TAB},
      {GDK_KEY_Clear, VKEY_CLEAR},
      {GDK_KEY_Return, VKEY_RETURN},
      {GDK_KEY_Pause, VKEY_PAUSE},
      {GDK_KEY_Escape, VKEY_ESCAPE},
      {GDK_KEY_space, VKEY_SPACE},
      {GDK_KEY_KP_Next, VKEY_NEXT},          // <=
      {GDK_KEY_End, VKEY_END},
      {GDK_KEY_Home, VKEY_HOME},
    
      {GDK_KEY_Left, VKEY_LEFT},
      {GDK_KEY_Up, VKEY_UP},
      {GDK_KEY_Right, VKEY_RIGHT},
      {GDK_KEY_Down, VKEY_DOWN},
      {GDK_KEY_Page_Up, VKEY_PAGEUP},
      {GDK_KEY_Page_Down, VKEY_PAGEDOWN},
      {GDK_KEY_KP_Page_Up, VKEY_PAGEUP},
      {GDK_KEY_KP_Page_Down, VKEY_PAGEDOWN}, // <=
      ....
    };

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

    using VirtMap = std::unordered_map<guint, uint16_t>;
    const VirtMap keyMap = {
      {0xff08, VKEY_BACK},
      {0xff09, VKEY_TAB},
      {0xfe20, VKEY_TAB},
      {0xff0b, VKEY_CLEAR},
      {0xff0d, VKEY_RETURN},
      {0xff13, VKEY_PAUSE},
      {0xff1b, VKEY_ESCAPE},
      {0x020, VKEY_SPACE},
      {0xff9b, VKEY_NEXT},     // <=
      {0xff57, VKEY_END},
      {0xff50, VKEY_HOME},
    
      {0xff51, VKEY_LEFT},
      {0xff52, VKEY_UP},
      {0xff53, VKEY_RIGHT},
      {0xff54, VKEY_DOWN},
      {0xff55, VKEY_PAGEUP},
      {0xff56, VKEY_PAGEDOWN},
      {0xff9a, VKEY_PAGEUP},
      {0xff9b, VKEY_PAGEDOWN}, // <=
      ....
    };

    Действительно, константы GDK_KEY_KP_Next и GDK_KEY_KP_PageDown имеют одинаковое значение 0xff9b. Вот только не понятно, что с этим делать, т.к. константы взяты из библиотеки GDK3.

    Несколько примеров из тестов


    V571 Recurring check. The 'if (vstPlug)' condition was already verified in line 170. vsttestsuite.cpp 172

    bool VstTestBase::teardown ()
    {
      if (vstPlug)
      {
        if (vstPlug)
        {
          vstPlug->activateBus (kAudio, kInput, 0, false);
          vstPlug->activateBus (kAudio, kOutput, 0, false);
        }
        plugProvider->releasePlugIn (vstPlug, controller);
      }
      return true;
    }

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

    bool VstTestBase::teardown ()
    {
      if (plugProvider) // <=
      {
        if (vstPlug)
        {
          vstPlug->activateBus (kAudio, kInput, 0, false);
          vstPlug->activateBus (kAudio, kOutput, 0, false);
        }
        plugProvider->releasePlugIn (vstPlug, controller);
      }
      return true;
    }

    V773 The function was exited without releasing the 'paramIds' pointer. A memory leak is possible. vsttestsuite.cpp 436

    bool PLUGIN_API VstScanParametersTest::run (....)
    {
      ....
      int32* paramIds = new int32[numParameters];
    
      bool foundBypass = false;
      for (int32 i = 0; i < numParameters; ++i)
      {
        ParameterInfo paramInfo = {0};
    
        tresult result = controller->getParameterInfo (i, paramInfo);
        if (result != kResultOk)
        {
          addErrorMessage (testResult,
            printf ("Param %03d: is missing!!!", i));
          return false; // Memory Leak
        }
    
        int32 paramId = paramInfo.id;
        paramIds[i] = paramId;
        if (paramId < 0)
        {
          addErrorMessage (testResult,
            printf ("Param %03d: Invalid Id!!!", i));
          return false; // Memory Leak
        }
      ....
      if (paramIds)
        delete[] paramIds;
    
      return true;
    }

    Функция run() имеет более десяти точек выхода, при которых происходит утечка памяти. Только при выполнении функции до конца будет выполнено освобождение памяти для этого массива по указателю paramIds.

    Замечания по коду


    V523 The 'then' statement is equivalent to the 'else' statement. mdaJX10Processor.cpp 522

    void JX10Processor::noteOn (....)
    {
      ....
      if (!polyMode) //monophonic retriggering
      {
        voice[v].env += SILENCE + SILENCE;
      }
      else
      {
        //if (params[15] < 0.28f) 
        //{
        //  voice[v].f0 = voice[v].f1 = voice[v].f2 = 0.0f;
        //  voice[v].env = SILENCE + SILENCE;
        //  voice[v].fenv = 0.0f;
        //}
        //else 
        voice[v].env += SILENCE + SILENCE; //anti-glitching trick
      }
      ....
    }

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

    V573 Uninitialized variable 'oldScrollSize' was used. The variable was used to initialize itself. cscrollview.cpp 482

    void CScrollView::setContainerSize (....)
    {
      CRect oldSize (containerSize);
      ....
      CRect oldScrollSize = vsb->getScrollSize (oldScrollSize);
      float oldValue = vsb->getValue ();
      ....
    }

    Анализатор обнаружил потенциальное использование неинициализированной переменной oldScrollSize. Как оказалось, ошибки не будет, но реализация функции getScrollSize() ужасна:

    CRect& getScrollSize (CRect& rect) const
    {
      rect = scrollSize;
      return rect;
    }

    Наверняка, такой код выглядел бы лучше:

    CRect oldScrollSize = vsb->getScrollSize();
    ....
    CRect& getScrollSize () const
    {
      return scrollSize;
    }

    Ещё несколько таких инициализаций:
    • V573 Uninitialized variable 'oldScrollSize' was used. The variable was used to initialize itself. cscrollview.cpp 503
    • V573 Uninitialized variable 'oldClip' was used. The variable was used to initialize itself. ctabview.cpp 359

    V751 Parameter 'column' is not used inside function body. pitchnamesdatabrowsersource.cpp 227

    void PitchNamesDataBrowserSource::dbCellTextChanged(
      int32_t row, int32_t column, ....)
    {
      if (pitchnames)
      {
        UString128 str (newText);
        if (str.getLength () == 0)
          pitchnames->removePitchName (0, (int16)row);
        else
          pitchnames->setPitchName (0, (int16)row, str);
      }
    }

    В функции dbCellTextChanged() не используется номер столбца, который был передан в функцию. Мне сложно сказать, есть тут ошибка или нет, поэтому авторам проекта следует перепроверить код.

    V570 The same value is assigned twice to the 'lpf' variable. mdaComboProcessor.cpp 274

    void ComboProcessor::recalculate ()
    {
      ....
      case 4: trim = 0.96f; lpf = filterFreq(1685.f);
          mix1 = -0.85f; mix2 = 0.41f;
          del1 = int (getSampleRate () / 6546.f);
          del2 = int (getSampleRate () / 3315.f);
          break;
    
      case 5: trim = 0.59f; lpf = lpf = filterFreq(2795.f); // <=
          mix1 = -0.29f; mix2 = 0.38f;
          del1 = int (getSampleRate () / 982.f);
          del2 = int (getSampleRate () / 2402.f);
          hpf = filterFreq(459.f); 
          break;
      ....
    }

    Небольшое замечание по коду: в коде присутствует лишнее присваивание переменной lpf. Скорее всего, это опечатка, случайным образом не приводящая к ошибке.

    Заключение


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

    Моя позиция по вопросу, какой код качественнее — открытый или закрытый — очень проста. Качество кода в большей степени зависит от руководителя проекта, чем от закрытости/открытости кода. С открытым кодом, конечно, хорошо: легко сообщить о баге, кто-то добавит функционал, кто-то исправит ошибку… но если руководитель не обеспечит использование в проекте методик контроля качества, то лучше не станет. Необходимо использовать все доступные бесплатные решения и по возможности добавлять к ним платные инструменты.

    Другие обзоры музыкального софта:


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

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


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

    Прочитали статью и есть вопрос?
    Часто к нашим статьям задают одни и те же вопросы. Ответы на них мы собрали здесь: Ответы на вопросы читателей статей про PVS-Studio, версия 2015. Пожалуйста, ознакомьтесь со списком.
    PVS-Studio
    Статический анализ кода для C, C++, C# и Java

    Комментарии 4

      +3
      У меня к Вам 2 вопроса/комментария.

      1. Не находите ли Вы, что в 2017 году ошибка вида «The 'p' variable is modified while being used twice between sequence points» несколько устарела? Не лучше ли использовать что-то типа «The multiple modifications of the 'p' variable are unsequenced» (тоже мне не нравится, но смысл должен быть ясен).

      2. Я считаю, что PVS нашёл UB там, где его нет. Этот код, насколько я понимаю, является корректным, а предупреждение PVS — нет. Можете доказать обратное?
        0
        В документации к диагностике V567 есть ссылки на дополнительные ресурсы. Там вы можете найти описание таких ситуаций и обсуждения на разных ресурсах. В итоге всё сводится к цитированию стандарта, в котором есть похожие примеры. Доказательство чего-либо в этом случае прерогатива камитета по стандартизации C++.

        Изменение сообщения обсудим с коллегами. Иногда диагностики переименовываются, но вряд ли это зависит от года.

          +2
          О чём я Вас и прошу: процитируйте стандарт и докажите, что пример в статье содержит UB. Я утверждаю, что он его не содержит (по крайней мере для C++11, 03 под рукой нет), и ваш анализатор ошибся. Мне кажется, это в интересах компании найти ошибку, или нет?

          P.S. Документация к диагностике V567 устарела и содержит примеры UB, которые больше им не являются (X[i]=++i и т.д.). Собственно там документация говорит о точках следования, которых уже 6 лет как нет.
            0
            Вы правы, диагностика устарела со временем. Мы подновим её и её описание.

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

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