Я добрался до кода широко известного клиента мгновенных сообщений http://www.miranda-im.org/. Вместе с различными плагинами это достаточно большой проект, размер которого составляет около 950 тысяч строк кода на C и C++. И, как в любом солидном проекте с историей развития, в нем имеется немалое количество ошибок и опечаток.
Введение
Рассматривая дефекты в различных приложениях, я заметил некоторые закономерности. И сейчас на примере дефектов, найденных в Miranda IM, я попробую сформулировать некоторые рекомендации, которые позволят избежать многих ошибок и опечаток ещё на этапе написания кода.
Для анализа Miranda IM я использовал анализатор http://www.viva64.com/ru/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.
http://www.viva64.com/ru/d/0101/ A call of the 'memcpy' function will lead to a buffer overflow or underflow. tabsrmm utils.cpp 1080
| Код |
typedef struct _textrangew |
Копируем только кусок строки. Ошибка банальна до безобразия, но от этого не перестаёт быть ошибкой. Скорее всего, раньше здесь использовалась строка, состоящая из 'char'. Затем перешли на Unicode строки, а поменять константу забыли.
Если изначально использовать для копирования строки функции, которые для этого и предназначаются, то такая ошибка просто не сможет возникнуть. Представим, что было написано так:
| Код |
strncpy(tr.lpstrText, "mailto:", 7); |
Тогда при переходе к Unicode строкам, число 7 изменять не надо:
| Код |
wcsncpy(tr.lpstrText, L"mailto:", 7); |
Я не говорю, что это идеальный код. Но он уже намного лучше, чем использование CopyMemory. Рассмотрим другой пример.
http://www.viva64.com/ru/d/0163/ It's odd that the argument of sizeof() operator is the '& ImgIndex' expression. clist_modern modern_extraimage.cpp 302
| Код |
void ExtraImage_SetAllExtraIcons(HWND hwndList,HANDLE hContact) |
Здесь хотелось обнулить массив, состоящий из 64-указателей. Но вместо этого мы обнулим только первый элемент. Эта же ошибка, кстати, присутствует ещё раз в другом файле. Скажем спасибо Copy-Paste:
http://www.viva64.com/ru/d/0163/ It's odd that the argument of sizeof() operator is the '& ImgIndex' expression. clist_mw extraimage.c 295
Корректный вариант кода должен выглядеть следующим образом:
| Код |
memset(&ImgIndex,0,sizeof(ImgIndex)); |
Кстати, взятие адреса у массива может только дополнительно запутать того, кто читает код. Здесь взятие адреса не имеет никакого эффекта. И код может быть написан так:
| Код |
memset(ImgIndex,0,sizeof(ImgIndex)); |
Следующий пример.
http://www.viva64.com/ru/d/0163/ It's odd that the argument of sizeof() operator is the '& rowOptTA' expression. clist_modern modern_rowtemplateopt.cpp 258
| Код |
static ROWCELL* rowOptTA[100]; |
Снова, вместо вычисления размера массива, мы вычисляем размер указателя. Правильным выражением будет "sizeof(rowOptTA)". Для очистки массива я предлагаю следующий вариант такого кода:
| Код |
const size_t ArraySize = 100; |
Я уже привык, что подобные строки любят размножаться по программе копированием:
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.
http://www.viva64.com/ru/d/0101/ A call of the 'memset' function will lead to a buffer overflow or underflow. clist_modern modern_image_array.cpp 59
| Код |
static BOOL ImageArray_Alloc(LP_IMAGE_ARRAY_DATA iad, int size) |
В этот раз размер копируемых данных вычислен корректно. Вот только перепутан местами второй и третий аргумент. Как следствие, мы заполняем 0 элементов. Корректный вариант:
| Код |
memset(&iad->nodes[iad->nodes_allocated_size], 0, |
Как переписать этот фрагмент кода более изящно, я не знаю. Вернее, его нельзя переписать красиво, не затрагивая другие участки программы и структуры данных.
Возникает вопрос, а как же обойтись без memset при работе с такими структурами, как OPENFILENAME:
| Код |
OPENFILENAME x; |
Очень просто. Создать обнуленную структуру можно так:
| Код |
OPENFILENAME x = { 0 }; |
2. Внимательно следите, работаете вы со знаковым или беззнаковым типом
Проблема путаницы между signed и unsigned типами может на первый взгляд показаться надуманной. Но этот вопрос зря недооценивается программистами.
В большинстве случаев, людям не нравится рассматривать предупреждения компилятора, связанные с тем, что переменная типа int сравнивается с переменной типа unsigned. И действительно, чаще всего подобный код совершенно корректен. Программисты отключают такие предупреждения или не обращают на них внимания. Или есть третий вариант, они вписывают явное приведение типа, чтобы заглушить предупреждение компилятора, не вдаваясь в подробности.
Предлагаю больше так не делать и каждый раз анализировать ситуацию, когда знаковый тип встречается с беззнаковым. И, вообще, быть внимательным к тому, какой тип имеет выражение или что возвращает функция. Теперь несколько примеров по рассматриваемой теме.
http://www.viva64.com/ru/d/0137/ Expression 'wParam >= 0' is always true. Unsigned type value is always >= 0. clist_mw cluiframes.c 3140
В коде программы имеется функция id2pos, которая возвращает значение '-1' в случае ошибки. С этой функцией всё в порядке. В другом месте результат работы функции id2pos используется следующим образом:
| Код |
typedef UINT_PTR WPARAM; |
Проблема в том, что переменная 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 всегда истинны или всегда ложны.
Продолжим. Следующий пример мне особенно нравится. А комментарий, просто прекрасен.
http://www.viva64.com/ru/d/0137/ Expression 'nOldLength < 0' is always false. Unsigned type value is never < 0. IRC mstring.h 229
| Код |
void Append( PCXSTR pszSrc, int nLength ) |
Думаю, пояснений к этому коду не требуется.
Конечно, в ошибках не всегда виноваты только программисты. Иногда свинью подкладывают и разработчики библиотек (в данном случае WinAPI).
| Код |
#define SRMSGSET_LIMITNAMESLEN_MIN 0 |
Если не обращать внимания на излишне сложное выражение, то код выглядит корректно. Кстати, это вообще была одна строка. Для удобства я разбил её на несколько. Впрочем, форматирование кода сейчас мы затрагивать не будем.
Проблема в том, что функция GetDlgItemInt() возвращает вовсе не 'int', как ожидал программист. Эта функция возвращает UINT. Вот её прототип из файла "WinUser.h":
| Код |
WINUSERAPI |
В результате PVS-Studio выдает сообщение:
http://www.viva64.com/ru/d/0137/ Expression is always true. Unsigned type value is always >= 0. scriver msgoptions.c 458
И это действительно так. Выражение "GetDlgItemInt(hwndDlg, IDC_LIMITNAMESLEN, NULL, TRUE) >= SRMSGSET_LIMITNAMESLEN_MIN" всегда истинно.
Конкретно в данном случае, возможно, никакой ошибки и не будет. Но смысл моего предупреждения, надеюсь, понятен. Внимательно смотрите, что возвращают вам функции.
3. Избегайте большого количества вычислений в одной строке
Каждый программист знает и при обсуждениях ответственно заявляет, что надо писать простой и понятный код. Но на практике иногда кажется, что он участвует в тайном конкурсе, кто напишет строку похитрее, с использованием интересной конструкции языка или продемонстрирует умение жонглировать указателями.
Чаще всего ошибки возникают там, где программист, с целью создать компактный код, собирает в одну строку сразу несколько действий. При незначительном выигрыше в изящности он существенно увеличивает риск опечатки или того, что не учтёт побочные действия. Рассмотрим пример:
http://www.viva64.com/ru/d/0162/ Undefined behavior. The 's' variable is modified while being used twice between sequence points. msn ezxml.c 371
| Код |
short ezxml_internal_dtd(ezxml_root_t root, char *s, size_t len) |
Здесь возникает неопределенное поведение (undefined behavior). Этот код может корректно функционировать в течение долгого периода жизни программы. Но нет никакой гарантии, что он также будет вести себя после смены версии компилятора или ключей оптимизации. Компилятор в праве вначале вычислить '++s', а затем взывать функцию 'strspn(s, EZXML_WS)'. Или, наоборот, вначале он может вызвать функцию, а уже затем увеличить значение переменной 's'.
Приведу другой пример, почему не стоит пытаться собрать всё в одну строку. В Miranda IM некоторые ветви исполнения программы отключены/включены с помощью вставок вида '&& 0'. Примеры:
| Код |
if ((1 || altDraw) && ... |
С приведенными сравнениями всё ясно и они хорошо заметны. А теперь представьте, что вы встречаете фрагмент показанный ниже. Код я отформатировал. В программе это ОДНА строка.
http://www.viva64.com/ru/d/0153/ A part of conditional expression is always false: 0. clist_modern modern_clui.cpp 2979
| Код |
LRESULT CLUI::OnDrawItem( UINT msg, WPARAM wParam, LPARAM lParam ) |
Если здесь нет ошибки, то всё равно непросто вспомнить и найти в этой строке слово FALSE. Вы его нашли? Согласитесь - непростая задача. А если это ошибка? Тогда вообще нет шансов найти её, просто просматривая код. Подобные выражения очень полезно выносить отдельно. Пример:
| Код |
UINT uFlags = DST_ICON; |
А я сам, пожалуй, написал бы это длинным, но более понятным способом:
| Код |
UINT uFlags; |
Да, это длиннее, но зато легко читается, а слово FALSE лучше заметно.