Статьи о проверке проектов с открытым исходным кодом - вещь полезная. Кто-то, в том числе и разработчики, узнает об ошибках, содержащихся в проекте, кто-то узнает о методологии статического анализа и начнет применять её для повышения качества своего кода. Для нас же это прекрасный способ популяризации анализатора PVS-Studio, а заодно возможность его дополнительного тестирования. На этот раз я проверил платформу Accord.Net и нашёл в коде много интересных фрагментов.
Accord.Net - платформа машинного обучения на базе .Net, написанная на языке C#. Платформа состоит из нескольких библиотек, охватывающих широкий диапазон задач, таких как статическая обработка данных, машинное обучение, распознавание образов и пр. Исходный код проекта доступен в репозитории на GitHub.
Для проверки проекта использовался статический анализатор кода PVS-Studio, доступный для загрузки по ссылке. Кроме того, вы можете ознакомиться с другими статьями о проверке открытых проектов или с "базой ошибок", в которой мы собираем найденные нами баги.
Анализатор выдал 91 предупреждение первого уровня и 141 предупреждение второго уровня достоверности. 109 из этих предупреждений были описаны или упомянуты в статье. Бегло просмотрев остальные предупреждения, ещё 23 я бы отнёс к ошибкам, но они не приведены в статье, так как показались недостаточно интересными или были схожи с уже описанными. Об остальных предупреждениях анализатора судить уже сложнее - нужно тщательнее разбираться и анализировать код. В итоге, минимум 132 из 232 предупреждений я бы отнёс к ошибкам. Это говорит о том, что число ложных срабатываний анализатора составляет примерно 46%. А, нет, погодите... Это говорит нам о том, что каждое второе предупреждение анализатора - ошибка в коде! По-моему, это достаточно весомый аргумент необходимости использования инструментов статического анализа. О том, как правильно и неправильно использовать статические анализаторы, написано в конце статьи. Пока же предлагаю посмотреть, что же интересного удалось обнаружить в исходном коде.
Одинаковые подвыражения
Допустить ошибку, отлавливаемую диагностическим сообщением V3001 достаточно легко, особенно если использовать copy-paste, или если названия переменных, используемых в выражении, схожи. Эти ошибки - одни из самых распространённых, и в этом проекте без них не обошлось. Попробуйте найти ошибку в фрагменте ниже, не подсматривая в предупреждение анализатора.
public Blob[] GetObjects(UnmanagedImage image,
bool extractInOriginalSize)
{
....
if ((image.PixelFormat != PixelFormat.Format24bppRgb) &&
(image.PixelFormat != PixelFormat.Format8bppIndexed) &&
(image.PixelFormat != PixelFormat.Format32bppRgb) &&
(image.PixelFormat != PixelFormat.Format32bppArgb) &&
(image.PixelFormat != PixelFormat.Format32bppRgb) &&
(image.PixelFormat != PixelFormat.Format32bppPArgb)
)
....
}Предупреждение PVS-Studio: V3001 There are identical sub-expressions 'image.PixelFormat != PixelFormat.Format32bppRgb' to the left and to the right of the '&&' operator. Accord.Imaging BlobCounterBase.cs 670
Здесь два раза повторяется подвыражение image.PixelFormat != PixelFormat.Format32bppRgb. С такими названиями элементов перечисления ошибиться было достаточно просто, что и произошло. Стоит отметить, что, просматривая такой код, очень легко пропустить лишнее подвыражение. А вот является ли одно из сравнений лишним, или подразумевалось иное значение перечисления - вопрос интересный. В одном случае будет просто избыточный код, а в другом - логическая ошибка.
Этот код встретился ещё раз в этом же файле в строке 833. Copy-paste... Copy-paste никогда не меняется.
Ошибочное условие выхода из цикла
Все мы привыкли называть счётчики циклов именами вроде i, j, k и т.п. Как правило, это удобно и является обыденной практикой, но иногда может встать боком. Как в примере ниже.
public static void Convert(float[][] from, short[][] to)
{
for (int i = 0; i < from.Length; i++)
for (int j = 0; i < from[0].Length; j++)
to[i][j] = (short)(from[i][j] * (32767f));
}Предупреждение PVS-Studio: V3015 It is likely that a wrong variable is being compared inside the 'for' operator. Consider reviewing 'i' Accord.Audio SampleConverter.cs 611
В условии выхода из второго цикла используется переменная i, хотя счётчиком этого цикла выступает j. При этом переменная i внутри тела вложенного цикла не изменяется. В итоге j будет инкрементироваться до тех пор, пока не выйдет за границы массива, и не будет сгенерировано исключение.
Разная логика для одинаковых условий
Встретился код, когда для двух одинаковых операторов if была предусмотрена разная логика.
public void Fit(double[][] observations,
double[] weights,
MultivariateEmpiricalOptions options)
{
if (weights != null)
throw new ArgumentException("This distribution does not support
weighted samples.", "weights");
....
if (weights != null)
weights = inPlace ? weights : (double[])weights.Clone();
....
}Предупреждение 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 Accord.Statistics MultivariateEmpiricalDistribution.cs 653
Странный код, правда? Особенно если учесть тот факт, что переменная weights является параметром метода и никак не используется между этими условиями. Таким образом, второй оператор if никогда не будет выполнен, так как если выражение weights != null истинно, будет сгенерировано исключение типа ArgumentException.
Этот код встретился ещё раз в этом же файле в строке 687.
Условия, значения которых всегда ложны
Диагностическое правило V3022 очень похорошело с первого релиза и не перестаёт меня удивлять. Давайте посмотрим, сможет ли оно удивить и вас. Для начала предлагаю найти ошибку в коде, не прибегая к помощи диагностического сообщения анализатора. При этом обращаю внимание на то, что это уже упрощённый вариант, с вырезанными фрагментами кода.
private static void dscal(int n, double da, double[] dx,
int _dx_offset, int incx)
{
....
if (((n <= 0) || (incx <= 0)))
{
return;
}
....
int _i_inc = incx;
for (i = 1; (_i_inc < 0) ? i >= nincx : i <= nincx; i += _i_inc)
....
}Предупреждение PVS-Studio: V3022 Expression '(_i_inc < 0)' is always false. Accord.Math BoundedBroydenFletcherGoldfarbShanno.FORTRAN.cs 5222
Безусловно, сейчас ошибку найти легче, так как из метода специально были вырезаны фрагменты, не интересные нам. И всё же сходу трудно сказать, где же в этом коде проблемное место. А дело в том (как вы могли догадаться, прочитав сообщение анализатора), что выражение (_i_inc < 0) всегда ложно. Здесь следует обратить внимание на то, что переменная _i_inc инициализируется значением переменной incx. В то же время значение переменной incx на момент инициализации _i_inc положительно, так как иначе был бы осуществлён выход из тела метода выше по коду. Следовательно, _i_inc может иметь только положительное значение, результат сравнения _i_inc < 0 всегда имеет значение false, и условием выхода из цикла всегда будет выражение i <= nincx.
Такой углубленный анализ стал доступен благодаря механизму виртуальных значений, который сильно улучшил некоторые диагностические правила анализатора.
private void hqr2()
{
....
int low = 0;
....
for (int i = 0; i < nn; i++)
{
if (i < low | i > high)
....
}
....
}Предупреждение PVS-Studio: V3063 A part of conditional expression is always false: i < low. Accord.Math JaggedEigenvalueDecompositionF.cs 571
Подвыражение i < low всегда будет иметь значение false, так как минимальное значение, принимаемое переменной i - 0, а значение low на момент сравнения также всегда будет равно 0. Следовательно, подвыражение i < low будет вычисляться "вхолостую".
Таких мест встретилось много. Все предупреждения приводить не буду, но несколько перечислю:
Целочисленное деление с приведением к вещественному типу
Анализатор нашёл в коде подозрительные математические вычисления. Достаточно просто забыть о том, что при делении целых выполняется целочисленное деление. Если подразумевалось вещественное деление, может возникнуть неприятная и трудноуловимая ошибка. В некоторых случаях стороннему разработчику бывает сложно сказать, содержится ли в таком выражении ошибка, но проверить подобные операции стоит. Предлагаю рассмотреть несколько подобных случаев.
public static double GetSpectralResolution(int samplingRate,
int samples)
{
return samplingRate / samples;
}Предупреждение PVS-Studio: V3041 The expression was implicitly cast from 'int' type to 'double' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. Accord.Audio Tools.cs 158
Данный метод выполняет деление двух целочисленных переменных, однако результат этого деления неявно приводится к типу double. Выглядит подозрительно.
А вот следующий код выглядит ещё более странно:
public static int GreatestCommonDivisor(int a, int b)
{
int x = a - b * (int)Math.Floor((double)(a / b));
while (x != 0)
{
a = b;
b = x;
x = a - b * (int)Math.Floor((double)(a / b));
}
return b;
}Предупреждения PVS-Studio:
Анализатору показалось подозрительным выражение (double)(a / b). Метод Floor возвращает наибольшее целое число, меньшее или равное заданному числу двойной точности с плавающей запятой (MSDN. Math.Floor). Но переменные a и b имеют тип int, следовательно - будет выполнено целочисленное деление, результатом которого будет целочисленное значение. Выходит, что нет никакого смысла в явном приведении этого значения к типу double и вызову метода Floor.
Для корректного выполнения операции было нужно привести к типу double один из операндов. Тогда было бы выполнено вещественное деление, и вызов метода Floor был бы оправдан:
Math.Floor((double)a / b)Параметр метода, значение которого всегда перезаписывается
Продолжим. Ошибки следующего типа встречаются не часто, но всё же попадаются.
private static double WeightedMode(double[] observations,
double[] weights,
double mode,
int imax,
int imin)
{
....
var bestValue = currentValue;
....
mode = bestValue;
return mode;
}Предупреждение PVS-Studio: V3061 Parameter 'mode' is always rewritten in method body before being used. Accord.Statistics TriangularDistribution.cs 646
Один из параметров метода - mode - перезаписывается и возвращается. При этом в теле метода mode никак не используется (не считая перезаписи). Не возьмусь утверждать, что это ошибка (среди подозрительных мест, найденных этим диагностическим правилом в других проектах, явно было видно ошибочные), но выглядит этот код странно.
Вообще есть интересная особенность: как правило, ошибки в этом проекте не встречаются поодиночке. Точно такой же код можно найти в нескольких местах. Действительно, copy-paste не меняется...
Разыменовывание нулевой ссылки
public override string ToString(string format,
IFormatProvider formatProvider)
{
....
var fmt = components[i] as IFormattable;
if (fmt != null)
sb.AppendFormat(fmt.ToString(format, formatProvider));
else
sb.AppendFormat(fmt.ToString());
....
}Предупреждение PVS-Studio: V3080 Possible null dereference. Consider inspecting 'fmt'. Accord.Statistics MultivariateMixture'1.cs 697
Обычно ошибки, приводящие к возникновению исключений, отлавливаются в процессе разработки, если код тестируется достаточно тщательно. Но не всегда, и пример выше - тому доказательство. В случае, если выражение fmt != null ложно, у объекта fmt вызывается экземплярный метод ToString. Результат? Исключение типа NullReferenceException.
И, как вы уже могли догадаться, такая же ошибка встретилась ещё раз: MultivariateMixture'1.cs 697
Взаимное присвоение ссылок
public class MetropolisHasting<T> : IRandomNumberGenerator<T[]>
{
....
T[] current;
T[] next;
....
public bool TryGenerate()
{
....
var aux = current;
current = next;
next = current;
....
}
....
}Предупреждение PVS-Studio: V3037 An odd sequence of assignments of this kind: A = B; B = A;. Check lines: 290, 289. Accord.Statistics MetropolisHasting.cs 290
Очевидно, что в приведённом фрагменте метода TryGenerate хотели поменять местами ссылки на массивы next и current (переменная aux больше нигде не используется). Но из-за ошибки получилось, что и current, и next теперь хранят ссылки на один и тот же массив - тот, ссылка на который содержалась в переменной next.
Правильный код должен был выглядеть так:
var aux = current;
current = next;
next = aux;Потенциальное деление на 0
При анализе обнаружилось несколько ошибок потенциального деления на 0. Мельком взглянем на них:
public BlackmanWindow(double alpha, int length)
: base(length)
{
double a0 = (1.0 - alpha) / 2.0;
double a1 = 0.5;
double a2 = alpha / 2.0;
for (int i = 0; i < length; i++)
this[i] = (float)(a0 -
a1 * Math.Cos((2.0 * System.Math.PI * i) / (length - 1)) +
a2 * Math.Cos((4.0 * System.Math.PI * i) / (length - 1)));
}Предупреждения PVS-Studio:
Анализатор счёл подозрительным следующий код:
(2.0 * System.Math.PI * i) / (length - 1)Возможно, что в реальности ошибки здесь и не будет (length - длина какого-то окна, и для возникновения ошибки она должна иметь значение 1), но как знать? Лучше перестраховаться, иначе велик шанс получить очень неприятную ошибку, которую к тому же будет тяжело обнаружить.
Встретился ещё один интересный фрагмент кода с потенциальным делением на 0.
public static double[,] Centering(int size)
{
if (size < 0)
{
throw new ArgumentOutOfRangeException("size", size,
"The size of the centering matrix must
be a positive integer.");
}
double[,] C = Matrix.Square(size, -1.0 / size);
....
}Предупреждение PVS-Studio: V3064 Potential division by zero. Consider inspecting denominator 'size'. Accord.Math Matrix.Construction.cs 794
Как по мне - весьма интересная и забавная ошибка. Анализатор отметил, что в выражении -1.0 / size возможно деление на 0. А теперь обратите внимание на проверку выше. Если size < 0, будет сгенерировано исключение, однако если size == 0, исключения не будет, а вот деление на 0 - будет. При этом в литерале, передаваемом конструктору исключения, написано, что размер матрицы должен быть положительным числом, но проверка при этом осуществляется на неотрицательные значения. А положительное и неотрицательное - всё же разные понятия. Полагаю, для исправления ошибки достаточно просто подправить проверку:
if (size <= 0)Использование битового оператора вместо логического
Порой приходится сталкиваться с тем, что не все программисты знают разницу между битовыми и логическими операторами ('|' и '||', '&' и '&&'). Это может приводить к совершенно разным последствиям - от лишних вычислений до падений. В данном проекте встретилось несколько подозрительных мест с использованием битовых операций:
public JaggedSingularValueDecompositionF(
Single[][] value,
bool computeLeftSingularVectors,
bool computeRightSingularVectors,
bool autoTranspose,
bool inPlace)
{
....
if ((k < nct) & (s[k] != 0.0))
....
}Предупреждение PVS-Studio: V3093 The '&' operator evaluates both operands. Perhaps a short-circuit '&&' operator should be used instead. Accord.Math JaggedSingularValueDecompositionF.cs 461
Тело оператора if будет выполнено, если оба подвыражения (k < nct и s[k] != 0.0) имеют значение true. Но при этом, даже если значение первого подвыражения (k < nct) имеет значение false, второе подвыражение всё равно будет вычисляться, чего не было бы при использовании оператора &&. Итак, если программист проверял значение k, чтобы не выйти за границу массива, то у него ничего из этого не вышло.
Схожие предупреждения:
Проверка в цикле одного и того же элемента
Встретилась ошибка, найденная диагностическим правилом из последнего релиза анализатора.
public override int[] Compute(double[][] data, double[] weights)
{
....
int cols = data[0].Length;
for (int i = 0; i < data.Length; i++)
if (data[0].Length != cols)
throw new DimensionMismatchException("data",
"The points matrix should be rectangular.
The vector at position {} has a different
length than previous ones.");
....
}Предупреждение PVS-Studio: V3102 Suspicious access to element of 'data' object by a constant index inside a loop. Accord.MachineLearning BinarySplit.cs 121
Интересная ошибка. Программист хотел проверить, что зубчатый массив data на самом деле является двумерным (т.е. является матрицей). Но ошибся в выражении data[0].Length != cols, использовав в качестве значения индекса не счётчик цикла i, а целочисленный литерал - 0. В итоге выражение data[0].Length != cols всегда будет ложно, так как оно эквивалентно выражению data[0].Length != data[0].Length. Если бы параметр data был двумерным массивом (Double[,]), этой ошибки, равно как и всей проверки, можно было бы избежать. Но возможно, что использование зубчатого массива обусловлено какими-то особенностями архитектуры приложения.
Передача методу в качестве аргумента вызывающего объекта
Следующий фрагмент кода также выглядит подозрительно.
public static double WeightedMean(this double[] values,
double[] weights)
{
....
}
public override void Fit(double[] observations,
double[] weights,
IFittingOptions options)
{
....
mean = observations.WeightedMean(observations);
....
}Предупреждение PVS-Studio: V3062 An object 'observations' is used as an argument to its own method. Consider checking the first actual argument of the 'WeightedMean' method. Accord.Statistics InverseGaussianDistribution.cs 325
Анализатор счёл подозрительным факт, что метод WeightedMean в качестве аргумента принимает тот же объект, у которого он вызывается. Это становится вдвойне странным, если учесть, что WeightedMean - метод-расширения. Я провёл дополнительное исследование, посмотрев, как используется данный метод в других местах. Во всех местах его использования в качестве второго аргумента выступал массив weights (обратите внимание, что такой массив есть и в рассматриваемом методе Fit), так что скорее всего это ошибка и правильный код должен выглядеть так:
mean = observations.WeightedMean(weights);Потенциальная ошибка сериализации
Обнаружилась потенциальная проблема сериализации одного из классов.
public class DenavitHartenbergNodeCollection :
Collection<DenavitHartenbergNode>
{ .... }
[Serializable]
public class DenavitHartenbergNode
{
....
public DenavitHartenbergNodeCollection Children
{
get;
private set;
}
....
}Предупреждение PVS-Studio: V3097 Possible exception: the 'DenavitHartenbergNode' type marked by [Serializable] contains non-serializable members not marked by [NonSerialized]. Accord.Math DenavitHartenbergNode.cs 77
При сериализации экземпляра класса DenavitHartenbergNode возможно возникновение исключения типа SerializationException. Всё зависит от выбранного типа сериализатора. Если в качестве сериализатора выступает, например, экземпляр типа BinaryFormatter, будет сгенерировано исключение, так как требуется, чтобы все сериализуемые члены (а это свойство тоже сериализуемо) были декорированы атрибутом [Serializable].
Некоторые способы решения:
Судя по окружающему коду (все остальные свойства имеют сериализуемые типы), необходимо реализовать последний случай.
Если же экземпляры данного типа сериализуются с использованием сериализаторов, не требующих, чтобы все сериализуемые члены были декорированы атрибутом [Serializable], можно не волноваться на счёт этого кода.
Анализатором было обнаружено очень много небезопасных фрагментов вызовов событий. Насколько много? 75 предупреждений V3083! Предлагаю рассмотреть один фрагмент кода, так как остальные, в принципе, схожи с ним.
private void timeUp_Elapsed(object sender, ElapsedEventArgs e)
{
....
if (TempoDetected != null)
TempoDetected(this, EventArgs.Empty);
}Предупреждение PVS-Studio: V3083 Unsafe invocation of event 'TempoDetected', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. Accord.Audio Metronome.cs 223
В данном фрагменте кода проверяется, есть ли подписчики у события TempoDetected, и, если они есть, вызывается событие. Предполагалось, что, если на момент проверки у TempoDetected не будет подписчиков, это позволит избежать возникновения исключения. Однако есть вероятность, что в момент между проверкой TempoDetected на неравенство null и вызовом события, у него не останется подписчиков (например, в других потоках произойдёт отписка от этого события). В итоге, если на момент вызова события у TempoDetected не будет подписчиков, будет сгенерировано исключение типа NullReferenceException. Избежать подобных проблем можно, например, использую null-условный оператор '?.', добавленный в C# 6.0. Более подробно прочитать о проблеме и других способах её решения можно в документации к этому диагностическому правилу.
Перед тем, как закончить, я бы хотел немного рассказать о том, как же всё-таки необходимо использовать инструменты статического анализа. Часто приходится сталкиваться с таким подходом: "Мы проверили перед релизом наш проект. Что-то ничего особо интересного не нашлось." Нет, нет, нет! Это самый неправильный способ использования. В качестве параллели можно привести такой пример - откажитесь от разработки программ в IDE, пишите всё в простом блокноте. И перед самым релизом начинайте работать в IDE. Странно? Ещё бы! Толку от этой IDE, если большую часть времени, когда от неё был бы толк, она пылилась у вас на SSD/HDD? Такая же ситуация и со статическими анализаторами. Применять их нужно регулярно, а не разово.
Если вы проверяете анализатором код приложения только перед релизом, очевидно, что многие ошибки будут уже исправлены. Но какой ценой? Ценой нервов и времени разработчиков, писавших код, ценой многочисленных тестов, призванных выявлять эти самые ошибки. В итоге стоимость таких ошибок становится, мягко говоря, немалой.
В то же время всего этого можно было бы избежать, если правильно внедрить анализатор в процесс разработки. Установив анализатор локально на машину каждому разработчику, можно добиться того, чтобы большинство ошибок, фиксируемых анализатором, обнаруживались и исправлялись ещё до момента попадания в репозиторий. Более того, обнаруженная и исправленная ошибка, ещё не успевшая обрасти различными зависимостями, будет стоить гораздо дешевле. Ещё больше поспособствовать обнаружению ошибок поможет режим инкрементального анализа, использование которого позволяет выявить ошибку сразу после её появления.
Полезным шагом будет внедрение статического анализа и в процесс ночных сборок. Это также позволит достаточно быстро обнаруживать ошибки, а также позволит узнать, кто допустил их попадание в репозиторий. Для разработчиков это также будет дополнительным стимулом к более аккуратному написанию кода.
Таким образом, именно регулярное использование инструментов статического анализа позволит получить от них наибольшую пользу.
В очередной раз удалось проверить интересный проект, найти не менее интересные ошибки и рассказать о них. Возможно, кто-то что-то возьмёт на заметку, узнает что-то новое или просто будет аккуратнее при написании кода. Тем не менее все мы люди, а человеку свойственно ошибаться. Помочь исправить ошибки, допущенные в программном коде, поможет PVS-Studio. Регулярное применение статического анализатора позволит уменьшить головную боль, связанную с поиском ошибок и их исправлением.