Как вы знаете, основная наша деятельность – это разработка анализаторов кода PVS-Studio и CppCat. И хотя мы давно и, как нам кажется, успешно этим занимаемся, недавно у нас появилась необычная мысль. Все-таки мы не пользуемся своими инструментами в том режиме, что и наши клиенты. Нет, конечно, мы проверяем код PVS-Studio с помощью PVS-Studio. Но откровенно говоря, проект PVS-Studio не такой уж большой. И работа с кодом PVS-Studio по стилю и характеру отличается от, к примеру, работы с кодом Chromium или LLVM.
Нам хотелось побывать в шкуре своих клиентов для того, чтобы понять, как наш инструмент используется в долгосрочных проектах. Ведь проверки проектов, которые мы делаем регулярно и, про которые пишем много статей, это как раз тот стиль использования анализатора, против которого мы активно выступаем. Неправильно запустить разово анализатор на проекте, исправить несколько ошибок и повторить это через год. При написании кода анализатор надо использовать регулярно, каждый день.
Ну да ладно, к чему это все? Наши теоретические желания попробовать себя в других проектах совпали с практическими предложениями, которые постепенно стали к нам поступать. В прошлом году мы решили выделить у нас в компании команду, которая бы занималась – о ужас! – разработкой на заказ. То есть участвовала в сторонних проектах в качестве программистов. Причем нам было интересно участвовать в долгосрочных и довольно крупных проектах, т.е. не менее 2-3 разработчиков и не менее 6 месяцев разработки. У нас было две цели:
- попробовать альтернативный тип бизнеса (заказную разработку помимо продуктовой разработки);
- самим посмотреть на использование PVS-Studio в долгосрочных проектах.
И первая, и вторая задача оказались удачными. Но эта статья не про бизнес по заказной разработке, а про наш опыт. Имеется в виду не организационный опыт. Про это и так много статей. Про опыт работы с кодом чужих проектов. Про это мы и хотим рассказать.
Замеченные интересные моменты
Сторонние проекты многому учат нас. Думаю, этой тематике мы посвятим далеко не одну статью. А сейчас мы начнём с некоторых интересных наблюдений. Мы заметили 3 яркие особенности кода, которые проявили себя в больших и старых проектах. Уверены, со временем будут публикации и про другие наблюдения.
У нас есть достаточно много статей посвященных смене разрядности платформы с 32-бит на 64-бита. С портированием связано множество ошибок, которые начинают проявлять себя в 64-битных программах. Мы называем их 64-битные ошибки.
Но помимо перехода на 64-битные системы, произошли и другие изменения в коде, которые не так заметны. Произошло это в связи с развитием компиляторов, библиотек и взрослением самих проектов. Последствия этих изменений хорошо заметны в коде, имеющем длинную историю развития. Предлагаем их обсудить. Думаю, это будет интересно и полезно. Возможно, кто-то захочет провести обзор своего старого кода, чтобы выявить аналогичные проблемы.
Рассмотренные паттерны ошибок были замечены нами, благодаря анализатору PVS-Studio или CppCat (см. возможности анализаторов). Многие из этих ошибок скрыты. Код почти работает благодаря удачному стечению обстоятельств. Однако, каждая такая ошибка — маленькая бомба замедленного действия, которая может сработать в любой момент.
Примечание. Чтобы избежать ограничений NDA, мы изменили имена и отредактировали код. Собственно, это совсем не тот код, который был в программе. Однако, «всё основано на реальных событиях».
Изменение в поведении оператора new
Давным-давно оператор 'new' возвращал NULL в случае ошибки выделения памяти. Затем компиляторы начали поддерживать современный стандарт языка Си++, и бросать в таких ситуациях исключения std::bad_alloc. Можно заставить оператор 'new' возвращать NULL, но сейчас речь не об этом.
Видимо, эти изменения были замечены сообществом программистов весьма поверхностно. Взяли себе на заметку этот факт и начали писать код с учётом нового поведения. Есть, конечно, программисты, которые до сих пор не в курсе, что оператор 'new' кидает исключение. Но мы говорим о нормальных, адекватных программистах. Личности, которые ничего не хотят знать и продолжают писать в стиле, как делали это 20 лет назад, нам не интересны.
Тем не менее, даже те, кто знает, что оператор 'new' изменил своё поведение, не провели обзор существующего кода. Кто-то просто не сообразил это сделать. Кто-то хотел, но ему было некогда, а потом он забыл про это. В результате, в огромном количестве программ продолжают жить некорректные обработчики ситуаций, когда невозможно выделить память.
Некоторые фрагменты кода безобидны:
int *x = new int[N];
if (!x) throw MyMemoryException(); // 1
int *y = new int[M];
if (!y) return false; // 2В первом случае вместо MyMemoryException будет сгенерировано исключение std::bad_alloc. Скорее всего, эти исключения обрабатываются одинаковым образом. Проблем нет.
Во втором случае проверка не сработает. Функция не вернёт значение 'false'. Вместо этого возникнет исключение, которое как-то будет потом обработано. Это ошибка. Поведение программы изменилось. Но на практике это почти никогда не приводит к проблемам. Просто программа немного по-разному будет реагировать на нехватку памяти.
Важнее предупредить о случаях, когда поведение программы изменяется не чуть-чуть, а весьма существенно. Таких ситуаций в больших старых проектах можно встретить огромное количество.
Вот пара примеров, когда при нехватке памяти должны быть выполнены определённые действия:
image->SetBuffer(new unsigned char[size]);
if (!image->GetBuffer())
{
CloseImage(image);
return NULL;
}
FormatAttribute *pAttrib = new FormatAttribute(sName, value, false);
if (pAttrib )
{
m_attributes.Insert(pAttrib);
}
else
{
TDocument* pDoc = m_state.GetDocument();
if (pDoc)
pDoc->SetErrorState(OUT_OF_MEMORY_ERROR, true);
}Такой код намного более опасен. Например, пользователь может потерять содержимое своего документа в случае нехватки памяти, хотя вполне можно было дать возможность сохранить документ в файл.
Рекомендация. Найдите в своей программе все операторы 'new'. Проверьте, не собирается ли программа предпринять какие-то действия, если указатель будет нулевым. Перепишите такие места.
Анализаторы PVS-Studio и CppCat помогают обнаружить бессмысленные проверки с помощью диагностики V668.
Замена char * на std::string
С переходом от Си к Си++ и с ростом популярности стандартной библиотеки, в программах стали широко использовать классы строк, такие как std::string. Это понятно и объяснимо. Проще и безопасней работать с полноценной строкой, а не с указателем «char *».
Класс строки стали использовать не только в новом коде, но и изменять некоторые старые фрагменты. Как раз с этим и могут возникать неприятности. Достаточно ослабить внимание, и код становится опасным или вовсе некорректным.
Но начнём не со страшного, а скорее забавного. Иногда попадаются вот такие неэффективные циклы:
for (i = 0; i < strlen(str.c_str()); ++i)Явно, когда-то переменная 'str' была обыкновенным указателем. Плохо вызывать функцию strlen() при каждой итерации цикла. Это крайне неэффективно на длинных строках. Однако, после превращения указателя в std::string, код стал выглядеть ещё более глупо.
Сразу видно, что замена типов происходила бездумно. Это может приводить к подобному неэффективному коду или вообще к ошибкам. Поговорим про ошибки:
wstring databaseName;
....
wprintf("Error: cannot open database %s", databaseName); // 1
CString s;
....
delete [] s; // 2В первом случае, указатель 'wchar_t *' превратился в 'wstring'. Беда возникнет, если не удастся открыть базу, и нужно будет распечатать сообщение. Код компилируется, но на экран будет распечатана абракадабра. Или программа вообще упадёт. Причина — забыли добавить вызов функции c_str(). Корректный вариант:
wprintf("Error: cannot open database %s", databaseName.c_str());Второй случай вообще эпичен. Как это не удивительно, но такой код успешно компилируется. Используется весьма популярный строковый класс CString. Класс CString неявно может преобразовываться к указателю. Именно это здесь и происходит. Результат — двойное удаление буфера.
Рекомендация. Если вы меняете указатели на класс строки, делайте это аккуратно. Не используйте массовую замену без просмотра каждого случая. То, что код компилируется, вовсе не означает, что он работает. Лучше оставить в покое код, использующий указатели, если нет явной необходимости его править. Пусть лучше код корректно работает с указателями, чем некорректно с классами.
Анализаторы PVS-Studio и CppCat помогают обнаружить некоторые из ошибок, возникшие из-за замены карателей на классы. В этом могут помочь диагностики V510, V515 и т.д. Но полностью надеяться на анализаторы не стоит. Может попасться крайне творческий код, ошибку в котором с трудом сможет найти не только анализатор, но и опытный программист.
Замена char на wchar_t
Проект развивается, появляется желание сделать интерфейс приложения многоязычным. И вот в какой-то момент происходит массовая замена char на wchar_t. Поправлены ошибки, выданные компилятором. «Готова» unicode-версия приложения.
На практике, после такой замены приложение превращается в решето. Ошибки, которые возникнут после такой замены, могут проявлять себя десятилетиями и с трудом воспроизводиться.
Как-же такое может быть? Очень просто. Многие фрагменты кода не готовы, что размер символа изменится. Код компилируется без ошибок и без предупреждений. Но работает только на «50%». Сейчас вы поймете, что мы имеем в виду.
Примечание. Напомню, что мы не запугиваем плохим кодом, полученным от студентов. Мы рассказываем о суровых реалиях программистского бытия. В больших и старых проектах неизбежно появляются такие ошибки. И их сотни. Серьёзно. Сотни.
Примеры:
wchar_t tmpBuffer[500];
memset(tmpBuffer, 0, 500); // 1
TCHAR *message = new TCHAR[MAX_MSG_LENGTH];
memset(charArray, 0, MAX_MSG_LENGTH*sizeof(char)); // 2
LPCTSTR sSource = ...;
LPTSTR sDestination = (LPTSTR) malloc (_tcslen(sSource) + 12); // 3
wchar_t *name = ...;
fprintf(fp, "%i : %s", i, name); // 4В первом случае программист вообще не задумался, что размер символа со временем изменится. Поэтому очистил только половину буфера. Вот откуда мои слова про 50%.
Во втором случае программист догадывался, что размер символа изменится. Однако, подсказка "* sizeof(char)" никак не помогла при массовой замене типов. Надо было делать не так. Правильный вариант:
memset(charArray, 0, MAX_MSG_LENGTH * sizeof(charArray[0]));Третий пример. С типами всё хорошо. Для подсчёта длины строки используется функция _tcslen(). На первый взгляд всё замечательно. Однако, когда символы стали иметь тип 'wchar_t', программа опять стала работать на 50%.
Выделяется в 2 раза меньше памяти, чем необходимо. По факту, программа будет успешно работать до тех пор, пока длина сообщения не будет превышать половину от максимально возможной длины. Неприятная ошибка которая надолго задержалась в коде.
Четвёртый пример. Поправили функции printf(), sprintf() и так далее, но забыли проверить frintf(). В результате, в файл записывается мусор. Надо было сделать как-то так:
fwprintf(fp, L"%i : %s", i, name);Или так, если это обыкновенный ASCII-файл:
fprintf(fp, "%i : %S", i, name);Примечание. Сейчас подумал, что говорил про 50%, с точки зрения Windows. В Linux тип wchar_t занимает не 2, а 4 байта. Так что в Linux мире программа будет работать вообще только на 25% :).
Рекомендация. Если вы уже массово заменили char на wchar_t, не знаем, что может помочь. Этим можно было внести массу интересных ошибок. Внимательно просмотреть все места, где используется wcahr_t, не реально. Отчасти вам помогут статические анализаторы кода. Часть ошибок они выявят. Показанные выше ошибки найдут анализаторы PVS-Studio и CppCat. Последствия замены char на wchar_t весьма разнообразны и выявляются различными диагностиками. Перечислять их не имеет смысла. В качестве примера, можно назвать V512, V576, V635 и т.д.
Если вы ещё не делали замену, но планируете, то подойдите к ней со всей серьезностью. Заменить типы автоматически и поправить ошибки компиляции займет, например, 2 дня. Сделать те же замены вручную, с просмотром кода, займет на порядок больше времени. Например, вы потратите на это 2 недели. Но это лучше, чем потом втечение 2 лет вылавливать все новые ошибки:
- неправильное форматирование строк;
- выход за границы выделенных буферов памяти;
- работа с буферами, очищенными на половину;
- работа с буферами, скопированными на половину;
- и так далее.
Заключение
Мы остались довольны своим опытом участия в сторонних заказных разработках, так это позволило нам посмотреть на мир другими глазами. Мы продолжим свое участие в сторонних проектах в качестве разработчиков, так что, если кому интересно пообщаться с нами на эту тему, пишите. Если боитесь, что потом про вас будет разоблачительная статья (про найденные ошибки), то все-равно пишите – да поможет нам всем NDA!
Автор: Andrey2008
