Главная
Блог разработчиков phpBB
 
+ 17 предустановленных модов
+ SEO-оптимизация форума
+ авторизация через соц. сети
+ защита от спама

Проверка VirtualDub

Anna | 24.06.2014 | нет комментариев
PVS-Studio, VirtualDub
Только что, я сел и проверил план VirtualDub с поддержкой PVS-Studio. Выбор был случаен. Я считаю, самое основное регулярно проверять/перепроверять разные планы, Дабы показать, как прогрессирует анализатор кода PVS-Studio. А какой план будет проверен, не так значимо. Ошибки есть всюду. План VirtualDub мы теснее проверяли в 2011 году, но тогда примерно ничего увлекательного не нашлось. Вот я и решил посмотреть, как обстоят дела, через 2 года.

Я скачал с сайта VirtualDub архив VirtualDub-1.10.3-src.7z. Для обзора я применял PVS-Studio версии 5.10. На обзор я потратил около часа, так что не судите сурово. Наверно, я что-то пропустил. И напротив, мог поcчитать правильный код за подозрительный. Умоляю, тех, кто поддерживает план VirtualDub не полагаться на мой отчет, а произвести независимую проверку. Мы неизменно идём на встречу open-source сообществу и готовы выделить регистрационный ключ.

Ещё я хочу попросить Эвери Ли отнестись к этой статье с пониманием. В прошлый раз, он дюже болезненновоспринял припоминание VirtualDub в одной из моих статей. Я совсем не хотел, и не хочу сказать, что какая-то программа является глючной. Программные ошибки есть всюду. Моя цель показать пользу, которую может давать применение статического обзора кода. Заодно, это сделает open-source планы немножко надёжнее. Это восхитительно.

Безусловно, разовые проверки непроизводительны. Но с этим, к сожалению, я ничего сделать не могу. Применять либо не применять регулярно инструменты статического обзора, зависит от разработчиков. Я только могу пытаться объяснить, в чем польза регулярного применения. Вот одна из увлекательных заметок на эту тему: Лев Толстой и статический обзор кода.

Однако, эта статья про ошибки, а не методику применения статического обзора. Давайте посмотрим, что нового увлекательного обнаружил анализатор PVS-Studio в VirtualDub.

Виртуальные деструкторы

В языке программирования C деструктор полиморфного базового класса должен объявляться виртуальным. Только так обеспечивается правильное уничтожение объекта производного класса через указатель на соответствующий базовый класс.

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

В VirtualDub есть класс VDDialogBaseW32:

class VDDialogBaseW32 {
  ....
  ~VDDialogBaseW32();
  ....
  virtual INT_PTR DlgProc(....) = 0;
  virtual bool PreNCDestroy();
  ....
}

Как видите, он содержит виртуальные функции. Впрочем деструктор не объявлен виртуальным. Безусловно, он него наследуются классы. Скажем, VDDialogAudioFilterFormatConvConfig:

class VDDialogAudioFilterFormatConvConfig :
  public VDDialogBaseW32
{ .... };

Оплошность разрушения объекта выглядит вот так:

INT_PTR CALLBACK VDDialogBaseW32::StaticDlgProc(....) {
  VDDialogBaseW32 *pThis =
    (VDDialogBaseW32 *)GetWindowLongPtr(hwnd, DWLP_USER);
  ....
  delete pThis;
  ....
}

Предупреждение выданное PVS-Studio: V599 The destructor was not declared as a virtual one, although the ‘VDDialogBaseW32′ class contains virtual functions. VirtualDub gui.cpp 997

Как видите, для разрушения объекта применяется указатель на базовый класс. Такое удаление объекта приведет к неопределенному поведению программы.

Аналогичная напасть существует и с классом VDMPEGAudioPolyphaseFilter.

Ещё о неопределённом поведении

По поводу ошибок, связанных с виртуальным деструктором, вопросов не появляется. Гораздо больше склизкой темой являются операции сдвига. Вот один из таких примеров:

void AVIVideoGIFOutputStream::write(....) {
{
  ....
  for(int i=0; i<palsize;   i)
    dict[i].mPrevAndLastChar = (-1 << 16)   i;
  ....
}

Дозволено сколько желательно утверждать, что это абсолютно надёжный код, тот, что удачно применяется теснее огромнее 10 лет. Однако, всё равно, тут мы имеем дело с неопределенным поведением программы. Вот что эталон говорит на тему таких конструкций:

The shift operators << and >> group left-to-right.

shift-expression << additive-expression

shift-expression >> additive-expression

The operands shall be of integral or unscoped enumeration type and integral promotions are performed.

1. The type of the result is that of the promoted left operand. The behavior is undefined if the right operand is negative, or greater than or equal to the length in bits of the promoted left operand.

2. The value of E1 << E2 is E1 left-shifted E2 bit positions; vacated bits are zero-filled. If E1 has an unsigned type, the value of the result is E1 * 2^E2, reduced modulo one more than the maximum value representable in the result type. Otherwise, if E1 has a signed type and non-negative value, and E1*2^E2 is representable in the result type, then that is the resulting value; otherwise, the behavior is undefined.

3. The value of E1 >> E2 is E1 right-shifted E2 bit positions. If E1 has an unsigned type or if E1 has a signed type and a non-negative value, the value of the result is the integral part of the quotient of E1/2^E2. If E1 has a signed type and a negative value, the resulting value is implementation-defined.

То, что код работает, это везение. При осваивании нового компилятора либо других ключей для оптимизации, программа может невзначай изменить своё поведение. Подробнее про сдвиги и о том, стоит либо не стоит править код, дозволено почитать в статье “Не зная брода, не лезь в воду. Часть третья“.

Вот полный список мест, где анализатор PVS-Studio нашел Undefined behavior либо Unspecified behavior в плане VirtualDub.

Опечатки

static ModuleInfo *CrashGetModules(void *&ptr) {
  ....
  while(*pszHeap  );
    if (pszHeap[-1]=='.')
      period = pszHeap-1;
  ....
}

Диагностическое сообщение выданное PVS-Studio: V529 Odd semicolon ‘;’ after ‘while’ operator. VirtualDub crash.cpp 462

Обратите внимание на точку с запятой, стоящую позже ‘while’. Тут либо оплошность либо код неверно отформатирован. Я думаю, это именно оплошность. Цикл «while(*pszHeap );» пройдет до конца строки и в итоге, переменная ‘pszHeap’ будет указывать на память позже терминального нуля. Проверка «if (pszHeap[-1]==’.')» не имеет смысла. По адресу «pszHeap[-1]» неизменно находится терминальный нуль.

Разглядим иную опечатку, при работе со строками.

void VDBackfaceService::Execute(...., char *s) {
  ....
  if (*s == '"') {
    while(*s && *s != '"')
        s;
  } else {
  ....
}

Диагностическое сообщение выданное PVS-Studio: V637 Two opposite conditions were encountered. The second condition is always false. Check lines: 183, 184. VirtualDub backface.cpp 183

Данный код должен пропустить всё, что содержится в кавычках. По крайней мере, мне так кажется. Впрочем, условие (*s && *s != ‘”‘) сразу является ложным. Допустимо, код должен был выглядеть так:

if (*s == '"') {
    s;
  while(*s && *s != '"')
      s;
}

Оператор new генерирует исключения при ошибке выделения памяти

В ветхом коде, Зачастую дозволено встретить проверку того, что вернул оператор new. Выглядит это так:

int *p = new int[10];
if (!p)
  return false;

Современные компиляторы, поддерживающие эталон языка Си , обязаны сгенерировать исключение, если не удалось выделить память. Дозволено сделать, Дабы оператор ‘new’ не генерировал исключения, но к рассматриваемым случаям, теперь это отношения не имеет.

Таким образом, проверка if (!p) является лишней. В целом такой код безвреден. Лишняя проверка. Ничего ужасного.

Впрочем ветхий код может привести к неприятным итогам. Разглядим фрагмент из плана VirtualDub.
void HexEditor::Find(HWND hwndParent) {
  ....
  int *next = new int[nFindLength 1];
  char *searchbuffer = new char[65536];
  char *revstring = new char[nFindLength];
  ....
  if (!next || !searchbuffer || !revstring) {
    delete[] next;
    delete[] searchbuffer;
    delete[] revstring;
    return;
  }
  ....
}

Диагностическое сообщение выданное PV-Studio: V668 There is no sense in testing the ‘next’ pointer against null, as the memory was allocated using the ‘new’ operator. The exception will be generated in the case of memory allocation error. VirtualDub hexviewer.cpp 2012

Если в строке «char *revstring = new char[nFindLength];» возникнет исключение, то произойдет утрата памяти. Операторы delete[] не будут вызваны. Не критичная оплошность, но всё равно стоит про неё упомянуть.

Тут, перечислены все места, где в VirtualDub проверяется указатель позже вызова оператора ‘new’.

Ссылка на уничтоженный объект

vdlist_iterator& operator--(int) {
  vdlist_iterator tmp(*this);
  mp = mp->mListNodePrev;
  return tmp;
}

Диагностическое сообщение выданное PVS-Studio: V558 Function returns the reference to temporary local object: tmp. VirtualDub vdstl.h 460

Функция реализована ненормально. Она возвращает ссылку на локальный объект ‘tmp’. Позже выхода из функции, он будет теснее разломан. Работа с этой ссылкой, приведёт к неопределенному поведению.

Кстати, оператор , находящийся рядом, реализован верно.

В начале используем, потом проверяем

В различных программах, Зачастую дозволено встретить ошибку, когда указатель вначале разыменовывается, а теснее только потом, сравнивается с NULL. Такие ошибки не проявляют себя дюже длинное время, так как равенство указателя NULL, это внештатная редкая обстановка. Эти недочёты, существуют и в коде VirtualDub. Пример:

LRESULT YUVCodec::DecompressGetFormat(BITMAPINFO *lpbiInput,
                                      BITMAPINFO *lpbiOutput)
{
  BITMAPINFOHEADER *bmihInput    = &lpbiInput->bmiHeader;
  BITMAPINFOHEADER *bmihOutput  = &lpbiOutput->bmiHeader;
  LRESULT res;

  if (!lpbiOutput)
    return sizeof(BITMAPINFOHEADER);
  ....
}

Диагностическое сообщение выданное PVS-Studio: V595 The ‘lpbiOutput’ pointer was utilized before it was verified against nullptr. Check lines: 82, 85. VirtualDub yuvcodec.cpp 82

В начале указатель «lpbiOutput» разыменовывается. После этого проверяется: «if (!lpbiOutput)». Такие оплошность обыкновенно появляюсь в процессе рефакторинга. Новейший код вставляется до необходимых проверок. Дабы поправить приведённый код, необходимо поменять последовательность действий:

LRESULT YUVCodec::DecompressGetFormat(BITMAPINFO *lpbiInput,
                                      BITMAPINFO *lpbiOutput)
{
  if (!lpbiOutput)
    return sizeof(BITMAPINFOHEADER);
  BITMAPINFOHEADER *bmihInput    = &lpbiInput->bmiHeader;
  BITMAPINFOHEADER *bmihOutput  = &lpbiOutput->bmiHeader;
  LRESULT res;
  ....
}

Другие места, где анализатор выдает предупреждение V595, перечислены тут.

Работа с типом HRESULT

VDPosition AVIReadTunnelStream::TimeToPosition(VDTime timeInUs) {
  AVISTREAMINFO asi;
  if (AVIStreamInfo(pas, &asi, sizeof asi))
    return 0;

  return VDRoundToInt64(timeInUs * (double)asi.dwRate / (double)asi.dwScale * (1.0 / 1000000.0));
}

Диагностическое сообщение выданное PVS-Studio: V545 Such conditional expression of ‘if’ operator is incorrect for the HRESULT type value ‘AVIStreamInfoA(pas, & asi, sizeof asi)’. The SUCCEEDED or FAILED macro should be used instead. VirtualDub avireadhandlertunnelw32.cpp 230

Функция AVIStreamInfo() возвращает значение типа HRESULT. Данный тип невозможно интерпретировать как ‘bool’. Информация, хранящаяся в переменной типа HRESULT, имеет довольно трудную конструкцию. Для проверки значения HRESULT нужно применять макрос SUCCEEDED либо FAILED, объявленные в «WinError.h». Вот как устроены эти макросы:

#define FAILED(hr) (((HRESULT)(hr)) < 0)
#define SUCCEEDED(hr) (((HRESULT)(hr)) >= 0)

Верный вариант кода:

if (FAILED(AVIStreamInfo(pas, &asi, sizeof asi)))

Схожие предупреждения PVS-Studio выдаёт на следующие строки:

  • avireadhandlertunnelw32.cpp 238
  • avireadhandlertunnelw32.cpp 335
  • inputfileavi.cpp 440
  • context_d3d11.cpp 959

Волшебные числа

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

bool VDOpenGLBinding::Attach(....) {
  ....
  if (!memcmp(start, "GL_EXT_blend_subtract", 20))
  ....
}

Диагностическое сообщение выданное PVS-Studio: V512 A call of the ‘memcmp’ function will lead to underflow of the buffer ‘«GL_EXT_blend_subtract»’. Riza opengl.cpp 393

Длина строки «GL_EXT_blend_subtract» не 20, а 21 символ. Оплошность некритична. На практике коллизий не возникнет. Тем не менее, следует чураться таких магических чисел. Отменнее применять особый макрос для подсчета длины строки. Пример:

#define LiteralStrLen(S) (sizeof(S) / sizeof(S[0]) - 1)

В Си дозволено сделать больше неопасную шаблонную функцию:

template <typename T, size_t N>
char (&ArraySizeHelper(T (&array)[N]))[N];

template <typename T, size_t N>
size_t LiteralStrLen(T (&array)[N]) {
  return sizeof(ArraySizeHelper(array)) - 1;
}

Преобладание второго варианта — невозможно нечаянно передать в качестве довода примитивный указатель. Подробнее данный приём описан в статье “PVS-Studio vs Chromium“.

Безусловные пути

VDDbgHelpDynamicLoaderW32::VDDbgHelpDynamicLoaderW32()
{
  hmodDbgHelp = LoadLibrary(
    "c:\program files\debugging tools for windows\dbghelp");
  if (!hmodDbgHelp) {
    hmodDbgHelp = LoadLibrary("c:\program files (x86)\......
  ....
}

Диагностическое сообщение выданное PVS-Studio: V631 Consider inspecting the ‘LoadLibraryA’ function call. Defining an absolute path to the file or directory is considered a poor style. VirtualDub leaks.cpp 67, 69

Думаю ясно, чем данный код дрянен. Безусловно, код связан с отладкой, и вряд ли как-то отрицательно скажется на финальных пользователях. Но всё равно, было бы отменнее получить верный путь до Program Files.

Неверный довод

sint64 rva;

void tool_lookup(....) {
  ....
  printf("%08I64x   %s   %x [%s:%d]n",
    addr, sym->name, addr-sym->rva, fn, line);
  ....
}

Диагностическое сообщение выданное PVS-Studio: V576 Incorrect format. Consider checking the fourth actual argument of the ‘printf’ function. The argument is expected to be not greater than 32-bit. Asuka lookup.cpp 56

Переменная ‘rva’ является 64-битным типом. Это значит, что в стек будет размещено 8 байт. Функция printf() является функцией с переменным числом доводов. Тип данных, которые она должна обработать, задаётся с поддержкой строки форматирования. В данном случае, переменная ‘rva’ будет обработана как 32-битная переменная (“%x”).

Приведёт данная оплошность к сбою либо нет, зависит от того, как организует передачу доводов компилятор и от разрядности платформы. Скажем, в Win64 все целочисленные типы сначала приводятся к 64-битному типу, и теснее после этого помещаются в стек. Задачи, что переменная займет в стеке огромнее места, чем нужно, не будет.

Однако, если переменная ‘rva’ хранит значения больше INT_MAX, её значение в любом случае будет распечатано некорректно.

Схожие предупреждения анализатор выдаёт тут:

  • dubstatus.cpp 360
  • lookup.cpp 58

Неправильные сопоставления

void VDVideoCompressorVCM::GetState(vdfastvector<uint8>& data) {
  DWORD res;
  ....
  res = ICGetState(hic, data.data(), size);
  ....
  if (res < 0)
    throw MyICError("Video compression", res);
}

Диагностическое сообщение выданное PVS-Studio: V547 Expression ‘res < 0′ is always false. Unsigned type value is never < 0. Riza w32videocodecpack.cpp 828

Переменная ‘res’ имеем беззнаковый тип DWORD. Это значит, что выражение «res < 0» неизменно равно значению ‘false’.

Аналогичная проверка содержится тут: w32videocodec.cpp 284.

Разглядим ещё одну схожую ошибку.

#define ICERR_CUSTOM           -400L
static const char *GetVCMErrorString(uint32 icErr) {
  ....
  if (icErr <= ICERR_CUSTOM) err = "A codec-specific error occurred.";
  ....
}

Диагностическое сообщение выданное PVS-Studio: V605 Consider verifying the expression: icErr <= — 400L. An unsigned value is compared to the number -400. system error_win32.cpp 54

Переменная ‘ icErr’ имеет тип ‘unsigned’. Следственно, перед сопоставлением число ‘-400′ будет неявно преобразовано в ‘unsigned’. Значение ‘-400′ превратится в 4294966896. Таким образом, сопоставление (icErr <= -400) эквивалентно (icErr <= 4294966896). По каждой видимости, это не то, что хотел программист.

Различное необычное

void AVIOutputFile::finalize() {
  ....
  if (stream.mChunkCount && hdr.dwScale && stream.mChunkCount)
  ....
}

Диагностическое сообщение выданное PVS-Studio: V501 There are identical sub-expressions ‘stream.mChunkCount’ to the left and to the right of the ‘&&’ operator. VirtualDub avioutputfile.cpp 761

Два раза проверяется переменная ‘stream.mChunkCount’. Одна проверка лишняя либо позабыли проверить что-то ещё.

void VDVideoCompressorVCM::Start(const void *inputFormat,
                                 uint32 inputFormatSize,
                                 const void *outputFormat,
                                 uint32 outputFormatSize,
                                 const VDFraction& frameRate,
                                 VDPosition frameCount)
{
  this->hic = hic;
  ....
}

Диагностическое сообщение выданное PVS-Studio: V570 The ‘this->hic’ variable is assigned to itself. Riza w32videocodecpack.cpp 253

void VDDialogAudioConversionW32::RecomputeBandwidth() {
  ....
  if (IsDlgButtonChecked(mhdlg, IDC_PRECISION_NOCHANGE)) {
    if (mbSourcePrecisionKnown && mbSource16Bit)
      bps *= 2;
    else
      bps = 0;
  } if (IsDlgButtonChecked(mhdlg, IDC_PRECISION_16BIT))
    bps *= 2;
  ....
}

Диагностическое сообщение выданное PVS-Studio: V646 Consider inspecting the application’s logic. It’s possible that ‘else’ keyword is missing. VirtualDub optdlg.cpp 120

Допустимо, код ненормально отформатирован. А допустимо, позабыто ключевое слово ‘else’.

bool VDCaptureDriverScreen::Init(VDGUIHandle hParent) {
  ....
  mbAudioHardwarePresent = false;
  mbAudioHardwarePresent = true;
  ....
}

Диагностическое сообщение выданное PVS-Studio: V519 The ‘mbAudioHardwarePresent’ variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 274, 275. VDCapture cap_screen.cpp 275

Завершение

Как видите, даже при разовом запуске, статический обзор может быть пригоден. Но гораздо пригоднее запускать его регулярно. чай предупреждения (warnings) в компиляторе, программисты включают не один раз перед релизом, а пользуются ими непрерывно. Та же самая обстановка и с инструментами статического обзора. Непрерывное их применение разрешает оперативно устранять ошибки. Дозволено рассматривать PVS-Studio как некую надстройку над компилятором, которая выдаёт огромнее увлекательных предупреждений. Лучший вариант, это применение инкрементального обзора кода. Вы найдете новые ошибки сразу позже компиляции измененных файлов.

Источник: programmingmaster.ru

Оставить комментарий
Форум phpBB, русская поддержка форума phpBB
Рейтинг@Mail.ru 2008 - 2017 © BB3x.ru - русская поддержка форума phpBB