Святослав Размыслов

02 Фев 2016

Теги:

#Cpp

"Почему ещё не изобрели искусственный интеллект?" или проверка инструментария CNTK от Microsoft Research

02 Фев 2016

Автор: Святослав Размыслов

Введение
Эх, опечатки
Работа с файлами
Работа с памятью
Разные ошибки
Заключение

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

Введение

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

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

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

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

Эх, опечатки

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

V501 There are identical sub-expressions '!Input(0)->HasMBLayout()' to the left and to the right of the '||' operator. 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 There are identical sub-expressions to the left and to the right of the '-' operator: 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 The object was created but it is not being used. The 'throw' keyword could be missing: 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 should not be compared with a value of the 'char' type. The 'c' should be of the 'int' type. fileutil.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).

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

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

Исправленный фрагмент кода:

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

V547 Expression 'val[0] == 0xEF' is always false. The value range of char type: [-128, 127]. file.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 The 'm_rowIndices' pointer was utilized before it was verified against nullptr. Check lines: 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 The 'sprintf_s' function is not expected to receive class-type variable as third actual argument. binaryfile.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);
  ....
}

В качестве фактических параметров функции sprint_s() могут выступать только POD типы. POD - это аббревиатура от "Plain Old Data", что можно перевести как "Простые данные в стиле Си".

"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 The 'malloc' function is used to allocate memory for an array of objects which are classes containing constructors. latticeforwardbackward.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();

И естественно для освобождения памяти потребуется вызывать оператор 'delete'.

V599 The virtual destructor is not present, although the 'IDataWriter' class contains virtual functions. 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 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '|' operator. sequenceparser.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 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '|' operator. sequenceparser.h 433
V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '|' operator. sequenceparser.h 598

V530 The return value of function 'size' is required to be utilized. basics.h 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 macro, название которого однозначно даёт понять, что параметр функции не используется намеренно:

#define UNREFERENCED_PARAMETER(P) (P)

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

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

V530 The return value of function 'empty' is required to be utilized. utterancesourcemulti.h 340

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

Не имеет смысла не использовать результат функции empty(). Скорее всего тут хотели очистить вектор с помощью функции clear().

Похожее место:

V530 The return value of function 'empty' is required to be utilized. utterancesourcemulti.h 364

V688 The 'm_file' local variable possesses the same name as one of the class members, which can result in a confusion. 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 The 'm_file' local variable possesses the same name as one of the class members, which can result in a confusion. sequencereader.cpp 1554
V688 The 'm_mbStartSample' function argument possesses the same name as one of the class members, which can result in a confusion. sequencereader.cpp 2062
V688 The 'm_file' local variable possesses the same name as one of the class members, which can result in a confusion. lusequencereader.cpp 417

Заключение

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

#Cpp