OpenCV is an open-source library of computer vision and image processing algorithms and general-purpose numerical algorithms. The library is well known among C++ developers. Besides C++, there are also versions for Python, Java, Ruby, Matlab, Lua, and other languages. Since C#, which is the language I specialize in, is not on that list, I chose OpenCvSharp, a C# wrapper of OpenCV, to check it with PVS-Studio. The results of that check are discussed in this article.
Before I became part of the PVS-Studio team, I had been involved in the making of robots to present at exhibitions. My duties included the most basic repair work (major failures were handled by another person) as well as development of software and utilities of every kind.
Me, tired and new to town, with a freshly unpacked KIKI robot.
By the way, the development part was pretty funny. Each time one of us had an idea about some new way to surprise exhibition visitors, we brought it up for discussion and if everyone liked it, we'd get down to work. Once it occurred to us to make a robot that could recognize a human face and respond with a welcome speech.
I googled for some library for my needs and stumbled upon OpenCV, a computer-vision algorithms library. But I got disappointed very soon as I figured out that OpenCV was implemented in C++. My knowledge of C++, which I had studied at college, was obviously not enough. So I googled a bit more and found OpenCvSharp, a wrapper of the library for C#, which is the language I specialize in. It's been about half a year since then, the program long written and in use, and now I finally decided to peek "under the hood" of OpenCvSharp and scan its source code with the PVS-Studio static analyzer.
OpenCvSharp is a wrapper of OpenCV for use in C# projects. By the way, we already checked OpenCV in the past. The strong points of OpenCvSharp are the large collection of code samples, cross-platform support (it runs on any platform supported by Mono), and easy installation.
The wrapper is a small project about 112,200 lines of C# code long. 1,2% of these are comments, which, I should say, is suspiciously few. On the other hand, there are quite a few bugs for a project that small. I picked over 20 examples for this article, but the analyzer actually found many more, which aren't that interesting or obvious.
PVS-Studio is a tool for detecting bugs and potential vulnerabilities in the source code of programs written in C, C++, C#, and Java. It runs on Windows, Linux, and macOS. In addition to unreachable code, programming mistakes, and typos, PVS-Studio, as was already mentioned, is capable of detecting potential security issues. Therefore, it can be viewed as a Static Application Security Testing (SAST) tool.
What makes the WriteableBitmapConverter method special is that it triggered four warnings of the same type at once:
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; }
....
}The PixelFormats class is defined in the System.Windows.Media namespace and is a collection of various pixel formats. The analyzer points out that the elements optimumChannels[PixelFormats.Indexed1] and optimumChannels[PixelFormats.Indexed8] are assigned values for a second time in the WriteableBitmapConverter method, which doesn't make any sense. It's unclear whether this is just a typo or the programmer meant something else. By the way, this snippet is a vivid example of how static analyzers can be helpful: looking at a bunch of similar lines makes you less focused – no wonder typos stay unnoticed despite the code review. Static analyzers, though, don't have trouble maintaining attention and they don't need rest, so they can catch bugs like that with no effort.
Feel the might of static analysis.
PVS-Studio diagnostic message: 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;
....
}This bug is somewhat similar to the previous one. The developer is checking the same condition twice. It doesn't make sense here as the then branch of the "duplicate" if statement will never execute because:
This code needs revising; it's very likely that the second copy of Vec2s was actually meant to be some other variable.
PVS-Studio diagnostic message: 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();
}The RotatedRectangleIntersection method is accessed through the intersectingRegion parameter and returns an array of elements of type Point2f. Once the intersectingRegion has been filled with values, the ToString() method is called on the array. This doesn't affect the array's elements in any way and no useful work is performed in the last line, so it would be fair to assume that the developer simply forgot to remove that piece.
PVS-Studio diagnostic messages:
public static double CalibrateCamera(....)
{
if (objectPoints == null)
throw new ArgumentNullException(nameof(objectPoints));
if (objectPoints == null)
throw new ArgumentNullException(nameof(objectPoints));
....
}We have cloned code here, hence the two warnings. The first says that both if statements check the same condition. If that condition is true, the method will return in the then branch of the first if statement. Consequently, the second condition will always be false, which is what the second warning is telling us. It seems the programmer cloned that fragment using copy-paste but forgot to change it.
Cute Copy-Paste.
Other warnings of this type:
PVS-Studio diagnostic message: 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();
....
}
....
}
....
}
}
}A variable named label is created and initialized to 0. If a certain condition is true, it will get incremented by one. What's more, this variable never gets decremented in this snippet. Therefore, checking it for the constant -1, as in the line pointed at by the analyzer, doesn't make any sense.
PVS-Studio diagnostic message: 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);
....
}To understand what the analyzer is telling us, let's take a look at the photo_fastNlMeansDenoisingMulti method's parameters:
public static extern void photo_fastNlMeansDenoisingMulti(
IntPtr[] srcImgs,
int srcImgsLength,
IntPtr dst,
int imgToDenoiseIndex,
int temporalWindowSize,
float h,
int templateWindowSize,
int searchWindowSize)Let's simplify it even more to make it completely straightforward. Compare these lines:
NativeMethods.photo_fastNlMeansDenoisingMulti(
....
templateWindowSize, ....
templateWindowSize, ....);
public static extern void photo_fastNlMeansDenoisingMulti(
....
int temporalWindowSize, ....
int templateWindowSize, ....)The templateWindowSize variable is declared twice, but the first time it's mentioned should actually be the declaration of temporalWindowSize. Another thing that the analyzer didn't like is that the value of temporalWindowSize is not used in the photo_fastNlMeansDenoisingMulti method at all. This could be a conscious decision, but I'd take a closer look at this code if I were the author.
Other warnings of this type:
The next example is somewhat similar to the previous one.
PVS-Studio diagnostic message: 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);
....
}
}Let's look at the calib3d_Rodrigues_MatToVec method's parameters:
public static extern void calib3d_Rodrigues_MatToVec(
IntPtr vector, IntPtr matrix, IntPtr jacobian)It seems the calib3d_Rodrigues_MatToVec method is called with the arguments matrixM.CvPtr and vectorM.CvPtr accidentally swapped. The authors should check this snippet: there might be a mistake that hinders correct computations.
PVS-Studio diagnostic message: 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)
....
}The analyzer reports that the second check data == null will never be true because if data is equal to null in the first condition, an exception will be raised and execution will never reach the second check.
I know you're tired, but we're almost done.
PVS-Studio diagnostic message: 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)); // <=
....
}The analyzer spotted a typo in this snippet. The variables are checked for null and, if true, each check throws an exception. However, it doesn't work quite properly for the window variable. If its value is equal to null, a corresponding exception is thrown too but with the wrong text. It won't be mentioning window; it will be src2 instead. The condition should apparently be revised as follows:
if (window == null)
throw new ArgumentNullException(nameof(window));PVS-Studio diagnostic message: V3142 Unreacheble code detected. It is possible that an error is present. MatOfT.cs 873
Now, just for a change, let's take a look at the case where the analyzer is technically correct about unreachable code, but there's actually no error. It's a warning that can be called both true and false at the same time.
public new Mat<TElem> SubMat(params Range[] ranges)
{
Mat result = base.SubMat(ranges);
return Wrap(result);
}The analyzer tells us that the return statement is unreachable. Let's look at the body of the SubMat method to see if the analyzer is telling the truth.
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;*/
}As you can see, the function is currently incomplete and will always throw an exception. The analyzer is absolutely correct pointing out the unreachable code – but it's not a genuine bug.
The next three defects are of the same type, but they are so cool I couldn't help including all the three.
PVS-Studio diagnostic message: V3022 Expression 'String.IsNullOrEmpty("winName")' is always false. Cv2_highgui.cs 46
public static void
DestroyWindow(string winName)
{
if (String.IsNullOrEmpty("winName"))
....
}PVS-Studio diagnostic message: V3022 Expression 'string.IsNullOrEmpty("fileName")' is always false. FrameSource.cs 37
public static FrameSource
CreateFrameSource_Video(string fileName)
{
if (string.IsNullOrEmpty("fileName"))
....
}PVS-Studio diagnostic message: V3022 Expression 'string.IsNullOrEmpty("fileName")' is always false. FrameSource.cs 53
public static FrameSource
CreateFrameSource_Video_CUDA(string fileName)
{
if (string.IsNullOrEmpty("fileName"))
....
}Sometimes V3022 warnings (about always true/false expressions) point at really strange or funny bugs. All the three examples above have the same mistake in them. The method has a parameter of type string whose value must be checked. What is checked instead, though, is a string literal whose text is the variable's name, i.e. the variable's name enclosed in quotation marks.
The programmer must have written a faulty block of code once and then cloned it through copy-paste.
The developers of OpenCvSharp have done a big and important job, and, as the user of their library, I am totally thankful for that. Thank you guys!
But now that I have become part of the PVS-Studio team and seen the library's code, I have to say that the quality aspect wasn't given proper attention. The project doesn't look like one being regularly checked with static analyzers, and many of the bugs are apparently fixed using more expensive techniques (such as testing or user feedback), and some of the bugs just keep living inside the code and it's them that we catch with our analyzer. This subject is discussed in more detail in this small post on the static analysis philosophy.
Since OpenCvSharp is open-source and freely available on GitHub, its authors can use one of the free licensing options for PVS-Studio to start using it on a regular basis.
Thanks for reading. Don't hesitate to download a trial copy of PVS-Studio to check your own projects.