Георгий Грибков

27 Июл 2018

Теги:

#Cpp #Java

Обзор дефектов исходного кода видеоигры "Вангеры"

27 Июл 2018

Автор: Георгий Грибков

Введение
Потенциальные утечки памяти
Повторные присваивания
Неаккуратный Copy-Paste
Ошибки в операторе switch
Недостижимый код
Излишне усложненная проверка
Потенциальное разыменование нулевого указателя
Пониженная производительность и остатки от рефакторинга
Неправильное освобождение динамической памяти
Заключение

Совсем недавно игре "Вангеры" исполнилось 20 лет. В честь этой даты мы решили проверить проект и подготовить обзор некоторых интересных ошибок, найденных в исходном коде игры. Этим занялся наш новый сотрудник Георгий. Проверка проекта – хороший способ познакомиться с возможностями статического анализатора кода PVS-Studio и потренироваться в написании статьи.

Введение

Вангеры (англ. Vangers: One For The Road) - компьютерная игра, написанная на языке C++ и выпущенная российской компанией К-Д ЛАБ в 1998 году. Переизданная игра доступна в Steam и запускается на современных операционных системах, но пока только в разрешении 800x600.

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

Потенциальные утечки памяти

Итак, первым в очереди на рассмотрение является следующий код:

void iInitText(....)
{
  char* buf;
  buf = new char[text_len];
  memcpy(buf,text,text_len);

  ....
  
  i = 0;
  while(i < text_len){
    while(!buf[i]) i ++;
    if(i < text_len){
      ....
      while(buf[i]) i ++;
    }
  }
}

Предупреждение PVS-Studio: V773 CWE-401 Visibility scope of the 'buf' pointer was exited without releasing the memory. A memory leak is possible. iscr_fnc.cpp 1174

Это функция для обработки строк, состоящих из нескольких слов. Для хранения обрабатываемой строки используется указатель buf на массив char, память под который выделяется с помощью оператора new[]. Этот указатель находится в области видимости функции iInitText.

Когда функция закончит свою работу, указатель buf выйдет из своей области видимости и прекратит своё существование, вследствие чего область памяти, с которой этот указатель был связан, окажется более недоступной. Такие ошибки являются утечками памяти - неконтролируемое уменьшение объема свободной оперативной (или виртуальной) памяти компьютера.

Чтобы быть уверенным, что такого не произойдет, необходимо освобождать память, которая больше не нужна. В данном случае перед последней закрывающей фигурной скобкой следует добавить выражение "delete [] buf". Еще более хорошим решением будет использовать умные указатели.

Повторные присваивания

Переходим к следующему участку кода.

void VangerUnit::CreateVangerUnit(void)
{
  ....
  
  DoorFlag = 0;
  ExternalMode = EXTERNAL_MODE_NORMAL;
  ExternalTime = 0;
  ExternalLock = 0;
  ExternalDraw = 1;
  ExternalObject = ExternalSensor = ExternalSensor = NULL;
  ExternalTime2 = 0;
  ExternalAngle = 0;
  Go2World();
  
  ....
};

Предупреждение PVS-Studio: V570 The same value is assigned twice to the 'ExternalSensor' variable. mechos.cpp 5828

Подозрительно, что одной переменной два раза присваивается одно и то же значение. Давайте рассмотрим структуру VangerUnit:

struct VangerUnit : TrackUnit , uvsUnitType , aiFactorType
{
  ....
  
  int ExternalMode, ExternalTime, ExternalLock,
      ExternalDraw, ExternalAngle;
  int ExternalTime2;
  SensorDataType* ExternalObject;
  SensorDataType* ExternalLastSensor;
  SensorDataType* ExternalSensor;
  int RandomUpdate;
  
  ....
};

Теперь, исходя из схожести названий и одинаковости типа переменных ExternalObject, ExternalLastSensor и ExternalSensor, можно сделать вывод, что изначально программист задумывал написать так:

void VangerUnit::CreateVangerUnit(void)
{
  ....
  
  DoorFlag = 0;
  ExternalMode = EXTERNAL_MODE_NORMAL;
  ExternalTime = 0;
  ExternalLock = 0;
  ExternalDraw = 1;
  ExternalObject = ExternalLastSensor = ExternalSensor = NULL;
  ExternalTime2 = 0;
  ExternalAngle = 0;
  Go2World();
  
  ....

};

Чем же чревата такая ошибка? Тем, что указатель ExternalLastSensor остался неинициализированным, что может повлечь за собой ошибку времени выполнения. Использование такого указателя означает попытку доступа к несуществующему объекту в произвольной области памяти, что может вызвать непредсказуемые последствия. Найти такую ошибку порой бывает весьма трудно. Кстати, если взглянуть 8000 строками ниже, там как раз находится точно такой же код, вставленный туда с помощью Copy-Paste.

V570 The same value is assigned twice to the 'ExternalSensor' variable. mechos.cpp 13967

Неаккуратный Copy-Paste

Следующий пример оставил у меня весьма забавное впечатление:

const char* iGetJoyBtnNameText(int vkey,int lang)
{
  const char* ret;
  if(vkey & VK_BUTTON){
    if(vkey >= VK_BUTTON_1 && vkey <= VK_BUTTON_32){
      ret = (lang) 
        ? iJoystickButtons2[vkey - VK_BUTTON_1] 
        : iJoystickButtons1[vkey - VK_BUTTON_1];
      return ret;
    }
    else
      return NULL; //WARNING NEED VIEW!!!
  }
  if(vkey & VK_STICK_SWITCH){
    if(vkey >= VK_STICK_SWITCH_1 && vkey <= VK_STICK_SWITCH_9){
      ret = (lang) 
        ? iJoystickStickSwitch2[vkey - VK_STICK_SWITCH_1] 
        : iJoystickStickSwitch2[vkey - VK_STICK_SWITCH_1];
      return ret;
    }
    else
      return NULL; //WARNING NEED VIEW!!! 
  }
  return NULL; //WARNING NEED VIEW!!!
}

Наверняка вам, как и мне, бросились в глаза комментарии, оставленные разработчиками. Я решил проверить, куда может уйти NULL, если функция iGetJoyBtnNameText вдруг его вернет. И обнаружил всего два вызова. Выглядят они вот так:

//NEED Full Rewrite
/*if(!(key & iJOYSTICK_MASK)){
str = iGetKeyNameText(key,iRussian);
}
else {
  str = iGetJoyBtnNameText(key,iRussian);
}*/

//NEED rewrite
/*if(!(k & iJOYSTICK_MASK))
  key_name = iGetKeyNameText(k,iRussian);
else
  key_name = iGetJoyBtnNameText(k,iRussian);
*/

Видимо, данные участки кода еще находятся в разработке, и я заглянул сюда в самый разгар строительства. Сразу представилась табличка "ВЕДУТСЯ РАБОТЫ", за которой шум, пыль, а большой экскаватор перекапывает землю. Под весь этот шумок в коде функции укрылась ошибка, на которую PVS-Studio выдал предупреждение:

V583 CWE-783 The '?:' operator, regardless of its conditional expression, always returns one and the same value. iextern.cpp 2461

Ошибка кроется во втором операторе '?:'. Классическая ошибка Copy-Paste.

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

Ошибки в операторе switch

Еще один пример. Просматривая отчет анализатора, я наткнулся на ошибку, расположенную в длиннющем операторе switch. Для удобства читателя привожу его в сокращенном виде.

int uvsgetDGdata(int code){
switch( code ){ 
    ....
    // about 230 lines of case
    ....
    case DG_EXTERNS::HERE_PALOCHKA:
      return 
        (uvsReturnTreasureStatus
          (UVS_ITEM_TYPE::PALOCHKA, uvsTreasureInShop)
         ||
         uvsReturnTreasureStatus
          (UVS_ITEM_TYPE::PALOCHKA, 0));
      break;
    case DG_EXTERNS::HERE_NOBOOL:
      return
        (uvsReturnTreasureStatus
          (UVS_ITEM_TYPE::NOBOOL, uvsTreasureInShop)
         ||
         uvsReturnTreasureStatus
          (UVS_ITEM_TYPE::NOBOOL, 0));
      break;
    case DG_EXTERNS::HERE_PIPKA:
      return 
        (uvsReturnTreasureStatus
          (UVS_ITEM_TYPE::PIPKA, uvsTreasureInShop)
         ||
         uvsReturnTreasureStatus
          (UVS_ITEM_TYPE::PIPKA, uvsTreasureInShop)); 
      break;
      ....
      // 40 more lines
      ....
    }// end switch
  return 0;
}

Нашли ошибку? Если PIPKA привлекла ваше внимание, то вы на правильном пути.

Предупреждение PVS-Studio: V501 CWE-570 There are identical sub-expressions 'uvsReturnTreasureStatus(UVS_ITEM_TYPE::PIPKA, uvsTreasureInShop)' to the left and to the right of the '||' operator. univang.cpp 10230

И снова старый добрый Copy-Paste. В блоке case, соответствующему константному выражению DG_EXTERNS::HERE_PIPKA, с обеих сторон от оператора '||' располагается одинаковое выражение. Очевидно, что исправленный код должен выглядеть так:

case DG_EXTERNS::HERE_PIPKA:
      return 
        (uvsReturnTreasureStatus
          (UVS_ITEM_TYPE::PIPKA, uvsTreasureInShop)
          ||
        uvsReturnTreasureStatus
          (UVS_ITEM_TYPE::PIPKA, 0));

Увы, в данном случае вероятность нахождения опечатки через визуальный просмотр кода практически равна нулю, ведь оператор switch занимает более 300 строк, и все блоки case имеют похожую структуру. Найти здесь ошибку самому - это как найти иголку в стоге сена!

Недостижимый код

Теперь попробуйте быстро найти ошибку в данном фрагменте:

void uvsVanger::break_harvest(void){
  ....

  pg = Pworld -> escT[0] -> Pbunch 
    -> cycleTable[Pworld -> escT[0] -> Pbunch -> currentStage].Pgame;

  if (!pg) {
    return;
    ErrH.Abort("uvsVanger::break_harvest : don't know where to go ");
  }
  
  ....
}

Предупреждение PVS-Studio: V779 CWE-561 Unreachable code detected. It is possible that an error is present. univang.cpp 4441

Метод ErrH.Abort() находится под оператором return. Поэтому в случае, если указатель pq окажется нулевым, функция завершит свою работу, но ошибка останется необработанной. Для корректной обработки ошибки нужно поменять местами вызов Err.Abort() и оператор return.

Излишне усложненная проверка

Иногда некоторые логические выражения можно упростить. Например:

void iScreen::CheckScanCode(int sc)
{
  ....
  iScreenObject* obj;
  iScreenEvent* p;
  ....
  obj = (iScreenObject*)objList -> last;
  while(obj){
    ....
    while(p){
      if(
        (!(obj -> flags & OBJ_LOCKED) && !(p -> flags & EV_IF_LOCKED)) 
        || 
        ((obj -> flags & OBJ_LOCKED) && (p -> flags & EV_IF_LOCKED))){
        ....
      }
    }
    ....
  }
  ....
}

Предупреждение PVS-Studio: V728 An excessive check can be simplified. The '(A && B) || (!A && !B)' expression is equivalent to the 'bool(A) == bool(B)' expression. iscreen.cpp 2221

Анализатор предупреждает о том, что проверка в if может быть упрощена, и предлагает эквивалентную замену. Действительно, выражение в условии равнозначно выражению

if(bool(obj -> flags & OBJ_LOCKED) == bool(p -> flags & EV_IF_LOCKED))

Потенциальное разыменование нулевого указателя

Код:

void XZIP_FileHeader::SetName(char* p)
{
  int i,sz = strlen(p);
  fileName = strdup(p); 
  for(i = 0; i < sz; i ++)
    if(fileName[i] == '/') fileName[i] = '\\';
}

Предупреждение PVS-Studio: V522 CWE-690 There might be dereferencing of a potential null pointer 'fileName'. Check lines: 72, 70. zip_resource.cpp 72

Не осуществлена проверка указателя fileName. Функция strdup() создает копию C-style строки в куче и возвращает указатель на неё. В случае неудачи с выделением памяти она возвращает NULL. Таким образом, если вдруг у strdup(p) не получится выделить память, двумя строками ниже это обернется попыткой разыменовать нулевой указатель, что влечет за собой неопределенное поведение и является серьезной ошибкой.

Также в коде Вангеров присутствует довольно похожий недочет:

char* iGetMergedName(char* name,char* path)
{
  ....
  return strdup(out.c_str());
}

void ivrtMap::fileLoad(void)
{
  ....
  XBuffer buf;
  buf < fileName < (isCompressed ? ".vmc" : ".vmp");
  std::string sbuf=strdup(iGetMergedName(buf.GetBuf(),iniName));
  std::string sbuf2;
  ....
}

Если функция iGetMergedName() возвратит NULL, то он будет передан функции strdup().Это также повлечет за собой разыменование нулевого указателя и неопределенное поведение.

Что же с этим делать? Ответ довольно простой: нужно всегда проверять указатель, который возвращают функции типа malloc(), calloc() и strdup() и им подобные. И в случае, если возвращенное значение является нулевым указателем, обрабатывать это как ошибку, например - с помощью исключений. Если данная рекомендация кажется вам неубедительной, то предлагаю познакомиться со статьёй "Почему важно проверять, что вернула функция malloc".

Анализатор обнаружил еще несколько похожих ошибок:

V522 CWE-690 There might be dereferencing of a potential null pointer 'item'. ascr_fnc.cpp 5356
V522 CWE-690 There might be dereferencing of a potential null pointer. A constructor of the string class expects a valid pointer. ivmap.cpp 309
V522 CWE-690 There might be dereferencing of a potential null pointer 'idxName'. Check lines: 94, 92. zip_resource.cpp 94
V575 CWE-628 The potential null pointer is passed into 'strlen' function. Inspect the first argument. Check lines: 2156, 2155. road.cpp 2156
V575 CWE-628 The potential null pointer is passed into 'strlen' function. Inspect the first argument. Check lines: 810, 809. vmap.cpp 810
V575 CWE-628 The potential null pointer is passed into 'strlen' function. Inspect the first argument. Check lines: 813, 812. vmap.cpp 813

Пониженная производительность и остатки от рефакторинга

Еще одна ошибка, найденная анализатором:

int dastPoly3D::quant_make_sign(void){
  ....
  for( int s = 0; s < dastResSign->once[n]; s++){
    ....
    switch (dastResSign -> type[n][count]){
    ....
      case DAST_SIGN_TYPE::DOWN:
      {
        uchar *data = new uchar[16];
        if ( dastResSign -> type[n][count] == DAST_SIGN_TYPE::DOWN )
          memset(data, 251, 16);
        else
          memset(data, 2, 16);
        ....
        }
        delete[] data;
        break;
      }
    ....
    }
    ....
  }
  return (count != dastResSign->poly[n]);
}

Предупреждение PVS-Studio: V819 Decreased performance. Memory is allocated and deleted multiple times inside the body of the loop. Consider moving memory allocation outside of the loop. poly3d.cpp 161

Здесь имеет место пониженная производительность. Выделение и освобождение динамической памяти расположены внутри цикла и выполняются каждую итерацию. Подобные функции лучше выносить за пределы цикла, дабы сэкономить ценные вычислительные ресурсы. Это особенно это актуально для компьютерных игр. Судя по всему, буфер uchar *data и все связанные с ним вызовы функций остались здесь после рефакторинга. Массив создается, заполняется числами, удаляется - и всё, больше он ни с чем не взаимодействует. И "болтается" он так каждую итерацию цикла. Поэтому разработчикам стоит пересмотреть код этой функции, и удалить все ненужные строки. Тогда и работать она будет быстрее, и анализатор больше не будет выдавать предупреждения.

Неправильное освобождение динамической памяти

Наконец, приведу последний пример.

void aciPackFile(char* fname)
{
  int sz,sz1;
  char* p,*p1;
  
  ....
  
  p = new char[sz];
  p1 = new char[sz1];

  ....

  delete p;
  delete p1;
}

Предупреждения PVS-Studio:

V611 CWE-762 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] p;'. ascr_fnc.cpp 4401
V611 CWE-762 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] p1;'. ascr_fnc.cpp 4402

На весь проект анализатор выдал много предупреждений V611 - около двадцати пяти. Недочет кроется в операторе освобождения памяти: для удаления массива необходимо использовать оператор delete[], а не скалярный delete.

Что же происходит при подобной ошибке? Результат: неопределённое поведение. На некоторых реализациях такой код может случайно отработать без ошибок, но тем не менее, он неправилен.

Можно предположить такой сценарий: вместо освобождения памяти, которую занимают массивы p и p1, происходит удаление только их первых элементов, которые также являются указателями на сам массив. Оставшаяся память останется неосвобожденной, а получить доступ к ней мы уже не сможем.

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

Обычно реализация оператора new[] устроена так, что в начало участка памяти, выделенного под массив, дополнительно помещается размер этого участка и количество элементов в массиве. Если вызвать оператор delete (без квадратных скобок) для блока, выделенного с помощью new[], то информация о размере блока и числе элементов, вероятно, будет интерпретирована неправильно, и результат такой операции не определен.

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

Следует отметить, что компилятор не предупредит о том, что вы пытаетесь удалить массив как скаляр, так как он не отличает указатель на массив от указателя на отдельный элемент. Поэтому нужно внимательно следить за соответствием операторов выделения и освобождения памяти. Ответственность за это лежит на разработчике. Если память выделялась оператором new, то освобождаться она должна оператором delete, а если с помощью оператора new[], то освобождаться она должна соответственно оператором delete[]. В противном случае это может привести к любым последствиям: повреждению кучи, порче памяти, аварийному завершению программы - и отследить происходящее для каждой реализации очень сложно.

Заключение

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

Предлагаю любому желающему скачать PVS-Studio и проверить с его помощью свой проект: ссылка для скачивания.

#Cpp #Java