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

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

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

Теперь, посмотрим на ошибку, найденную анализатором PVS-Studio в проекте FreeCAD. Для масштаба, я постарался не сильно сокращать проблемный фрагмент кода:

TechDraw::DrawPage* DrawGuiUtil::findPage(Gui::Command* cmd, bool findAny)
{
  ....
  //check Selection for a page
  std::vector<App::DocumentObject*> selPages = cmd->getSelection().
                     getObjectsOfType(TechDraw::DrawPage::getClassTypeId());
  if (selPages.empty()) 
  {
    //no page in selection, try this document
    auto docPages = cmd->getDocument()
                  ->getObjectsOfType(TechDraw::DrawPage::getClassTypeId());
    if (docPages.empty()) 
    {
        //we are only to look in this document, and there is no ....
        QMessageBox::warning(Gui::getMainWindow(), QObject::tr("No page found"),
                    QObject::tr("No Drawing Pages in document."));
      return nullptr;
    }
    if (docPages.size() > 1) 
    {
      //multiple pages in document, use active page if there is one
      Gui::MainWindow* w = Gui::getMainWindow();
      Gui::MDIView* mv = w->activeWindow();
      MDIViewPage* mvp = dynamic_cast<MDIViewPage*>(mv);
      if (mvp) 
      {
        QGSPage* qp = mvp->getViewProviderPage()->getQGSPage();
        return qp->getDrawPage();
      }
      else 
      {
        // none of pages in document is active, ask for help
        for (auto obj : selPages) // <=
        {
          ....
        }
        ....
      }
      ....
    }
    ....
  }
  ....
}

PVS-Studio выдал на этом коде следующее предупреждение:

V1078: An empty container is iterated. The loop will not be executed.

Итак, у нас есть контейнер selPages, который инициализируется функцией getObjectsOfType. Он может быть пустым, о чём нам говорит проверка selPages.empty(). Если условие истинно, в одном из вложенных условий происходит попытка обойти данный контейнер. Однако его размер к этому моменту не меняется.

Посмотрев на такой фрагмент кода, становится очень интересно то, каким он был до коммита. Вот ссылка на изменения.

Программист теперь не модифицирует контейнер selPages, а сохраняет результат в новую переменную docPages. Затем он переписал код, используя везде новую переменную, но забыл исправить цикл.

Таким образом, ошибка могла бы не просочиться в репозиторий при регулярном использовании статического анализа перед коммитом. Даже если вы что-то упустили во время code review, анализатор подсветит вам проблемное место.

P.S.: Разработчики исправили ошибку, спустя пару часов после публикации статьи :)

Предыдущие публикации:

1. Как PVS-Studio защищает от поспешных правок кода, пример N5.

2. Как PVS-Studio защищает от поспешных правок кода, пример N4.

3. Как PVS-Studio защищает от поспешных правок кода, пример N3.

4. Как PVS-Studio защищает от поспешных правок кода, пример N2.

5. Как PVS-Studio защищает от поспешных правок кода.