Скачай PVS-Studio и найди ошибки в С, С++,C# коде
559,96
рейтинг
25 ноября 2014 в 10:43

Разработка → Проект Miranda NG получает приз «дикие указатели» (часть первая)

Miranda NG
Я добрался до проекта Miranda NG и проверил его с помощью анализатора кода PVS-Studio. К сожалению, с точки зрения работы с памятью и указателями это самый неаккуратный проект из виданных мной. Хотя я внимательно не анализировал результаты, ошибок столь много, что я решил разбить собранный материал на 2 статьи. Первая статья будет посвящена указателям, а вторая всему остальному. Желаю приятного чтения, и не забудьте взять попкорн.

О проверке Miranda NG


Проект Miranda NG — это преемник популярного мультипротокольного IM-клиента для Windows, Miranda IM.

Вообще, изначально я не собирался проверять Miranda NG. Нам просто нужно несколько активно развивающихся проектов, чтобы тестировать новую фичу PVS-Studio. Можно использовать специальную базу, в которой хранится информация о сообщениях, которые не надо выдавать. Подробнее про это рассказано здесь. Идея в следующем. Иногда сложно внедрить статический анализ в большой проект, так как выдаётся слишком много предупреждений и непонятно, что с ними делать и кому это поручить. Но хочется начать получать пользу от статического анализа уже сейчас. Поэтому для начала можно скрыть все предупреждения и рассматривать только новые, которые появляются в процессе написания нового кода или рефакторинга. А уж потом, когда есть силы и желание, можно потихоньку править ошибки в старом коде.

Одним из активно развивающихся проектов оказался Miranda NG. Но когда я посмотрел, что выдал PVS-Studio при первом запуске, я понял, что у меня есть материал на новую статью.

Итак, давайте посмотрим, что нашел статический анализатор кода PVS-Studio в исходных кодах Miranda NG.

Для проверки Miranda NG из репозитория был взят Trunk. Хочу отметить, что я смотрел отчёт анализатора весьма поверхностно и наверняка многое упустил. Я пробежался только по диагностикам общего назначения 1 и 2-ого уровня. Третий уровень я даже не стал смотреть. Мне и первых двух хватило с избытком.

Часть первая. Указатели и работа с памятью



Разыменовывание нулевого указателя


void CMLan::OnRecvPacket(u_char* mes, int len, in_addr from)
{
  ....
  TContact* cont = m_pRootContact;
  ....
  if (!cont)
    RequestStatus(true, cont->m_addr.S_un.S_addr);
  ....
}

Предупреждение PVS-Studio: V522 Dereferencing of the null pointer 'cont' might take place. EmLanProto mlan.cpp 342

Здесь все просто. Раз указатель равен NULL, то давайте его разыменуем и посмотрим, что весёлого из этого получится.

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


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

Типичный пример:
void TSAPI BB_InitDlgButtons(TWindowData *dat)
{
  ....
  HWND hdlg = dat->hwnd;
  ....
  if (dat == 0 || hdlg == 0) { return; }
  ....
}

Предупреждение PVS-Studio: V595 The 'dat' pointer was utilized before it was verified against nullptr. Check lines: 428, 430. TabSRMM buttonsbar.cpp 428

Если в функцию BB_InitDlgButtons() передать NULL, то проверка будет сделана слишком поздно. Анализатор выдал на код проекта Miranda NG ещё 164 таких предупреждения. Нет смысла приводить их в статье. Вот все они списком: MirandaNG-595.txt.

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


BSTR IEView::getHrefFromAnchor(IHTMLElement *element)
{
  ....
  if (SUCCEEDED(....)) {
    VARIANT variant;
    BSTR url;
    if (SUCCEEDED(element->getAttribute(L"href", 2, &variant) &&
        variant.vt == VT_BSTR))
    {
      url = mir_tstrdup(variant.bstrVal);
      SysFreeString(variant.bstrVal);
    }
    pAnchor->Release();
    return url;
  }
  ....
}

Предупреждение PVS-Studio: V614 Potentially uninitialized pointer 'url' used. IEView ieview.cpp 1117

Если условие if (SUCCEEDED(....)) не выполняется, то переменная 'url' остаётся неинициализированной и функция должна вернуть непонятно что. Но не всё так просто. Код содержит ещё одну ошибку. Неправильно поставлена закрывающаяся скобка. В результате макрос SUCCEEDED применяется к выражению типа 'bool', что не имеет смысла.

Второй баг компенсирует первый. Посмотрим, что такое макрос SUCCEEDED:
#define SUCCEEDED(hr) (((HRESULT)(hr)) >= 0)

Выражение типа 'bool' равно 0 или 1. В свою очередь, 0 или 1 всегда >= 0. Получается, что макрос SUCCEEDED всегда возвращает истину и переменная 'url' всегда инициализируется.

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

Если иcправить 2 ошибки, то код будет выглядеть так:
BSTR url = NULL;
if (SUCCEEDED(element->getAttribute(L"href", 2, &variant)) &&
    variant.vt == VT_BSTR)

Анализатор подозревает неладное ещё в 20 местах. Вот они: MirandaNG-614.txt.

Путаница между размером массива и количеством элементов


Количество элементов в массиве и размер массива в байтах — две разные сущности. Однако, если быть недостаточно внимательным, их вполне можно перепутать. Проект Miranda NG демонстрирует множество способов, как может выглядеть такая путаница.

Больше всего вреда нанёс макрос SIZEOF:
#define SIZEOF(X) (sizeof(X)/sizeof(X[0]))

Этот макрос вычисляет количество элементов в массиве. Но, видимо, кто-то из программистов считает, что это аналог оператора sizeof(). Правда, не понятно, зачем он тогда использует макрос, а не стандартный sizeof(). Так что есть и другой вариант — кто-то не знает, как использовать функцию memcpy().

Типичный случай:
int CheckForDuplicate(MCONTACT contact_list[], MCONTACT lparam)
{
  MCONTACT s_list[255] = { 0 };
  memcpy(s_list, contact_list, SIZEOF(s_list));
  for (int i = 0;; i++) {
    if (s_list[i] == lparam)
      return i;
    if (s_list[i] == 0)
      return -1;
  }
  return 0;
}

Предупреждение PVS-Studio: V512 A call of the 'memcpy' function will lead to underflow of the buffer 's_list'. Sessions utils.cpp 288

Функция memcpy() скопирует только часть массива, так как третий аргумент задаёт размер массива в байтах.

Точно так же неправильно макрос SIZEOF() используется ещё в 8 местах: MirandaNG-512-1.txt.

Следующая беда. Часто программисты забывают поправить вызовы memset()/memcpy() когда внедряют в программу Unicode:
void checkthread(void*)
{
  ....
  WCHAR msgFrom[512];
  WCHAR msgSubject[512];
  ZeroMemory(msgFrom,512);
  ZeroMemory(msgSubject,512);
  ....
}

Предупреждения PVS-Studio:
  • V512 A call of the 'memset' function will lead to underflow of the buffer 'msgFrom'. LotusNotify lotusnotify.cpp 760
  • V512 A call of the 'memset' function will lead to underflow of the buffer 'msgSubject'. LotusNotify lotusnotify.cpp 761

Функция ZeroMemoty() очистит только половину буфера, так как символы типа WCHAR занимают 2 байта.

А вот пример частичного копирования строки:
INT_PTR CALLBACK DlgProcMessage(....)
{
  ....
  CopyMemory(tr.lpstrText, _T("mailto:"), 7);
  ....
}

Предупреждение PVS-Studio: V512 A call of the 'memcpy' function will lead to underflow of the buffer 'L«mailto:»'. TabSRMM msgdialog.cpp 2085

Будет скопирована только часть строки. Каждый символ строки занимает 2 байта. Значит скопировать надо 14 байт, а не 7.

Аналогично:
  • userdetails.cpp 206
  • weather_conv.cpp 476
  • dirent.c 138

Следующая ошибка допущена просто по невнимательности:
#define MSGDLGFONTCOUNT 22

LOGFONTA logfonts[MSGDLGFONTCOUNT + 2];

void TSAPI CacheLogFonts()
{
  int i;
  HDC hdc = GetDC(NULL);
  logPixelSY = GetDeviceCaps(hdc, LOGPIXELSY);
  ReleaseDC(NULL, hdc);

  ZeroMemory(logfonts, sizeof(LOGFONTA) * MSGDLGFONTCOUNT + 2);
  ....
}

Предупреждение PVS_Studio: V512 A call of the 'memset' function will lead to underflow of the buffer 'logfonts'. TabSRMM msglog.cpp 134

Кто-то поспешил и смешал в кучу размер объекта и их количество. Прибавлять двойку нужно до умножения. Корректный код:
ZeroMemory(logfonts, sizeof(LOGFONTA) * (MSGDLGFONTCOUNT + 2));

В следующем примере хотели, как лучше, использовали sizeof(), но вновь запутались с размерами. Получили значение больше, чем нужно.
BOOL HandleLinkClick(....)
{
  ....
  MoveMemory(tr.lpstrText + sizeof(TCHAR)* 7,
             tr.lpstrText,
             sizeof(TCHAR)*(tr.chrg.cpMax - tr.chrg.cpMin + 1));
  ....
}

Предупреждение PVS-Studio: V620 It's unusual that the expression of sizeof(T)*N kind is being summed with the pointer to T type. Scriver input.cpp 387

Переменная 'tr.lpstrText' указывает на строку, состоящую из символов типа wchat_t. Если хочется пропустить 7 символов, то нужно просто прибавить 7. Не нужно умножать на sizeof(wchar_t).

Такая же ошибка здесь: ctrl_edit.cpp 351

К сожалению, это ещё не конец. Ещё можно ошибиться так:
INT_PTR CALLBACK DlgProcThemeOptions(....)
{
  ....
  str = (TCHAR *)malloc(MAX_PATH+1);
  ....
}

Предупреждение PVS-Studio: V641 The size of the allocated memory buffer is not a multiple of the element size. KeyboardNotify options.cpp 718

Забыли умножить на sizeof(TCHAR). В этом же файле можно увидеть ещё 2 ошибки в строках 819 и 1076.

И последний фрагмент кода на тему количества элементов:
void createProcessList(void)
{
  ....
  ProcessList.szFileName[i] =
    (TCHAR *)malloc(wcslen(dbv.ptszVal) + 1);

  if (ProcessList.szFileName[i])
    wcscpy(ProcessList.szFileName[i], dbv.ptszVal);
  ....
}

Предупреждения PVS-Studio: V635 Consider inspecting the expression. The length should probably be multiplied by the sizeof(wchar_t). KeyboardNotify main.cpp 543

Ещё стоит дописать умножение на sizeof(TCHAR) здесь: options.cpp 1177, options.cpp 1204.

С размерами разобрались, перейдём к другим способам выстрелить себе в ногу указателем.

Выход за границу массива


INT_PTR CALLBACK DlgProcFiles(....)
{
  ....
  char fn[6], tmp[MAX_PATH];
  ....
  SetDlgItemTextA(hwnd, IDC_WWW_TIMER,
    _itoa(db_get_w(NULL, MODNAME, strcat(fn, "_timer"), 60),
    tmp, 10));
  ....
}

V512 A call of the 'strcat' function will lead to overflow of the buffer 'fn'. NimContact files.cpp 290

Строка "_timer" не влезает в массив 'fn'. Хотя в строке всего 6 символов, нужно не забывать про терминальный 0. Теоретически здесь имеет место неопределённое поведение. На практике получится, что будет задет массив 'tmp'. В нулевой элемент массива 'tmp' будет записан '0'.

Следующий пример ещё печальней. Здесь испортится HANDLE какой-то иконки:
typedef struct
{
  int cbSize;
  char caps[0x10];
  HANDLE hIcon;
  char name[MAX_CAPNAME];
} ICQ_CUSTOMCAP;

void InitCheck()
{
  ....
  strcpy(cap.caps, "GPG AutoExchange");
  ....
}

Предупреждение PVS-Studio: V512 A call of the 'strcpy' function will lead to overflow of the buffer 'cap.caps'. New_GPG main.cpp 2246

Вновь не учтён терминальный 0. Возможно, здесь уместнее было бы воспользоваться функцией memcpy().

Другие места с проблемным кодом:
  • main.cpp 2261
  • messages.cpp 541
  • messages.cpp 849
  • utilities.cpp 547

Великая и ужасная функция strncat()


Многие слышали про опасность функции strcat() и используют на их взгляд более безопасную strncat(). Вот только редко кто умеет обращаться с этой функцией правильно. Она намного опаснее, чем может показаться. Дело в том, что третий аргумент задаёт не максимальную длину буфера, а сколько в буфере осталось свободного места.

Вот такой код совершенно некорректен:
BOOL ExportSettings(....)
{
  ....
  char header[512], buff[1024], abuff[1024];
  ....
  strncat(buff, abuff, SIZEOF(buff));
  ....
}

Предупреждение PVS-Studio: V645 The 'strncat' function call could lead to the 'buff' buffer overflow. The bounds should not contain the size of the buffer, but a number of characters it can hold. Miranda fontoptions.cpp 162

Если, 'buff' занят на половину, то такой код не обратит на это внимание и позволит добавить ещё 1000 символов. И таким образом произойдет выход за границу массива. Причем очень существенный. С таким же успехом можно было просто писать strcat().

Вообще, запись strncat(...., ...., SIZEOF(X)) в принципе неверна. Она означает, что в массиве ВСЕГДА есть ещё свободное место.

В Miranda NG есть ещё 48 мест где так неправильно используется strncat(). Вот они: MirandaNG-645-1.txt.

Кстати, эти места в коде можно рассматривать как потенциальные уязвимости.

В оправдание программистов из Miranda NG, стоит отметить, что некоторые всё таки читали описание функции strncat(). Они пишут так:
void __cdecl GGPROTO::dccmainthread(void*)
{
  ....
  strncat(filename, (char*)local_dcc->file_info.filename,
          sizeof(filename) - strlen(filename));
  ....
}

Предупреждение PVS-Studio: V645 The 'strncat' function call could lead to the 'filename' buffer overflow. The bounds should not contain the size of the buffer, but a number of characters it can hold. GG filetransfer.cpp 273

К сожалению, это опять неправильно. По крайней мере так можно испортить 1 байт за пределами массива. И я думаю, вы уже догадались, что причина в злосчастном терминальном нуле. Он не учтён.

Поясним эту ошибку на простом примере:
char buf[5] = "ABCD";
strncat(buf, "E", 5 - strlen(buf));

В буфере уже нет места для новых символов. В нём находится 4 символа и терминальный ноль. Выражение «5 — strlen(buf)» равно 1. Функция strncpy() скопирует символ «E» в последний элемент массива 'buf'. Терминальный 0 будет записан уже за пределами буфера.

Остальные 34 места кода собраны здесь: MirandaNG-645-2.txt.

Эротика с использованием new[] и delete


Кто-то систематически забывает писать квадратные скобки для оператора delete:
extern "C" int __declspec(dllexport) Load(void)
{
  int wdsize = GetCurrentDirectory(0, NULL);
  TCHAR *workingDir = new TCHAR[wdsize];
  GetCurrentDirectory(wdsize, workingDir);
  Utils::convertPath(workingDir);
  workingDirUtf8 = mir_utf8encodeT(workingDir);
  delete workingDir;
  ....
}

Предупреждение PVS-Studio: 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 [] workingDir;'. IEView ieview_main.cpp 68

Вот здесь ещё 20 таких мест: MirandaNG-611-1.txt.

Впрочем, такие ошибки часто обходятся без видимых последствии. Именно поэтому я отнёс их к разделу «эротика». Более жесткое зрелище ожидает ниже.

Разврат с использованием new, malloc, delete и free


Путаются способы выделения и освобождения памяти:
void CLCDLabel::UpdateCutOffIndex()
{
  ....
  int *piWidths = new int[(*--m_vLines.end()).length()];
  ....
  free(piWidths);
  ....
}

Предупреждение PVS-Studio: V611 The memory was allocated using 'new' operator but was released using the 'free' function. Consider inspecting operation logics behind the 'piWidths' variable. MirandaG15 clcdlabel.cpp 209

Ещё 11 поз из Камасутры здесь: MirandaNG-611-2.txt.

Бессмысленные проверки


В случае нехватки памяти обыкновенный оператор 'new' генерирует исключение. Поэтому нет смысла проверять указатель, который вернул оператор 'new' на равенство нулю.

Обычно лишняя проверка безобидна. Однако, иногда встречается вот такой код:
int CIcqProto::GetAvatarData(....)
{
  ....
  ar = new avatars_request(ART_GET); // get avatar
  if (!ar) { // out of memory, go away
    m_avatarsMutex->Leave();
    return 0;
  }
  ....
}

Предупреждение PVS-Studio: V668 There is no sense in testing the 'ar' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. ICQ icq_avatar.cpp 608

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

Предлагаю разработчикам проверить следующие 83 аналогичных предупреждений анализатора: MirandaNG-668.txt.

Путаница между SIZEOF() и _tcslen()


#define SIZEOF(X) (sizeof(X)/sizeof(X[0]))
....
TCHAR *ptszVal;
....
int OnButtonPressed(WPARAM wParam, LPARAM lParam)
{
  ....
  int FinalLen = slen + SIZEOF(dbv.ptszVal) + 1;
  ....
}

Предупреждение PVS-Studio: V514 Dividing sizeof a pointer 'sizeof (dbv.ptszVal)' by another value. There is a probability of logical error presence. TranslitSwitcher layoutproc.cpp 827

Здесь написано что-то странное. Макрос SIZEOF() применяется к указателю, что не имеет никакого смысла. Есть подозрения, что хотели подсчитать длину строки. Тогда для этого следует использовать функцию _tcslen().

Аналогично:
  • layoutproc.cpp 876
  • layoutproc.cpp 924
  • main.cpp 1300

Порча vptr


class CBaseCtrl
{
  ....
  virtual void Release() { }
  virtual BOOL OnInfoChanged(MCONTACT hContact, LPCSTR pszProto);
  ....
};

CBaseCtrl::CBaseCtrl()
{
  ZeroMemory(this, sizeof(*this));
  _cbSize = sizeof(CBaseCtrl);
}

Предупреждение PVS-Studio: V598 The 'memset' function is used to nullify the fields of 'CBaseCtrl' class. Virtual method table will be damaged by this. UInfoEx ctrl_base.cpp 77

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

Аналогично:
  • ctrl_base.cpp 87
  • ctrl_base.cpp 103.

Время жизни объектов


static INT_PTR CALLBACK DlgProcFindAdd(....)
{
  ....
  case IDC_ADD:
    {
      ADDCONTACTSTRUCT acs = {0};

      if (ListView_GetSelectedCount(hwndList) == 1) {
        ....
      }
      else {
        ....                                         
        PROTOSEARCHRESULT psr = { 0 };                 <<<---
        psr.cbSize = sizeof(psr);
        psr.flags = PSR_TCHAR;
        psr.id = str;

        acs.psr = &psr;                                <<<---
        acs.szProto = (char*)SendDlgItemMessage(....);
      }
      acs.handleType = HANDLE_SEARCHRESULT;
      CallService(MS_ADDCONTACT_SHOW,
                  (WPARAM)hwndDlg, (LPARAM)&acs);
    }
    break;
  ....
}

Предупреждение PVS-Studio: V506 Pointer to local variable 'psr' is stored outside the scope of this variable. Such a pointer will become invalid. Miranda findadd.cpp 777

Объект 'psr' перестанет существовать, когда произойдёт выход из else-ветви. Однако, указатель на этот объект был сохранён и будет в дальнейшем использоваться. Пример настоящего «дикого указателя». К чему приведёт работа с ним — неизвестно.

Ещё один аналогичный пример:
HMENU BuildRecursiveMenu(....)
{
  ....
  if (GetKeyState(VK_CONTROL) & 0x8000) {
    TCHAR str[256];
    mir_sntprintf(str, SIZEOF(str),
      _T("%s (%d, id %x)"), mi->pszName,
      mi->position, mii.dwItemData);

    mii.dwTypeData = str;
  }
  ....
}

Предупреждение PVS-Studio: V507 Pointer to local array 'str' is stored outside the scope of this array. Such a pointer will become invalid. Miranda genmenu.cpp 973

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

Удивительно, как такие программы вообще работают. Ещё 9 мест, где обитают дикие указатели: MirandaNG-506-507.txt.

Мучения 64-битных указателей


Я не изучал 64-битные диагностики. Посмотрел только предупреждения с номером V220. Почти каждое такое предупреждение — самая настоящая ошибка.

Пример некорректного кода с точки зрения 64-битности:
typedef LONG_PTR LPARAM;

LRESULT
WINAPI
SendMessageA(
    __in HWND hWnd,
    __in UINT Msg,
    __in WPARAM wParam,
    __in LPARAM lParam);

static INT_PTR CALLBACK DlgProcOpts(....)
{
  ....
  SendMessageA(hwndCombo, CB_ADDSTRING, 0, (LONG)acc[i].name);
  ....
}

Предупреждение PVS-Studio: V220 Suspicious sequence of types castings: memsize -> 32-bit integer -> memsize. The value being casted: 'acc[i].name'. GmailNotifier options.cpp 55

Куда-то нужно передать 64-битный указатель. Для этого, его нужно превратить в тип LPARAM. Однако, вместо этого указатель насильственно превращают в 32-битный тип LONG. И только потом он будет автоматически расширен до LONG_PTR. Эта ошибка пришла из 32-битных времён, когда типы LONG и LPARAM совпадали. Теперь это не так. Будут испорчены старшие 32 бита в 64-битном указателе.

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

Вот ещё 20 мест, где портятся 64-битные указатели: MirandaNG-220.txt.

Неочищенные приватные данные


void CAST256::Base::UncheckedSetKey(....)
{
  AssertValidKeyLength(keylength);
  word32 kappa[8];
  ....
  memset(kappa, 0, sizeof(kappa));
}

Предупреждение PVS-Studio: V597 The compiler could delete the 'memset' function call, which is used to flush 'kappa' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. Cryptlib cast.cpp 293

В Release версии компилятор удалит вызов функции memset(). Почему так, можно узнать из описания диагностики.

Не затрутся приватные данные ещё в 6 местах: MirandaNG-597.txt.

Разное


Есть ещё пара предупреждений анализатора, которые я свалю в одну кучу.
void LoadStationData(...., WIDATA *Data)
{
  ....
  ZeroMemory(Data, sizeof(Data));
  ....
}

Предупреждение PVS-Studio: V512 A call of the 'memset' function will lead to underflow of the buffer 'Data'. Weather weather_ini.cpp 250

Выражение 'sizeof(Data)' возвращает размер указателя, а не WIDATA. Будет обнулена только часть объекта. Правильно будет написать: sizeof(*Data).
void CSametimeProto::CancelFileTransfer(HANDLE hFt)
{
  ....
  FileTransferClientData* ftcd = ....;

  if (ftcd) {
    while (mwFileTransfer_isDone(ftcd->ft) && ftcd)
      ftcd = ftcd->next;
  ....
}

Предупреждение PVS-Studio: V713 The pointer ftcd was utilized in the logical expression before it was verified against nullptr in the same logical expression. Sametime files.cpp 423

В условии цикла указатель 'ftcd' в начале разыменовывается, а только потом проверяется. Видимо, выражение стоит переписать так:
while (ftcd && mwFileTransfer_isDone(ftcd->ft))

Заключение


Работа с указателями и памятью — не единственное, из чего состоят программы на Си++. В следующей статье мы рассмотрим другие разновидности ошибок, обнаруженных в Miranda NG. Их поменьше, но всё равно достаточно много.

Эта статья на английском


Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Miranda NG Project to Get the «Wild Pointers» Award (Part 1).

Прочитали статью и есть вопрос?
Часто к нашим статьям задают одни и те же вопросы. Ответы на них мы собрали здесь: Ответы на вопросы читателей статей про PVS-Studio и CppCat, версия 2014. Пожалуйста, ознакомьтесь со списком.


UPD: Продолжение здесь.
Автор: @Andrey2008
PVS-Studio
рейтинг 559,96
Скачай PVS-Studio и найди ошибки в С, С++,C# коде

Комментарии (222)

  • +18
    А оно вообще из trunk работало?
    • +27
      Не знаю. Да это и не важно. Не из-за транка же сотни ошибок. Ну может к релизу парочку бы из перечисленных ошибок поправили. Не более. Почти все из представленных ошибок живут в программах годами и изредка (но сильно) портят жизнь странными невоспроизводимыми глюками и падениями программы.
      • НЛО прилетело и опубликовало эту надпись здесь
  • +8
    Virtual method table will be damaged by this.

    Порча указателя на таблицу виртуальных методов

    Чувствуете разницу?
    • +8
      Тут относительный момент. На самом деле неизвестно, как реализуются виртуальные функции. Таблица теоретически может быть и внутри класса.
      В стандартах C++ нет четкого определения как должна реализовываться динамическая координация, но компиляторы зачастую используют некоторые вариации одной и той же базовой модели.
      Обычно компилятор создает отдельную vtable для каждого класса. После создания объекта указатель на эту vtable, называемый виртуальный табличный указатель или vpointer, добавляется как скрытый член данного объекта (а зачастую как первый член). Компилятор также генерирует «скрытый» код в конструкторе каждого класса для инициализации vpointer'ов его объектов адресами соответствующей vtable.

      Ключевое слово — обычно. В диагностике хотели сказать, что виртуальной таблицей больше пользоваться будет нельзя. Она испорчена. Вернее не она, а указатель на неё. Но никакой разницы нет.
      Но вообще спасибо за замечание. Возможно исправим название диагностики.
      • +3
        На самом деле, разница есть. Потому что конструктор производного класса этот указатель исправит. Таким образом, базовый класс не работает, а все производные от него — работают.

        Если бы таблица хранилась прямо в объекте, такого бы не произошло (конструктор производного класса исправлял бы только те записи, которые были перегружены).
        • +4
          >> На самом деле, разница есть. Потому что конструктор производного класса этот указатель исправит.

          Это настолько деталь реализации, что полагаться на это — не стоит ни разу. Например, в производном классе может не быть виртуальных функций, и нигде в программе нет динамических проверок на данный тип — тогда и указатель не нужно обновлять. Или оно может попробовать дернуть что-то через этот указатель до его обновления. Да мало ли что… U.B. — это U.B., не надо пытаться его обыграть.
          • +2
            Разумеется. Просто в одном случае программа вообще работать не может, а в другом — «вроде работает».
  • –33
    Когда же люди на Rust перейдут?..
    • +38
      Когда создадут параллельный мир, в котором не будет ничего, кроме Rust — это единственный возможный вариант перехода абсолютно всех людей на Rust =)
      • +12
        В таком мире будет слишком большой соблазн изобрести Python.
        • +10
          Кстати, было бы неплохо изобрести его заного. Чтобы сразу третий…
          • +1
            И ещё бы неплохо удалить все системы распространения пакетов в Python и написать единую. Идеальную. С нуля.
            • +2
              А потом форкнуть питон и переименовать в Rust. Облом.
    • +4
      Руст-то ладно, он только что зарелизился. Вот почему D никто не юзает — для меня загадка. Уже десяток лет ему точно есть.
      • +3
        По моему, боятся сборки мусора. Там без нее не везде можно обойтись.
        • 0
          Наверное, Вы правы.
      • +4
        Где-то же разбиралось. Одно из мнений было в том, что D опоздал и упустил свой шанс. Если бы он выпустился значительно раньше C++11, то мог бы и взлететь. А сейчас «C++ made right» уже не так интересен по сравнению со всякими Rust. И у него нет киллер-фичи, за которую можно было бы простить слабую экосистему.

        А никто не использует — это как посмотреть. Большие продукты™ никто не будет переписывать с /Си.*/ на что-либо ещё. И начинать их писать на маргинальном языке никто не будет. Наколенные проектики — вполне, да, но для больших продуктов™ замкнутый круг. Который можно было бы разорвать, протолкнув ту киллер-фичу, но…
    • 0
      Он ещё не зарелизился, мало (по сравнению с С++) библиотек (или хотя бы врапперов) для него, и ещё много чего нет по сравнению с более зрелыми языками. Года через два он, вероятно, станет очень серьёзным конкурентом C++, а пока, когда когда каждую неделю ломают обратную совместимость, слишком рано, чтобы в массовом порядке переходить на Rust.
      • 0
        Каждые два года люди говорят, что через два года быть может D взлетит.
        Язык клевый, но он правда опоздал.
  • 0
    Будет скопирована только часть строки. Каждый символ строки занимает 2 байта. Значит скопировать надо 14 байт, а не 7.

    Интереса ради, в C++ используют суррогатные пары в wchar (т. е. символ u+00010000 и выше, представленный двумя wchar)?
    • 0
      Используют.
    • 0
      В winapi это используется (потому что когда микрософт перешёл на юникод никаких символов больше 00010000 не было). Поэтому программы под Винду также зачастую хранят строки в utf-16.
    • +1
      Кстати говоря, на *NIX платформах обычно sizeof(wchar_t) == 4. Ещё один аргумент в пользу никогда не использовать wchar_t, если в программе используется более одной кодировки текста.
      • 0
        Нынче модны char16_t и char32_t.
        • 0
          Обыкновенные char и uint8_t всегда в моде. И таки можно всегда хранить внутри все строки в utf8 и конвертировать на границе ответственности.
          • 0
            Тут проблема в том, что две основные десктопные ОС хотят именно UTF-16, а конвертирование несколько неудобно, да и накладные расходы добавляет. Хотя жизнь, несомненно, была бы проще, если бы везде было UTF-8.
            • +1
              Посмотрел на _NET_* свойства, они хотят utf-8… Или вы про другую вторую ОС с NSString в utf-16?

              Перекодировать из utf-8 (который вполне удобен как для сети, так и для работы внутри) в ucs-2 для символов до u+7f тривиально, до u+7fff просто. Если нужно дальше, то генерировать суррогатные пары — не велик труд. Так что накладных расходов копейки (основные — лишнее выделение памяти). Но при этом неплохо экономится память на ascii-текстах и нервы при передаче по сети (где utf-8 куда интереснее utf-16, т. к. не надо думать об endianess).

              Хотя для внешнего обмена и хранения мне очень нравится BOCU-1, но у неё есть одна проблема, и имя ей — IBM. Для индексов lucene/solr оно выглядит очень интересно (т. к. позволяет хранить мультиязычный текст с плотностью около 1 байта на символ для символов, представимых в однобайтной кодировке).
            • –1
              Хотя жизнь, несомненно, была бы проще, если бы везде было UTF-8.
              С символами, занимающими от одного до шести байтов?
              Сомнительное упрощение.
              • +4
                А какая вам разница, что по одной-две единицы хранения на символ (UTF-16), что по одной-шести (UTF-8)? Что так, что так нужно работать с переменной длиной.

                Зато в UTF-8 этот факт почти для всех очевиден. А вот UTF-16 для очень многих символов позволяет допущение одно слово = один символ, и в итоге люди пишут сломанный код, не зная об этом.
                • +2
                  А какая вам разница, что по одной-две единицы хранения на символ (UTF-16), что по одной-шести (UTF-8)?
                  После появления суррогатных пар в UTF-16 всё стало не так радужно, конечно, но и отчего жизнь стала бы вдруг проще при повсеместной победе UTF-8 мне тоже не ясно. Вот UCS-4 — другое дело, но он очень уж затратный по памяти.
                  Зато в UTF-8 этот факт почти для всех очевиден.
                  Для тех, чей родной язык/язык повседневного общения использует для письма только символы из первой половины ASCII-таблицы, это далеко не всегда очевидно.
                  С ходу найденный пример: http://stackoverflow.com/q/6257818/882813
                  И подобных вопросов там немало.
                  • 0
                    Ну согласитесь все же, что языков с символами только из ASCII все же в разы меньше, чем таковых из UCS2. Собственно, мне, кроме английского, ничего в голову не приходит — да и там встречаются слова вроде naïve, плюс заимствования из испанского с ñ в американском английском.
                  • +1
                    После появления суррогатных пар в UTF-16 всё стало не так радужно, конечно, но и отчего жизнь стала бы вдруг проще при повсеместной победе UTF-8 мне тоже не ясно.
                    У utf-8 есть ещё бонус, что его представление не зависит от host endianess. В случае utf-16 это не так (из-за чего и существует bom), что приводит к лишним конверсиям на одной стороне: на современных процессорах это всего лишь дополнительные 1-3 такта на символ, на тех, что не поддерживает swap может быть куда дороже.
                    • +1
                      Справедливости ради, представление UTF-16 LE тоже не зависит от host endianess. И UTF-16 BE не зависит. Надо было создателям стандарта бросить кубики и выбрать одну из этих двух кодировок вместо придумывания bom-а.
                      • 0
                        Нет, если endianess строки в памяти не совпадает с endianess платформы, вся сложность переехала бы в софт, который делает посимвольную обработку. Наример, ф-цию
                        vector<utf18be_string> split(const utf18be_string& line, uint16_t separator);
                        нужно было бы вызывать не как
                        split(aLine, ':'),
                        а как
                        split(aLine, TO_UTF16BE_CHAR(':'));

                        Все коды символов в текстовой записи (не в бинарной, где можно договориться до BE), хранимые в базах/конфигах или получаемые по сети (например, HTML "& #58;") нужно было бы конвертить в big endianess перед тем, как использовать в посимвольной обработке.
                        • 0
                          Можно было бы объявить функцию как vector<utf16be_string> split(const utf16be_string& line, char16_t separator); и вызывать как split(aLine, L':').

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

                          Действительно различалась бы в таком случае обработка отдельных символов. Так, если для преобразования арабской цифры в число достаточно выполнить проверку ('0' <= ch && ch <= '9') и потом преобразование (ch — '0') — то на платформе с отличающейся endianess пришлось бы делать что-то более сложное.

                          С другой стороны, проще корректировать endianess в процессе ввода-вывода, и хранить строки в памяти всегда в наиболее удобном формате. Тем более, что сейчас так и делается — и я не виду причин, по которым все усложнилось бы просто из-за исчезновения из мира лишней кодировки.
                          • 0
                            Можно было бы объявить функцию как vector<utf16be_string> split(const utf16be_string& line, char16_t separator); и вызывать как split(aLine, L':').

                            А если разделитель лежит как первый символ строки aLine или в любой другой строке:
                            split(aLine, CHAR_BE_TO_CHAR_W(aLine[0]))

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

                            Это не просто произвольная кодировка, ничем не лучшая других аналогичных. У неё есть особенность — она с нативным для архитектуры endianess.
                            • 0
                              Это не просто произвольная кодировка, ничем не лучшая других аналогичных. У неё есть особенность — она с нативным для архитектуры endianess.
                              Вы так говорите, как будто строка в памяти обязана иметь кодировку.
                              • 0
                                Просто хранение — редкая задача. Часто нужна минимальная обработка.
                                Например, для определения длины строки, важна кодировка.
                                • 0
                                  Я не предлагаю хранить строку как попало. Я предлагаю скрыть все подробности хранения в стандартной библиотеке, чтобы «на выходе» строка была всего лишь последовательностью символов, независимо от способа хранения этих символов. И обрабатывать строку надо как последовательность абстрактных символов, а не байт.

                                  А вот именно при хранении строки, или при передаче — и возникает понятие кодировки.
                                  • 0
                                    Так можно. Но незнание представления в памяти породит оверхед там, где он не нужен. Тот же доступ str[i] может делать bswap, а может не делать.
                      • 0
                        вместо придумывания bom

                        BOM по хорошему нужен и для UTF-8 (камень в огород тех кто говорит что он ему не нужен), ибо не во всех случаях костыли для определения UTF-8 работают. Стоит влезть какому-нибудь NUL или ещё чем-нибудь из подобного рода и капец, пока не включишь подсветку таких знаков и не найдёшь или не пройдёшься (созданной горьким опытом) регуляркой, фиг поймёшь в чём проблема.
                        Вот регулярка кстати, вдруг кому-нибудь пригодится:
                        (\0|\x01|\x02|\x03|\x04|\x05|\x06|\x07|\x08|\x0B|\x0C|\x0E|\x0F|\x10|\x11|\x12|\x13|\x14|\x15|\x16|\x17|\x18|\x19|\x1A|\x1B|\x1C|\x1D|\x1E|\x1F|\x7F)
                        

                        Прогоняю обычно через Notepad++.

                        А между прочим у меня была ещё такая ситуация когда такие служебные знаки должны быть в тексте (отладочном), при этом должны соседствовать с UTF-8 строками. Когда вот BOM добавишь всё хорошо, а без него везде слетает в ANSI.
                        • +1
                          костыли для определения UTF-8 работают
                          Вы либо знаете кодировку текста (utf8, utf16, какое-нибудь хитрое cjk или очередная однобайтная копрофосилия мамонта типа cpxxxx), либо определяете её эвристически (по статистике n-грамм, например). Наличие 0xfffe или 0xfeff в начале файла не говорит ничего о кодировке. Может там текст в cp1251 начинается с «яю»/«юя»?

                          BOM (byte order mark, напомню) нужен, чтобы определить endianess системы, на которой текущее байтовое представление было нативным для int16_t (ucs-2, utf-16) или int32_t (utf-32). Он не является прямым признаком используемой кодировки. Чтобы его интерпретировать нужно знать кодировку данного набора байт заранее.

                          А между прочим у меня была ещё такая ситуация когда такие служебные знаки должны быть в тексте (отладочном), при этом должны соседствовать с UTF-8 строками. Когда вот BOM добавишь всё хорошо, а без него везде слетает в ANSI.
                          Это паркетно-половые трудности используемых редакторов и некоторая «особенность» одной ОС. Эта же ОС, например, рапортует, что у неё в консоли cp866/ibm866, а имена файлов из этой же консоли передаёт перекодированными в cp1251. Happy debug, как говорится.
              • +1
                Сложно жить с utf-1, благо она не взлетела. А utf-8 самосинхронизируется. И поиск начала следующего codepoint'а не сложнее, чем проверка на суррогатность пары в utf-16
  • +16
    Видимо именно поэтому я перестал ей пользоваться, ибо текло и падало.
    • –9
      Ну так Вы же программист. Никто не мешал провести отладку. От этого и Вам и людям польза. Людям то что их любимая Miranda NG стабильнее, а Вам дополнительная строчка в резюме. Замечу хвалиться достижениями сделанными в коммерческих проектах очень сложно ибо NDA и др., а вот то что сделано в OpenSource очень даже помогает при собеседованиях!
      • +37
        Если я буду допиливать чужие проекты, мне не останется времени на свои.
        • +3
          А вы считайте чужие, которые дорабатывайте, своими :)
          • +8
            Ага, а свои, на которые не хватило времени, чужими.
        • –3
          Извините за возможно глупый вопрос, но тем не менее: можете пояснить проекты, которые Вы называете «своими»?
          • +5
            Все просто на самом деле. Любой человек каждый день проходит мимо глупости, не оптимальных решений, не решенных проблем. Просто проходит мимо.
            Конечно наш мир стал бы лучше если бы все видя лежащий на земле мусор доносили бы его до ближайшей мусорки, помогали бы всем встреченным нуждающимся в помощи, ремонтировали бы механизмы. Но все это можно делать только за счет своих личных времени и сил. Аналогия думаю ясна.
      • +24
        Ох, если бы все сантехники чинили все унитазы, которыми им приходится пользоваться, например в общественных туалетах…
    • НЛО прилетело и опубликовало эту надпись здесь
      • НЛО прилетело и опубликовало эту надпись здесь
        • +7
          Раз вы не понимаете, за что минусы — расскажу, хотя и не хотел заниматься декларациями очевидного. Кстати, минус вместо комментария зачастую «прилетает» именно от нежелания писать банальность в 100500й раз, а не от желания нагадить.

          Банальность
          Если у кого-то программа работает — это еще не значит, что она будет работать у всех. Дело не только в количестве протоколов или включенных плагинов, работоспособность зависит еще от многих других причин. Например, от сервера, к которому вы подключаетесь. Даже для закрытой-презакрытой аськи существуют прокси-серверы (не знаю, что именно они делают, но ни раз видел инструкцию вида «как выйти в аську из нашей локалки») — а другой сервер может немного по-другому использовать протокол, что приведет к активации других ветвей выполнения программы. Я уже не говорю про протокол XMPP, который настолько расширяемый, что у каждого сервера появляются свои особенности.

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

          Разумеется, все это не означает, что Миранда должна падать у всех. В конце концов, этот мессенджер живет уже кучу лет, оставаясь при этом на слуху — а значит, он обязан «в целом скорее работать, чем падать». Более того, поскольку (со слов Ivan_83) разработчики Miranda NG собирают крешдампы — все оставшиеся баги возникают достаточно редко, частые баги давно починили.

          Однако, все эти выявленные анализатором места — это запах кода. Плотность таких глупых ошибок при написании кода в целом постоянна — и если в каких-то ветках их уже нет — значит, они там были. Каким образом они могли исчезнуть? Их могли найти в процессе отладки, тестирования или эксплуатации. Но программисты, пишущие такое, отладкой и тестированием обычно не заморачиваются. Следовательно, они были выявлены в процессе эксплуатации, то есть когда-то Миранда действительно текла и падала. И причина этих утечек и падений — именно в тех самых «диких указателях», раскрытых в статье.

          PS Вы вот в комментарии ниже обвиняете оппонентов в «кармадрочестве» — а сами при этом не забываете смотреть на свои циферки :) Позвольте дать совет. Одиночный комментарий может собрать хоть -100 — но при этом, если он является корректным (не содержит оскорблений и полного бреда, а также имеет внутри хоть какую-то смысловую нагрузку) — в карму, как правило, прилетает не больше трех минусов. А вот если оскорблять собеседников, или отказываться читать их сообщения, говоря одно и то же по 10 раз — то комментирование раз в пять минут практически гарантировано.
          • НЛО прилетело и опубликовало эту надпись здесь
            • –3
              Не буду говорить про пафос. Скажу про то, что видно — гадкие люди тоже любят уезжать из России, это радует. Эти 10-50-100% денег ничего стоить не будут, когда ЯБ будет над головой пролетать.

              Ах… да, думаю тупость будет публике видна даже в том, что субъект пишет на русском портале. При этом его ненавидя. Клоун, что с него еще взять?

              Лучше бы следил за йеллоустоуном.
              • НЛО прилетело и опубликовало эту надпись здесь
  • +5
    > element->getAttribute(L"href", 2, &variant)
    Тут еще один баг, IHTMLElement::getAttribute в качестве аргумента ожидает BSTR, а не wchar_t*. На моей практике этот метод не падает, но думаю только потому, что в MS написали дебилоустойчивый код.

    Показывает ли Ваш анализатор этот момент?
    • +3
      Пока нет. У нас подобные диагностики в todo записаны, но до них никак руки не дойдут. Там правда много полуложных срабатываний будет. Многие не различают BSTR и wchar_t * и хранят как попало. Но работаю только как с wchar_t * (т.е. BSTR им не нужен на самом деле). И поэтому программа работает.
      • +1
        Там проблема еще в том, что BSTR — это и есть wchar_t*, с т.з. типов. MSDN:
        typedef WCHAR OLECHAR;
        typedef OLECHAR* BSTR;
        typedef BSTR* LPBSTR;
        


        А то, что в контракт там входит еще и длина, это есть только на уровне документации.
  • +70
    Опубликовал ссылку на статью в Miranda NG Software в Facebook. Ответили:
    PVS constantly tries to PR using Miranda but PVS itself is too ugly to detect any real problem. none of these 'problems' really affect the program execution.

    А потом кажись и ссылку удалили. Хм… Ну с таким подходом… Я переживаю за качество. :)

    P.S. Эксперт, который за 5 минут смог сделать вывод что всё ok, ещё бугогашечки из второй статьи не видел. :)

    Ну чем не красоты:

    _tcscpy(src, src);
    
    for (i = MAX_PATH; 5; i--){

    Но всему своё время. Не всё сразу.

    • +4
      Похоже что просто обиделись, бывает :)
      Как по мне, так статический анализ – то, что действительно помогает перенести значительную часть внимания на идею и меньше думать о реализации. Может это сомнительное дело, вроде «пусть за меня думает компилятор», но в жизни нередко имеет место быть.
      • +8
        Обидеться на подобный баг репорт — это просто расписаться в собственной некомпетенции и поставить крест на себе как на профессионале.
        • +9
          Как-то все противоречиво. За такие баг репорты, с указанием конктретного места в коде и разбором полетов, нужно деньги предлагать, а не обижаться.
    • +6
      Помнится с VirtualDub'ом та же история была.
    • +11
      Там картинка в сообществе показательная. :)
    • +7
      Это отписка «ничего не знаем, у нас всё работает!»
    • –13
      Строго говоря они правы, во-первых, вы проверяете код не случайного васи пупкина, а известного проекта, стало быть это пиар, во-вторых, вы могли бы отправить им отчет и не писать никакой статьи и сохранили бы имидж проекта и помогли бы ему, т.е. снова пиар, в-третьих, сам тон статьи крайне пренебрежительный, это было бы справедливо в отношении платного софта, но никак не опенсурс, люди работают за идею и не просят за это денег, в отличие от вас.

      И вместо того, чтобы осознать это, вы продолжаете:
      > P.S. Эксперт, который за 5 минут смог сделать вывод что всё ok, ещё бугогашечки из второй статьи не видел. :)

      Никого не оправдываю, но призываю к пониманию.
      • +13
        Им нашли кучу ошибок. Не так разве? А они вместо того, чтобы принять их к сведенью, обиделись.
      • +18
        Я понимаю, что что мы как приносим пользу своими проверками, так и несём людям негативную энергию. Приходится показывать людям неприятные вещи. К сожалению (радости) этот вид рекламы работает, а значит мы будем продолжать этим заниматься. Как говорится, ничего личного, просто бизнес.

        Проверять и втихомолку отправлять отчёт нам смысла нет. Про отчёт узнает человек 10-20. Если они расскажут кому-то ещё, ну человек 100. Не будет никто про это статью писать, так что никакой 1000 читателей не будет. А вот эту статью читают тысячи. Десятки и тысячи читателей сравнивать нельзя.

        Кстати, чтобы найти все описанные в статье ошибки будет достаточно и CppCat (третий уровень GA в PVS-Studio и прочие диагностики я всё равно не смотрел). Я думаю, среди разработчиков Miranda NG, найдется пара студентов, которые воспользуются нашим предложение и прошерстят проект. См. Бесплатный CppCat для студентов.
        • +8
          Всё это хорошо и правильно, только не забывайте, что мы все люди, и не каждый может отделить критику своей работы от личной обиды. Open Source для многих – любимое хобби, как выращивание цветов или лепка глиняных котиков, и здесь как раз-таки стоит постараться обойтись без «негативной энергии».
        • НЛО прилетело и опубликовало эту надпись здесь
          • +12
            А что ещё можно ожидать в отчёте об использовании программы, которая создана для того, чтобы искать ошибки людей, не умеющих писать идеальный код с первого раза, и тыкать в них носом, чтобы они больше не повторялись?

            Вот так?
            Моя подруга с её парнем писали плагин к Miranda NG, я настоятельно не рекомендую им делать <так-то>

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

            <...>

            Типичный пример:

            void API_2 Function_234(Class_23 *var_1)
            {
              ....
              HWND handle = var_1->handle;
              ....
              if (var_1 == 0 || handle == 0) { return; }
              ....
            }


            Предупреждение PVS-Studio: V595 The 'var_1' pointer was utilized before it was verified against nullptr. Check lines: 428, 430. Plugin_8 source_file_42.cpp 428

            Неумение или нежелание отделять код от личности его автора — это ребячество. В статье нет ни одного прямого наезда на конкретных Джонов и Иванов, которые писали это код. Я думаю, уничижительный стиль вызван именно отвращением к качеству самого коду, а не к качествам разработчиков.

            Не важно, кто пишет код. Если это плохой код, его открытость и популярность — это отнюдь не оправдание. Только показатель того, какой части разработчиков безразлично качество кода. А воспринимать ли наезд на всё сообщество разработчиков как личное оскорбление — это зависит от того, осознаёт ли разработчик свою личную ответственность за замеченный, но не прибранный срач в куске кода по соседству. Коллективная безответственность творит чудеса.
            • НЛО прилетело и опубликовало эту надпись здесь
              • +8
                Статический анализатор именно что тыкает носом. Он не говорит вам идти и исправлять код, только: «Я вижу в строке 428 неправильное использование указателей».

                Статья же указывает на ошибки и даже даёт как разъяснения по некоторым проблемам (не делать больше так, как написано), так и конструктивные предложения по проекту вообще: использовать статические анализаторы, желательно сами-знаете-чей; и в один из следующих релизов запланировать чуть меньше перделок и чуть больше рефакторинга. Ну и да, выражает личное отвращение автора.

                я карму не минусовал.)
                • НЛО прилетело и опубликовало эту надпись здесь
                  • +9
                    PR-щик коммерческого продукта даже не дал себе труд воспользоваться MirandaNG

                    Извините, что влезаю — но зачем? Что это докажет? Есть некоорая ошибка, которая при определенном стечении обстоятельств может привести к сбою. В течение этих двух дней она почти наверняка не проявится. Смысл?

                    P.S. «сперва добейся» не работает.
                    • НЛО прилетело и опубликовало эту надпись здесь
                    • НЛО прилетело и опубликовало эту надпись здесь
      • +22
        Строго говоря, они все-таки не правы. Во-первых, каким бы ни был тон статьи, нет смысла отказываться от бесплатной помощи. Во-вторых, проблемы в статье описаны достаточно серьезные. Самое глупое, что можно сделать в этой ситуации — написать: «none of these 'problems' really affect the program execution». Прямо классика жанра.

        Из опыта могу представить, сколько мелких баков, протечек и неожиданных вылетов можно исправить просто пробежавшись по составленному списку. Работа с указателями — это все-таки работа с указателями. Иногда ошибку с затиранием какого-нибудь указателя можно искать всей командой неделю, был у меня такой случай. Проект большой, все на указателях. Где-то портился указатель на метод в объекте. Проявлялось редко и рандомно. Ушла неделя на то, чтобы найти выход за границу выделенной памяти при копировании блоков.

        А пиар… Много всего делается ради пиара. Когда магазин ради пиара устраивает распродажу со скидками, глупо этим не воспользоваться. Про «бесплатно» и говорить нечего. Умные люди и из этого могли бы извлечь выгоду, пропиариться в ответ: «Мы исправили 146% багов и теперь Миранда стала еще надежней».
        • –6
          Товарищ Yaruson грамотно все описал, так как люди тратят свое личное время, они могут себе позволить нерациональные решения, но этого всего можно было легко избежать.

          Я вам предлагаю даже эксперимент, громко объявите на весь оффис, что класс (допустим там будет баг), который написал ваш коллега — это худший код, котрый вы видели за всю вашу карьеру :)
          • +5
            так как люди тратят свое личное время, они могут себе позволить нерациональные решения

            Напомнило мне одну историю. Там водитель предлагал куда-то бегущей сотруднице подвезти ее. На что она ответила: «Мне некогда, я спешу».
            • 0
              я думаю, что вы меня поняли.
              • 0
                Да, понял. Просто я заострил внимание на одной стороне, вы — на другой. Думаю, обе стороны могли бы повести себя лучше. Пожалуй, извлеку и для себя урок.
        • 0
          Вы понимаете, что общественное порицание — это «бесплатная» помощь? Ооо, я знаю много людей, даже наверное я сам, отказался бы от такой бесплатной помощи.
        • –3
          А по моему они правы. Они удалили ссылку на негативный пиар своего продукта. Исправят они баги или нет — неизвестно. Если не исправят — вот тогда они будут не правы.
          • +9
            Возможно, я не прав, но одним из ключевых преимуществ опенсурса как раз приводят всегда то, что код открыт и любой может его проверить, что повышает доверие к продукту.

            В данном случае ребята получили крутейший способ пропиариться, и так убого его испортили.

            Offtopic: есть старый анекдот, как русские начали красить луну в красный (коммунистический) цвет, а амеры этим воспользовались, написав белым Coca-Cola.

            В данном случае ребята из Миранды вместо того, чтобы взять в руки кисть и довершить рекламу такого масштаба (малыми ресурсами добившись многого), начали тулить непонятные отмазки
            • 0
              В данном случае ребята из Миранды вместо того, чтобы взять в руки кисть и довершить рекламу такого масштаба (малыми ресурсами добившись многого), начали тулить непонятные отмазки

              всё уже исправаили
    • +4
      Ничего не удалили, есть ссылка: www.facebook.com/miranda.newgen/posts/1255536397803385
      • 0
        Да. Я сказал «кажись». Просто Facebook странно показывает ленту. Моё сообщение скрылось для меня и я решил, что его удалили.
        • +28
          Похоже сначала ответил их пиарщик, а потом — два(три) программиста ))
          image
          • 0
            Просто фейспалм.
      • 0
        Ссылка-то, может и есть, но в ленте я этого не вижу.
        • 0
          Теперь тоже не видите?
          • 0
            Теперь вижу. А куда делось 80% сообщения?.. Впрочем, этот вопрос надо не вам задавать…
            • 0
              Оно всё там, по клику раскрывается.
  • +4
    Складывается впечатление, что к проекту приложили руку школьники, которые C++ недавно увидели.
    • –5
      Вы абсолютно правы, более того многие «школьники» С++ и не видели. Я по крайней мере не видел, и учился во время разработки.

      Вообще там все написано на «Си с плюсовым синтаксисом».

      И перефразируя «это опенсоурс детка, тут и говнокодить могут». Вы бы еще по моим курсовым 10 летней давности PVS студией прошлись бы.

      З.Ы, кстати на моем рабочем проекте демоверсия тупо в корки упало, но я же не кричу об этом на каждом углу.
      • +1
        Одно дело курсовая, другое — массовый продукт.
        • +2
          :) Опенсорс — это не массовый продукт, это Опенсорс. Кто то пишет курсовые, кто то учиться кодить.
          Вас никто не заставляет им пользоваться, никто не навязывает, никто не продает, никто ничего не обещает. Я вот при написании clist_modern откровенно учился программировать, экспериментировал с WinApi и с разными недокументированными вещами. Когда научился — перестал писать для миранды.

          Я этого никогда не скрывал, и скрывать не собираюсь. Да мне сейчас стыдно за свой код. Но это код был учебным. Я конечно мог поступить наоборот и никому не показывать свой код. И все мирандовцы могли. Этой статьи бы не было, но код бы лучше от этого не стал.
          • +4
            Т.е. опенсорс не предполагается нигде использовать? Его создают исключительно ради самого процесса написания?
            • +11
              Ну кто как. Я лично создавал свой модуль исключительно для себя. Потом появились люди которым он понравился им тоже стали пользоваться. Они присылали какие то замечания которые по возможности исправлялись, какие то новые баги писались. Я его создавал именно ради самого процесса написания. Ну и мне хотелось иметь полупрозрачное окно мессенджера с различными шкурками еще в эпоху windows 9x/XP. Но опять таки для себя. Я не зажал ни код, ни модуль. Поделился чем имел. Это и есть мир настоящего опенсорс.
            • +1
              Чтобы учиться программировать, нельзя писать программы в стол. Чем большее количество людей пользуется той поделкой которую ты написал, тем больше фидбэка ты получаешь. И опенсорс в таком ключе мне кажется идеальным вариантом.

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

              А тот же «Google Summer of Code» вас не смущает, когда опенсорс целенаправленно используется для обучения студентов и от которого обе стороны только выигрывают
      • +12
        Это вы перегнули. Посмотрите на исходники Постгреса, например. Там академичность кода такая, что за патч с не выровненными строками, вас выдрючат и высушат.
        • +1
          Ну постгрес это постгрес. Я понимаю если бы тот же Гоша за каждый ответ на вопрос «как в настройках миранды включить вот это» просил бы хотя бы $1. Ну или хоть какой то платный саппорт. Или если бы была модерация патчей в миранде — то не было бы такого… разнообразия модулей, в чем и есть ее популярность.

      • +12
        >> это опенсоурс детка, тут и говнокодить могут
        А в проприетарных конечно не могут, там же не люди сидят, а волшебные феи.
        • –3
          Могут и там и там. Просто отбор в opensource гораздо ниже. В проприетарных хоть какое-то собеседование проводят прежде чем допустить работника к коду.
          • +2
            Ничего не имею против Индии и ее населения, но вот код, произведенный ими иногда поражает. Да, в коммерческих проектах.
    • –1
      Ну вот, кто-то пришел и начал молча минусовать в карму.
  • –48
    Спасибо конечно за багрепорт (своих строчек вроде не увидел :) )

    Мы это ценим, уважаем и любим. Вот если бы еще патч приложили :) Правда практика показывает, что толку от вашего анализа чуть… после первого же фрагмента перестал читать дальше — так проглядывал: Как вы думаете если у нескольких миллионов пользователей за несколько лет в этом месте ничего не падало, то каковы шансы что этот указатель окажется NULL?

    А прежде чем обвинять канал Miranda NG Software в Facebook, научитесь пользоваться FB :) никто ничего не удалял:

    www.facebook.com/miranda.newgen/posts/1255536397803385
    • +25
      Я не раз видел, как работал код, который работать не мог в принципе. А потом, как правило, оказывалось, что вокруг прикреплено куча костылей для того, что бы все работало нормально, не смотря на этот баг.
      Я к тому, что если и так все работает, то это не повод оставлять явную ошибку в коде.
      • +6
        Кстати, в следующей статье будет красивый пример сдвоенного бага. Баг — переменная может остаться неинициализированной. Но из-за второй ошибки (не там поставленной скобка в условии), она инициализируется всегда. Одна ошибка исчезает и код типа даже иногда работает. :)
        • +4
          Не в следующей, а в этой же. Третьим по счету вы его показали.
          • +1
            Ой. Действительно. Перепутал.
        • 0
          код типа даже иногда работает. :)

          В том примере, что вы описали, код работает всегда, а точнее программа работает правильно, только никто не говорит, что это правильно написанная программа.

          В хорошо оттестированном продукте статитеческий анализатор больше находит как не надо писать код, а не сами ошибки продукта.
    • +7
      Ну и что, что говно в коде. Все ж работает!
      • +1
        Да, тоже хотел сказать — не оспаривая выводы статьи, ради справедливости замечу, что Miranda NG, в отличие от предыдущей Миранды, не падала у меня пока ни разу за несколько месяцев, которые помню. При том, что запущена постоянно, с разными плагинами.
        • 0
          А вот у меня падала и не раз и внезапно по стабильности была хуже IM и это совсем недавно. Так что это у кого как.
          И это я вовсе не имею ввиду что IM хороша, а NG плоха, просто проблемы у NG тоже есть (хотя будущее определённо за NG, с этим даже никто не спорит).
    • +37
      Я смотрю, у Вас там единый стиль, что в написании кода, что в общении с оппонентами. Жаль, очень, правда. Большая удача, когда кто-то указывает тебе на твои ошибки, а не ждет с ухмылкой когда ты потонешь.
    • +8
      Как вы думаете если у нескольких миллионов пользователей за несколько лет в этом месте ничего не падало, то каковы шансы что этот указатель окажется NULL?

      Лучше перефразировать вопрос, каковы шансы что RequestStatus() будет вызван успешно? Абсолютно никаких. Такое отношение к разработке наводит на некоторые мысли.
    • +23
      Божтымой, святая простота-то какая. мимими.

      Т.е. вот прям так взять и при всём честном народе сказать "мы пишем одноразовый код, который никогда не будем рефакторить". Будем сидеть и бояться дышать на легаси код, потому что там живут огнедышащие тараканы. А когда достигнете критической массы костылей и заплаток — так, что любой чих в коде будет вызывать непредсказуемые (причем зачастую отложенные) последстия в совершенно не связанных с ним (казалось бы) областях, то… А, я знаю! Сделаете ещё один форк со словами "там всё плохо, а вот теперь-то мы всё сделаем как надо!", да?
      • –17
        :) вы забавны. Цитирую буквально свой статус в jabber в эпоху разработки: «Пошел нафиг дальше баги писать. ВирандаИМ — Больше, тормознутее, глюкавее.»

        А разработчики иначе как «стройной системой костылей и подпорок» ее и не называли"
        • +23
          У вас, товарищ, забавноуказатель инвертировался.

          Нормальной реакцией было бы "о! руки не доходили, а тут уже за нас сделали. спасибо, надо глянуть".
          Вместо этого — попытки огрызаться из разряда "самдурак", вялые оправдания "вы бы видели, что там РАНЬШЕ было!" и тому подобное.
          Все ж, вроде, взрослые люди. Понимают, что в опенсурсе может быть что угодно. Тем более, если этому опенсурсу столько лет и он столько раз испытывал боль перерождения. Чо оправдываться-то? Поблагодарить, взять на заметку и пользоваться, пока другие за вас работу делают. Удобно ведь, нет?

          А позиция "у всех работает" просто наивна. Довольно толстый проект без единого юнит-теста, живущий на соплях вида "уф, собралось!", даже без вменяемой обратной связи… Когда сообразительному пользователю надоедает, что миранда падает на каждый чих — он выясняет, какой плагин виноват и отключает/обновляет/заменяет его. Если же пользователь ленив или недостаточно технически подкован — он просто сносит миранду к чертям и пользуется чем-нибудь ещё. В обоих случаях вы НИЧЕГО об этом не узнаёте и дальше радостно празднуете "стабильные релизы". И только самые дотошные продираются через дебри «поддержки» на форуме, чтоб оставить багрепорт и заметить, что предыдущее сообщение было пару-тройку лет назад. Т.е. этот плагин вообще никто давно не тащит на себе.

          А кичиться багодельней — вообще детсад. "Нашла, дура, чем гордиться".
          • –6
            Я вот не понял, нафига было так резко наезжать на проект?
            Потому и реакция такая.

            В проекте 180+ плагинов, это значит что минимум 100 разных людей их написало в разное время за последние лет 10+.
            Так изначально исторически сложилось что весь доступный код плагинов попал в транк, его правили но везде по разному, некоторый код просто положили в «не поддерживается/сломано/никому не надо». Некоторые допилили до состояния собирается и не роняет миранду.
            Нет у проекта сил вылизывать весь код. НЕТ и не было.

            Потому в миранду добавили крешдампер, который делает лог по которому понятно где упало.
            Когда она падает, пользователь может прислать лог и вероятно это починят.
            Потому «у всех всё работает» а что не работает — то чинится, процесс отлажен.

            Всякие юнит тесты и прочие вещи — добавляйте, кто ж мешает. У нас нет сил/времени/желания/потребности.
            Вот как то так.

            Потому спасибо за анализ, будем фиксить по мере сил.
            И идите нахер со своими наездами на проект.
            • +5
              По сути, на проект никто не наезжал. Да, автор статьи мог бы и уделить хотя бы пару слов оговорке, что это доисторический проект с жутко разнородной толпой абсолютно никак не организованных контрибуторов — тогда хотя бы выглядело не так ультимативно.
              Однако, здесь уже (если пойти от моего комментария вверх и посмотреть, кто же начал эту ветку) речь идёт не о проекте в целом, а о позиции одного конкретнов взятого персонажа, который первым заговорил от лица разработчиков проекта, попутро неся чушь и поливая фекалиями всех вокруг.
              • 0
                И вдогонку: я думаю, этому персонажу уже итак довольно доходчиво объяснили степень его заблуждений относительно подхода к разработке.
                Я рад, если на проекте есть вменяемые люди, которые хотят и пытаются что-то сделать. Но реальность такова, что в коде — одна сплошная большая ягодичная мышца. Обижаться тут не на что- это просто факт.

                А вот автору добавить в статью пару слов про дремучую опенсурсность, я думаю, не сложно.
                • –1
                  За «персонажа» личное спасибо :)
                  И про подход к разработке у «персонажа» несколько отличается для свободного OpenSource, для коммерческого OpenSource, и для проприетарного кода.
                  И кстати «персонаж» уже 4 года никакого отношения (ну кроме моральной поддержки) ни к проекту Miranda IM, ни к проекту Miranda NG не имеет.
              • –4
                Тем что написал FYR в миранде пользуются как минимум тысячи людей, ваши мнения и минуса для него «очень важны».

                Давайте по пунктам — строкам.

                1. «Я добрался до проекта Miranda NG и проверил его с помощью анализатора кода PVS-Studio. К сожалению, с точки зрения работы с памятью и указателями это самый неаккуратный проект из виданных мной.»
                Много ли было претензий отдельно к ядру?
                Почему сгребли в кучу ядро, которое вылизывают, и плагины, многие из которых давно заброшены?

                2. «В начале используем указатель, потом проверяем
                Таких ошибок в Mirannda NG тьма тьмущая, как и в любом приложении.»
                11 штук.
                Все их я описал ниже, ничего фатального.
                Остальное из плагинов.

                «Обычно такой код работает, так как в функцию приходит ненулевой указатель. А вот если нулевой, то функции к этому не готовы.»
                А им часто и не нужно быть готовыми, и вот на конкретно этом примере это хорошо видно — никто не вызывает эту функцию передавая туда NULL.

                3. «В Miranda NG есть ещё 48 мест где так неправильно используется strncat(). Вот они: MirandaNG-645-1.txt.
                Кстати, эти места в коде можно рассматривать как потенциальные уязвимости.
                В оправдание программистов из Miranda NG, стоит отметить, что некоторые всё таки читали описание функции strncat().»
                Всего 3 из них в ядре, остальное в плагинах.
                Нет никаких «программистов из Miranda NG»!
                А лично я strncat() вообще не пользуюсь ни в каких видах.

                4. «Эротика с использованием new[] и delete

                Вот здесь ещё 20 таких мест: MirandaNG-611-1.txt.»
                И ни одного в ядре.
                Больше половины в одном плагине — MirOTR. Xfire тоже отличился.

                5. «Разврат с использованием new, malloc, delete и free
                Путаются способы выделения и освобождения памяти:

                Ещё 11 поз из Камасутры здесь: MirandaNG-611-2.txt.»
                В Xfire и MirandaG15, один в YAMN.
                0 в ядре.

                6. «Бессмысленные проверки

                Предлагаю разработчикам проверить следующие 83 аналогичных предупреждений анализатора: MirandaNG-668.txt.»
                0 в ядре. Больше половины приходится на 3 плагина.

                7. «Время жизни объектов

                Удивительно, как такие программы вообще работают. Ещё 9 мест, где обитают дикие указатели: MirandaNG-506-507.txt.»
                2 в ядре.
                Первый пример — код не корректен с точки зрения языка, но во время исполнения память на стёке скорее всего не освобождается, потому оно работает годами. Не хорошо, но не фатально, исправим.
                Притом что ваш анализатор спокойно относится к return("") хотя оно из той же оперы.

                8. «Мучения 64-битных указателей

                Вот ещё 20 мест, где портятся 64-битные указатели: MirandaNG-220.txt.»
                0 в ядре.

                9. «Неочищенные приватные данные

                Не затрутся приватные данные ещё в 6 местах: MirandaNG-597.txt.»
                0 в ядре.

                В итоге полили говном весь проект (п1 и п2) и всех разработчиков (п3 — сгребли в кучу) из за 5-6 плагинов, некоторые из которых полузаброшенные.
                Я бы попросил не обобщать впредь.
                Охота полить говном — тыкайте отдельных авторов отдельных плагинов/строк.
                • +5
                  никто не вызывает эту функцию передавая туда NULL.

                  Извините, но эта фраза не очень хорошо Вас характеризует
                • +1
                  Не буду подробно комментировать. Замечу только пару моментов. Я беру и проверяю проекты. Их много и вникать в каждый и разбираться, что надо проверять, а что нет, я не буду. Кому-то везёт, кому-то нет. :)
                  А по поводу return(""), предлагаю подумать и подучить Си++. Это корректный код.
                  • –13
                    Ну вот и не обижайтесь потом что в фб и прочих соцпомойках вам говорят: «Спасибо за проделанную работу, идите нахер со своим мнением о нашем проекте».
                  • НЛО прилетело и опубликовало эту надпись здесь
                    • +3
                      image
                      • НЛО прилетело и опубликовало эту надпись здесь
    • +6
      А не из-за проблем ли с кодом оригинальной Miranda IM появилась Miranda NG?

      Ваши же принципы, разве нет?
      Принципы проекта:
      Прекращена поддержка версий ANSI, как полностью морально устаревших. Будут только Unicode (x32 и x64-версии).
      Поддерживаются только компиляторы Visual Studio 2010, 2013.
      Подключение и отключение плагинов «на лету», без перезапуска программы.
      Все плагины находятся в одном репозитории, при изменениях в ядре необходимые правки вносятся сразу, без ожидания реакции разработчиков, которая может длиться месяцами у Miranda IM.
      Проведена достаточно большая работа по унификации — многое из того, что каждый плагин реализовывал самостоятельно, было перенесено в ядро.
      Избавление от старых «костылей» и багов, повышающее общую стабильность программы, повышена скорость запуска и работы программы.
      Возвращение в разработку Miranda NG разработчиков, покинувших или бросивших по тем или иным причинам разработку плагинов под Miranda IM.
      Расфиксировать на время API плагинов.[12] Это значит, что некоторое время разработка новых плагинов будет невозможна (будут работать только те, что лежат в репозитории и централизованно обновляются). Это даст чистое API, которое впоследствии можно будет и заморозить.


      В чем тогда смысл идти таким же путем?
      • –5
        Даже не знаю с чего начать, некто Георгий Хазан смог бы подробнее описать. Но основной мотив был политический — товарищи из Miranda IM ну откровенно достали. В итоге те разработчики модулей что были послабее духовно или позанятее — бросили код. Например я — просто пропал интерес к IM, к разработке интерфейсов, просто к OpenSource.
        С учетом того что основная масса разработчиков из наших включая фактически основного разработчика ядра и чистильщика авгиевых конюшен — Гоше удалось соорганизовать оставшихся и собрать код воедино и тащить его на себе.
        Лично я бы это не осилил. Вы не представляете сколько он откровенного… оттуда выгреб. Но сколько еще там осталось. Поэтому я с изрядным скепсисом отношусь к этой статье, ничуть не умаляя заслуг статистических анализаторов.
        • +12
          Правда практика показывает, что толку от вашего анализа чуть… после первого же фрагмента перестал читать дальше — так проглядывал
          ничуть не умаляя заслуг статистических анализаторов.
          • –5
            Не понял сарказма. Для меня разницы нет между PVS (за который надо платить деньги) и warning Visual Studio (за который деньги уже плачены) или warning gcc или clang (за которые платить не надо).

            Да это инструмент, который может решить какую то задачу. Беда в том, что инструмент ничего не гарантирует, в том что -W4 решит ее лишь немногим хуже. Зато инструмент гарантированно добавляет работы: надо же разобраться корректно ли его предупреждение в данном конкретном случае, не повлечет ли исправление изменений в логике работы ибо даже здесь в комментах были проблемы двойных ошибок. Технически анализвтор вторую мог не найти и код после исправления ошибки начал бы падать в другом месте, а так как ошибка довольно специфическая падал бы он еще «иногда» редко и при невыясненных сценариях.

            Лучше стоимость этого продукта и свои силы на его внедрение вложить в совершенствование процесса разработки (проектирование, ревью кода, юнит тестирование, ТДД, ), дополнительное обучение сотрудников (теже паттерны, стандартные библиотеки, ООП), найм более опытных сотрудников, лишний сервер под юнит тестирование.

            А присвоение в if мне и grep найдет.
            • +8
              Для меня разницы нет между PVS (за который надо платить деньги) и warning Visual Studio (за который деньги уже плачены) или warning gcc или clang (за которые платить не надо).

              Компиляторы от части уже реализуют статический анализ кода и выдают предупреждения. Именно warning, так-как если бы компилятор был уверен, что код ошибочен, он бы выдал error. И именно поэтому у предупреждений компиляторов есть уровни. Они задают порог вероятности, что компилятор выдаст не ложное сообщение.

              А разница в том, что статические анализаторы — это специализированные инструменты, а анализ в компиляторах в любом случае делается по остаточному принципу. Грубая аналогия — есть Paint встроенный в Windows и есть профессиональный Photoshop. Замечательно что есть Paint и для многих задач его достаточно. Но если вы задумали серьезное, то вам понадобится специализированный платный инструмент Photoshop.

              Да это инструмент, который может решить какую то задачу. Беда в том, что инструмент ничего не гарантирует, в том что -W4 решит ее лишь немногим хуже.

              Это неправда.

              Если бы было правдой, то у нас не было бы наших уважаемых клиентов. А тем более продлений лицензий.

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

              У Вас странный подход. Если нет желания исправлять старые ошибки, то анализатор конечно только добавит работы. Если стоит цель сделать код более безглючным, то статический анализ экономит массу времени и денег. Собственно, именно по этому он и нужен. Если подход «х*сь — х*сь и в продакшен», то тут конечно от анализатор один вред. Именно по этому, например, мы не стремимся что-то делать в сторону анализа мобильных приложений и т.п. Там срок жизни проекта несколько месяцев. Мы же ориентируемся на клиентов с большой кодовой базой, которые рады любому способу помогающему повысить контроль над своим кодом.

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

              А присвоение в if мне и grep найдет.

              И этот человек что-то говорит про ложные срабатывания. Ну-ну… :)
              Найти можно, толку то. Будет нереальное количество как ложных срабатываний и наоборот много будет не найдено. Кстати, на эту тему: "Статический анализ и регулярные выражения".
        • +9
          Вы молодцы, да. Но в чем проблема статьи? Вам сказали, что, дескать, было найдено много ошибок, вот они.

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

          В любом проекте есть ошибки, почему бы их не признать и исправить — вопрос.
          • –10
            Проблема статьи? Наверное в том, что это пиар коммерческого софта на популярном OpenSource проекте, и даже не на одном, а на многих, причем этим даже хвастаются.

            Лично я предпочитаю не находить ошибки, я предпочитаю их исправлять. Я больше ценю багрепорт юзера нашедшего ОДИН сценарий при котором чтото упадет. Чем такой вот отчет о том что мы нашли кучу косяков в коде из-за которого «МОЖЕТ» упасть, а может и не упасть, ибо код просто недостижим или вообще заброшен и лежит тут для истории.

            Вот если бы автор и компания вместо «Обновляемый список open-source проектов, которые мы проверили с помощью PVS-Studio» вела «Обновляемый список КОММИТОВ и PULL-REQUEST в open-source проекты, которые мы НАШЛИ с помощью PVS-Studio и ИСПРАВИЛИ» или «Обновляемый список open-source проектов в которых наша компания из супер-пупер продвинутых с++ разработчиков выполняет премодерацию патчей» тогда да, вот тогда это громко.

            А так — ну да какието проблемы в коде нашли, но «их много — разбираться не будем». Спасибо конечно, но нам то толку мало. Все равно что ходить по улице и орать «вот смотрите тут окурок валяется, и вообще грязно» и звать уборщицу. Окружающим от этого проку то немного будет и чище врядли станет.
            • +13
              Может им за вас ещё и весь мессенджер оставшийся дописать? Человек сделал платной (!) тулзой анализ кода и бесплатно (!) выложил результаты. Да, это пиар. Но пиар, которым вы можете воспользоваться, улучшив качество вашего кода. Не хотите, это уже другой вопрос. Но почему вы считаете, что после анализа, за вас ещё должны ваш код поправить и патч выкатить?

              А на то что вам кажется, что подобные ошибки, а это ошибки, мало влияют на суть — ну у нас же всё работает, так это от уж извините, от недальновидности. Каждая такая ошибка — бомба замедленного действия. Ну да, у вас пока что нет ситуации, когда в функцию приходит NULL, ну да у вас адреса 64х-битных указателей не вылазят за младшие адреса. Это всё меняется, это всё проявляет себя. Придёт новый человек и допустит ошибку в вызове, направив NULL. И тогда такая бомба проявит себя. Такие ошибки могут стоить даже дороже потому что на их отладку можно потратить не один рабочий день, когда они закопаны глубоко в легаси-коде.

              Я за свой опыт насмотрелся уже проектов, которые просто тонули в проблемах своего легаси. Почему вы не хотите обеспечить себе счастливое будущее проекта, просто поправив ошибки на которые вам уже бесплатно указали, да при этом ещё напрасно хаете человека который для вас это, опять же повторяюсь, бесплатно сделал — решительно непонятно.
            • +9
              Это блог PVS-Studio, они за него деньги платят, это общеизвестный факт. Что хотят, то и пишут в него.

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

              Вы что-нибудь, например, о undefined behavior слышали? Это когда 100 раз работает нормально, а один раз падает. И все потому, что программист не знает языка, на котором пишет.

              PVS-Studio проверили дикое количество проектов (от boost и Qt до PostgreSQL и Source), во всех случаях разрабы были благодарны за проделанную работу. Но Вы прям уникальные парни, я смотрю.
            • +8
              >> Лично я предпочитаю не находить ошибки, я предпочитаю их исправлять. Я больше ценю багрепорт юзера нашедшего ОДИН сценарий при котором чтото упадет. Чем такой вот отчет о том что мы нашли кучу косяков в коде из-за которого «МОЖЕТ» упасть, а может и не упасть, ибо код просто недостижим или вообще заброшен и лежит тут для истории.

              Facepalm. И эти люди пишут сетевые клиенты. Которые получают и обрабатывают данные хз откуда. И как они их обрабатывают — мы видим на примере данной статьи.

              Вам фраза «удаленное выполнение кода» о чем-нибудь говорит?

              Ппц. Судя по продемонстрированному коду, а еще в большей степени — по реакции на него, Миранду использовать нельзя ни в коем случае, потому что вероятность того, что через неё вас удаленно отэксплойтят, резво стремится к единице.
    • 0
      Если магия работает, то это случайность, а если не работает то очевидное поведение. Имено так надо относится к магии.
    • +4
      >> Как вы думаете если у нескольких миллионов пользователей за несколько лет в этом месте ничего не падало, то каковы шансы что этот указатель окажется NULL?

      Эти шансы вполне могут оказаться стопроцентными, когда вы, например, обновитесь на новую версию компилятора, которая решит креативно соптимизировать данный кусок кода, поскольку там все равно U.B. gcc этим особо славен, но MSVC тоже иногда может.
  • +15
    I'm afraid this is the worst project in regard to memory and pointers handling issues I've ever seen.


    Догадайтесь, на что они обиделись.
    • +4
      Смысл на зеркало пенять? Ну, неприятно, ткнули носом в твои ошибки. Неприятно, да, бывает. Разве это плохо? Станешь чуточку умнее и аккуратнее.
      • +4
        Я-то прекрасно понимаю, но не все. Плюс в англоязычной среде этикет немного другой, эта цитата резче, чем на русском звучит.
        • +6
          Плюс в англоязычной среде этикет немного другой

          /me вспоминает пальцы Линуса, его опусы про gcc-4.9 и многое другое.

          А не путаете ли вы часом англоязычныую опенсорсную среду с платной поддержкой какого-нибудь RedHat'а, где специально обученные люди ни в коем случае не будут называть недалекого клиента так, как он того заслуживает?
          • +7
            В книге «Русские проблемы в английской речи» Линн Виссон есть глава «Я прав, а ты нет», в которой рассматривается проблема выражения резкой и категоричной критики, когда вроде бы по-русски нормально звучит (хоть и неприятно), а в английской – просто ужасно. Думаю, marapper имел в виду этот момент.
            • 0
              Огромное спасибо за книжку, выглядит интересно.
          • 0
            Нет, это действительно так к сожалению, прямой перевод русского на английский почти всегда звучит почти оскорбительно. Проблема давняя и известная.
  • +10
    Как вы думаете если у нескольких миллионов пользователей за несколько лет в этом месте ничего не падало, то каковы шансы что этот указатель окажется NULL?
    Хорошая логика. Важна не вероятность, а то, что это может случиться. Если подушка безопасности не помогает в большинстве аварий, то зачем нужна подушка безопасности… хм?
    А откуда Вы знаете, что у пользователей не падает в этом месте? У вас посылаются coredump-ы после каждого падения? Вроде для программ вроде Miranda это не типично.
    • +3
      А откуда Вы знаете, что у пользователей не падает в этом месте? У вас посылаются coredump-ы после каждого падения? Вроде для программ вроде Miranda это не типично.

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

      Вот пример как в проекте FlylinkDC++ побороли неотлавиваемые и редкие проблемы путём прикручивания стороннего сервиса анализа дампов, это не реклама, это просто часть моего личного опыта.

      P.S. и да, я уже не в первый раз выражаю искреннюю благодарность команде PVS-Studio! Благодаря анализатору в FlylinkDC++ удалось найти и исправить кучу мест, в которые уже давно не заглядывали или просто не видели в них ошибок. Однако в этих потаённых местах было много легаси кода, собственно до сих пор всё ещё очень много кода из 90-х, хоть постепенно проект и переписывается на C++11.
  • +23
    1. С удовольствием поправлю все ошибки в mra, sms и может ещё чем что я кодил или лазал.

    2. Много ошибок в плагинах которые фактически заброшены.
    Есть говнокод. Есть вообще плагины на которые забили и они лежат в нерабочем состоянии. Просто не очень нужны и силы на них не тратят.

    3. То что весь проект собран в одном месте и собирается — уже огромная победа Георгия, потому что до того оно было кусочками, часто без исходников.

    Других приличных месенгеров под винду на этой планете не существует.
    Если что то мешает — жалуйтесь, поправят, или сами правьте.

    Я именно так и сделал: пользуюсь с января 2003 года, а в 2005 году мне не хватало мейловского агента, я и написал плагин.
    Потом переписал смс плагин, когда в мылагенте смс стало оно доступно.
    «сборка» — собственная.
    Проблемы иногда бывали, но ничего досаждающего.

    Если что месенгер пережил 9х винду, переезд на юникод, теперь и х64.
    И всё это на чистом энтузиазме.
  • –12
    И ещё раз, многие найденные варнинги и ошибки никогда не проявлялись по простой причине — в других функциях всегда передаются правильные параметры.

    Вот пример с BB_InitDlgButtons() — можно вообще dat не проверять, вызывающие её функции всегда передают dat != NULL.
    Это нормально.
    В некоторых случаях оптимизирующий компилятор вообще выкидывает такие проверки нафиг ибо знает что переменная никогда не может быть NULL.
    Но анализатор их помечает, это правильно но лучше бы он ещё отдельно помечал случаи когда туда реально может прилететь NULL из вызывающего кода а не просто всегда.
    Те пример с BB_InitDlgButtons() — откровенно слабый, вот именно в неё NULL не прилетает в принципе.
    • +12
      Если есть ограничение на параметры — его надо описать явно. Например, конструкцией assert, которая именно для того и нужна.
    • +15
      А контрактное программирование? А защитное программирование? Слышали такое?
      То, что сегодня никто не вызывает с dat == nullptr, не значит, что завтра в ваш опенсорс проект не придет некий Вася из 9-го класса, и не вызовет эту функцию с dat == nullptr.
      • –5
        Хотите верьте, хотите нет, но 14!!! лет назад мне было 19 лет и я не только не слышал ни про контрактное ни про защитное программирование, но я и не знал что такое Си а что такое Си++. Что такое объекты, автоуказатели, стандартная библиотека. С трудом понимал разницу между SendMessage и PostMessage. Суррогатный аналог виртуальных таблиц для меня был просто откровением и изящнейшим архитектурным решением.. И с удивлением узнавал что не стоит одновременно менять значения переменных из разных потоков.
        Я расстраивался что «Банда четырых» уже изобрела фабрику и другие «мои» изящные решения. :)

        Я как раз и был тем «Васей» правда со второго курса который эту функцию вызывал и не с nullptr, а с '1' :)

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

        Немного грустно от того, что кто-то даже на таком «плохом и ужасном» коде в который я хоть и не много, но вложил своих сил — сделает деньги и не малые. Но это очередной пример того, как даже столь ужасный код можно монетизировать.

        На месте автора статьи (конечно бы если бы я преследовал иную цель) этот лог бы я направил прежде всего разработчикам. А уж потом бы пиарился. Даже в этой статье есть отсылки к следующим статьям: «ой а там ваще ужас ужас». А так статья воспринимается как просто наглый пиар на по настоящему открытому и бесплатному софту.

        Вот разработчикам Миранды было не слабо выложить в открытый доступ реверс закрытых протоколов, свою реализацию XMPP. Поделиться этой реализацией с QIP (согласиться на нарушение GPL). Разработчикам CLANG тоже не слабо поделиться своими разработками. Вот ни грамма бы не обиделся бы на критику миранды от них или от Беркуса или Момджана. Но тут извините.
    • +1
      Давайте поразбираемся.

      1.
      V595 The 'gce' pointer was utilized before it was verified against nullptr. Check lines: 519, 522. Miranda chat_svc.cpp 519

      Открываем chat_svc.cpp 519

      if ((gce->pDest->iType == GC_EVENT_ADDSTATUS || gce->pDest->iType == GC_EVENT_REMOVESTATUS) && !(gce->dwFlags & GCEF_ADDTOLOG))
      return 0;

      if (gce && gce->pDest->iType == GC_EVENT_JOIN && gce->time == 0)
      return 0;

      видим что gce проверяется ниже, но в самом начале фунции есть:
      if (gce == NULL)
      return GC_EVENT_ERROR;

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

      2. V595 The 'group->cl.items' pointer was utilized before it was verified against nullptr. Check lines: 137, 139. Miranda clcitems.cpp 137
      круто, но в проекте аж три таких файла, теперь мне нужно во все заглядывать и искать.
      Тут же:
      mir_free(group->cl.items[i]);
      поздравляю, вы потратили моё время впустую, mir_free — сама проверяет.

      3. V595 The 'text' pointer was utilized before it was verified against nullptr. Check lines: 357, 372. Miranda clcutils.cpp 357
      int testlen = lstrlen(text); — оно само проверяет на NULL.
      Опять потратили моё время.

      4. V595 The 'c1->proto' pointer was utilized before it was verified against nullptr. Check lines: 107, 108. Miranda contact.cpp 107
      contact.cpp — 6 файлов, какой смотреть?
      rc = lstrcmpA(c1->proto, c2->proto);
      if (rc != 0 && (c1->proto != NULL && c2->proto != NULL))
      return rc;
      голову на отсечение не дам что lstrcmpA нормально жуёт NULL в аргументах, но 60% что именно так.
      Ок, фиксим.

      5. V595 The 'iod->name' pointer was utilized before it was verified against nullptr. Check lines: 110, 118. Miranda genmenuopt.cpp 110
      if (lstrcmp(iod->name, iod->defname) != 0)
      опять.
      Ок.
      Но раз уж такой придирчивый то почему он пропустил:
      MenuItemOptData *iod = (MenuItemOptData*)tvi.lParam;
      if (iod->pimi) {

      6. V595 The 'opi.odp' pointer was utilized before it was verified against nullptr. Check lines: 894, 897. Miranda profilemanager.cpp 894
      mir_free((char*)opi.odp[i].pszTemplate);
      mir_free(opi.odp);
      мимо.

      7. V595 The 'szProto' pointer was utilized before it was verified against nullptr. Check lines: 964, 966. Miranda findadd.cpp 964
      TCHAR *szProto = (TCHAR*)alloca(sizeof(TCHAR)*(len + 1));
      *szProto = '\0';
      SendDlgItemMessage(hwndDlg, IDC_PROTOLIST, CB_GETLBTEXT, SendDlgItemMessage(hwndDlg, IDC_PROTOLIST, CB_GETCURSEL, 0, 0), (LPARAM)szProto);
      db_set_ts(NULL, «FindAdd», «LastSearched», szProto? szProto: _T(""));
      Во втором случае мимо, но там и условие лишнее.
      фиксед.

      8. V595 The 'nloc' pointer was utilized before it was verified against nullptr. Check lines: 534, 547. Miranda netlibopenconn.cpp 534
      А почему анализатор не выругался на ещё кучу строк, и наиболее очевидное:
      unsigned int dwTimeout = (nloc->cbSize == sizeof(NETLIBOPENCONNECTION) && nloc->flags & NLOCF_V2)? nloc->timeout: 0;
      в самом начале функции?
      фиксед.

      9. V595 The 'item2' pointer was utilized before it was verified against nullptr. Check lines: 252, 257. Miranda hotkey_opts.cpp 252

      10. V595 The 'p.szProtoname' pointer was utilized before it was verified against nullptr. Check lines: 77, 81. AVS options.cpp 77
      Логика кода исключала ошибку: проверка происходила немного раньше в одной из функций куда параметр передавался аргументом.
      Поменял логику для явного выполнения проверки.

      11.
      V595 The 'pEntry' pointer was utilized before it was verified against nullptr. Check lines: 1920, 1924. Miranda xmlparser.cpp 1920
      V595 The 'outlen' pointer was utilized before it was verified against nullptr. Check lines: 3031, 3033. Miranda xmlparser.cpp 3031
      да, пожалуй во втором случае однозначно да.

      12. V595 The 'tag_value' pointer was utilized before it was verified against nullptr. Check lines: 964, 969. AdvaImg pluginjpeg.cpp 964
      да, исправлено.

      остальное позже досмотрю / пофикшу.
      • 0
        > круто, но в проекте аж три таких файла, теперь мне нужно во все заглядывать и искать.

        Не нужно никуда заглядывать и ничего искать. Нужно установить и запустить анализатор CppCat. Демонстрационная версия работать неделю с полной функциональность. Этого более чем достаточно для проверки проекта, по крайне мере однократной.

        А потом можно и купить CppCat-ов на всю команду. :)
        • +8
          видим что gce проверяется ниже, но в самом начале фунции есть:
          if (gce == NULL)
          return GC_EVENT_ERROR;

          И точно, есть. В строке 401 trac.miranda-ng.org/browser/trunk/src/modules/chat/chat_svc.cpp
          Так что кто кому баг нашёл, ещё вопрос :)
        • 0
          Это хобби, накой мне покупать его, если он такие странные отчёты генерит?
          У меня и вижал студия экспресс, оно же не взлетит с ней.

          Лучше дайте нормальные линки на ваши результаты по миранде, а то в публикации как то отрывочно.
          • +3
            У меня и вижал студия экспресс, оно же не взлетит с ней.
            Не касаемо CppCat, а вообще, уже есть Visual Studio Community 2013. Она умеет если не всё, то почти всё что и профессиональная (плагины уж точно).
      • +4
        V595 The 'group->cl.items' pointer was utilized before it was verified against nullptr. Check lines: 137, 139. Miranda clcitems.cpp 137
        круто, но в проекте аж три таких файла, теперь мне нужно во все заглядывать и искать.
        Тут же:
        mir_free(group->cl.items[i]);
        поздравляю, вы потратили моё время впустую, mir_free — сама проверяет.
        И как она это проверяет, интересно? Значение group->cl.items[i], может быть, она и проверяет — а вот group->cl.items она не может проверить в принципе.

        6. V595 The 'opi.odp' pointer was utilized before it was verified against nullptr. Check lines: 894, 897. Miranda profilemanager.cpp 894
        mir_free((char*)opi.odp[i].pszTemplate);
        mir_free(opi.odp);
        мимо.
        То же самое.

        • –1
          1.
          void fnFreeGroup(ClcGroup *group)
          {
          int i;
          for (i=0; i < group->cl.count; i++) {
          cli.pfnFreeContact(group->cl.items[i]);
          mir_free(group->cl.items[i]);
          }
          if (group->cl.items)
          mir_free(group->cl.items);
          group->cl.limit = group->cl.count = 0;
          group->cl.items = NULL;
          }

          cli.pfnFreeContact(group->cl.items[i]);
          mir_free(group->cl.items[i]);
          чем вторая строка хуже первой?
          Внутренняя структутар подразумевает что group->cl.count != 0 и тогда group->cl.items тоже != NULL.
          Формально придраться тут можно, но почему оно не обращает внимания на то, что group не проверяется совсем?
          Другой момент что проверка перед mir_free(group->cl.items); вообще не нужна, и тогда с точки зрения анализатора всё чисто.

          Вся проблема в том, что оно очень примитивно парсит: смотрит проверку на NULL а потом выше ищет использование этой переменной.
          Вот если бы оно ещё искало хотя бы более раннюю проверку проверку и отсутствие изменения переменной.

          Ок, пусть будет
          void fnFreeGroup(ClcGroup *group)
          {
          if (!group)
          return;
          if (group->cl.items) {
          for (int i=0; i < group->cl.count; i++) {
          cli.pfnFreeContact(group->cl.items[i]);
          mir_free(group->cl.items[i]);
          }
          mir_free(group->cl.items);
          group->cl.items = NULL;
          }
          group->cl.limit = group->cl.count = 0;
          }

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

          2. Чуть выше в коде:
          DetailsPageInit opi = { 0 };
          Те opi никак не NULL.
          opi.pageCount показывает сколько opi.odp[i] доступно.
          mir_free(opi.odp); — само проверяет на NULL, и по сути если бы не лишняя проверка перед ней, то анализатор не выдал бы ничего.
          Лучше бы он придрался что скобок фигурных нет:
          if (rc != -1)
          for (int i = 0; i < opi.pageCount; i++) {
          • +4
            Диагностика V595 очень проста и как не странно, этим же и удачна. Она работает следующим образом:

            Выдать предупреждение, если в начале указатель разыменовывается, а затем проверяется на равенство 0. И всё. Есть некоторые отдельные исключения, но они к делу не относятся.

            Диагностика не всегда указывает на ненастоящую ошибку. Иногда, указатель никогда не равен нулю. Но тогда вопрос, зачем нужна такая проверка? Чтобы запутать читающего код? Таким образом, анализатор если выявляет не ошибку, то по крайней мере код, который пахнет. Часто его можно упростить или улучшить. Например, если функция всегда принимает только ненулевой указатель, то возможно его стоит заменить на ссылку.
          • 0
            2. Оно ругается на вот это: opi.odp[i]
            • 0
              Насколько я понял, там примерно следующая ситуация: есть переменная size — размер массива, и есть ptr — указатель на первый элемент массива (названия условные). Если size == 0, то ptr — нулевой указатель, если size > 0, то ptr указывает на первый элемент массива длины size. Сначала они бегут по элементам массива от 0 до size – 1 (если указатель равен 0, то и size равен 0, а следовательно тело цикла не выполняется ни разу), а потом вместо проверки if (size > 0) используют совершенно равнозначную ей (при соблюдении вышеописанного инварианта) проверку if (ptr). В принципе ничего такого криминального.
              • +2
                Разумеется, ничего криминального. Только вот если писать один и тот же код в 100 разных местах — то пару раз напишется что-то другое. Кто проверит, что работа с этими структурами всегда выполняется корректно?

                PS Так и хочется написать что-то вроде assert((size == 0) == (ptr == nullptr))
                PPS А еще сильнее хочется переписать, используя векторы и умные указатели
                • 0
                  PS Так и хочется написать что-то вроде assert((size == 0) == (ptr == nullptr))

                  Сомневаюсь, что PVS-студия достаточно умная, чтобы понять такой намёк.
                  • +2
                    Зато программист, увидев такой код (ассерт), сразу пометит строку с диагностированной проблемой как ложное срабатывание и, не вглядываясь в остальной код, пойдёт дальше.
          • 0
            V595 The 'wlid' pointer was utilized before it was verified against nullptr. Check lines: 357, 363. MSN msn_threads.cpp 357
            V595 The 'wlid' pointer was utilized before it was verified against nullptr. Check lines: 417, 422. MSN msn_threads.cpp 417
            V595 The 'proto' pointer was utilized before it was verified against nullptr. Check lines: 607, 617. MSN msn_threads.cpp 607

            вообще не могу найти: trac.miranda-ng.org/log/trunk/protocols/MSN/src/msn_threads.cpp
            в какой ревизии?
            • –4
              В одной из свежих. Не вижу смысла в таком подходе. Нужно установить анализатор и воспользоваться им.
          • +2
            Замечательный образец ложного срабатывания:
            V595 The 'domain_parms[idx].desc' pointer was utilized before it was verified against nullptr. Check lines: 467, 478. Libgcrypt ecc.c 467

            for (idx = 0; domain_parms[idx].desc; idx++)
            if (!strcmp (curve_aliases[aliasno].name,
            domain_parms[idx].desc))
            break;

            if (!domain_parms[idx].desc)
            return GPG_ERR_INV_VALUE;

            V595 The 'hashnames[i].name' pointer was utilized before it was verified against nullptr. Check lines: 1477, 1480. Libgcrypt pubkey.c 1477
            for (i=0; hashnames[i].name; i++)
            {
            if ( strlen (hashnames[i].name) == n
            && !memcmp (hashnames[i].name, s, n))
            break;
            }
            if (hashnames[i].name)
            algo = hashnames[i].algo;

            кто там чем попахивает?)
            • 0
              Методология статического анализа подразумевает некоторый процент ложных срабатываний. С ними можно и нужно бороться. И разработчики статических анализаторов ведут эту невидимую борьбу. Тем не менее статический анализ, это не искусственный интеллект и задача инструмента выделить места кода для последующего изучения программистом.
              • 0
                По опыту использования PVS-Studio в 2009-2010 году могу сказать, что для того, чтобы найти одну реальную ошибку нужно было пролопатить сотни, если не тысячу предупреждений ни о чём. Это же кстати относится и к статическому анализатору VS2013 и Coverity 6. Вам об этом разработчики и пытаются сказать, возможно слишком резким тоном. Вы им: «да ваш год отстой, 80% указателей не проверяются перед использованием», а они вам «да ваш статический анализатор отстой, у вас 80% предупреждений ни о чём». Чтобы зря друг на друга не наезжать и не переливать воду в ступе, разработчикам Miranda NG нужно было бы написать статью «PVS-Studio получает приз «чемпион по ложным срабатываниям»», но вам за статьи деньги платят, а Miranda NG это чистый энтузиазм, поэтому такая статья врядли появится.

                Единственный вариант использования статических анализаторов, который я вижу — это подключить их к CI и в обязательном порядке просматривать новые предупреждения. Их будет не очень много и с ними можно будет справиться. Починить же тысячи предупреждений (большинство из которых false positive) в проекте с многолетней историей по-моему не представляется возможным.
                • +4
                  Есть смысл посмотреть на актуальную версию PVS-Studio и не смотреть на 64-битные диагностики. Результат будет намного лучше.
                  • 0
                    Скачал свежий триал. Поставил. PVS-Studio->Check Current File.

                    В окошке где должны быть результаты
                    License information is incorrect. Please check your registration data or contact Customer Support at support@viva64.com 1

                    И красным по черному окну Clicks remaining: 18 (can be extended). Это я видимо уже дважды кликнул по надписи о некорректной лицензии.
                    • 0
                      Отпишите, пожалуйста, в почту — подскажем что не так.
                      • +1
                        А, разобрался, у меня была в настройках вбита лицензия ещё с 2009 года. Вытер и лицензия стала Trial.

                        Проверил несколько файлов, отключил 64-битные предупреждения, оставил только GA:
                        Туча предупреждений на код в файлах c:\Windows Kits\Include\shared\minwindef.h, c:\Windows Kits\Include\um\winnt.h, winuser.h, shellapi.h и пр.
                        V677 Custom declaration of a standard 'SHORT' type. The system header file should be used: #include <WinNT.h>. winnt.h 344
                        Исключил весь C:\Windows Kits, не ясно только, почему он не исключен по умолчанию.

                        V668 There is no sense in testing the 'task' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error.
                        В классе определен operator new, который возвращает NULL при неудаче.

                        V668 There is no sense in testing the 'newPointee' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error.
                        В шаблонной функции вызывается newPointee = new T();, где T — параметр шаблона. Откуда уверенность, что в T не перегружен operator new?

                        V512 A call of the 'memcmp' function will lead to underflow of the buffer '& OpPattern'.
                        V512 A call of the 'memcmp' function will lead to underflow of the buffer '& maxSupportedOpPattern'.
                        Анализатор решил, что программист должен сравнивать структуры полностью, а в коде сравниваются только первые 12 байт «префикса».

                        V571 Recurring check. The 'if (((HRESULT)(hr)) >= 0)' condition was already verified in line 2124.
                        Ну как бы да, лишняя вложенная проверка. Но это никак не влияет на работоспособность кода.

                        V580 An odd explicit type casting: (void * *) & pInterface. Consider verifying it.
                        Типичный каст для COM. Проверифаить все вызовы QueryInterface не представляется возможным.

                        Ну что я о плохом да о плохом:
                        Нашел одну ошибку типа
                        V519 The variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 7195, 7196.
                        Другое дело, что это скорее всего где-то дальше в коде обрулено, но всё равно надо починить.

                        V690 The 'Class' class implements the '=' operator, but lacks a copy constructor. It is dangerous to use such a class.
                        Эти предупреждения тоже стоит изучить детальнее.

                        Но в общем и целом мало что изменилось — реальных ошибок очень мало и их очень тяжело выискивать среди десятков и сотен приведеных выше false positiveов. В общем, как я уже писал выше — добавлять в CI и чинить/глушить новые предупреждения. Времени на починку 1000 подобных предупреждений, из которых менее 1% валидны никто не даст.
                    • –1
                      И красным по черному окну Clicks remaining: 18 (can be extended). Это я видимо уже дважды кликнул по надписи о некорректной лицензии.
                      Не ошибка ли, что засчитываются щелчки по сообщениям, не приводящие к переходу в код?
      • +1
        Но раз уж такой придирчивый то почему он пропустил:
        MenuItemOptData *iod = (MenuItemOptData*)tvi.lParam;
        if (iod->pimi) {

        Ну а что тут не так? iod после этого на nullptr не проверяется — такой ворнинг и не генерируется.

        Позвольте еще и за имена переменных поругать: что за iod, pimi, pimo?
        Почему часть локальных переменных в camelCase, а часть в PascalCase?
        • 0
          Потому что если iod = NULL то получим исключение за чтение откуда нельзя.
          Те если уж делать совсем код с защитой от дурака который его вызывает то нужно кучу проверок добавлять.
          В этом смысле варнинги на использование и чуть позже проверку не всегда адекватны, я это уже показал на примерах выше.

          Ругайте :)
          Потому что пишет много людей, единого стиля кода не предусмотрено. Как минимум для плагинов и всего что не входит в ядро. С ядром строже.
          В этом плане проект очень демократичный, если ты смог написать плагин то его включат в общее дерево.
          У меня у самого много что поменялось в стиле уже после того mra был написан и заброшен (работает, вот и нет повода трогать).
          Я стараюсь не сильно менять стиль чужого кода когда делаю там правки — я гость и скорее всего этой одной-двумя правками и ограничусь, а автору потом ещё тянуть это всё.
          • +6
            Так PVS не ругается, если указатель просто не проверен на nullptr перед использованием. Может вы и вправду уверены, что он никогда не равен nullptr.
            Но согласитесь, подозрительно, что указатель сначала использовали, а потом проверили. Т.е. вроде как программист не уверен, что указатель не null, раз проверка есть, но при этом он его до этого использует без проверок.
            • –1
              В ряде случаев в коде была просто лишняя проверка.
              В некоторых был запутанный код.
              В некоторых проверка вроде как была нужна но код не падал потому что NULL никогда не приходил и придти не мог. В некоторых мог но не приходил. Ещё были места где проверка есть, но после использования и она вроде как нужна, но оно опять же не падало.

              Соответственно некоторый код оставил как есть, местами удалил лишние проверки, местами распутал, местами добавил/переместил проверки.

              Есть и откровенно злачные месте, типа QuickMessages и видимо ещё дальше.
              • 0
                kosmos89 прав, прислушайтесь к нему, такой код надо переписывать. Если остаются сомнения переставляйте проверку выше, заменив её ассертом. Если же там всегда валидный указатель, то убирайте странную проверку постфактум. Убрать проверку необходимо поскольку она может скрывать за собой проблему, к примеру, уже не работающую логику, зависящую от этой проверки. Просто задумайтесь вот над чем: код имеет очень долгосрочную поддержку, уже много раз переписывался. Теперь представьте ситуацию: раньше там было значение его проверяли на неравенство нулю, а потом провели оптимизацию потребления памяти или изменили логику работы с объектом и заменили значение на указатель, а логику исправить забыли. В общем, это очень серьёзная проблема, такого «подозрительного» кода просто не должно быть в проекте поскольку он затуманивает восприятие и не позволяет видеть похожие проблемы в других местах как во время рефакторинга, так и в процессе написания нового кода.
      • +4
        Если вы хотите что-то максимально полезное вынести — начните не с нулевых указателей, а с некорректного использования sizeof/SIZEOF, и прочих багов с подсчетом кол-ва элементов.
  • +6
    сейчас, по прошествии ~10 лет могу только себе представить, что бы выдал анализатор на history++ :)
    но, дельфи оно не умеет.
    • 0
      Какая разница, если в итоге он отлично работал. Спасибо Вам за этот плагин :)
  • +21
    Занятно. В обсуждении поста на Reddit появился /u/FigBug, который начал проект Miranda IM. Никакой желчи, никаких упрёков и обид в сторону анализатора. Признался, что качество кода было не очень:
    Скрытый текст
    I must apologize for Miranda, even though there is probably none of my code left, I set the tone and style for the project.

    I had no idea what I was doing when I started the project and I never thought it would get as popular as it did and be around for 14+ years.

    I had less than 2 months Windows programming experience when I started the project. I used C and not C++ because I couldn't figure out how to get a C++ WinProc to compile. The entire point of the project was to hook a chat bot to ICQ to harass my friends. There was no 'design' or 'architecture'. I had no idea how to run an open source project so I just let anybody commit whatever they wanted and then I got bored of it and let the project run on auto pilot until others took it over.

    Sad thing is, the worst code I've ever written is by far the most popular thing I have ever created.
    • +10
      Вот это я понимаю. Очень жизненно. «Слова не отрока, но мужа». Насчет самого ужасного творения, которое стало самым популярным, я его очень хорошо понимаю. Хотя даже самым ужасным моим творениям очень далеко до популярности Миранды )
  • +9
    Ого! Попкорн при прочтении комментариев очень пригодился, давно такого не было на хабре, спасибо!
  • 0
    О, Андрей, давно хочу написать, да всё руки не доходят. А можете проект, в котором я участвую, проверить? Надеюсь, и вам, и мне, и всем интересно будет. Там вам:
    • и C++11 (а местами уже и 14, так что те модули только clang'ом, только clang'ом);
    • и неинтересные результаты на прочих анализаторах: clang тот же нашёл за всё время, наверное, с пяток действительно важных багов и ещё несколько запашков, а так очень много false positives, а Coverity вообще дохнет и не может проанализировать практически ни один файл;
    • и практически полное отсутствие прямого дёрганья системного API, как следствие, однотипных ошибок вокруг memset/memcpy/str* должно быть мало;
    • и восьмилетняя история проекта, со всеми вытекающими, мне вот было бы банально интересно взглянуть на корреляцию количества ошибок с возрастом кода.


    Отсутствие адекватной документированной процедуры сборки под Windows в комплекте, msvs не поддерживается от слова вообще, бонусом также идёт овер 9000 внешних библиотек для полноценного билда.

    Это leechcraft, если что. В общем, если что, могу попробовать попросить человека, которому удаётся собирать вполне работоспособные билды под вин, набросать какое-нибудь руководство по установке.
    • +3
      Не обещаю, но можно попробовать. Напишите нам на почту.
      • 0
        Ага, ушло письмо.
  • +1
    Да давненько не было…

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

    А вот модули разрабатывали кто во что горазд мой еще тесно связан с ядром — приходилось подстраиваться. А многие так вообще. И жили они кто где, на разных репозитариях, процентов 70 откровенно заброшенных, но тем не менее популярных. И то что их разработчики просто не хотели больше поддерживать свой модуль откровенно мешало разработке ядра.
    К примеру 30% пользователей используют модуль clist_modern, а с новым релизом ядра ломается совместимость и модуль перестает работать. Причем модуль просто по незнанию использовал какой то глюк в интерфейсе взаимодействии и его хотели починить. Делов то поправить пару строк… Но я этим заниматься не хочу — потерял интерес. И что делать разработчикам ядра?
    Вот и старались стащить все в одно место, чтобы хоть как то контролировать и приглядывать за разработкой хотябы популярных модулей.
  • –2
    И да если кому-то интересно мнение о данной статье живого мейнтейнера проекта то пожалуйста:
    forum.miranda-ng.org/index.php?topic=3731.msg9258#msg9258 и да я с ним абсолютно солидарен по всем пунктам.
    • +7
      Я не могу воспринимать того человека как программиста после вот этого высказывания:
      программа, которая может выдавать сообщения типа
      > V303 The function 'lstrlen' is deprecated in the Win64 system. It is safer to use the 'strlen' function.
      вообще не заслуживает ни доверия, ни того, чтобы на нее даже толком смотреть.
      • –3
        А как вас воспринимать?

        Вот MSDN: msdn.microsoft.com/ru-RU/library/windows/desktop/ms647492(v=vs.85).aspx
        никакого деприкейтед там и в помине нет!
        Есть рекомендация использовать StringCbLength или StringCchLength.

        Рекомендация PVS вообще неадекватна: strlen() — упадёт если передать NULL, а lstrlen() нет (начиная с 2к, насколько помню).
        По факту получается не то что вредный а вредительский совет.
        • +3
          Зато там указан возвращаемый тип, который int. Следовательно, эта функция не работает со строками длиннее 2 Гб.

          Если вы мне возразите, что в Миранде не бывает таких длинных строк — я соглашусь. Но нельзя говорить про все программы на примере одной только Миранды. Может, кто-то перешел на 64 бита исключительно из-за длинных строк? Тогда эта диагностика окажется для него очень даже полезна. А значит, в синтаксическом анализаторе, выросшем из проекта Viva64 — ей самое место.

          Если же именно вам именно в Миранде больше нравится использовать lstrlen — то эту диагностику можно выключить.
          • 0
            Вам возразит MSDN: msdn.microsoft.com/ru-RU/library/windows/desktop/ms647539
            StringCchLength is a replacement for the following functions: strlen, wcslen, _tcslen

            Те рекомендация PVS всё равно дуратская: если ей последовать то нормальный рабочий код может легко стать не рабочим, как раз из за отсутствующей проверки на NULL в strlen.
            Лучше бы оно рекомендовало сразу нормальную замену.

            От себя замечу:
            — искать конец строки по нулевому символу для 2гб строк это полный абзац!
            — при переходе на х64 нужно переходить на size_t, на высоких варнингах вроде даже msvc ругается на такие вещи, но если не ругается то стоит поискать чеклист перехода на х64, там явно должно о таком упоминаться.
        • +2
          Выбрали неудачную функцию для обсуждения. Я понимаю, что всячески хочется показать никчёмность анализатора, но это неконструктивный путь.

          64-битные диагностики по своей сути дают много ложных срабатываний и с этим ничего нельзя сделать. Именно поэтому мы не включили их в CppCat. Программисты не подумав включают всё на максимум, а потом жалуются, что много ложных срабатываний. Мы вообще со временем думаем спрятать 64-битные диагностики подальше от шаловливых рук. :)

          Полный набор 64-битных диагностик нужен только тем, кто создаёт ресурсоёмкие приложения, потребляющие большой объём памяти и обрабатывающий большие массивы. Если больших массивов нет, но всё-таки хочется использовать 64-битные диагностики, то можно ограничиться подмножеством. Например, можно выключить все 64-битные предупреждения кроме V114, V204, V205, V220, V301. Но возможно, что-то захочет просмотреть и V112. Тут всем не угодишь, поэтому мы не выделяем какое-то подмножество.

          Такое подмножество можно выбрать самостоятельно, отключая неинтересные диагностики. Это делается очень просто. Можно использовать настройки или зайти в контекстное меню (правый клик мыши на сообщении).

          А по поводу lstrlen(). Она устарела с точки 64-битности, ибо не вернёт правильное значение, если длина строки превысит INT_MAX символов. Понятно, что это что-то необычное. Но раз это 64-битная программа, то теоретически это возможно. А нулевые указатели тут вообще ни при чём. Это не забота 64-битных диагностик. Они говорят о своём специфическом.
          • –3
            Я изрядно покопался в www.viva64.com/external-pictures/txt/MirandaNG-595.txt, и если бы я хотел опустить ваш продукт то потратил бы ещё больше времени чтобы накатать портянку, где прокомментировал бы каждую строчку из того отчёта и подсчитал в конце все ложнопозитивы, всё что попало в точку, всё что он «угадал» и что ему показалось, и отдельно написал бы о том, вызывается данная функция когда либо с NULL или такая вероятность исключена полностью.

            Честно говоря, меня больше раздражает не ложнопозитивы, а предложения купить и поставить его.
            Уже два раза.

            По поводу lstrlen() я написал чуть выше.

            Касательно х64, я вот ещё проблем нашёл:
            l# grep -R "_WNDPROC" /root/MirandaNG | grep "_WNDPROC, (" | grep -v «LONG_PTR»
            /root/MirandaNG/plugins/ListeningTo/src/players/winamp_mlt/mlt_winamp.cpp: SetWindowLongPtr(plugin.hwndParent, GWLP_WNDPROC, (LONG) oldMainWndProc);
            /root/MirandaNG/plugins/ListeningTo/src/players/winamp_mlt/mlt_winamp.cpp: SetWindowLongPtr(hPlWnd, GWLP_WNDPROC, (LONG) oldWndProc);
            /root/MirandaNG/plugins/ListeningTo/src/players/winamp_mlt/mlt_winamp.cpp: oldMainWndProc = (WNDPROC)SetWindowLongPtr(plugin.hwndParent, GWLP_WNDPROC, (LONG) MainWndProc);
            /root/MirandaNG/plugins/ListeningTo/src/players/winamp_mlt/mlt_winamp.cpp: oldWndProc = (WNDPROC) SetWindowLongPtr(hPlWnd, GWLP_WNDPROC, (LONG)WndProc);
    • +7
      «В .NET я не хочу разбираться, я обижен на МС за то что закопали мой любимый Visual Basic.»

      Это многое объясняет.
      • –1
        Вы прежде чем критиковать лучше бы историю поучили.
        Я не один такой: russian.joelonsoftware.com/Articles/HowMicrosoftLosttheWaronA.html

        VB и VC были нашим всем года до 2004.
        Они закрывали вообще все потребности в винде.
        VB был великолепен для обучения и кодинга относительно простых программ.
        VC для сложных и ресурсоёмких.
        Они в принципе сочетались, можно было написать dll на сях и дёргать её из вб.
        Всё было просто зашибись. Все довольны. Обучение шло, книжки писались («VB крепкий орешек»), программы кодились.

        Потом какой то утырок предложил .NET.
        Предложил, а ниши для него не было, он был никому нафиг не нужен.
        И чтобы его хоть как то продвинуть МС берёт и закапывает ВБ.
        У ВБ были недостатки, но ничего фатального, чтобы его вот взять и закопать живьём(!).
        И ведь для кого то это был отличный инструмент для работы и зарабатывания. Притом инструмент платный.
        Без этого принудительного закапывания ВБ .NET до сих пор бы с ним конкурировал, и скорее всего проигрывал в популярности.
        VBA — практически тот же VB до сих пор жив и никуда не делся.

        Отсюда вопрос: зачем мне ещё раз проверять грабли от МС?
        Более того, с винды я сваливаю, под не виндой оно нафик не впёрлось.
        Лучше уж я посмотрю в пхп, перл, джаву, если мне нужно будет что то «новенькое».
        Только по правде говоря, у меня и на сях дел выше крыши и мне более чем хватает его портабельности и скорости работы.

        Если .NET ваши первые грабли от МС, поздравляю, теперь (10 лет спустя) у них мягкая ручка покрытая пингвинячьим мехом и удобный костяной шип от бисти.
        • 0
          Что означает «закопали живьем»? Больше нельзя писать скрипты .vbs? Больше нельзя писать макросы на VBA? VB.NET — это не язык программирования?
          • –1
            Значит что все кучи проектов которые у меня были на вб 5 и 6 теперь не работают и всё зачем то нужно переписывать.
            • +3
              А что мешает им продолжать работать?
          • +2
            Больше нет компилятора VB, выпускающего native код, как я понимаю. Последний — VB6 98го года.
            • –2
              Он больше не работает?
              • 0
                Было бы интересно попробовать установить Visual Studio 98 на Windows 8.1
                • 0
                  Угу, учитывая, что при установке надо было иметь, емнип, IE4.0 и что-то с SP4 для W2K.
              • +1
                Даже в XP SP3 запустить проблемно. За прошедшие 16 лет все ушло вперед, начиная от библиотек-компонентов, и заканчивая оптимизацией под новые процессоры.
                А так-то что — вполне можно MSVC 2.0 поставить и клепать на нем программы. Никто не мешает.
  • –2
    Кстати автор уже дважды пиариться на одном коде. В 2011 году же была серия статей по анализу кода миранды. Я даже ради комментов к этим статьям зарегистрировался тут.

    Andrey2008, вы действительно расчитывали, что в OpenSource проекте с более чем 14 летним кодом, с максимум десятком активных разработчиков к настоящему моменту, с отсутствием «правильных» подходов к процессу разработки ваш анализатор найдет что то новое?

    Кстати новая статья намного хуже. Первый цикл все таки выглядел как «давайте я научу вас как не стоит писать код», «избегайте популярных косяков». Та статья действительно учила и лишь немного выглядела рекламой (такая скрытая и ненавязчивая «Выставляйте высокий уровень предупреждений у компилятора и используйте статические анализаторы» в шестом и последнем разделе статьи).

    Новая же «глядите как наш анализатор умеет клево находить очевидные косяки» часть из которых явно наследие копипасты, а уж вещи типа new[] / delete / free c головой выдают начинающих программеров, увы я не поверю что кто то может как вы сказали «забыть» квадратные скобки у delete или случайно использовать free. Вы бы хоть объяснили почему так не надо делать.

    P.S. Кстати удивлен отсутствием пиара фичи «мы можем находить оператор присвоения в условиях под if»
    • +2
      Статьи бывают очень разные. Поскольку в этот раз было слишком много предупреждений я решил говорить только о них, не рассматривая рекомендации. Иначе вообще слишком длинно получится. По поводу «оператор присвоения в условиях под if» — это будет во второй части. Скоро опубликую. Жду перевода на английский язык.
  • +4
    Ух, какое мясо-то! Давненько такого не было, спасибо!

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

Самое читаемое Разработка