Документуємо помилки в Doxygen



У цій статті мова піде про перевірку системи документування doxygen. Цей відомий і широко використовуваний проект, який має підстави заявою розробників, що став фактично стандартом для документування програмного забезпечення, написаного на мові C++, ще не був перевірений PVS-Studio. Doxygen переглядає вихідний код програми і генерує з нього документацію. Настав час нам заглянути в його вихідні коди і подивитися, що зможе знайти PVS-Studio.


ВведенняDoxygen — платформна система документування вихідних текстів, яка має широкий набір цільових мов: C++, C, Objective-C, Python, Java, C#, PHP, IDL, Fortran, VHDL і частково підтримує D. Doxygen генерує документацію на основі вихідних текстів із коментарями особливого виду і може бути налаштований на витяг структури програми з недокументированными джерела. Вихідними форматами можуть бути HTML, LATEX, man, rtf, xml. Doxygen використовується в проектах KDE, Mozilla, Drupal, Pidgin, AbiWorld, FOX toolkit, Torque Game Engine, Crystal Space.

Підготовка та перевірканайсвіжіші исходники doxygen можуть бути взяті з github.com/doxygen/doxygen. Спочатку репозиторій не містить проектних файлів для Visual Studio, але так як розробники використовують cmake, то їх можна згенерувати. Я скористався консольної версією програми та командою «cmake-G „Visual Studio 12“» для генерації проектного файлу для VS 2013. Для запуску перевірки досить натиснути Check Solution у вкладці PVS-Studio, Visual Studio.

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

Попередження PVS-Studio: V519 The '* outListType1' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 8326, 8327. util.cpp 8327
void convertProtectionLevel(MemberListType inListType,
int *outListType1,
int *outListType2)
{
static bool extractPrivate;
....
switch (inListType)
{
....
case MemberListType_priSlots:
if (extractPrivate)
{
*outListType1=MemberListType_pubSlots;
*outListType1=MemberListType_proSlots;<<<<====
}
else
{
*outListType1=-1;
*outListType2=-1;
}
break;
....
}
}

У тілі if міститься подвійне призначення однієї і тієї ж змінної. Очевидно, що це помилка або помилка при копіюванні. Блок else підказує, що значення «MemberListType_proSlots» повинно бути записано в "*outListType2". Ще одну подібну помилку можна знайти тут: doxygen.cpp 5742 (див. змінну'da->type').

Наступне попередження PVS-Studio: V519 The 'pageTitle' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 970, 971. vhdldocgen.cpp 971
QCString VhdlDocGen::getClassTitle(const ClassDef *cd)
{
QCString pageTitle;
if (cd == 0) 
return "";
pageTitle += cd->displayName();
pageTitle = VhdlDocGen::getClassName(cd);
....
}

Зверніть увагу на операцію присвоювання. Швидше за все, це помилка і замість "=" має використовуватися "+=". До питання про стиль, у вихідному коді ця функція не містить пробілів між операторами і значеннями, що істотно погіршує читаність коду. Це збільшує шанс виникнення помилки, так як важко побачити відсутність "+" серед безперервного потоку символів. Після додавання прогалин помилка стає більш помітною. Ще одна схожа помилка ховається тут:

V519 The 'nn' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 2166, 2167. vhdldocgen.cpp 2167

Перейдемо до наступного попередження.

Попередження PVS-Studio: V523 The 'then' statement is equivalent to the 'else' statement. docparser.cpp 521
static void checkUndocumentedParams()
{
....
if(g_memberDef->inheritsDocsFrom())
{
warn_doc_error(g_memberDef->getDefFileName(),
g_memberDef->getDefLine(),
substitute(errMsg,"%","%%"));
}
else
{
warn_doc_error(g_memberDef->getDefFileName(),
g_memberDef->getDefLine(),
substitute(errMsg,"%","%%"));
}
....
}

Метод програмування copy-paste може не тільки зекономити час при написанні коду, але і привнести в нього помилки. Тут код з блоку if був скопійований для використання в блоці else, але не змінений після вставки. При кожному використанні copy-paste слід дотримуватися правила «Один раз скопіюй, сім разів перевір».

Попередження PVS-Studio: V523 The 'then' statement is equivalent to the 'else' statement. translator_tw.h 769
class TranslatorChinesetraditional : public Translator
{
public:
....
virtual QCString trGeneratedFromFiles(bool single,....)
{ 
....
QCString result=(QCString)"?";
....
if (single) result+=":"; else result+=":";
....
}
....
}

Аналогічне попередньому попередження. У блоці if в незалежності від умови до рядку result додається один і той же символ. Навряд чи це поведінка, адже тоді саме умова не має сенсу. І знову я схиляюся до того, що якщо б, дотримуючись загальноприйнятого стилю, цей блок був рознесений на 4 рядки, то він не просто би виглядав красивіше, але і зробив опечатку більш очевидною. Цікаво, що ця конструкція була двічі скопійована для подальшого використання у функціях, але помилка так і не була помічена. У підсумку ще два попередження припадають на ці рядки:
  • V523 The 'then' statement is equivalent to the 'else' statement. translator_tw.h 1956
  • V523 The 'then' statement is equivalent to the 'else' statement. translator_tw.h 1965
Попередження PVS-Studio: V530 The return value of function 'toupper' is required to be utilized. classdef.cpp 1963
void ClassDef::writeDocumentationContents(....)
{
QCString pageType = " ";
pageType += compoundTypeString();
toupper(pageType.at(1));
....
}

Тут програмістом був невірно зрозумілий принцип роботи функції toupper. Можливо, він очікував, що функція змінить переданий символ на заголовну літеру. Насправді функція повертає заголовний еквівалент символу-аргумент, а не змінює його. Ось як функція оголошена toupper в заголовочном файлі «ctype.h».
int toupper (int__c);

Вже з оголошення функції видно, що параметр приймається за значенням, а значить переданий символ не може бути змінений. Щоб не допускати таких помилок, досить уважно читати опис до використовуваних функцій, у поведінці яких ви не впевнені.

Попередження PVS-Studio:
V560 A part of conditional expression is always false: (flags() &!0x0008). qfile_win32.cpp 267
#define IO_Truncate 0x0008 

bool QFile::open(....)
{
....
int length = INT_MAX;
if ((flags() & !IO_Truncate) && length == 0 && isReadable())
....
}

Дана умова завжди буде хибним з-за того, що логічне заперечення ненульового значення завжди дає нуль. Подальше застосування «І» не має значення, коли один з аргументів є нулем. В результаті умова не залежить від інших параметрів. Більш логічним тут здається використання оператора побітовою інверсії '~'.

Попередження PVS-Studio: V560 A part of conditional expression is always true: !found. util.cpp 4264
bool getDefs (....)
{
....
bool found=FALSE;
MemberListIterator mmli(*mn);
MemberDef *mmd;
for(mmli.toFirst();((mmd=mmli.current()) && !found);++mmli)
{
....
}
....
}

Відразу скажу, що в тілі циклу for змінна found не змінюється. З-за цього умова виходу з циклу залежить тільки від результату методу mmli.current. Це загрожує тим, що цикл завжди буде проходити від початку до кінця в незалежності від того, чи було знайдено шукане значення чи ні.

Попередження PVS-Studio:
V595 The 'bfd' pointer was utilized before it was verified against nullptr. Check lines: 3371, 3384. dot.cpp 3371
void DotInclDepGraph::buildGraph(....)
{
....
FileDef *bfd = ii->fileDef;
QCString url="";
....
url=bfd->getSourceFileBase();
....
if (bfd)
.... 
}

V595 є, мабуть, найпоширенішим попередженням серед усіх проектів. Не завжди перед використанням покажчика замислюєшся, чи може він бути нульовим. Деколи про це згадуєш після другого або третього використання і ставиш перевірку. Але між нею і першим разыменованием може бути дуже велика кількість коду, що робить помилку досить важкою для виявлення. Аналогічні попередження:
  • V595 The 'cd' pointer was utilized before it was verified against nullptr.
    Check lines: 6123, 6131. doxygen.cpp 6123
  • V595 The 'p' pointer was utilized before it was verified against nullptr.
    Check lines: 1069, 1070. htmldocvisitor.cpp 1069
  • V595 The 'Doxygen::mainPage' pointer was utilized before it was verified against nullptr.
    Check lines: 3792, 3798. index.cpp 3792
  • V595 The 'firstMd' pointer was utilized before it was verified against nullptr.
    Check lines: 80, 93. membergroup.cpp 80
  • V595 The 'lastCompound' pointer was utilized before it was verified against nullptr.
    Check lines: 410, 420. vhdljjparser.cpp 410
  • V595 The 'len' pointer was utilized before it was verified against nullptr.
    Check lines: 11960, 11969. qstring.cpp 11960
  • V595 The 'len' pointer was utilized before it was verified against nullptr.
    Check lines: 11979, 11988. qstring.cpp 11979
  • V595 The 'fd' pointer was utilized before it was verified against nullptr.
    Check lines: 2077, 2085. doxygen.cpp 2077
Попередження PVS-Studio:
V595 The 'lne' pointer was utilized before it was verified against nullptr. Check lines: 4078, 4089. index.cpp 4078
static void writeIndexHierarchyEntries(OutputList &ol,....)
{
QListIterator<LayoutNavEntry> li(entries);
LayoutNavEntry *lne;
for(li.toFirst();(lne=li.current());++li)
{
LayoutNavEntry::Kind kind = lne->kind();
....
bool addToIndex=lne==0 || lne->visible();
....
}
}

Я не описую однотипні перевірки, тому що вони занадто нудні для статті. Тим не менше я хочу розглянути ще одне попередження V595. Тут вхід в цикл здійснюється, лише якщо обчислене значення li.current() (яке присвоюється вказівником lne) буде не NULL. Це означає, що всередині циклу покажчик гарантовано не буде нульовим, роблячи подальшу перевірку непотрібною. Мені захотілося виписати цей приклад, тому що зазвичай попередження V595 вказує на потенційне розіменування нульового покажчика, а тут воно показало зайву перевірку.

Попередження аналізатора: V601 The bool type is implicitly cast to the class type. docsets.cpp 473
struct IncludeInfo
{
....
bool local;
};

void DocSets::addIndexItem(Definition *context,MemberDef *md,
const char *,const char *)
{
QCString decl;
....
IncludeInfo *ii = cd->includeInfo();
....
decl=ii->local;
....
}

Аналізатор звернув увагу на дивне приведення до типу bool класу. У класі QCString не визначено перевантажений оператор присвоювання для аргументу типу bool, але визначений конструктор з вхідним параметром типу int, який позначає розмір рядка. Саме він викликається для створення тимчасового об'єкта при даному присвоювання. Компілятор знайде конструктор з аргументом int і викличе його, попередньо перетворивши bool до int. Змінна local може мати лише 2 значення: true або false, що відповідає 1 і 0. У першому випадку конструктором буде створена рядок з одного елемента. У другому випадку вийде порожній рядок. В кінці буде викликаний оператор присвоювання з аргументом типу QCString. Схожий, але менш очевидне приведення здійснюється в наступних місцях:
  • V601 The bool type is implicitly cast to the class type. Inspect the fifth argument. context.cpp 2315
  • V601 The bool type is implicitly cast to the class type. Inspect the fifth argument. context.cpp 2675
  • V601 The bool type is implicitly cast to the class type. Inspect the fifth argument. context.cpp 4456
Попередження PVS-Studio: V614 Potentially uninitialized pointer 't' used. vhdlparser.cc 4127
QCString VhdlParser::extended_identifier()
{
Token *t;
if (!hasError)
t = jj_consume_token(EXTENDED_CHARACTER);
return t->image.c_str();
assert(false);
}

У цьому ділянці коду можливо розіменування неинициализированного покажчика. Реальний код погано оформлений, тому ця помилка і залишилася непоміченою. Для статті я відформатував код і помилка стала відразу добре помітна. Ще дві такі помилки можна знайти тут:
  • V614 Potentially uninitialized pointer 'tmpEntry' used. vhdlparser.cc 4451
  • V614 Potentially uninitialized pointer 't' used. vhdlparser.cc 5304
Попередження PVS-Studio: V668 There is no sense testing in the 'file' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. outputgen.cpp 47
void OutputGenerator::startPlainFile(const char *name)
{
....
file = new QFile(fileName);
if (!file)
....
}

Ні для кого вже не новина, що new у разі невдалого виділення пам'яті викидає виключення, а не повертає nullptr. Цей код — своєрідний архаїзм програмування. Для сучасних компіляторів ці перевірки більше не мають сенсу, їх можна видалити. Ще 3 таких перевірки:
  • V668 There is no sense testing in the 'expr' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. template.cpp 1981
  • V668 There is no sense testing in the 'n' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. qglist.cpp 1005
  • V668 There is no sense testing in the 'nd' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. qstring.cpp 12099
Попередження PVS-Studio: V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'd' is lost. Consider assigning realloc() to a temporary pointer. qcstring.h 396
class BufStr 
{
public:
....
void resize(uint newlen)
{
....
m_buf = (char *)realloc(m_buf,m_size);
....
}
private:
uint m_size;
char *m_buf;
....
}

Аналізатор виявив неправильне використання функції «realloc». При невдалому виділення пам'яті «realloc» поверне nullptr, перезаписавши попереднє значення покажчика. Щоб запобігти цьому, рекомендується зберігати значення вказівника під тимчасової змінної перед використанням «realloc». Всього, крім цієї, у проекті було знайдено 8 потенційних витоків пам'яті:
  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'd' is lost. Consider assigning realloc() to a temporary pointer. qcstring.h 396
  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'str' is lost. Consider assigning realloc() to a temporary pointer. growbuf.h 16
  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'str' is lost. Consider assigning realloc() to a temporary pointer. growbuf.h 23
  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'str' is lost. Consider assigning realloc() to a temporary pointer. growbuf.h 33
  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'm_str' is lost. Consider assigning realloc() to a temporary pointer. vhdlstring.h 61
  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'shd->data' is lost. Consider assigning realloc() to a temporary pointer. qgarray.cpp 224
  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'm_data' is lost. Consider assigning realloc() to a temporary pointer. qgstring.cpp 114
  • V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'm_data' is lost. Consider assigning realloc() to a temporary pointer. qgstring.cpp 145
ВисновокУ висновку хочеться сказати, що аналізатор чудово впорався зі своєю роботою і знайшов багато підозрілих місць, незважаючи на те, що doxygen є популярним проектом і широко використовується великою кількістю як дрібних, так і великими компаніями. Я навів основні попередження і залишив такі нецікаві, як зайві перевірки, використовувані змінні і т. п. за рамками цієї статті. Варто згадати, що в деяких місцях мене здивувало дуже недбале, на мою думку, оформлення коду.

Хочеться побажати читачам писати гарний, читабельний код і не допускати помилок. І якщо перше цілком залежить від програміста, то з другим може допомогти аналізатор. Завантажити безкоштовно і безкоштовно спробувати PVS-Studio на своєму проекті можна тут: http://www.viva64.com/ru/pvs-studio-download/


Якщо хочете поділитися цією статтею з англомовної аудиторією, то прошу використовувати посилання на переклад: Igor Shtukarev. Documenting Bugs in Doxygen.

Прочитали статтю і є питання?Часто до наших статей задають одні і ті ж питання. Відповіді на них ми зібрали тут: Відповіді на питання читачів статей про PVS-Studio, версія 2015. Будь ласка, ознайомтеся зі списком.


Джерело: Хабрахабр

0 коментарів

Тільки зареєстровані та авторизовані користувачі можуть залишати коментарі.