На днях компания ObjectArts полностью открыла исходники и выпустила язык, и среду разработки Dolphin Smalltalk под открытой лицензией MIT! Я не смог пройти мимо, не попробовав проверить этот проект с помощью анализатора кода PVS-Studio. Могу поздравить разработчиков с тем, что у них получилось создать код высокого качества. Мне не удалось найти значимых ошибок. Однако как всегда есть некоторое количество багов и пахнущего кода. Надеюсь благодаря этой статье код станет чуть лучше.

О проекте

Dolphin Smalltalk — это среда разработки на собственном диалекте Smalltalk для Windows. Ключевыми особенностями является тесная интеграция с нативными виджетами и подсистемами операционной системы, включая COM и ActiveX, и приятный глазу графический дизайн.

Долгое время Dolphin Smalltalk был доступен в двух вариантах: условно-бесплатная ограниченная версия (community edition) и платный пакет для профессиональной разработки. Последний давал доступ ко всем функциям, включая продвинутые редакторы и публикацию приложений в standalone режиме, однако стоил около четырехсот долларов.

С помощью PVS-Studio 6.00 были проверены открытые исходники Dolphin Smalltalk Virtual Machine. Далее представлены результаты проверки статическим анализатором. Несмотря на то, что проект DolphinVM очень маленький, в его коде всё равно встречаются подозрительные места.

Результаты проверки

Предупреждение N1: V611 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 [] msg;'. compiler.cpp 379

Compiler::StaticType Compiler::FindNameAsStatic(....)
{
  ....
  char* msg = new char[strlen(szPrompt)+name.size()+32];
  ::wsprintf(msg, szPrompt, name.c_str());
  char szCaption[256];
  ::LoadString(GetResLibHandle(), IDR_COMPILER, szCaption, ....);
  int answer = ::MessageBox(NULL, msg, szCaption, ....);
  delete msg;  //<==??
  ....
}

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

После вызова оператора «new []» необходимо освобождать память с помощью оператора «delete []».

Предупреждение N2: V716 Suspicious type conversion in return statement: returned BOOL, but function actually returns HRESULT. idolphinstart.cpp 78

#define STDMETHODIMP    HRESULT STDMETHODCALLTYPE

STDMETHODIMP CDolphinSmalltalk::GetVersionInfo(LPVOID pvi)
{
  extern BOOL __stdcall GetVersionInfo(VS_FIXEDFILEINFO* ....);
  return ::GetVersionInfo(static_cast<VS_FIXEDFILEINFO*>(pvi));
}

В этом фрагменте кода тип «BOOL» неявным образом приводится к типу «HRESULT». В то время, как такая операция вполне допустима с точки зрения языка C++, она не имеет практического смысла. Тип HRESULT предназначен для хранения статуса, имеет достаточно сложный формат и не имеет ничего общего с типом BOOL.

Предупреждение N3: V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'elems' is lost. Consider assigning realloc() to a temporary pointer. compiler.cpp 2922

POTE Compiler::ParseByteArray()
{
  NextToken();
  while (m_ok && !ThisTokenIsClosing())
  {
    if (elemcount>=maxelemcount)
    {
      _ASSERTE(maxelemcount > 0);
      maxelemcount *= 2;
      elems = (BYTE*)realloc(elems, maxelemcount*sizeof(BYTE));
    }
    ....
  }
  ....
}

Данный код является потенциально опасным: рекомендуется результат функции realloc() сохранять в другой переменной. Функция realloc() производит изменение размера некоторого блока памяти. В случае, если изменение размера блока памяти в данный момент невозможно, функция вернёт нулевой указатель. Главная проблема заключается в том, что при использовании конструкции вида «ptr = realloc(ptr, ...)» указатель ptr на этот блок данных может быть утерян.

Аналогичные опасные места:

• V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'm_pAllocations' is lost. Consider assigning realloc() to a temporary pointer. alloc.cpp 436
• V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'pUnmarked' is lost. Consider assigning realloc() to a temporary pointer. gc.cpp 217

Предупреждение N4: V547 Expression 'i >= 0' is always true. Unsigned type value is always >= 0. compact.cpp 35

// Answer the index of the last occuppied OT entry
unsigned __stdcall ObjectMemory::lastOTEntry()
{
  HARDASSERT(m_pOT);
//  HARDASSERT(m_nInCritSection > 0);

  unsigned i = m_nOTSize-1;
  const OTE* pOT = m_pOT;
  while (pOT[i].isFree())
  {
    ASSERT(i >= 0);
    i--;
  }

  return i;
}

Скорее всего ошибки здесь нет, но код подозрительный. По очереди перебираются элементы массива до тех пор, пока функция isFree() для одного из элементов не вернёт false. Неправильным здесь является ASSERT. Он, на самом деле, ничего не проверяет. Переменная 'i' беззнаковая, а значит она всегда больше или равна 0.

Ещё одно сравнение '>=0' с беззнаковым типом:

• V547 Expression is always true. Unsigned type value is always >= 0. loadimage.cpp 343

Предупреждение N5: V730 Not all members of a class are initialized inside the constructor. Consider inspecting: m_dwSize. imagefilemapping.h 13

class ImageFileMapping
{
  HANDLE m_hFile;
  HANDLE m_hMapping;
  LPVOID m_pData;
  DWORD  m_dwSize;

public:
  ImageFileMapping() : m_hFile(0), m_hMapping(0), m_pData(NULL){}
  ~ImageFileMapping() { Close(); }
  ....
};

Ещё один случай потенциально опасного кода. Класс «ImageFileMapping» содержит всего 4 поля, но в конструкторе присваивают начальное значение только трём из них. Член 'm_dwSize' остается неинициализированным.

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

Похожие классы:

• V730 Not all members of a class are initialized inside the constructor. Consider inspecting: m_flags, m_oopWorkspacePools, m_context, m_compiledMethodClass. compiler.cpp 84
• V730 Not all members of a class are initialized inside the constructor. Consider inspecting: m_tokenType, m_integer, tp, m_cc, m_base. lexer.cpp 40

Предупреждение N6: V665 Possibly, the usage of '#pragma warning(default: X)' is incorrect in this context. The '#pragma warning(push/pop)' should be used instead. Check lines: 99, 101. compact.cpp 101

// Perform a compacting GC
size_t ObjectMemory::compact()
{
  ....
  #pragma warning (disable : 4127)
  while(true)
  #pragma warning (default : 4127)
  ....
}

Программисты часто считают, что после директивы «pragma warning(default: X)» опять начнут действовать предупреждения, отключенные ранее помощью «pragma warning(disable: X)». Это не так. Директива 'pragma warning(default: X)' устанавливает предупреждение с номером 'X' в состояние, которое действует ПО УМОЛЧАНИЮ. Это далеко не одно и то же.

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

size_t ObjectMemory::compact()
{
  ....
  #pragma warning(push)
  #pragma warning (disable : 4127)
  while(true)
  #pragma warning(pop)
  ....
}

Хорошая статья по данной теме: "Итак, вы хотите заглушить это предупреждение в Visual C++".

Весь список таких мест:

• V665 Possibly, the usage of '#pragma warning(default: X)' is incorrect in this context. The '#pragma warning(push/pop)' should be used instead. Check lines: 244, 246. expire.cpp 246
• V665 Possibly, the usage of '#pragma warning(default: X)' is incorrect in this context. The '#pragma warning(push/pop)' should be used instead. Check lines: 226, 241. expire.cpp 241
• V665 Possibly, the usage of '#pragma warning(default: X)' is incorrect in this context. The '#pragma warning(push/pop)' should be used instead. Check lines: 126, 128. interfac.cpp 128
• V665 Possibly, the usage of '#pragma warning(default: X)' is incorrect in this context. The '#pragma warning(push/pop)' should be used instead. Check lines: 385, 387. interprt.cpp 387

Предупреждение N7: V576 Incorrect format. Consider checking the fourth actual argument of the 'wsprintfA' function. To print the value of pointer the '%p' should be used. interfac.cpp 679

inline DWORD __stdcall
Interpreter::GenericCallbackMain(SMALLINTEGER id, BYTE* lpArgs)
{
  ....
#ifdef _DEBUG
  {
    char buf[128];
    wsprintf(buf, "WARNING: .... (%d, %x)\n", id, lpArgs);
    WarningWithStackTrace(buf);
  }
  #endif
  ....
}

Очень часто значение указателя пытаются распечатать, используя спецификатор '%x'.

Этот код ошибочен, поскольку будет работать только в тех системах, где размер указателя совпадает с размером типа 'int'. А, например, в Win64 этот код уже распечатает только младшую часть указателя 'ptr'. В этом случае необходимо использовать спецификатор '%p'.

Предупреждение N8: V547 Expression 'ch > 127' is always false. The value range of char type: [-128, 127]. decode.cpp 55

ostream& operator<<(ostream& stream, const VariantCharOTE* oteChars)
{
  ....
  char ch = string->m_characters[i];
  //if (ch = '\0') break;
  if (ch < 32 || ch > 127)  //<==
  {
    static char hexChars[16+1] = "0123456789ABCDEF";
    ....
  }
  ....
}

По умолчанию тип 'char' имеет диапазон значений, равный [-127;127]. С помощью флага компиляции /J можно сказать компилятору, чтобы использовался диапазон [0;255]. Но для компиляции этого исходного файла такой флаг не указывается, поэтому проверка «ch > 127» не имеет смысла.

Предупреждение N9: V688 The 'prev' function argument possesses the same name as one of the class members, which can result in a confusion. thrdcall.h 126

void LinkAfter(T* prev)
{
  T* pThis = static_cast<T*>(this);
  this->next = prev->next;
  if (this->next)
    this->next->prev = pThis;
  this->prev = prev;
  prev->next = pThis;
}

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

Предупреждение N10: V601 The 'false' value is implicitly cast to the integer type. compiler.cpp 1940

int Compiler::ParseUnaryContinuation(...., int textPosition)
{
  int continuationPointer = m_codePointer;
  MaybePatchLiteralMessage();
  while (m_ok && (ThisToken()==NameConst)) 
  {
    int specialCase=false;  //<==
    ....
    if (!specialCase)       //<==
    {
      int sendIP = GenMessage(ThisTokenText(), 0, textPosition);
      AddTextMap(sendIP, textPosition, ThisTokenRange().m_stop);
    }
    ....
  }
  ....
}

В данном случае, это предупреждение носит рекомендательный характер. Если с переменной 'specialCase' везде работает как с логической переменной, то лучше использовать стандартный тип 'bool' для этого.

Заключение

Ещё один проект отправился в список проверенных нами открытых проектов.

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

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

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. A Tribute to Opening Up Dolphin Smalltalk 7's Source Code.