Андрей Карпов

25 Июн 2013

Теги:

#Cpp

Повторная проверка TortoiseSVN с помощью анализатора кода PVS-Studio

25 Июн 2013

Автор: Андрей Карпов

Странные условия
Подозрительное формирование строки
Неправильное обнуление массивов
Странное
64-битные ошибки
Устаревшие проверки нулевых указателей
И так сойдёт
Вывод
Ссылки

Мы отправили разработчикам TortoiseSVN на некоторое время бесплатный ключ для анализатора PVS-Studio. Пока они не успели им воспользоваться, я решил быстро скачать исходные коды TortoiseSVN и самостоятельно выполнить анализ. Цель понятна. Очередная небольшая статья для рекламы PVS-Studio.

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

Время от времени мы повторяем проверки, чтобы продемонстрировать пользу от регулярного использования инструмента. Нет смысла один или два раза проверить проект. В изменяемом коде постоянно появляются новые ошибки. А потом медленно и печально исправляются. Соответственно, максимальная польза будет достигнута при ежедневном использовании PVS-Studio. А ещё лучше, при использовании инкрементального анализа.

Итак, посмотрим, что удалось найти интересного в проекте с помощью PVS-Studio версии 5.05. Исходные коды TortoiseSVN были взяты 19.06.2013 из tortoisesvn.googlecode.com/svn/trunk. Кстати, проект TortoiseSVN является очень качественным и имеет огромную базу пользователей-программистов. Так что если найти хоть что-то, это уже большое достижение.

Странные условия

static void ColouriseA68kDoc (....)
{
  if (((sc.state == SCE_A68K_NUMBER_DEC) && isdigit(sc.ch))
      ....
      || ((sc.state == SCE_A68K_MACRO_ARG) && isdigit(sc.ch))
      || ((sc.state == SCE_A68K_MACRO_ARG) && isdigit(sc.ch))
      ....
}

Диагностическое сообщение: V501 There are identical sub-expressions '((sc.state == 11) && isdigit(sc.ch))' to the left and to the right of the '||' operator. lexa68k.cxx 160

Присутствует два одинаковых сравнения. Возможно, имеет место опечатка.

Опечатка, наверное, присутствует и в следующем коде. Два раза проверяется значение переменной 'rv'.

struct hentry * AffixMgr::compound_check(
  ....
  if (rv && forceucase && (rv) && ....)
  ....
}

Диагностическое сообщение: V501 There are identical sub-expressions to the left and to the right of the '&&' operator: rv && forceucase && (rv):

affixmgr.cxx 1784
affixmgr.cxx 1879

Следующий фрагмент кода с неправильным сравнением:

int main(int argc, char **argv)
{
  ....
  DWORD ticks;
  ....
  if (run_timers(now, &next)) {
    ticks = next - GETTICKCOUNT();
    if (ticks < 0) ticks = 0;
  } else {
    ticks = INFINITE;
  }
  ....
}

Диагностическое сообщение: V547 Expression 'ticks < 0' is always false. Unsigned type value is never < 0. winplink.c 635

Переменная 'ticks' является беззнаковой. Это значит, что проверка "if (ticks < 0)" не имеет смысла. Ситуация возникновения переполнения не будет обработана.

Рассмотрим ошибку, из-за которой функция 'strncmp' сравнивает строки не полностью.

int  AffixMgr::parse_convtable(...., const char * keyword)
{
  char * piece;
  ....
  if (strncmp(piece, keyword, sizeof(keyword)) != 0) {
  ....
}

Диагностическое сообщение: V579 The strncmp function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. affixmgr.cxx 3654

Оператор 'sizeof' вычисляет размер указателя. Это значение никак не связанно с длиной строки.

Подозрительное формирование строки

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

class CTSVNPath
{
  ....
private:
  mutable CString m_sBackslashPath;
  mutable CString m_sLongBackslashPath;
  mutable CString m_sFwdslashPath;
  ....
};

const FileStatusCacheEntry * SVNFolderStatus::BuildCache(
  const CTSVNPath& filepath, ....)
{
  ....
  CTraceToOutputDebugString::Instance() (_T(__FUNCTION__)
    _T(": building cache for %s\n"), filepath);
  ....
}

Диагностическое сообщение: V510 The 'operator()' function is not expected to receive class-type variable as second actual argument:

svnfolderstatus.cpp 150
svnfolderstatus.cpp 355
svnfolderstatus.cpp 360

Спецификатор "%s" указывает, что в качестве фактического аргумента функция ожидает строку. Однако, переменная 'filepath' вовсе не строка, а сложный объект, состоящий из множества строк. Затрудняюсь сказать, что будет распечатано и не упадёт ли вообще этот код.

Опасно использовать такие функции, как 'printf()' следующим образом: "printf(myStr);". Если внутри 'myStr' будут присутствовать управляющие спецификаторы, то программа может распечатать то, что ей не полагается или аварийно завершиться.

Рассмотрим код из TortoiseSVN:

BOOL CPOFile::ParseFile(....)
{
  ....
  printf(File.getloc().name().c_str());
  ....
}

Диагностическое сообщение: V618 It's dangerous to call the 'printf' function in such a manner, as the line being passed could contain format specification. The example of the safe code: printf("%s", str); pofile.cpp 158

Если имя файла будет "myfile%s%i%s.txt", то результат будет плачевен.

Примечание. У нас есть интересная заметка на тему опасности использования функции printf().

Неправильное обнуление массивов

Я не знаю, насколько для TortoiseSVN опасно оставить содержимое буферов, не обнулив их. Возможно, вообще безопасно. Однако есть код для обнуления буферов. А поскольку он не работает, стоит про это упомянуть. Ошибки выглядят так:

static void sha_mpint(SHA_State * s, Bignum b)
{
  unsigned char lenbuf[4];
  ....
  memset(lenbuf, 0, sizeof(lenbuf));
}

Диагностическое сообщение: V597 The compiler could delete the 'memset' function call, which is used to flush 'lenbuf' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. sshdss.c 23

Перед выходом из функции, массив 'lenbuf' следует очистить. Так как затем массив больше не используется, оптимизатор удалит вызов функции 'memset'. Чтобы этого не происходило, требуется использовать специальные функции.

Другие места, где компилятор удалит вызов 'memset()':

sshdss.c 37
sshdss.c 587
sshdes.c 861
sshdes.c 874
sshdes.c 890
sshdes.c 906
sshmd5.c 252
sshrsa.c 113
sshpubk.c 153
sshpubk.c 361
sshpubk.c 1121
sshsha.c 256

Странное

BOOL InitInstance(HINSTANCE hResource, int nCmdShow)
{
  ....
  app.hwndTT; // handle to the ToolTip control
  ....
}

Диагностическое сообщение: V607 Ownerless expression 'app.hwndTT'. tortoiseblame.cpp 1782

Скорее всего, в функции 'InitInstance()' , член 'hwndTT' должен чем-то инициализироваться. Однако, из-за опечатки, код оказался недописанным.

64-битные ошибки

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

64-битные ошибки я просматриваю ещё более поверхностно. Очень сложно судить, не зная структуру проекта, возможно возникновение той или иной ошибки или нет.

Приведу только пару опасных мест:

void LoginDialog::CreateModule(void)
{
  ....
  DialogBoxParam(g_hmodThisDll, MAKEINTRESOURCE(IDD_LOGIN),
                 g_hwndMain, (DLGPROC)(LoginDialogProc),
                 (long)this);
  ....
}

Диагностическое сообщение: V220 Suspicious sequence of types castings: memsize -> 32-bit integer -> memsize. The value being casted: 'this'. logindialog.cpp 105

Указатель 'this' явно приводится к типу 'long'. Затем он неявно расширяется до типа LPARAM (LONG_PTR). Важно то, что указатель на какое-то время превращается в 'long'. Это плохо, если программа является 64-битной. Указатель занимает 64-бита. Тип 'long' в Win64 по прежнему является 32-битным типом. В результате старшие биты 64-биной переменной будут потеряны.

Если объект создан за пределами младших 4 Гб оперативной памяти, то работа программы станет непредсказуема. Вероятность такого события конечно не велика, но зато такую ошибку крайне сложно воспроизвести.

Правильный код: DialogBoxParam(...., (LPARAM)this);

Рассмотрим другое опасное приведение типа:

static int cmpforsearch(void *av, void *bv)
{
  Actual_Socket b = (Actual_Socket) bv;
  unsigned long as = (unsigned long) av,
                bs = (unsigned long) b->s;
  if (as < bs)
    return -1;
  if (as > bs)
    return +1;
  return 0;
}

Диагностическое сообщение: V205 Explicit conversion of pointer type to 32-bit integer type: (unsigned long) av:

winnet.c 139
winhandl.c 359
winhandl.c 348

Указатели явным образом приводятся к типу 'unsigned long' и помещаются в переменные 'as' и 'bs'. Так как старшие биты адреса при этом могут быть потеряны, сравнение может работать неправильно. Вообще не понятно, зачем здесь указатели приводятся к целочисленным типам. Можно просто сравнить указатели.

Устаревшие проверки нулевых указателей

Оператор 'new', когда не может выделить память, давным-давно не возвращает NULL. Он генерирует исключение std::bad_alloc. Конечно, можно сделать, чтобы оператор 'new' возвращал 0, но это сейчас к делу не относится.

Тем не менее, в программах продолжает жить код следующего вида:

int _tmain(....)
{
  ....
  pBuf = new char[maxlength];
  if (pBuf == NULL)
  {
    _tprintf(_T("Could not allocate enough memory!\n"));
    delete [] wc;
    delete [] dst;
    delete [] src;
    return ERR_ALLOC;
  }
  ....
}

Диагностическое сообщение: V668 There is no sense in testing the 'pBuf' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error.

subwcrev.cpp 912
repositorybrowser.cpp 2565
repositorybrowser.cpp 4225
svnstatuslistctrl.cpp 5254
svnprogressdlg.cpp 2357
bugtraqassociations.cpp 116
xmessagebox.cpp 792
xmessagebox.cpp 797
hyperlink_base.cpp 166
affixmgr.cxx 272
hashmgr.cxx 363
hashmgr.cxx 611

И так сойдёт

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

void CBaseView::OnContextMenu(CPoint point, DiffStates state)
{
  ....
  popup.AppendMenu(MF_STRING | oWhites.HasTrailWhiteChars ?
                   MF_ENABLED : (MF_DISABLED|MF_GRAYED),
                   POPUPCOMMAND_REMOVETRAILWHITES, temp);
  ....
}

Диагностическое сообщение: V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '|' operator. baseview.cpp 2246

В зависимости от значения переменной 'oWhites.HasTrailWhiteChars' требуется получить одно из сочетаний констант:

MF_STRING | MF_ENABLED
MF_STRING | MF_DISABLED | MF_GRAYED

Но код работает совсем не так. Приоритет операции '|' выше, чем приоритет операции '?:'. Расставим скобки для наглядности:

(MF_STRING | oWhites.HasTrailWhiteChars) ? MF_ENABLED : MF_DISABLED | MF_GRAYED

Код корректно работает, только благодаря тому, что константа 'MF_STRING' равна 0. Она не оказывает никакого влияния на результат. В результате, неправильное выражение работает правильно.

Рассмотрим другой пример везения. В программе TortoiseSVN тип HWND нередко используется как тип 'unsigned'. Для этого приходится выполняться явные преобразования типа. Например, так, как показано в следующих функциях:

HWND m_hWnd;
UINT_PTR uId;
INT_PTR CBaseView::OnToolHitTest(....) const
{
  ....
  pTI->uId = (UINT)m_hWnd;
  ....
}

UINT_PTR  idFrom;
HWND m_hWnd;

BOOL CBaseView::OnToolTipNotify(
  UINT, NMHDR *pNMHDR, LRESULT *pResult)
{
  if (pNMHDR->idFrom != (UINT)m_hWnd)
    return FALSE;
  ....
}

Или, например, значение переменной типа HWND печатается, как если бы это был тип 'long'.

bool CCommonAppUtils::RunTortoiseProc(....)
{
  ....
  CString sCmdLine;
  sCmdLine.Format(L"%s /hwnd:%ld",
    (LPCTSTR)sCommandLine, AfxGetMainWnd()->GetSafeHwnd());
  ....
}

Формально этот код неверен. Дело в том, что тип 'HWND' представляет собой указатель. А значит, его нельзя превращать в 32-битные целочисленные типы. И анализатор PVS-Studio переживает по этому поводу, выдавая предупреждения.

Но интересно то, что этот код будет работать совершенно корректно!

Тип HWND используется для хранения дескрипторов, которые используются в Windows для работы с различными системными объектами. Такими же типами являются HANDLE, HMENU, HPALETTE, HBITMAP и так далее.

Хотя дескрипторы являются 64-битными указателями, для большей совместимости (например, для возможности взаимодействия между 32-битынми и 64-битными процессами) в них используется только младшие 32-бита. Подробнее смотри "Microsoft Interface Definition Language (MIDL): 64-Bit Porting Guide" (USER and GDI handles are sign extended 32b values).

Помещая тип HWND в 32-битные типы, разработчики вряд ли основывались именно на этих допущениях. Скорее всего, это не очень аккуратный код, работающий корректно благодаря везению и стараниям разработчиков Windows API.

Вывод

Используйте статический анализ регулярно при разработке, и вы найдете множество ошибок на самых ранних этапах. Я естественно рекомендую в первую очередь познакомиться с анализатором кода PVS-Studio. Однако, есть много других хороших анализаторов кода: инструменты статического анализа кода.