Pull to refresh
281.78
PVS-Studio
Static Code Analysis for C, C++, C# and Java

Разработчики Intel тоже говнокодят

Reading time 5 min
Views 2.6K
PVS-Studio vs IEC SDK
В последнее время, рассказывая о проверке очередного проекта, я всё время повторял, что это очень качественный код и ошибок в нём практически не найдено. Примером может служить анализ таких проектов, как Apache, MySQL, Chromium. Почему мы выбираем такие приложения, я думаю понятно. Про них всех знают и никому не интересно, какие ужасы можно найти в дипломной работе студента Васи. Однако иногда мы поверяем и те проекты, которые просто случайно попали под руку. Некоторые такие проекты оставляют тяжёлые впечатления в моей тонкой и ранимой душе. В этот раз мы проверили Intel® Energy Checker SDK (IEC SDK).

Intel Energy Checker SDK — это маленький проект на Си, содержащий всего 74500 строк кода. Для сравнения, размер проекта WinMerge составляет 186 000 строк кода, а размер проекта Miranda IM с плагинами около 950 000 строк кода.
Intel Energy Checker SDK
Кстати, пока мы находимся в начале статьи, попробуйте угадать, сколько в этом проекте может быть операторов 'goto'. Загадали число? Если да, то тогда продолжим.

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

IEC SDK вполне можно отнести к проектам, рассматривая текст которых, в голове всплывает слово "говнокод". Не скажу, что код ужасен, бывает намного хуже. Но смотрите сами. Во всем проекте всего 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.

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

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. :-)
Tags:
Hubs:
+62
Comments 116
Comments Comments 116

Articles

Information

Website
pvs-studio.com
Registered
Founded
2008
Employees
31–50 employees