Проверка обёртки OpenCvSharp над OpenCV с помощью PVS-Studio

OpenCV — библиотека алгоритмов компьютерного зрения, обработки изображений и численных алгоритмов общего назначения с открытым кодом, знакомая многим разработчикам на C++. Помимо C++, OpenCV также разрабатывается для Python, Java, Ruby, Matlab, Lua и других языков. Так как среди этих языков нет моего основного, C#, то я решила обратить внимание на OpenCvSharp — wrapper библиотеки под C# и проверить этот проект. Что же из этого вышло, можно узнать в данной статье.

Введение

Ранее, до прихода в PVS-Studio, я занималась робототехникой в рамках выставок. В мои задачи входила самая элементарная починка (если случалась крупная поломка, то робота отдавали другому человеку), а также разработка самого разнообразного софта и утилит.

Уставшая и недавно приехавшая в новый город я вместе со свежераспакованным роботом KIKI.

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

Поискав в интернете какую-нибудь библиотеку под мои нужды, я набрела на сайт OpenCV, библиотеки алгоритмов компьютерного зрения. Очень скоро меня ждало разочарование — OpenCV реализована на C++. Моих знаний о плюсах, доставшихся из колледжа, явно не хватало. Поэтому, путем недолгого гугления, я наткнулась на OpenCvSharp — wrapper этой библиотеки под C#, мой основной язык. С тех пор прошло с полгода, программа уже давно написана и используется, а я решилась залезть «под капот» к OpenCvSharp и проверить её исходный код при помощи статического анализатора PVS-Studio.

Проверяемый проект

OpenCvSharp — это обёртка над OpenCV для использования библиотеки в C# проектах. Библиотеку OpenCV, к слову, мы тоже проверяли. К плюсам OpenCvSharp можно отнести большую коллекцию примеров кода, кроссплатформенность (может работать на любой платформе, которую поддерживает Mono) и легкость в установке.

Обёртка представляет собой маленький проект и содержит около 112 200 строк кода на языке C#. Из них 1,2% — комментарии, что, кстати, как-то подозрительно мало. Зато для такого маленького проекта находится немало ошибок. В статье я выписала более 20 ошибок, но были и другие не столь интересные или очевидные.

Анализатор кода PVS-Studio

PVS-Studio — это инструмент для выявления ошибок и потенциальных уязвимостей в исходном коде программ, написанных на языках С, C++, C# и Java. Работает на Windows, Linux и macOS. Помимо недостижимого кода, ошибок и опечаток, PVS-Studio умеет находить потенциальные уязвимости, как было отмечено выше. Следовательно, его можно рассматривать как средство статического тестирования безопасности приложений (Static Application Security Testing, SAST).

Участки кода, привлекшие внимание при рассмотрении отчета анализатора

Метод WriteableBitmapConverter привлекает внимание сразу четырьмя однотипными предупреждениями PVS-Studio:

• V3005 The 'optimumChannels[PixelFormats.Indexed1]' variable is assigned to itself. WriteableBitmapConverter.cs 22
• V3005 The 'optimumChannels[PixelFormats.Indexed8]' variable is assigned to itself. WriteableBitmapConverter.cs 23
• V3005 The 'optimumTypes[PixelFormats.Indexed1]' variable is assigned to itself. WriteableBitmapConverter.cs 50
• V3005 The 'optimumTypes[PixelFormats.Indexed8]' variable is assigned to itself. WriteableBitmapConverter.cs 51

static WriteableBitmapConverter()
{
  optimumChannels = new Dictionary
                        <PixelFormat, int>();
  optimumChannels[PixelFormats.Indexed1] =         // <=
  optimumChannels[PixelFormats.Indexed8] =         // <=
  optimumChannels[PixelFormats.Gray2] =
  optimumChannels[PixelFormats.Gray4] =
  optimumChannels[PixelFormats.Gray8] =
  optimumChannels[PixelFormats.Gray16] =
  optimumChannels[PixelFormats.Gray32Float] =
  optimumChannels[PixelFormats.Indexed1] =         // <=
  optimumChannels[PixelFormats.Indexed2] =
  optimumChannels[PixelFormats.Indexed4] =
  optimumChannels[PixelFormats.Indexed8] =         // <=
  ....

  optimumTypes = new Dictionary
                 <PixelFormat, MatType>();
  optimumTypes[PixelFormats.Indexed1] =            // <=
  optimumTypes[PixelFormats.Indexed8] =            // <=
  optimumTypes[PixelFormats.Gray2] =
  optimumTypes[PixelFormats.Gray4] =
  optimumTypes[PixelFormats.Gray8] =
  optimumTypes[PixelFormats.Indexed1] =            // <=
  optimumTypes[PixelFormats.Indexed2] =
  optimumTypes[PixelFormats.Indexed4] =
  optimumTypes[PixelFormats.Indexed8] =            // <=
  optimumTypes[PixelFormats.BlackWhite] = 
  ....
}
....
public static class PixelFormats
{
  ....
  public static PixelFormat Indexed8 { get; }
  ....
  public static PixelFormat Indexed1 { get; }
  ....
}

Класс PixelFormats определён в пространстве имен System.Windows.Media и представляет из себя коллекцию разнообразных форматов пикселей. Анализатор обращает внимание на то, что в методе WriteableBitmapConverter происходит повторное присваивание значений элементам optimumChannels[PixelFormats.Indexed1] и optimumChannels[PixelFormats.Indexed8], в чем нет практического смысла. Непонятно, это простая опечатка или имелось в виду что-то другое. К слову, этот участок кода наглядно демонстрирует пользу статических анализаторов. Когда перед глазами куча однотипных строк, то взгляд начинает «замыливаться», а внимательность рассеиваться — не удивительно, если даже после code review в программу закрадется какая-нибудь опечатка. А у статического анализатора проблем с внимательностью нет и ему не надо отдыхать, поэтому ему проще найти подобного рода ошибки.

Прочувствуйте мощь и силу статического анализа.

Предупреждение PVS-Studio: V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless InputArray.cs 394

private static MatType EstimateType(Type t)
{
  ....
  if (t == typeof(Vec2b))
    return MatType.CV_8UC2;
  if (t == typeof(Vec3b))
    return MatType.CV_8UC3;
  if (t == typeof(Vec4b))
    return MatType.CV_8UC4;
  if (t == typeof(Vec6b))
    return MatType.CV_8UC(6);
  if (t == typeof(Vec2s))         // <=
    return MatType.CV_16SC2;
  ....
  if (t == typeof(Vec2s))         // <=
    return MatType.CV_32SC2;
  ....
}

Эта ошибка немного похожа на предыдущую. Программист допустил ситуацию, когда проверяется одно и то же условие дважды. В данном случае это не имеет смысла — then-ветвь «продублированного» оператора if не будет выполнена, так как:

• если истинно первое условное выражение, будет осуществлён выход из метода;
• если первое условие ложно, второе также будет ложно, так как проверяемая переменная — t — между условными выражениями не изменяется.

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

Предупреждение PVS-Studio: V3010 The return value of function 'ToString' is required to be utilized. ImgProcTest.cs 80

public static RectanglesIntersectTypes
RotatedRectangleIntersection(RotatedRect rect1, 
                             RotatedRect rect2, 
                             out Point2f[] intersectingRegion)
{
  using (var intersectingRegionVec = new VectorOfPoint2f())
  {
    int ret = NativeMethods
                .imgproc_rotatedRectangleIntersection_vector(
                         rect1, rect2, intersectingRegionVec.CvPtr);
    intersectingRegion = intersectingRegionVec.ToArray();
    return (RectanglesIntersectTypes) ret;
  }
}

public void RotatedRectangleIntersectionVector()
{
  var rr1 = new RotatedRect(new Point2f(100, 100),
                            new Size2f(100, 100), 
                            45);
  var rr2 = new RotatedRect(new Point2f(130, 100), 
                            new Size2f(100, 100), 
                            0);

  Cv2.RotatedRectangleIntersection(rr1, rr2,
                out var intersectingRegion);

  ....

  intersectingRegion.ToString();
}

Метод RotatedRectangleIntersection возвращает массив элементов типа Point2f через параметр intersectingRegion. После того, как программа заполняет intersectingRegion значениями, у этого массива вызывается метод ToString(). С элементами массива от этого никаких изменений не происходит и никакой полезной работы в последней строке не производится, поэтому есть основания подозревать разработчика в том, что он просто забыл это убрать.

Предупреждения PVS-Studio:

• V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless Cv2_calib3d.cs 1370
• V3022 Expression 'objectPoints == null' is always false. Cv2_calib3d.cs 1372

public static double CalibrateCamera(....)
{
  if (objectPoints == null)
    throw new ArgumentNullException(nameof(objectPoints));
  if (objectPoints == null)
    throw new ArgumentNullException(nameof(objectPoints));
  ....
}

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

Милый Copy-Paste.

Другие подобные предупреждения анализатора:

• V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless Cv2_calib3d.cs 1444
• V3022 Expression 'objectPoints == null' is always false. Cv2_calib3d.cs 1446

Предупреждение PVS-Studio: V3022 Expression 'label == MarkerValue' is always false. Labeller.cs 135

internal static class Labeller
{
  ....
  private const int MarkerValue = -1;
  public static int Perform(Mat img, CvBlobs blobs)
  {
    ....
    int label = 0;
    int lastLabel = 0;
    CvBlob lastBlob = null;

    for (int y = 0; y < h; y++)
    {
      for (int x = 0; x < w; x++)
      {
        if (imgIn[x + y * step] == 0)
          continue;

        bool labeled = labels[y, x] != 0;
        if (....)
        {
          labeled = true;

          // Label contour.
          label++;
          if (label == MarkerValue)    // <=
            throw new Exception();
          ....
        }
        ....
      }
      ....
    }
  }
}

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

Предупреждение PVS-Studio: V3038 The argument was passed to method several times. It is possible that other argument should be passed instead. Cv2_photo.cs 124

public static void FastNlMeansDenoisingMulti(....)
{
  ....
  NativeMethods.photo_fastNlMeansDenoisingMulti(
    srcImgPtrs, 
    srcImgPtrs.Length, 
    dst.CvPtr, 
    imgToDenoiseIndex, 
    templateWindowSize,
    h, 
    templateWindowSize,
    searchWindowSize);
  ....
}

Чтобы понять, что имеет в виду анализатор, посмотрим на параметры метода photo_fastNlMeansDenoisingMulti:

public static extern void photo_fastNlMeansDenoisingMulti(
  IntPtr[] srcImgs, 
  int srcImgsLength,
  IntPtr dst, 
  int imgToDenoiseIndex, 
  int temporalWindowSize, 
  float h, 
  int templateWindowSize,
  int searchWindowSize)

Упростим ещё больше, чтобы стало совсем очевидно. Сравните эти строки:

NativeMethods.photo_fastNlMeansDenoisingMulti(
  ....
  templateWindowSize, ....
  templateWindowSize, ....);

public static extern void photo_fastNlMeansDenoisingMulti(
  ....
  int temporalWindowSize, ....
  int templateWindowSize, ....)

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

Похожие предупреждения анализатора:

• V3038 The argument was passed to method several times. It is possible that other argument should be passed instead. Cv2_photo.cs 149
• V3038 The argument was passed to method several times. It is possible that other argument should be passed instead. Cv2_photo.cs 180
• V3038 The argument was passed to method several times. It is possible that other argument should be passed instead. Cv2_photo.cs 205

Следующая ошибка будет немного похожа на предыдущую.

Предупреждение PVS-Studio: V3066 Possible incorrect order of arguments passed to 'calib3d_Rodrigues_MatToVec' method: 'matrixM.CvPtr' and 'vectorM.CvPtr'. Cv2_calib3d.cs 86

public static void Rodrigues(double[,] matrix, out double[] vector,
  out double[,] jacobian)
{
  ....
  using (var jacobianM = new Mat<double>())
  {
    NativeMethods.calib3d_Rodrigues_MatToVec
    (matrixM.CvPtr, vectorM.CvPtr, 
     jacobianM.CvPtr);
    ....
  }
}

Посмотрим на параметры calib3d_Rodrigues_MatToVec

public static extern void calib3d_Rodrigues_MatToVec(
  IntPtr vector, IntPtr matrix, IntPtr jacobian)

Возможно, при вызове метода calib3d_Rodrigues_MatToVec были перепутаны местами аргументы matrixM.CvPtr и vectorM.CvPtr. Разработчикам стоит повнимательнее посмотреть на этот код. Есть вероятность, что в него закралась ошибка, мешающая правильным расчётам.

Предупреждение PVS-Studio: V3063 A part of conditional expression is always false if it is evaluated: data == null. Mat.cs 3539

private void CheckArgumentsForConvert(....)
{
  ....
  if (data == null)
    throw new ArgumentNullException(nameof(data));

  MatType t = Type();
  if (data == null || (data.Length * dataDimension)      // <=
    (data.Length * dataDimension) % t.Channels != 0) 
   ....
}

Анализатор указывает на то, что вторая проверка data == null никогда не будет true, т.к. если в первом условии data имеет значение null, то будет сгенерировано исключение, и исполнение программы до второй проверки не дойдет.

Понимаю, что вы уже устали, но осталось совсем немного.

Предупреждение PVS-Studio: V3127 Two similar code fragments were found. Perhaps, this is a typo and 'window' variable should be used instead of 'src2' Cv2_imgproc.cs 1547

public static Point2d PhaseCorrelateRes(....)
{
  if (src1 == null)
    throw new ArgumentNullException(nameof(src1));
  if (src2 == null)
    throw new ArgumentNullException(nameof(src2));
  if (window == null)
    throw new ArgumentNullException(nameof(src2));   // <=
  ....
}

А тут анализатор нашел опечатку. В этом участке кода значение переменных проверяется на null, и, если это условие выполняется, то для каждой из переменных генерируется исключение. Однако с переменной window не все так просто. Если эта переменная равна null, то для нее тоже генерируется исключение, но текст этого исключения записан неправильно. В тексте этого исключения не фигурирует сама переменная window, вместо нее там указана src2. Судя по всему, последнее условие должно быть таким:

if (window == null)
  throw new ArgumentNullException(nameof(window));

Предупреждение PVS-Studio: V3142 Unreacheble code detected. It is possible that an error is present. MatOfT.cs 873

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

public new Mat<TElem> SubMat(params Range[] ranges)
{
  Mat result = base.SubMat(ranges);
  return Wrap(result);
}

Анализатор утверждает, что недостижим здесь оператор return. Чтобы это проверить, посмотрим на тело метода SubMat.

public Mat SubMat(params Range[] ranges)
{
  throw new NotImplementedException();
  /*
  if (ranges == null)
   throw new ArgumentNullException();

  ThrowIfDisposed();
  CvSlice[] slices = new CvSlice[ranges.Length];
  for (int i = 0; i < ranges.Length; i++)
  {
    slices[i] = ranges[i];
  }

  IntPtr retPtr = NativeMethods.core_Mat_subMat1(ptr, ranges.Length,
  ranges);
  Mat retVal = new Mat(retPtr);
  return retVal;*/
}

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

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

Предупреждение PVS-Studio: V3022 Expression 'String.IsNullOrEmpty(«winName»)' is always false. Cv2_highgui.cs 46

public static void 
DestroyWindow(string winName)
{
  if (String.IsNullOrEmpty("winName"))
    ....
}

Предупреждение PVS-Studio: V3022 Expression 'string.IsNullOrEmpty(«fileName»)' is always false. FrameSource.cs 37

public static FrameSource 
CreateFrameSource_Video(string fileName)
{
  if (string.IsNullOrEmpty("fileName"))
    ....
}

Предупреждение PVS-Studio: V3022 Expression 'string.IsNullOrEmpty(«fileName»)' is always false. FrameSource.cs 53

public static FrameSource 
CreateFrameSource_Video_CUDA(string fileName)
{
  if (string.IsNullOrEmpty("fileName"))
    ....
}

Иногда за предупреждением анализатора V3022 (выражение всегда истинно/ложно) скрываются и впрямь странные или забавные вещи. Во всех трёх случаях наблюдается одна и та же ситуация. В коде метода имеется параметр типа string, значение которого надо проверить. Однако проверяется не значение переменной, а строковый литерал с её именем, т.е. имя зря взято в кавычки.

Судя по всему, разработчик один раз опечатался и при помощи копипасты размножил эту ошибку по коду.

Заключение

Разработчики OpenCvSharp проделали важную и большую работу. И я, как пользователь этой библиотеки, им весьма благодарна. Спасибо.

Однако оказавшись в команде PVS-Studio и заглянув в код, я вынуждена констатировать, что вопрос его качества проработан недостаточно. Скорее всего, в этом проекте на регулярной основе не используются статические анализаторы кода. И многие ошибки правятся более дорогими методами (тестированием, по отзывам пользователей, например). А некоторые вообще остаются надолго жить в коде и их-то мы как раз и обнаруживаем. Чуть подробнее эта мысль изложена в небольшой заметке на тему философии использования методологии статического анализа.

Поскольку проект открыт и расположен на GitHub, его разработчики имеют возможность воспользоваться бесплатным вариантом лицензирования PVS-Studio и начать применять анализ на регулярной основе.

Спасибо за внимание. Скачайте и проверьте ваши проекты с помощью триальной версии PVS-Studio.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Ekaterina Nikiforova. Checking the OpenCvSharp Wrapper for OpenCV with PVS-Studio.

Автор: Екатерина Никифорова

Источник