Как программисты с PVS-Studio ошибки в проектах искали

Недавно сайт Pinguem.ru совместно с командой PVS-Studio устраивали конкурс, в котором программистам было необходимо в течение месяца использовать статический анализатор PVS-Studio для нахождения и исправления ошибок в коде open-source проектов. Благодаря их стараниям, программы в мире стали чуточку безопаснее и надежнее. В статье мы рассмотрим парочку наиболее интересных ошибок, которые были найдены при помощи PVS-Studio.

И как прошел конкурс?

Конкурс проходил с 23 октября по 27 ноября 2017 года в два этапа для русскоязычной аудитории. На первом этапе конкурсантам было необходимо отправить как можно больше Pull Request'ов авторам проектов. Второй этап был несколько сложнее: нужно было найти ошибку и описать последовательность действий, при которых она бы себя проявила в работе приложения. Лучше всех с заданиями справился Николай Шалакин и стал победителем конкурса. Поздравляем его!

За время проведения конкурса было отправлено немало хороших Pull Request'ов, желающие могут ознакомиться с ними по этой ссылке. Мы же предлагаем рассмотреть наиболее интересные ошибки, найденные конкурсантами на втором этапе.

QtCreator

Многие ли из Вас используют QtCreator для программирования на Python? Как и многие IDE, он тоже подсвечивает некоторые встроенные функции и объекты. Возьмем QtCreator 4.4.1 и напишем несколько зарезервированных слов:

Что же такое? Почему встроенные функции oct и chr не подсвечиваются? Взглянем на код:

// List of python built-in functions and objects
static const QSet<QString> builtins = {
"range", "xrange", "int", "float", "long", "hex", "oct" "chr", "ord",
"len", "abs", "None", "True", "False"
};

Функции объявлены, они должны подсвечиваться. И здесь помогает PVS-Studio:

V653 A suspicious string consisting of two parts is used for initialization. It is possible that a comma is missing. Consider inspecting this literal: «oct» «chr». pythonscanner.cpp 205

Действительно, между литералами «oct» и «chr» забыли поставить запятую, поэтому два литерала слились в один «octchr», и именно он будет подсвечиваться средой разработки:

Здесь можно ознакомиться с Pull Request'ом на исправление.

ConEmu

Вы работаете над проектом ConEmu и в отладочной версии решили проверить работу некоторых настроек:

Давайте взглянем на код, почему вылетает предупреждение «ListBox was not processed»:

INT_PTR CSetPgViews::OnComboBox(HWND hDlg, WORD nCtrlId, WORD code)
{
  switch (code)
  {
  ....
  case CBN_SELCHANGE:
    {
      ....
      UINT val;
      INT_PTR nSel = SendDlgItemMessage(hDlg, 
                                        nCtrlId, 
                                        CB_GETCURSEL,
                                        0,
                                        0);
      switch (nCtrlId)
      {
        ....
        case tThumbMaxZoom:
          gpSet->ThSet.nMaxZoom = max(100,((nSel+1)*100));
        default:
          _ASSERTE(FALSE && "ListBox was not processed");
      }
    }
  }
}

Из-за пропущенного break управление перейдет к ветке default после того, как отработают выражения в ветке tThumbMaxZoom. Об этом предупреждает PVS-Studio:

V796 It is possible that 'break' statement is missing in switch statement. setpgviews.cpp 183

Здесь можно ознакомиться с Pull Request'ом на исправление.

UniversalPauseButton

Довольно интересный проект, особенно полезный для геймеров. При нажатии на кнопку Pause программа приостанавливает работу окна, находящегося на переднем плане:

Можно переназначить кнопку приостановки/возобновления на другую при помощи файла settings.txt:

Если ввести код ключа не менее, чем 20 символов, и не более, чем 30 символов, это приведет к порче стека:

Разберемся, почему это происходит. Нас интересует функция LoadPauseKeyFromSettingsFile:

int LoadPauseKeyFromSettingsFile(_In_ wchar_t* Filename)
{
  HANDLE FileHandle = CreateFile(Filename, 
                                 GENERIC_READ,
                                 FILE_SHARE_READ,
                                 NULL,
                                 OPEN_EXISTING,
                                 FILE_ATTRIBUTE_NORMAL,
                                 NULL);

  if (FileHandle == INVALID_HANDLE_VALUE)
  {
    goto Default;
  }
  
  char  KeyLine[32] = { 0 };
  char  Buffer[2]   = { 0 };
  DWORD ByteRead    = 0;

  do
  {
    if (!ReadFile(FileHandle, Buffer, 1, &ByteRead, NULL))
    {
      goto Default;
    }

    if (Buffer[0] == 'r' || Buffer[0] == 'n')
    {
      break;
    }

    size_t Length = strlen(KeyLine);
    if (Length > 30)                                            // <=
    {
      goto Default;
    }

    KeyLine[Length] = Buffer[0];    
    memset(Buffer, 0, sizeof(Buffer));
  } while (ByteRead == 1);

  if (!StringStartsWith_AI(KeyLine, "KEY="))
  {
    goto Default;
  }

  char KeyNumberAsString[16] = { 0 };                           // <=

  for (DWORD Counter = 4; Counter < strlen(KeyLine); Counter++) // <=
  {
    KeyNumberAsString[Counter - 4] = KeyLine[Counter];
  }
  ....

  Default:
  if (FileHandle != INVALID_HANDLE_VALUE && FileHandle != NULL)
  {
    CloseHandle(FileHandle);    
  }
  return(0x13);
}

В цикле считывается побайтово первая строка. Если она превышает длину в 30 символов, то выполнение переходит по метке Default,при этом освобождается ресурс и возвращается символ с кодом 0x13. Если чтение происходит успешно, и первая строка начинается с «KEY=», то происходит копирование подстроки после символа "=" в 16-байтовый буфер KeyNumberAsString. При вводе ключа от 20 до 30 символов произойдет переполнение буфера. Об этом предупреждает PVS-Studio:

V557 Array overrun is possible. The value of 'Counter — 4' index could reach 26. main.cpp 146

Здесь можно ознакомиться с Pull Request'ом на исправление.

Explorer++

В этом проекте был найден баг, связанный с сортировкой избранных вкладок:

Посмотрим на код сортировки:

int CALLBACK SortByName(const NBookmarkHelper::variantBookmark_t
                          BookmarkItem1,
                        const NBookmarkHelper::variantBookmark_t
                          BookmarkItem2)
{
  if (   BookmarkItem1.type() == typeid(CBookmarkFolder)
      && BookmarkItem2.type() == typeid(CBookmarkFolder))
  {
    const CBookmarkFolder &BookmarkFolder1 =
      boost::get<CBookmarkFolder>(BookmarkItem1);
    const CBookmarkFolder &BookmarkFolder2 =
      boost::get<CBookmarkFolder>(BookmarkItem2);

    return BookmarkFolder1.GetName()
           .compare(BookmarkFolder2.GetName());
  }
  else
  {
    const CBookmark &Bookmark1 = 
      boost::get<CBookmark>(BookmarkItem1);
    const CBookmark &Bookmark2 =
      boost::get<CBookmark>(BookmarkItem1);

    return Bookmark1.GetName().compare(Bookmark2.GetName());
  }
}

В ветке else ошиблись и дважды использовали BookmarkItem1, вместо BookmarkItem2. Об этой ошибке и предупреждает PVS-Studio:

• V537 Consider reviewing the correctness of 'BookmarkItem1' item's usage. bookmarkhelper.cpp 535
• и еще 5 дополнительных предупреждений.

Здесь можно ознакомиться с Pull Request'ом на исправление.

Заключение

Команда PVS-Studio выражает огромную благодарность всем программистам, принявшим участие в конкурсе. Вы проделали огромную работу в устранении багов в open-source проектах, сделав их надежнее, безопаснее и лучше. В будущем мы, возможно, проведем подобный конкурс и для англоязычной аудитории.

Мы также предлагаем всем остальным скачать и попробовать анализатор PVS-Studio, он прост в обращении и может быть Вам полезен.