Приветствую всех любителей чистого кода! Сегодня мы разбираем проект PascalABC.NET. В 2017 году мы уже находили ошибки в этом проекте. Мы использовали два инструмента статического анализа (точнее, плагины для SonarQube): SonarC# и PVS-Studio. Сегодня мы анализируем этот проект с помощью последней версии анализатора PVS-Studio для C#. Давайте посмотрим, какие ошибки мы можем найти сегодня, особенно когда наш анализатор стал более продвинутым и получил новые возможности: он может находить более изощренные ошибки и потенциальные уязвимости.
Введение
У меня есть интересная история о PascalABC.NET. Сразу после публикации Анализ PascalABC.NET с помощью плагинов SonarQube: SonarC# и PVS-Studio мы случайно пересеклись с разработчиками на одной из конференций. Похоже, мы сделали это специально: написали статью об ошибках, найденных в их проекте, и поехали на конференцию, чтобы обсудить эти ошибки с разработчиками. Конечно, мы никогда этого не планировали, это было совпадение. Но это было забавно. После этого я рассматривал идею перепроверить проект, но у меня не было на это времени. Теперь пришло время.
PascalABC.NET — это современная реализация языка Pascal в .NET. Вы можете посетить сайт проекта, чтобы прочитать описание и убедиться, что проект развивается. Последняя версия 3.8.1 вышла в августе 2021 года. Хорошая новость — нет смысла перепроверять заброшенный проект. Это послужило дополнительной мотивацией для написания этой статьи. Развивающийся проект — это исправление старых ошибок и появление новых.
Для анализа я взял исходники с GitHub от 12.10.2021. Обратите внимание, что пока я писал статью, код мог измениться. Пожалуйста, примите во внимание этот факт, если вы собираетесь самостоятельно проверять исходный код PascalABC.NET. Кстати, вы легко можете запросить пробную версию PVS-Studio. Не забывайте о нашей новой функции Лучшие предупреждения, которая сразу показывает самые интересные ошибки. Это важно, когда вы работаете с такими большими проектами.
К сожалению, многие ошибки, обнаруженные в 2017 году, так и не были исправлены. После публикации статьи мы всегда отправляем разработчикам баг-репорты. Однако исправить эти ошибки могут только разработчики. Это было дополнительной проблемой, так как приходилось исключать старые ошибки из отчета. Несмотря на это, при перепроверке проекта нам удалось найти несколько новых и интересных ошибок. Вы можете увидеть их ниже.
Ошибки
Начнем с классики — ошибок копипаст. Невероятно, но разработчики снова и снова допускают такие ошибки. Это означает, что у PVS-Studio определенно будет работа. Кроме того, такие ошибки показывают важное преимущество инструментов статического анализа: постоянное внимание к деталям. У людей не всегда это происходит из-за усталости и других причин.
V3001 Слева и справа от оператора || находятся одинаковые подвыражения. NETGenerator.cs 461
public class CompilerOptions
{
public enum PlatformTarget { x64, x86, AnyCPU,
dotnet5win, dotnet5linux, dotnet5macos };
....
}
....
bool IsDotnet5()
{
return
comp_opt.platformtarget ==
CompilerOptions.PlatformTarget.dotnet5win ||
comp_opt.platformtarget ==
CompilerOptions.PlatformTarget.dotnet5linux ||
comp_opt.platformtarget ==
CompilerOptions.PlatformTarget.dotnet5linux;
}В этом фрагменте кода разработчик повторно сравнивает метод IsDotnet5() со значением перечисления CompilerOptions.PlatformTarget.dotnet5linux. Если мы посмотрим на объявление перечисления PlatformTarget, то можем предположить, что код должен выглядеть так:
bool IsDotnet5()
{
return
comp_opt.platformtarget ==
CompilerOptions.PlatformTarget.dotnet5win ||
comp_opt.platformtarget ==
CompilerOptions.PlatformTarget.dotnet5linux ||
comp_opt.platformtarget ==
CompilerOptions.PlatformTarget.dotnet5macos;
}Обратите внимание, что код был отформатирован для удобства чтения. В исходной версии все выражение return записывается в одну строку.
V3001 Слева и справа от оператора || одинаковые подвыражения ctn2.compiled_type == TypeFactory.ObjectType. NETGenerator.cs 8518
private void AssignToDereferenceNode(....)
{
....
if (.... && (ctn2.compiled_type == TypeFactory.ObjectType ||
(ctn2.compiled_type == TypeFactory.ObjectType ||
ctn2.compiled_type.IsInterface)))
....
}Здесь разработчик сравнивает то же значение со значением TypeFactory.ObjectType. Код был отформатирован еще раз. В исходной версии выражение if записывалось в одну строку. Думаю, человеку довольно сложно заметить проблемы в таком коде. Трудно сказать, как исправить эту ошибку, так как в классе TypeFactory очень много полей.
V3001 Слева и справа от оператора || одинаковые подвыражения SK == SymKind.field. LightScopeHelperClasses.cs 30
public enum SymKind { var, field, param, procname, funcname,
classname, recordname, interfacename };
....
public class SymInfoSyntax
{
public override string ToString()
{
....
if (SK == SymKind.var ||
SK == SymKind.field ||
SK == SymKind.field ||
SK == SymKind.param)
....
}
....
}В одном из сравнений SK == SymKind.field есть ошибка. Он должен содержать другое значение перечисления SymKind. Возможно, разработчик, написавший этот фрагмент кода, мог бы объяснить, что происходит.
V3004 [CWE-691] Оператор тогда эквивалентен оператору еще. Таблица символов.cs 870
private Scope FindClassScope(Scope scope)
{
while (scope != null && !(scope is ClassScope))
if(scope is ClassMethodScope)
scope = scope.TopScope;
else
scope = scope.TopScope;
return scope;
}Другой шаблон ошибки, тот же копипаст: оба блока кода оператора if идентичны. Здесь нам также нужен разработчик, чтобы проверить и исправить эту ошибку.
V3005 Переменная e присваивается самой себе. generics.cs 430
public static type_node determine_type(....)
{
....
try
{
return ....;
}
catch(Exception e)
{
e = e;
}
....
}Немного странный код. Это может быть ошибка копирования-вставки, а также попытка подавить предупреждение о неиспользуемой переменной. Или это может быть следствием рефакторинга. Возможно, раньше была какая-то внешняя переменная e относительно блока catch, и ее потом удалили. В любом случае, код выглядит неряшливо.
Помимо ошибок копирования-вставки, я обнаружил и другие проблемы в коде PascalABC.NET.
V3022 [CWE-570] Выражение t != null всегда ложно. Посетитель.cs 598
public void prepare_collection(....)
{
myTreeNode t;
....
if (t == null)
{
....
if (t != null)
t.Nodes.Add(tn);
else
nodes.Add(tn);
....
}
....
}Это произошло после рефакторинга? Был ли разработчик чрезмерно осторожен или просто невнимателен? В результате ветвь then функции t.Nodes.Add(tn) в блоке if никогда не выполняется. Код нужно исправить.
V3027 [CWE-476] Переменная fn.return_value_type использовалась в логическом выражении до того, как она была проверена на нуль в том же логическом выражении. NetHelper.cs 1109
private static function_node get_conversion(....)
{
....
function_node fn = si.sym_info as function_node;
if (.... || fn.return_value_type.original_generic == to || ....
&& fn.return_value_type != null && ....)
{
return fn;
}
....
}Переменная fn.return_value_type разыменовывается без проверки null. Автор предположил, что переменная может быть null, потому что она проверяется явно.
V3032 [CWE-835] Ожидание этого выражения ненадежно, так как компилятор может оптимизировать некоторые переменные. Чтобы избежать этого, используйте изменчивые переменные или примитивы синхронизации. RemoteCompiler.cs 407
CompilerState compilerState = CompilerState.Reloading;
....
public string Compile()
{
....
compilerState = CompilerState.CompilationStarting;
....
while (compilerState != CompilerState.Ready)
Thread.Sleep(5);
....
}Интересная ошибка, связанная с особенностями компилятора. Проблема может проявиться в релизной версии: из-за оптимизаций цикл while будет бесконечным. Особенности этой ошибки и варианты исправления описаны в документации V3032.
V3043 [CWE-483] Логика работы кода не соответствует его форматированию. Оператор имеет отступ вправо, но выполняется всегда. Возможно, фигурные скобки отсутствуют. Компилятор.cs 2196
public string Compile()
{
....
int n = 1;
try
{
n = 2;
....
if (File.Exists(pdb_file_name))
File.Delete(pdb_file_name);
n = 5;
....
}
....
}Может показаться, что выражение n = 5 относится к блоку if, но это не так. Код был плохо отформатирован. Это предупреждение является лишь примером. Редкая ошибка, которая не приводит к ошибке в данном случае. Но это не всегда так. На нашем сайте есть раздел со списком найденных ошибок в проектах. В этом списке есть ошибки, обнаруженные в V3043, среди многих других. Одна из перечисленных здесь ошибок V3043 относится к проекту PascalABC.NET. Я описал ее, когда впервые проверил проект в 2017 году. Эта ошибка похожа на другие ошибки, но она более опасна. Вы можете перейти по ссылке и посмотреть на эту ошибку. Просто прокрутите немного вниз, чтобы добраться до PascalABC.NET.
Перед тем, как перейти к следующей ошибке, предлагаю посмотреть фрагмент кода и самостоятельно найти ошибку:
public static typed_expression
GetTempFunctionNodeForTypeInference(....)
{
....
for (int i = 0; i < def.formal_parameters.params_list.Count; i++)
{
....
for (int j = 0;
j < def.formal_parameters.params_list[i].idents.idents.Count;
j++)
{
var new_param = new common_parameter(....,
visitor.get_location(
def.formal_parameters.params_list[i].idents.idents[0]));
....
}
}
....
}Вы нашли это? Честно говоря, даже при предупреждении анализатора я не сразу понял в чем проблема. И да, код был отформатирован для удобства чтения. Первоначальная версия была менее читабельной. Вот предупреждение анализатора: V3102 Подозрительный доступ к элементу объекта def.formal_parameters.params_list[i].idents.idents по постоянному индексу внутри цикла. LambdaHelper.cs 402
Посмотрите внимательно на вычисление значения переменной new_param. Все итерации вложенного цикла используют доступ к нулевому элементу списка def.formal_parameters.params_list[i].idents.idents[0]. Все указывает на то, что вместо 0 нужно было использовать индекс j.
Ниже последняя ошибка, которую я хотел вам показать.
V3146 [CWE-476] Возможно нулевое разыменование. symbolInfo.FirstOrDefault() может возвращать нулевое значение по умолчанию. SystemLibInitializer.cs 112
public class SymbolInfo
{
....
}
....
List<TreeConverter.SymbolInfo> symbolInfo = null;
....
public List<TreeConverter.SymbolInfo> SymbolInfo
{
get
{
if (symbolInfo != null && ....)
{
if (symbolInfo.FirstOrDefault().sym_info is common_type_node)
....
}
}
}Посмотрите на состояние второго блока if. Ссылка symbolInfo была проверена на null ранее, здесь нет вопросов. Однако разработчики забыли, что метод FirstOrDefault() может возвращать значение по умолчанию (null) для типа SymbolInfo, если список symbolInfo не содержать любой элемент. Это вызовет проблемы при доступе к свойству sym_info по нулевой ссылке.
Заключение
Это небольшая статья. Но это не значит, что в PascalABC.NET мало ошибок. Большинство из этих ошибок я описал в 2017 году, но разработчики так и не исправили их. После последней проверки анализатор выдал 400 предупреждений на уровне High. На Среднем уровне — 1364 предупреждения. Среди них много однотипных ошибок, поэтому описывать их не вижу смысла. Читатели смогут убедиться в этом сами, если решат проверить проект PascalABC.NET с помощью PVS-Studio и поискать ошибки, которые я описал в этой и предыдущих статьях.
На самом деле поздние исправления ошибок в открытом исходном коде — распространенная проблема. Мой товарищ по команде Андрей Карпов даже написал об этом статью: 1000 глаз, которые не хотят проверять открытый код.
Также должен отметить, что в ходе анализа я понял, что использование анализатора время от времени может быть неэффективным и неудобным. Ведь действительно сложно искать настоящие ошибки среди тысяч предупреждений. Кроме того, старые ошибки не исправляются, а предупреждения анализатора не подавляются. Я не думаю, что разработчики захотят выполнять такую тяжелую работу. Я понимаю их.
На наш взгляд, смысл статического анализатора в регулярных проверках. Код должен быть проверен сразу после того, как он был написан. Если анализатор находит ошибки в коде, их нужно немедленно исправлять.
Напомню, что современные статические анализаторы, в том числе и PVS-Studio, имеют массу возможностей для удобной работы с большими проектами. Особенно на этапе реализации с большой кодовой базой. В этом случае мы рекомендуем использовать подавление всех старых предупреждений и работать только с выданными для нового кода (инкрементный анализ). Старые ошибки можно исправлять понемногу, и они не будут отображаться в отчете анализатора. Об этих возможностях можно прочитать в статьях «Результаты базового анализа (подавление предупреждений для существующего кода)» и Режим инкрементного анализа в PVS-Studio.
На этом я заканчиваю эту статью и желаю всем вам чистого кода. Удачи.
