FreeCAD и C++ код с неопределённым поведением для медитации

Изучая код проекта с помощью статического анализатора, иногда задаёшься вопросом: "Как возникла ошибка и почему её до сих пор не заметили?" Хотите посмотреть пример? Тогда приглашаем познакомиться с этой статьёй.

Противоестественное

Перед вами классическая статья про проверку открытого проекта с помощью статического анализатора кода PVS-Studio. Мы регулярно публикуем такие статьи для популяризации методологии статического анализа кода. Заодно в проверенных проектах становится меньше багов.

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

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

Для полной ясности предлагаю вот такую аналогию.

Предупреждения анализатора и предупреждения компилятора – это, по сути, одно и то же. Просто профессиональные анализаторы выполняют больше проверок кода и используют различные технологии для выявления сложных паттернов ошибок.

Глупо разрабатывать проект с отключёнными предупреждениями компилятора. Если вы будете включать предупреждения раз в пару месяцев, это будет неэффективно:

1. Вы сразу потонете в большом количестве предупреждений и быстро устанете их изучать. В результате вы можете посчитать ложными некоторые вполне полезные предупреждения.
2. Часть ошибок вы уже исправили в процессе отладки. А могли бы исправить ещё на этапе компиляции кода, если бы предупреждения были включены.
3. С некоторыми предупреждениями будет непонятно, что делать. Например, они указывают не на ошибки, а на избыточный код. Заниматься большим рефакторингом, чтобы поправить все такие места — плохая идея. Оставить всё как есть и вновь рассматривать эти участки кода при следующем анализе кода — плохая идея. В общем, лучше не доводить до такой ситуации, а рефакторить код постепенно.

Вот то же самое и с предупреждениями статических анализаторов кода. Эти инструменты нужно запускать не от случая к случаю, а внедрять в процесс разработки.

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

Проект FreeCAD

FreeCAD — параметрическая САПР общего назначения с открытым исходным кодом, написанная в основном на языках C, C++ и Python. PVS-Studio пока не поддерживает Python, так что будет проверяться только C и C++ часть. Ссылка на код проекта.

Это не первая наша статья про проверку проекта FreeCAD. Предыдущую мы писали в 2015 году. 8 лет назад — это так давно. Можно было и не вспоминать, но, если что, вот та публикация.

Давайте посмотрим ту самую особую ошибку, которую я обещал в аннотации.

Опечатка и неопределённое поведение

QGVPage* QGIView::getQGVPage(TechDraw::DrawView* dView)
{
  ViewProviderPage* vpp = getViewProviderPage(dView);
  if (!vpp) {
    return vpp->getQGVPage();
  }
  return nullptr;
}

Предупреждение PVS-Studio: V522 [CWE-476, CERT-EXP34-C] Dereferencing of the null pointer 'vpp' might take place. QGIView.cpp 592

Автор кода опечатался при написании условия:

• Если указатель ненулевой, то функция ничего не делает и возвращает nullptr.
• Если указатель нулевой, то произойдёт его разыменование. Возникнет неопределённое поведение.

Всё просто и понятно. Однако меня этот случай заинтересовал сразу по нескольким причинам.

Первый повод для медитации. В своей предыдущей статье я как раз рассматривал почти идентичную ошибку! Вот эта статья: "Ошибка настолько проста, что программисты её не замечают". Код оттуда:

if (!SomeGlobalPointer)
{
  delete[] SomeGlobalPointer;
}

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

P.S. Ладно, это я для красного словца. Нет ничего в этом необычного. Причина, по которой на меня это произвело впечатление, это срабатывание феномена Баадера — Майнхофа. Если что-то дважды повторилось в непродолжительный период времени, то кажется, что это везде.

Второй повод для медитации. Интересно, как вообще появилась такая ошибка! Дело в том, что по соседству есть функция-близнец. В ней условие правильное! Посмотрите сами:

QGVPage* QGIView::getQGVPage(TechDraw::DrawView* dView)
{
  ViewProviderPage* vpp = getViewProviderPage(dView);
  if (!vpp) {
    return vpp->getQGVPage();
  }
  return nullptr;
}

QGSPage* QGIView::getQGSPage(TechDraw::DrawView* dView)
{
  ViewProviderPage* vpp = getViewProviderPage(dView);
  if (vpp) {
    return vpp->getQGSPage();
  }
  return nullptr;
}

Непонятно, как так получилось.

С большой вероятностью вторая функция была написана с помощью copy-paste. Но не сходится… Странно, что во второй функции ошибку поправили, а в первой она осталась. Не верится в такой сценарий.

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

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

Во-вторых, эти функции используются одинаково часто и иногда в паре. Странно заметить ошибку в поведении одной функции и не заметить в другой.

Загадка!

Третий повод для медитации. Непонятно, как будет проявляться такая ошибка. Если указатель нулевой, его разыменование приведёт к неопределённому поведению. Причем благодаря условию компилятор знает, что указатель нулевой. Интересно, как он поступит и какой код сгенерирует.

Я знаю, что нет смысла пытаться предсказать поведение компилятора и то, как проявит себя неопределённое поведение. Любые предположения будут неверны. Хорошая статья на эту тему: "Ложные представления программистов о неопределённом поведении".

Тем не менее, мне стало интересно, смогу ли я создать аналогичный код, который из-за неопределённого поведения будет работать как правильный. Ну или по крайней мере не будет падать. Это смогло бы частично объяснить, почему эта ошибка до сих пор не замечена.

Если честно, у меня не очень получилось. Всё очень нестабильно. Малейшее изменение кода сильно влияет на его поведение.

Тем не менее, вполне реальна ситуация, что падения не будет. Например, я могу показать такой код.

Скомпилированный в режиме без оптимизации (-O0), он аварийно завершается:

Program terminated with signal: SIGSEGV

Если его собрать с оптимизацией (-O2), то он распечатывает мусор:

1033569176

Зато не падает. В любом случае, было интересно поэкспериментировать. Если хотите, можете продолжить эксперименты и поделиться своими наблюдениями.

Другие опечатки

Классическая опечатка: повторный вызов функции hasText

void SheetTableView::contextMenuEvent(QContextMenuEvent*)
{
  const QMimeData* mimeData = QApplication::clipboard()->mimeData();
  ....
  actionPaste->setEnabled(
    mimeData && (mimeData->hasText() || mimeData->hasText()));
  ....
}

Предупреждение PVS-Studio: V501 [CWE-570] There are identical sub-expressions 'mimeData->hasText()' to the left and to the right of the '||' operator. SheetTableView.cpp 1115

Дважды проверятся, что объект может возвращать простой текст. Как должно быть написано правильно, точно сказать не могу. Рискну предположить, что вместо одного из вызовов функции hasText должна вызываться функция hasHtml.

Опечатка в последней строке

void Document::handleChildren3D(ViewProvider* viewProvider, bool deleting)
{
  ....
  std::vector<App::DocumentObject*> children = viewProvider->claimChildren3D();

  SoGroup* childGroup = viewProvider->getChildRoot();
  SoGroup* frontGroup = viewProvider->getFrontRoot();
  SoGroup* backGroup = viewProvider->getFrontRoot();

  if (deleting ||
      childGroup->getNumChildren() != static_cast<int>(children.size())) {
  ....
}

Анализатор указывает на аномалию в коде сразу двумя предупреждениями:

• V525 [CWE-682] The code contains the collection of similar blocks. Check items 'getChildRoot', 'getFrontRoot', 'getFrontRoot' in lines 2404, 2405, 2406. Document.cpp 2404
• V656 [CWE-665] Variables 'frontGroup', 'backGroup' are initialized through the call to the same function. It's probably an error or un-optimized code. Consider inspecting the 'viewProvider->getFrontRoot()' expression. Check lines: 2405, 2406. Document.cpp 2406

Думаю, перед нами проявление эффекта последней строки: ошибка, скорее всего, будет допущена в конце однотипных строчек/блоков кода.

В коде есть три очень похожих строчки кода. Скорее всего, этот фрагмент кода писался с помощью copy-paste. В какой-то момент была продублирована вторая строчка:

SoGroup* frontGroup = viewProvider->getFrontRoot();

В скопированной строке нужно было заменить "front" на "back". Таких замен в строке нужно было сделать две, но сделали только одну.

SoGroup* backGroup = viewProvider->getFrontRoot(); // неправильно
SoGroup* backGroup = viewProvider->getBackRoot();  // правильно

Такие опечатки тяжело находить на обзорах кода, и статический анализ может стать очень хорошим помощником.

Время Фредди

При чём здесь Фредди? Это отсылка к статье "Ноль, один, два, Фредди заберёт тебя". Опечатки притягиваются к коду, когда имена переменных содержат 0, 1 или 2. Это как раз такой случай.

TopoDS_Edge GeometryUtils::asCircle(TopoDS_Edge occEdge, bool& arc) { .... }

bool GeometryMatcher::compareBSplines(TopoDS_Edge &edge1, TopoDS_Edge &edge2)
{
  ....
  BRepAdaptor_Curve adapt1(edge1);
  BRepAdaptor_Curve adapt2(edge2);
  bool isArc1(false);
  bool isArc2(false);
  TopoDS_Edge circleEdge1;
  TopoDS_Edge circleEdge2;
  try {
    circleEdge1 = GeometryUtils::asCircle(edge1, isArc1);
    circleEdge2 = GeometryUtils::asCircle(edge2, isArc1);
  }
  catch (Base::RuntimeError&) {
    Base::Console().Error(....);
    return false;
  }
  if (!isArc1 && !isArc2) {
    return compareCircles(circleEdge1, circleEdge2);
  }
  if (isArc1 && isArc2) {
    return compareCircleArcs(circleEdge1, circleEdge2);
  }

  return false;
}

Предупреждение PVS-Studio: V537 [CWE-682] Consider reviewing the correctness of 'isArc1' item's usage. GeometryMatcher.cpp 242

Большой кусок кода. Но ничего страшного, сейчас мы его постепенно разберём. Начнём с функции asCircle.

TopoDS_Edge GeometryUtils::asCircle(TopoDS_Edge occEdge, bool& arc) { .... }

Обратите внимание, что она принимает второй аргумент по ссылке, чтобы вернуть через него информацию о типе ребра.

Теперь обратите внимание на этот код:

bool isArc1(false);
bool isArc2(false);
....
circleEdge1 = GeometryUtils::asCircle(edge1, isArc1);
circleEdge2 = GeometryUtils::asCircle(edge2, isArc1);

Вызов функций asCircle должен был инициализировать 4 переменные:

1. circleEdge1
2. isArc1
3. circleEdge2
4. isArc2

Из-за опечатки во второй строке повторно используется имя переменной isArc1. В результате переменная isArc1 инициализируется не тем значением, которым надо, а переменная isArc2 остаётся всегда равна false.

Соответственно, дальнейшие проверки не имеют смысла (работают не так, как задумано):

if (!isArc1 && !isArc2) {
....
if (isArc1 && isArc2) {

Нулевые указатели

Тест на вашу внимательность

Попробуйте самостоятельно найти ошибку в этом коде. Just for fun.

void QGIViewPart::highlightMoved(QGIHighlight* highlight, QPointF newPos)
{
  std::string highlightName = highlight->getFeatureName();
  App::Document* doc = getViewObject()->getDocument();
  App::DocumentObject* docObj = doc->getObject(highlightName.c_str());
  auto detail = dynamic_cast<DrawViewDetail*>(docObj);
  auto oldAnchor = detail->AnchorPoint.getValue();
  if (detail) {
    Base::Vector3d delta = Rez::appX(DrawUtil::toVector3d(newPos)) /
                           getViewObject()->getScale();
    delta = DrawUtil::invertY(delta);
    detail->AnchorPoint.setValue(oldAnchor + delta);
  }
}

Картинка, чтобы вы сразу на ответ не смотрели. Наш единорог пока подождёт вас.

На самом деле ошибка не очень сложная, и, думаю, вы её нашли. Другое дело, что это весьма скучное занятие. И поэтому такой "монолитный" код тяжело поддаётся проверке на обзорах кода. Суть — указатель detail разыменовывается до момента его проверки.

auto oldAnchor = detail->AnchorPoint.getValue();
if (detail) {

Бывает, что о некоторых ошибках анализатор сообщает двумя способами. Перед нами как раз такой случай:

1. V595 [CWE-476, CERT-EXP12-C] The 'detail' pointer was utilized before it was verified against nullptr. Check lines: 842, 843. QGIViewPart.cpp 842. Собственно, из самого названия диагностики понятно, почему оно выдано здесь. Если нет, то вот пояснение про диагностику V595.
2. V522 [CWE-690, CERT-MEM52-CPP] There might be dereferencing of a potential null pointer 'detail'. QGIViewPart.cpp 842. Здесь логика следующая. Оператор dynamic_cast может вернуть нулевой указатель. Следовательно, его опасно разыменовывать без предварительной проверки.

Чтобы исправить ошибку, достаточно перенести объявление и инициализацию переменной oldAnchor внутрь условного оператора. Корректный код:

if (detail) {
  auto oldAnchor = detail->AnchorPoint.getValue();
  Base::Vector3d delta = Rez::appX(DrawUtil::toVector3d(newPos)) /
                         getViewObject()->getScale();
  delta = DrawUtil::invertY(delta);
  detail->AnchorPoint.setValue(oldAnchor + delta);
}

Опасный конструктор

TaskTransformedParameters::TaskTransformedParameters(
  ViewProviderTransformed *TransformedView, QWidget *parent)
    : TaskBox(...., TransformedView->menuName, true, parent)    // <=
    , proxy(nullptr)
    , TransformedView(TransformedView)
    , parentTask(nullptr)
    , insideMultiTransform(false)
    , blockUpdate(false)
{
  selectionMode = none;

  if (TransformedView) {                                        // <=
     Gui::Document* doc = TransformedView->getDocument();
     this->attachDocument(doc);
  }
  ....
}

Предупреждение PVS-Studio: V664 [CWE-476, CERT-EXP34-C] The 'TransformedView' pointer is being dereferenced on the initialization list before it is verified against null inside the body of the constructor function. Check lines: 57, 66. TaskTransformedParameters.cpp 57

В конструктор передаётся некий указатель TransformedView. Он разыменовывается при конструировании базового класса:

: TaskBox(...., TransformedView->menuName, ....)

Это опасно, так как этот указатель может оказаться нулевым. Конечно, анализатор не считает каждое разыменование непроверенного указателя опасным. Будет слишком много ложных срабатываний (пояснение). Он делает вывод, что указатель бывает равен нулю, так как видит в теле конструктора эту проверку:

if (TransformedView)

Проверяется не тот указатель

void SectionCut::startCutting(bool isInitial)
{
  ....
  App::PropertyLinkList* CutLinkList =
    dynamic_cast<App::PropertyLinkList*>(
      CutCompoundBF->getPropertyByName("Objects"));

  if (!CutCompoundBF) {
    Base::Console().Error((std::string("SectionCut error: ")
                           + std::string(CompoundName)
                           + std::string(" could not be added\n")).c_str());
    return;
  }
  CutLinkList->setValue(ObjectsListLinks);
  ....
}

Здесь вновь про аномалии анализатор уведомляет двумя разными предупреждениями:

• V595 [CWE-476, CERT-EXP12-C] The 'CutCompoundBF' pointer was utilized before it was verified against nullptr. Check lines: 690, 691. SectionCutting.cpp 690
• V1051 [CWE-754] Consider checking for misprints. It's possible that the 'CutLinkList' should be checked here. SectionCutting.cpp 691

Первое предупреждение косвенное. Оно указывает на эти строки кода:

.... CutCompoundBF->getPropertyByName("Objects"));
if (!CutCompoundBF) {

Странно в начале разыменовывать указатель, а затем его проверять на равенство nullptr.

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

И действительно, в условии должна проверяться переменная CutLinkList. Тогда аномалии исчезнут. Корректный код:

App::PropertyLinkList* CutLinkList =
  dynamic_cast<App::PropertyLinkList*>(
    CutCompoundBF->getPropertyByName("Objects"));

if (!CutLinkList) {
  Base::Console().Error(....);
  return;
}
CutLinkList->setValue(ObjectsListLinks);

Неэффективные участки кода

Для оптимизации скорости работы программы следует использовать профилировщики. Статические анализаторы не могут предсказать, какой код будет тратить больше всего процессорного времени. Это зависит от входных данных, а статические анализаторы ничего не знают о них.

Однако некоторые микро-оптимизации в коде можно сделать и с помощью статического анализатора. Нет гарантии, что эти оптимизации приведут к какому-то заметному ускорению работы программы. Однако и хуже тоже не будет.

Вот несколько примеров того, что такое предупреждение на тему микро-оптимизаций.

Например, в проекте FreeCAD много лишних преобразований строк. Одно из них:

void TaskAttacher::onRefName(const QString& text, unsigned idx)
{
  std::string subElement;
  ....
  std::vector<std::string> refnames = pcAttach->Support.getSubValues();
  ....
  refnames[idx] = subElement.c_str();
  ....
}

Предупреждение PVS-Studio: V811 Decreased performance. Excessive type casting: string -> char * -> string. Consider inspecting the 'subElement.c_str()' expression. TaskAttacher.cpp 672

Неэффективно создавать копию строки std::string из C-строки (нуль-терминированной строки). Приходится делать дополнительный проход по C-строке, чтобы узнать её длину и выделить нужный буфер памяти. Хотя информация о длине есть в subElement.

В общем, можно написать более короткий код, который будет работать быстрее:

refnames[idx] = subElement;

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

Пример:

void TaskFilletParameters::apply()
{
  std::string name = getDressUpView()->getObject()->getNameInDocument();

  ui->filletRadius->apply();

  if(ui->listWidgetReferences->count() == 0)
    Base::Console().Warning(
      tr("Empty fillet created !\n").toStdString().c_str());
}

Предупреждение PVS-Studio: V808 'name' object of 'basic_string' type was created but was not utilized. TaskFilletParameters.cpp 186

Переменная name явно лишняя. Однако нет гарантии, что компилятор удалит её в процессе оптимизации. Строки — это сложные классы, выделяющие память. Выделение памяти — это побочный эффект, меняющий внешнее состояние. Компилятору сложно понять, лишняя вся эта сложная конструкция или нет.

Начиная с C++14, компилятор может выполнить такую оптимизацию и удалить переменную. Но это необязательная к исполнению оптимизация. Современные компиляторы её делают, однако всё зависит от сложности кода.

В любом случае нет смысла загромождать код программы лишними строками.

Другой пример:

void DlgRevertToBackupConfigImp::showEvent(QShowEvent* event)
{
  ....
  for (const auto& backup : backups) {
    auto filename = backup.filename().string();
    auto modification_date =
      QDateTime::fromSecsSinceEpoch(fs::last_write_time(backup));
    auto item = new QListWidgetItem(QLocale().toString(modification_date));
    item->setData(Qt::UserRole, QString::fromStdString(backup.string()));
    ui->listWidget->addItem(item);
  }
  ....
}

Предупреждение PVS-Studio: V808 'filename' object of 'basic_string' type was created but was not utilized. DlgRevertToBackupConfigImp.cpp 79

Здесь переменная filename бессмысленно многократно создаётся и уничтожается в цикле.

P.S. Кстати, иногда изучая код с такими "потеряшками", вдруг выясняется, что код содержит ошибку, и на самом деле переменная должна была использоваться. Не могу сказать, что это частая ситуация, но такое бывает.

Другие ошибки и опасные участки кода

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

Как я говорил в начале статьи, намного полезнее не просто выполнять разовые проверки, а интегрировать анализ в процесс разработки. Препятствием к этому может казаться большое количество предупреждений на начальном этапе. На самом деле никакой проблемы нет. Всё уже давно продумано. Приглашаю познакомиться с заметкой "Как внедрить статический анализатор кода в legacy проект и не демотивировать команду".

Итак, разберём ещё несколько предупреждений.

Выравнивание: ходьба по тонкому льду

template <int N>
void TRational<N>::ConvertTo (double& rdValue) const
{
  assert(m_kDenom != 0);
  if (m_kNumer == 0)
  {
    rdValue = 0.0;
    return;
  }

  unsigned int auiResult[2];
  ....
  rdValue = *(double*)auiResult;
  ....
}

Предупреждение PVS-Studio: V1032 [CWE-843, CERT-EXP36-C] The pointer 'auiResult' is cast to a more strictly aligned pointer type. Wm4TRational.inl 742

Массив из двух элементов типа unsigned int используется как одна переменная типа double. Тип unsigned int выравнивается по границе 4 байта, а double по границе 8 байт. Возникнет неопределённое поведение, если массив окажется выравнен не по границе 8 байт.

Рассматриваемый код нарушает правило strict aliasing, что влечёт неопределённое поведение. Впрочем, такой код может работать благодаря везению. Поскольку перед массивом нет других переменных, высока вероятность, что он будет находиться в стеке, выравненный по 8 байт. Поэтому всё будет работать, как и ожидал программист. По крайней мере, до поры до времени.

Однако такую хрупкую работоспособность легко сломать. Достаточно объявить другую переменную до массива.

template <int N>
void TRational<N>::ConvertTo (double& rdValue) const
{
  assert(m_kDenom != 0);
  if (m_kNumer == 0)
  {
    rdValue = 0.0;
    return;
  }

  int fooooo;                // Добавили переменную
  unsigned int auiResult[2]; // Выравнивание не кратно 8 байтам
  ....
  rdValue = *(double*)auiResult;
  ....
}

Если тема заинтересовала, почитайте на ночь этот страшный и поучительный рассказ: "A bug story: data alignment on x86".

Хорошо, а как же тогда правильно делать?

До C++20 – используйте memcpy и не бойтесь, компилятор отлично всё оптимизирует. Начиная с C++20 – используйте std::bit_cast. Ещё подписывайтесь на наши статьи, чтобы узнавать про такие полезные вещи :).

Ненадёжная проверка индекса массива

Для начала рассмотрим вспомогательную функцию getIndexFromName.

int DrawUtil::getIndexFromName(const std::string& geomName)
{
  ....
  if (boost::regex_search(begin, end, what, re, flags)) {
    return int(std::stoi(what.str()));
  } else {
    ErrorMsg << "getIndexFromName: malformed geometry name - " << geomName;
    throw Base::ValueError(ErrorMsg.str());
  }
}

Функция преобразовывает фрагмент строки в число (индекс). Теперь перейдём к коду, использующему эту функцию.

TechDraw::VertexPtr DrawViewPart::getVertex(std::string vertexName) const
{
  const std::vector<TechDraw::VertexPtr>
    allVertex(DrawViewPart::getVertexGeometry());

  size_t iTarget = DrawUtil::getIndexFromName(vertexName);
  if (allVertex.empty()) {
    //should not happen
    throw Base::IndexError("DVP::getVertex - No vertices found.");
  }
  if (iTarget > allVertex.size()) {                                // <=
    //should not happen
    throw Base::IndexError("DVP::getVertex - Vertex not found.");
  }

  return allVertex.at(iTarget);                                    // <=
}

Предупреждение PVS-Studio: V557 [CWE-119, CERT-ARR30-C] Array overrun is possible. The 'iTarget' index is pointing beyond array bound. DrawViewPart.cpp 809

По идее, значения индексов всегда должны быть корректны, но на всякий случай сделана защита. Однако есть небольшая ошибочка. Неправильно обрабатывается краевое значение:

if (iTarget > allVertex.size()) {

Предположим, что в векторе 100 элементов. Тогда максимальный правильный индекс должен быть равен значению 99, а не 100. Правильная проверка должна быть такой:

if (iTarget >= allVertex.size()) {

Опасность зацикливания при чтении файла

inline uint32 readUint32 ( istream &is ) {
  static const int buf_len = sizeof ( uint32 ) ;
  unsigned char buf [ buf_len ] ;
  int rsf = 0 ;
  std::streampos original_pos = is.tellg() ;
  while ( rsf < buf_len && !is.eof() ) {
    is.read ( reinterpret_cast< char * >( buf ) + rsf, buf_len - rsf ) ;
    rsf += static_cast< int >( is.gcount () ) ;
  }
  ....
}

Прежде чем поговорить про ошибку, хочется уделить немного времени вот этой строке:

static const int buf_len = sizeof ( uint32 ) ;

sizeof(uint32) – это константа времени компиляции. Вполне возможно, что разработчик решил дать красивое название этой константе. А ещё переживал, что она будет вычисляться каждый раз при входе в функцию, поэтому объявил ее как static.

Но static может, наоборот, повлечь дополнительные накладные расходы. Константа может быть помещена в секцию данных и вычисляться на рантайме в момент первого вхождения в функцию.

Намного лучше использовать constexpr (C++11):

constexpr size_t buf_len = sizeof ( uint32 );

Тогда у константы будет имя, а лишних действий не будет. Компилятор просто подставит константное значение в нужные места.

Теперь вернёмся к циклу.

while ( rsf < buf_len && !is.eof() )

Предупреждение PVS-Studio: V663 [CWE-834] Infinite loop is possible. The 'cin.eof()' condition is insufficient to break from the loop. Consider adding the 'cin.fail()' function call to the conditional expression. zipheadio.h 84

Если произойдёт какой-то сбой при чтении файла, то цикл может стать вечным. Конец файла ещё не достигнут, но ничего не читается. Для проверки такой ситуации служит функция fail.

Способ исправления кода зависит от того, какое поведение требуется. Если достаточно того, что было прочитано, то условие остановки цикла можно сделать таким:

while ( rsf < buf_len && !is.eof() && !is.fail() ) {

Впрочем, такая запись не очень красивая, и как вариант можно использовать функцию good:

while ( rsf < buf_len && is.good() ) {

Обращаю внимание, что это просто один из способов избежать вечного цикла, если не удаётся прочитать весь файл. Это не значит, что теперь код правильно исправлен.

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

Чтобы взаимодействовать с потоком ввода и понять, что что-то пошло не так при чтении, есть целый набор функций. На сайте cppreference есть даже на эту тему хорошая табличка.

Как видите, есть разные способы того, как реализовать чтение из потока и как обрабатывать ошибки чтения.

Для рассматриваемого кода чтение только части данных не выглядит корректным сценарием. Поэтому можно предложить такое изменение кода:

while (   rsf < buf_len
       && is.read(reinterpret_cast< char * >( buf ) + rsf, buf_len - rsf))
{
  rsf += static_cast< int >( is.gcount () ) ;
}

if (is.fail())
  throw Error("Unable to read file.");

Время проверить свои проекты

Какого-то особого вывода в этот раз у статьи нет. Статический анализ — это полезно. Всё :).

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

Лайфхак. Предлагаем начать знакомство с анализатором, выбрав режим "10 наиболее интересных предупреждений".

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. FreeCAD and undefined behavior in C++ code: meditation for developers.