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

28 Июл 2011

Теги:

#Cpp

Проверка проекта Intel Energy Checker SDK (IEC SDK) с помощью PVS-Studio

28 Июл 2011

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

А теперь десерт
Заключение
Отзыв от Джемила Тейеба, разработчика IEC SDK

В последнее время, рассказывая о проверке очередного проекта, я всё время повторял, что это очень качественный код и ошибок в нём практически не найдено. Примером может служить анализ таких проектов, как Apache, MySQL, Chromium. Почему мы выбираем такие приложения, я думаю понятно. Про них всех знают и никому не интересно, какие ужасы можно найти в дипломной работе студента Васи. Однако иногда мы поверяем и те проекты, которые просто случайно попали под руку. Некоторые такие проекты оставляют тяжёлые впечатления в моей тонкой и ранимой душе. В этот раз мы проверили Intel(R) Energy Checker SDK (IEC SDK).

Intel Energy Checker SDK - это маленький проект на Си, содержащий всего 74500 строк кода. Для сравнения, размер проекта WinMerge составляет 186 000 строк кода, а размер проекта Miranda IM с плагинами около 950 000 строк кода.

Кстати, пока мы находимся в начале статьи, попробуйте угадать, сколько в этом проекте может быть операторов 'goto'. Загадали число? Если да, то тогда продолжим.

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

Не скажу, что код ужасен, бывает намного хуже. Но смотрите сами. Во всем проекте всего 247 функций. Скажете мало? Конечно, мало. Зато размер некоторых из них шокирует. Четыре функции вообще имеют размер более 2000 строк:

V553 The length of 'pl_open' function's body is more than 2000 lines long. You should consider refactoring the code. pl_csv_logger productivity_link.c 379

V553 The length of 'pl_attach' function's body is more than 2000 lines long. You should consider refactoring the code. pl_csv_logger productivity_link.c 9434

V553 The length of 'main' function's body is more than 2000 lines long. You should consider refactoring the code. cluster_energy_efficiency cee.c 97

V553 The length of 'main' function's body is more than 2000 lines long. You should consider refactoring the code. pl2ganglia pl2ganglia.c 105

Показателен и вот такой способ получения длины имени директории:

#define PL_FOLDER_STRING "C:\\productivity_link"
#define PL_PATH_SEPARATOR_STRING "\\"
#define PL_APPLICATION_NAME_SEPARATOR_STRING "_"
...
pl_root_name_length = strlen(PL_FOLDER_STRING);
pl_root_name_length += strlen(PL_PATH_SEPARATOR_STRING);
pl_root_name_length += application_name_length;
pl_root_name_length += strlen(PL_APPLICATION_NAME_SEPARATOR_STRING);
pl_root_name_length += PL_UUID_MAX_CHARS;
pl_root_name_length += strlen(PL_PATH_SEPARATOR_STRING);

Я понимаю, что это место не критично для скорости и нет смысла оптимизировать расчет длины строк. Но просто из любви к искусству, можно было бы завести для этого вот такой макрос "#define STRLEN(s) (sizeof(s) / sizeof(*s) - 1)". Ещё больше моё чувство прекрасного страдает от того, что я вижу строку, содержащую "C:\\". Меня настораживают подобные макросы:

#define PL_INI_WINDOWS_FOLDER "C:\\productivity_link"

#define PL_INI_WINDOWS_LC_FOLDER "c:\\productivity_link"

#define PLH_FOLDER_SEARCH _T("C:\\productivity_link\\*")

Впрочем, этот код делает то, что нужно и не будем заострять внимание на стиле программирования. Гораздо больше пугает то количество ошибок, которые нашел PVS-Studio в проекте такого маленького размера. При этом стоит учитывать, что проверено далеко не 74000 строк кода. Около трети кода предназначено для LINUX/SOLARIS/MACOSX и находится в #ifdef/#endif ветках кода, которые не проверялись. Непроходимый лес из #ifdef/#endif вообще отдельная тематика, но я обещал больше не говорить об оформлении кода. Желающие могут сами заглянуть в исходные коды.

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

Есть код, обращающийся к памяти за пределами массива:

V557 Array overrun is possible. The '255' index is pointing beyond array bound. pl2ganglia pl2ganglia.c 1114

#define PL_MAX_PATH 255
#define PL2GANFLIA_COUNTER_MAX_LENGTH PL_MAX_PATH

char name[PL_MAX_PATH];

int main(int argc, char *argv[]) {
  ...
  p->pl_counters_data[i].name[
    PL2GANFLIA_COUNTER_MAX_LENGTH
  ] = '\0';
  ...
}

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

p->pl_counters_data[i].name[
   PL2GANFLIA_COUNTER_MAX_LENGTH - 1
] = '\0';

Есть ошибки обнуления структур.

V568 It's odd that the argument of sizeof() operator is the '& file_data' expression. pl_csv_logger productivity_link_helper.c 1667

V568 It's odd that the argument of sizeof() operator is the '& file_data' expression. pl_csv_logger productivity_link_helper.c 1831

V512 A call of the 'memset' function will lead to underflow of the buffer 'pconfig'. pl_csv_logger productivity_link_helper.c 1806

Пример подобного некорректного обнуления:

int plh_read_pl_folder(PPLH_PL_FOLDER_INFO pconfig) {
  ...
  WIN32_FIND_DATA file_data;
  ...
  memset(
    &file_data, 
    0, 
    sizeof(&file_data)
  );
  ...
}

Плохо пойдет работа по поиску файлов, если использовать структуру WIN32_FIND_DATA с мусором внутри. Впрочем, есть подозрение, что Windows версия этого произведения программистского искусства мало кого интересует. Например, код компилируется в режиме "Use Unicode Character Set", хотя он на это рассчитан не до конца. Видимо, никто и не задумывался. Создали проект для Visual Studio, а по умолчанию настройка "Character Set" как раз задает использование UNICODE.

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

V512 A call of the 'memset' function will lead to underflow of the buffer '(pl_cvt_buffer)'. pl_csv_logger productivity_link_helper.c 683

#define PL_MAX_PATH 255
typedef WCHAR TCHAR, *PTCHAR;
TCHAR pl_cvt_buffer[PL_MAX_PATH] = { '\0' };

int plh_read_pl_config_ini_file(...)
{
  ...
  ZeroMemory(
    pl_cvt_buffer, 
    PL_MAX_PATH
  );
  ...
}

Впрочем, есть места, где и отключение UNICODE не поможет. Здесь вместо текста будет распечатано что-то непонятное:

V576 Incorrect format. Consider checking the second actual argument of the 'wprintf' function. The pointer to string of wchar_t type symbols is expected. producer producer.c 166

int main(void) {
  ...
  char *p = NULL;
  ...
#ifdef __PL_WINDOWS__
  wprintf(
    _T("Using power link directory: %s\n"), 
    p
  );
#endif // __PL_WINDOWS__
  ...
}

Поясню. Функция wprintf ожидает строку типа "wchar_t *", а ей будет передана строка типа "char *".

Есть и другие ошибки, и небольшие ляпы, похожие на этот:

V571 Recurring check. The 'if (ret == PL_FAILURE)' condition was already verified in line 1008. pl_csv_logger pl_csv_logger.c 1009

if(ret == PL_FAILURE) {
  if(ret == PL_FAILURE) {
    pl_csv_logger_error(
      PL_CSV_LOGGER_ERROR_UNABLE_TO_READ_PL
  );

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

А теперь десерт

Помните, в начале, я просил загадать, сколько в проекте можно найти операторов 'goto'. Так вот, я думаю ваше число далеко от истины. Всего в проекте используется 1198 операторов goto. Один оператор goto на 60 строк кода. А я думал, что подобный стиль уже канул в небытие.

Заключение

А что собственно этой статьёй хотел сказать автор? Интелу СРОЧНО нужно внимательно присмотреться к PVS-Studio. :-)

Отзыв от Джемила Тейеба, разработчика IEC SDK

Дорогой Андрей и команда PVS-Studio,

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

Errare humanum est (Человеку свойственно ошибаться) — данная фраза поистине подходит для всех случаев, а в данном случае особенно и в отношении меня самого! Я написал практически весь код из "Energy Checker", поэтому мне кажется, что вашим потенциальным клиентам будет интересно, если я поделюсь с ними своим опытом работы с вашим инструментом. Помимо его проницательности в оценке — я вернусь к этому чуть позже — я очень положительно оцениваю хорошую интеграцию инструмента с Microsoft Visual Studio 2005. А это очень важно, поскольку я могу использовать ПО сразу, "прямо из коробки", и незамедлительно начинать работать над устранением своих ошибок. С другой стороны я, к сожалению, вынужден отметить скорость самого анализа как недостаток. Насколько я знаю, ваша компания сейчас работает над её улучшением, поэтому я уверен, что в будущем использование PVS-Studio станет ещё более лёгким и приятным.

А теперь вернемся к сути вопроса. PVS-Studio "ударила меня по рукам" за мои ошибки. И это как раз то, чего я и ожидал от подобного ПО. Конечно, можно сказать, что наш код работает хорошо, с хорошей производительностью и выполняя отведённую ему роль (но только до следующего отчёта об ошибке). Но во многих случаях я начинал чувствовать вину за то, что некоторые фрагменты кода функционируют только благодаря моему везению. Внесение же в них правок заставляет их работать уже потому, что они теперь правильны. А когда такая ошибка находится в макросе, большое удовольствие доставляет наблюдение за значительным уменьшением количества сообщений об ошибках сразу по завершению инкрементального анализа. Другим откровением, поразившим меня, было то, что "copy-paste" может быть коварным другом. Он, конечно, ускоряет создание нового сегмента кода, но вы должны быть очень и очень бдительны, когда делаете это. Безусловно, в некоторых случаях я не был достаточно бдителен. Анализатор PVS-Studio пробудил меня.

Ещё раз я благодарю вас за обратную связь, проницательность вашего ПО и за то, что предоставили мне возможность опробовать PVS-Studio.

Джемил Тейеб, разработчик IEC SDK

#Cpp