Microsoft предоставила открытый доступ к исходному коду набора инструментов, который используется в компании для ускорения разработки искусственного интеллекта: Computational Network Toolkit теперь доступен на Github. Разработчикам пришлось создавать собственное нестандартное решение, так как существующие инструменты работали недостаточно быстро.
Давайте посмотрим на результаты анализа исходного кода этого проекта, выполненного нашим статическим анализатором кода.

Введение

Computational Network Toolkit (CNTK) — это набор инструментов для проектирования и проектирования сетей различных типов, которые можно использовать для обнаружения изображений, распознавания речи, анализа текста и многого другого.

PVS-Studio — статический анализатор для обнаружения ошибок в исходном коде программ, написанных на языках C, C++ и C#. Инструмент PVS-Studio создан для разработчиков современных приложений и интегрируется в среды Visual Studio 2010–2015 годов.

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

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

Эти опечатки…

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

V501 Слева и справа от оператора || одинаковые подвыражения ‘!Input(0)-›HasMBLayout()’. trainingnodes.h 1416

virtual void Validate(bool isFinalValidationPass) override
{
  ....
  if (isFinalValidationPass &&
      !(Input(0)->GetSampleMatrixNumRows() ==
        Input(2)->GetSampleMatrixNumRows() &&
       (Input(0)->GetMBLayout() ==
        Input(2)->GetMBLayout() ||
       !Input(0)->HasMBLayout() ||            // <=
       !Input(0)->HasMBLayout())))            // <=
  {
    LogicError(..., NodeName().c_str(),OperationName().c_str());
  }
   ....
}

Форматирование этого фрагмента изменено для ясности. Только после этого стало очевидно, что есть два похожих «! Input(0)->HasMBLayout() «проверяет условие. Скорее всего, в одном из случаев нельзя использовать элемент с индексом «2».

V501 Слева и справа от оператора - одинаковые подвыражения: i0 — i0 ssematrix.h 564

void assignpatch(const ssematrixbase &patch,
                 const size_t i0,
                 const size_t i1,
                 const size_t j0,
                 const size_t j1)
{
  ....
  for (size_t j = j0; j < j1; j++)
  {
    const float *pcol = &patch(i0 - i0, j - j0);      // <=
    float *qcol = &us(i0, j);
    const size_t colbytes = (i1 - i0) * sizeof(*pcol);
    memcpy(qcol, pcol, colbytes);
  }
  ....
}

Из-за опечатки условие «i0-i0» всегда равно нулю. Возможно, здесь имелось в виду «i1-i0» или «j-i1» или что-то другое. Разработчикам обязательно стоит перепроверить это место.

V596 Объект создан, но не используется. Ключевое слово throw может отсутствовать: throw runtime_error(FOO); simplenetworkbuilder.cpp 1578

template <class ElemType>
ComputationNetworkPtr SimpleNetworkBuilder<ElemType>::
  BuildNetworkFromDbnFile(const std::wstring& dbnModelFileName)
{
  ....
  if (this->m_outputLayerSize >= 0)
    outputLayerSize = this->m_outputLayerSize;
  else if (m_layerSizes.size() > 0)
    m_layerSizes[m_layerSizes.size() - 1];
  else
    std::runtime_error("Output layer size must be...");     // <=
  ....
}

Ошибка в том, что ключевое слово «throw» было случайно забыто. В результате этот код не генерирует исключение в случае ошибки. Правильный вариант кода должен быть:

....
else
  throw std::runtime_error("Output layer size must be...");
....

Работа с файлами

V739 EOF не следует сравнивать со значением типа «char». «c» должен быть типа «int». файлutil.cpp 852

string fgetstring(FILE* f)
{
  string res;
  for (;;)
  {
    char c = (char) fgetc(f);        // <=
    if (c == EOF)                    // <=
      RuntimeError("error reading .... 0: %s", strerror(errno));
    if (c == 0)
      break;
    res.push_back(c);
  }
  return res;
}

Анализатор обнаружил, что константа EOF сравнивается с переменной типа char. Это показывает, что некоторые символы будут обрабатываться некорректно.

Давайте посмотрим, как объявляется EOF:

#define EOF (-1)

Как видите, EOF — это не что иное, как «-1» типа «int». Функция Fgetc() возвращает значение типа int. В частности, он может возвращать число от 0 до 255 или -1 (EOF). Прочитанные значения помещаются в переменную типа char. Из-за этого символ со значением 0xFF (255) превращается в -1, а затем обрабатывается так же, как и конец файла (EOF).

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

Например, в кодовой странице Windows 1251 последняя буква русского алфавита имеет код 0xFF и интерпретируется программой как символ конца файла.

Правильный фрагмент кода:

int c = fgetc(f);
if (c == EOF)
  RuntimeError(....);

V547 Выражение val[0] == 0xEF всегда ложно. Диапазон значений типа char: [-128, 127]. файл.cpp 462

bool File::IsUnicodeBOM(bool skip)
{
  ....
  else if (m_options & fileOptionsText)
  {
    char val[3];
    file.ReadString(val, 3);
    found = (val[0] == 0xEF && val[1] == 0xBB && val[2] == 0xBF);
  }
  // restore pointer if no BOM or we aren't skipping it
  if (!found || !skip)
  {
    SetPosition(pos);
  }
  ....
}

По умолчанию тип char имеет диапазон значений, равный [-127;127]. Используя флаг компиляции /J, мы можем указать компилятору использовать диапазон [0; 255]. Но для этого исходного файла такого флага нет, поэтому этот код никогда не определит, что этот файл содержит BOM.

Работа с памятью

V595 Указатель m_rowIndices использовался до того, как он был проверен на соответствие nullptr. Проверить строки: 171, 175. libsvmbinaryreader.cpp 171

template <class ElemType>
void SparseBinaryMatrix<ElemType>::ResizeArrays(size_t newNNz)
{
  ....
  if (m_nnz > 0)
  {
    memcpy(rowIndices, m_rowIndices, sizeof(int32_t)....);  // <=
    memcpy(values, this->m_values, sizeof(ElemType)....);   // <=
  }
  if (m_rowIndices != nullptr)
  {
    // free(m_rowIndices);
    CUDAPageLockedMemAllocator::Free(this->m_rowIndices, ....);
  }
  if (this->m_values != nullptr)
  {
    // free(this->m_values);
    CUDAPageLockedMemAllocator::Free(this->m_values, ....);
  }
  ....
}

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

Функция memcpy() копирует байты, расположенные по «m_rowIndices» и «m_values», при этом происходит разыменование этого указателя и в данном коде он потенциально может быть равен нулю.

V510 Функция sprintf_s не должна получать переменную типа класса в качестве третьего фактического аргумента. бинарный файл.cpp 501

const std::wstring& GetName()
{
  return m_name;
}
Section* Section::ReadSection(....)
{
  ....
  char message[256];
  sprintf_s(message,"Invalid header in file %ls, in header %s\n",
              m_file->GetName(), section->GetName());       // <=
  RuntimeError(message);
  ....
}

Только типы POD могут служить фактическими параметрами функции sprint_s(). POD — это аббревиатура от «Plain Old Data», что можно интерпретировать как «Простые данные в стиле C».

«std::wstring» не относится к типам POD. Вместо указателя в стек пойдет содержимое объекта. Этот код приведет к мусору в буфере или к краху программы.

Правильный вариант:

sprintf_s(message,"Invalid header in file %ls, in header %s\n",
          m_file->GetName().c_str(), section->GetName().c_str());

V630 Функция malloc используется для выделения памяти для массива объектов, которые являются классами, содержащими конструкторы. решеткафорвардбаквард.cpp 912

void lattice::forwardbackwardalign()
{
  ....
  aligninfo *refinfo;
  unsigned short *refalign;
  refinfo = (aligninfo *) malloc(sizeof(aligninfo) * 1);    // <=
  refalign = (unsigned short *) malloc(sizeof(....) * framenum);
  array_ref<aligninfo> refunits(refinfo, 1);
  array_ref<unsigned short> refedgealignmentsj(....);
  ....
}

В этом фрагменте кода анализатор обнаружил некорректное выделение динамической памяти для структуры типа «aligninfo». Дело в том, что в определении структуры есть конструкторы, но при таком способе выделения памяти конструктор вызываться не будет. Также не будет вызываться деструктор при освобождении памяти с помощью функции free().

Здесь вы видите фрагмент кода с описанием типа «aligninfo».

struct aligninfo // phonetic alignment
{
  unsigned int unit : 19;   // triphone index
  unsigned int frames : 11; // duration in frames
  unsigned int unused : 1; // (for future use)
  unsigned int last : 1;   // set for last entry
  aligninfo(size_t punit, size_t pframes)
      : unit((unsigned int) punit),
        frames((unsigned int) pframes), unused(0), last(0)
  {
    checkoverflow(unit, punit, "aligninfo::unit");
    checkoverflow(frames, pframes, "aligninfo::frames");
  }
  aligninfo() // [v-hansu] initialize to impossible values
  {
#ifdef INITIAL_STRANGE
    unit = unsigned int(-1);
    frames = unsigned int(-1);
    unused = unsigned int(-1);
    last = unsigned int(-1);
#endif
  }
  template <class IDMAP>
  void updateunit(const IDMAP& idmap /*[unit] -> new unit*/)
  {
    const size_t mappedunit = idmap[unit];
    unit = (unsigned int) mappedunit;
    checkoverflow(unit, mappedunit, "aligninfo::unit");
  }
};

Правильный вариант:

aligninfo *refinfo = new aligninfo();

И, конечно же, вам нужно будет вызвать оператор «удалить», чтобы освободить память.

V599 Виртуальный деструктор отсутствует, хотя класс IDataWriter содержит виртуальные функции. datawriter.cpp 47

IDataWriter<ElemType>* m_dataWriter;
....
template <class ElemType>
void DataWriter<ElemType>::Destroy()
{
    delete m_dataWriter; // <= V599 warning
    m_dataWriter = NULL;
}

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

template <class ElemType>
class DATAWRITER_API IDataWriter
{
public:
    typedef std::string LabelType;
    typedef unsigned int LabelIdType;
    virtual void Init(....) = 0;
    virtual void Init(....) = 0;
    virtual void Destroy() = 0;
    virtual void GetSections(....) = 0;
    virtual bool SaveData(....) = 0;
    virtual void SaveMapping(....) = 0;
};

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

m_dataWriter = new HTKMLFWriter<ElemType>();

Таким образом память выделяется для объекта производного класса «HTKMLFWriter». Это описание:

template <class ElemType>
class HTKMLFWriter : public IDataWriter<ElemType>
{
private:
    std::vector<size_t> outputDims;
    std::vector<std::vector<std::wstring>> outputFiles;
    std::vector<size_t> udims;
    std::map<std::wstring, size_t> outputNameToIdMap;
    std::map<std::wstring, size_t> outputNameToDimMap;
    std::map<std::wstring, size_t> outputNameToTypeMap;
    unsigned int sampPeriod;
    size_t outputFileIndex;
    void Save(std::wstring& outputFile, ....);
    ElemType* m_tempArray;
    size_t m_tempArraySize;
    ....
};

Из-за отсутствия виртуального деструктора в базовом классе этот объект не будет должным образом уничтожен. Для объектов outputDims, outputFiles деструкторы также вызываться не будут. Однако в общем случае невозможно предсказать все последствия, поэтому мы используем термин «неопределенное поведение».

Разные ошибки.

V502 Возможно, оператор ?: работает не так, как ожидалось. Оператор ‘?:’ имеет более низкий приоритет, чем оператор ‘|’. последовательностьparser.h 338

enum SequenceFlags
{
    seqFlagNull = 0,
    seqFlagLineBreak = 1, // line break on the parsed line
    seqFlagEmptyLine = 2, // empty line
    seqFlagStartLabel = 4,
    seqFlagStopLabel = 8
};
long Parse(....)
{
  ....
  // sequence state machine variables
  bool m_beginSequence;
  bool m_endSequence;
  ....
  if (seqPos)
  {
    SequencePosition sequencePos(numbers->size(), labels->size(),
      m_beginSequence ? seqFlagStartLabel : 0 | m_endSequence ?
      seqFlagStopLabel : 0 | seqFlagLineBreak);
    // add a sequence element to the list
    seqPos->push_back(sequencePos);
    sequencePositionLast = sequencePos;
  }
  
  // end of sequence determines record separation
  if (m_endSequence)
      recordCount = (long) labels->size();
  ....
}

Приоритет тернарного оператора «:?» ниже, чем у оператора побитового ИЛИ «|». Рассмотрим подробнее фрагмент, содержащий ошибку:

0 | m_endSequence ? seqFlagStopLabel : 0 | seqFlagLineBreak

Ожидается, что будут выполняться только побитовые операции с указанными флагами, однако из-за неожиданного порядка выполнения «0 | Сначала будет выполнен m_endSequence, а не m_endSequence? seqFlagStopLabel : 0 | seqFlagLineBreak».

На самом деле, это интересный случай. Несмотря на ошибку, код работает корректно. Побитовое ИЛИ с 0 ни на что не влияет.
Тем не менее, лучше исправить ошибку.

Есть еще два подобных фрагмента:

  • V502 Возможно, оператор «?:» работает не так, как ожидалось. Оператор ‘?:’ имеет более низкий приоритет, чем оператор ‘|’. последовательностьparser.h 433
  • V502 Возможно, оператор «?:» работает не так, как ожидалось. Оператор ‘?:’ имеет более низкий приоритет, чем оператор ‘|’. последовательностьparser.h 598

V530 Необходимо использовать возвращаемое значение функции размер. основы.ч 428

// TODO: merge this with todouble(const char*) above
static inline double todouble(const std::string& s)
{
  s.size(); // just used to remove the unreferenced warning
  double value = 0.0;
  ....
}

Здесь нет ошибки, которую мы видим в комментарии, но этот пример приведен здесь по двум причинам:

Во-первых, для отключения предупреждения компилятора есть макрос UNREFERENCED_PARAMETER, название которого дает понять, что параметр функции не используется преднамеренно:

#define UNREFERENCED_PARAMETER(P) (P)
static inline double todouble(const std::string& s)
{
  UNREFERENCED_PARAMETER(s);
  ....
}

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

V530 Необходимо использовать возвращаемое значение функции пусто. utterancesourcemulti.h 340

template <class UTTREF>
std::vector<shiftedvector<....>>getclassids(const UTTREF &uttref)
{
  std::vector<shiftedvector<....>> allclassids;
  allclassids.empty();  // <=
  ....
}

Нет смысла не использовать результат работы функции empty().

Возможно, вектор нужно было очистить с помощью функции clear().

Аналогичный фрагмент:

  • V530 Необходимо использовать возвращаемое значение функции «пусто». utterancesourcemulti.h 364

V688 Локальная переменная m_file имеет то же имя, что и один из членов класса, что может привести к путанице. sequencereader.cpp 552

template <class ElemType>
class SequenceReader : public IDataReader<ElemType>
{
protected:
  bool m_idx2clsRead;
  bool m_clsinfoRead;
  bool m_idx2probRead;
  std::wstring m_file;                               // <=
  ....
}
template <class ElemType>
template <class ConfigRecordType>
void SequenceReader<ElemType>::InitFromConfig(....)
{
  ....
  std::wstring m_file = readerConfig(L"file");       // <=
  if (m_traceLevel > 0)
  {
    fprintf(stderr, "....", m_file.c_str());
  }
  ....
}

Использование переменных с одинаковыми именами в классе, функциях класса и параметрах класса — очень плохой стиль программирования. Например: было ли объявление переменной «std::wstring m_file = readerConfig(L”file”);» должен был быть здесь, или он был добавлен временно для отладки, а затем был забыт?

Разработчикам также следует просмотреть следующие фрагменты:

  • V688 Локальная переменная m_file имеет то же имя, что и один из членов класса, что может привести к путанице. sequencereader.cpp 1554
  • V688 Аргумент функции m_mbStartSample имеет то же имя, что и один из членов класса, что может привести к путанице. sequencereader.cpp 2062
  • V688 Локальная переменная m_file имеет то же имя, что и один из членов класса, что может привести к путанице. lusequencereader.cpp 417

Вывод:

Computational Network Toolkit (CNTK), будучи относительно небольшим проектом, оказался довольно интересным программным обеспечением. Поскольку проект CNTK был открыт совсем недавно, мы с нетерпением ждем новых идей по его использованию и, конечно же, других проектов Microsoft с открытым исходным кодом.