Аннотация

Я добрался до кода широко известного клиента мгновенных сообщений Miranda IM. Вместе с различными плагинами это достаточно большой проект, размер которого составляет около 950 тысяч строк кода на C и C++. И, как в любом солидном проекте с историей развития, в нем имеется немалое количество ошибок и опечаток.

Введение



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

Для анализа Miranda IM я использовал анализатор PVS-Studio 4.14. Код проекта Miranda IM весьма качественен, что подтверждается популярностью этой программы. Я и сам являюсь пользователем этого клиента и не имею к нему претензий в плане качества. Проект собирается в Visual Studio с Warning Level 3 (/W3), а количество комментариев составляет 20% от всего текста программы.

1. Избегайте функции memset, memcpy, ZeroMemory и им аналогичные

Мы начнем с ошибок, возникающих при использовании низкоуровневых  функций работы с памятью такими, как memset, memcpy, ZeroMemory и им подобных.

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

1) Обработка больших массивов. То есть там, где оптимизированный алгоритм функции даст выигрыш по сравнению с обработкой элементов в простом цикле.

2) Обработка большого количества маленьких массивов. Также имеет смысл с точки зрения улучшения производительности.

Во всех остальных случаях лучше попробовать обойтись без них. Например, я считаю эти функции излишними в такой программе, как Miranda. Никаких ресурсоёмких алгоритмов или огромных массивов в ней нет. Следовательно, использование функций memset/memcpy проистекает только из удобства написания короткого кода. Однако, эта простота очень обманчива и, сэкономив несколько секунд на написании кода, можно неделями вылавливать нечётко проявляющуюся ошибку порчи памяти. Рассмотрим несколько примеров кода, взятых из проекта Miranda IM.

V512 A call of the 'memcpy' function will lead to a buffer overflow or underflow. tabsrmm utils.cpp 1080



Копируем только кусок строки. Ошибка банальна до безобразия, но от этого не перестаёт быть ошибкой. Скорее всего, раньше здесь использовалась строка, состоящая из 'char'. Затем перешли на Unicode строки, а поменять константу забыли.

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



Тогда при переходе к Unicode строкам, число 7 изменять не надо:



Я не говорю, что это идеальный код. Но он уже намного лучше, чем использование CopyMemory. Рассмотрим другой пример.

V568  It's odd that the argument of sizeof() operator is the '& ImgIndex' expression.  clist_modern  modern_extraimage.cpp  302



Здесь хотелось обнулить массив, состоящий из 64-указателей. Но вместо этого мы обнулим только первый элемент. Эта же ошибка, кстати, присутствует ещё раз в другом файле. Скажем спасибо Copy-Paste:

V568  It's odd that the argument of sizeof() operator is the '& ImgIndex' expression.  clist_mw  extraimage.c  295

Корректный вариант кода должен выглядеть следующим образом:



Кстати, взятие адреса у массива может только дополнительно запутать того, кто читает код. Здесь взятие адреса не имеет никакого эффекта. И код может быть написан так:



Следующий пример.

V568  It's odd that the argument of sizeof() operator is the '& rowOptTA' expression.  clist_modern  modern_rowtemplateopt.cpp  258



Снова, вместо вычисления размера массива, мы вычисляем размер указателя. Правильным выражением будет "sizeof(rowOptTA)". Для очистки массива я предлагаю следующий вариант такого кода:



Я уже привык, что подобные строки любят размножаться по программе копированием:

V568  It's odd that the argument of sizeof() operator is the '& rowOptTA' expression.  clist_modern  modern_rowtemplateopt.cpp  308

V568  It's odd that the argument of sizeof() operator is the '& rowOptTA' expression.  clist_modern  modern_rowtemplateopt.cpp  438

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

V512  A call of the 'memset' function will lead to a buffer overflow or underflow.  clist_modern  modern_image_array.cpp  59



В этот раз размер копируемых данных вычислен корректно. Вот только перепутан местами второй и третий аргумент. Как следствие, мы заполняем 0 элементов. Корректный вариант:



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

Возникает вопрос, а как же обойтись без memset при работе с такими структурами, как OPENFILENAME:



Очень просто.  Создать обнуленную структуру можно так:



2. Внимательно следите, работаете вы со знаковым или беззнаковым типом

Проблема путаницы между signed и unsigned типами может на первый взгляд показаться надуманной. Но этот вопрос зря недооценивается программистами.

В большинстве случаев, людям не нравится рассматривать предупреждения компилятора, связанные с тем, что переменная типа int сравнивается с переменной типа unsigned. И действительно, чаще всего подобный код совершенно корректен. Программисты отключают такие предупреждения или не обращают на них внимания. Или есть третий вариант, они вписывают явное приведение типа, чтобы заглушить предупреждение компилятора, не вдаваясь в подробности.

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

V547  Expression 'wParam >= 0' is always true. Unsigned type value is always >= 0.  clist_mw  cluiframes.c  3140

В коде программы имеется функция id2pos, которая возвращает значение '-1' в случае ошибки.  С этой функцией всё в порядке. В другом месте результат работы функции id2pos используется следующим образом:



Проблема в том, что переменная wParam имеет беззнаковый тип. Следовательно, условие 'wParam>=0' всегда истинно. Если функция id2pos вернет '-1', то условие проверки недопустимых значений не сработает, и мы начнем использовать отрицательный индекс.

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

if (wParam>=0 && wParam<nFramescount)

Компилятор Visual C++ выдал предупреждение "warning C4018: '<' : signed/unsigned mismatch". Это предупреждение как раз включено на Warning Level 3, с которым и собирается Miranda IM. И в этот момент этому месту было уделено недостаточное внимание. Предупреждение было подавлено явным приведением типа. Но ошибка не исчезла, а только спряталась. Корректным вариантом должен был стать следующий код:

if ((INT_PTR)wParam>=0 && (INT_PTR)wParam<nFramescount)

Так что внимание и ещё раз внимание к подобным местам. В Miranda IM я насчитал 33 условия, которые из-за путаницы с signed/unsigned всегда истинны или всегда ложны.

Продолжим. Следующий пример мне особенно нравится. А комментарий, просто прекрасен.

V547 Expression 'nOldLength < 0' is always false. Unsigned type value is never < 0. IRC mstring.h 229



Думаю, пояснений к этому коду не требуется.

Конечно, в ошибках не всегда виноваты только программисты. Иногда свинью подкладывают и разработчики библиотек (в данном случае WinAPI).



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

Проблема в том, что функция GetDlgItemInt() возвращает вовсе не 'int', как ожидал программист. Эта функция возвращает UINT. Вот её прототип из файла "WinUser.h":



В результате PVS-Studio выдает сообщение:

V547 Expression is always true. Unsigned type value is always >= 0. scriver msgoptions.c 458

И это действительно так. Выражение "GetDlgItemInt(hwndDlg, IDC_LIMITNAMESLEN, NULL, TRUE) >=    SRMSGSET_LIMITNAMESLEN_MIN" всегда истинно.

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

3. Избегайте большого количества вычислений в одной строке

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

Чаще всего ошибки возникают там, где программист, с целью создать компактный код, собирает в одну строку сразу несколько действий. При незначительном выигрыше в изящности он существенно увеличивает риск опечатки или того, что не учтёт побочные действия. Рассмотрим пример:

V567 Undefined behavior. The 's' variable is modified while being used twice between sequence points. msn ezxml.c 371



Здесь возникает неопределенное поведение (undefined behavior). Этот код может корректно функционировать в течение долгого периода жизни программы. Но нет никакой гарантии, что он также будет вести себя после смены версии компилятора или ключей оптимизации. Компилятор в праве вначале вычислить '++s', а затем взывать функцию 'strspn(s, EZXML_WS)'. Или, наоборот,  вначале он может вызвать функцию, а уже затем увеличить значение переменной 's'.

Приведу другой пример, почему не стоит пытаться собрать всё в одну строку. В Miranda IM некоторые ветви исполнения программы отключены/включены с помощью вставок вида '&& 0'. Примеры:



С приведенными сравнениями всё ясно и они хорошо заметны. А теперь представьте, что вы встречаете фрагмент показанный ниже. Код я отформатировал. В программе это ОДНА строка.

V560  A part of conditional expression is always false: 0.  clist_modern  modern_clui.cpp  2979



Если здесь нет ошибки, то всё равно непросто вспомнить и найти в этой строке слово FALSE. Вы его нашли? Согласитесь - непростая задача. А если это ошибка? Тогда вообще нет шансов найти её, просто просматривая код. Подобные выражения очень полезно выносить отдельно. Пример:



А я сам, пожалуй, написал бы это длинным, но более понятным способом:



Да, это длиннее, но зато легко читается, а слово FALSE лучше заметно.

Я бы этот код:Написал так: