MassTransit — Open Source платформа распределённых приложений для .NET. В этой статье мы расскажем о проблемных местах в коде проекта. С поиском таких мест нам поможет статический анализатор. Приятного чтения :).
Так как MassTransit предоставляет публичное API, качество кода такого проекта имеет особенно важную роль. Неожиданное поведение данного продукта может негативно сказаться не только на нём самом, но и на использующих его приложения. Это одна из причин, по которой мы решили проверить MassTransit. Для анализа был взят исходный код, соответствующий версии v8.0.15.
Не будем томить, приступим к разбору наиболее интересных фрагментов кода.
Issue 1
public async Task Send(OutboxConsumeContext<TMessage> context)
{
....
if ( !context.ReceiveContext.IsDelivered // <=
&& !context.ReceiveContext.IsDelivered) // <=
{
await context.NotifyConsumed(context,
timer.Elapsed,
_options.ConsumerType)
.ConfigureAwait(false);
}
....
}Предупреждение PVS-Studio: V3001 There are identical sub-expressions '!context.ReceiveContext.IsDelivered' to the left and to the right of the '&&' operator. OutboxMessagePipe.cs 57
Обратите внимание на условие if. Выглядит странно, ведь проверяется значение одного и того же свойства. Очевидно, что подобная проверка не имеет смысла. Однако если посмотреть свойства, к которым можно обратиться у объекта context.ReceiveContext, то можно найти IsFaulted. Возможно, именно оно должно фигурировать в одном из случаев.
Issue 2, 3
public static IEnumerable<Assembly> FindAssemblies(....)
{
var assemblyPath = AppDomain.CurrentDomain.BaseDirectory;
var binPath = string.Empty; // <=
if (string.IsNullOrEmpty(binPath)) // <=
return FindAssemblies(assemblyPath,loadFailure, includeExeFiles, filter);
if (Path.IsPathRooted(binPath))
return FindAssemblies(binPath, loadFailure, includeExeFiles, filter);
string[] binPaths = binPath.Split(';');
return binPaths.SelectMany(bin =>
{
var path = Path.Combine(assemblyPath, bin);
return FindAssemblies(path, loadFailure, includeExeFiles, filter);
});
}Здесь PVS-Studio выдаёт сразу два предупреждения:
Анализатор сообщает нам, что в данном коде обнаружено всегда истинное условие, а также недостижимый код. Давайте разберёмся, как же так получилось.
В переменную binPat присваивается пустая строка. Сразу за присваиванием проверяется, что значение этой переменной равно null или пустой строке. Результат такой проверки — всегда истина. Получается, что выполнение метода всегда заканчивается на этом условии, так как в блоке then возвращается значение.
Тяжело сказать, является это ошибкой или такое поведение реализовано намерено. В любом случае код выглядит подозрительно, так как теряется немалая часть логики метода.
Issue 4
public override Point? Read(ref Utf8JsonReader reader,
Type typeToConvert,
JsonSerializerOptions options)
{
if (reader.TokenType != JsonTokenType.StartObject) // <=
throw new JsonException("....");
var originalDepth = reader.CurrentDepth;
if (reader.TokenType == JsonTokenType.Null) // <=
{
reader.Read();
return null;
}
reader.Read();
var x = double.NaN;
var y = double.NaN;
while (reader.TokenType == JsonTokenType.PropertyName)
{
....
}
....
}Предупреждение PVS-Studio: V3022 Expression 'reader.TokenType == JsonTokenType.Null' is always false. DoubleActivity_Specs.cs 55
Рассмотрим первое условие метода Read. В нём производится проверка значения reader.TokenType. Если оно не соответствует JsonTokenType.StartObject, то будет выброшено исключение. Из этого следует, что после условия и до следующего вызова метода Read, reader.TokenType будет иметь значение JsonTokenType.StartObject. Получается, что в блок then второго if поток выполнения не зайдёт никогда. Возможно, после первого if был пропущен вызов Read.
Issue 5
private static void
ReturnClosureTypeToParamTypesToPool(Type[] closurePlusParamTypes)
{
var paramCount = closurePlusParamTypes.Length - 1; // <=
if (paramCount != 0 && paramCount < 8)
Interlocked.Exchange(ref _closureTypePlusParamTypesPool[paramCount],
closurePlusParamTypes);
}Предупреждение PVS-Studio: V3106 Possible negative index value. The value of 'paramCount' index could reach -1. ExpressionCompiler.cs 555
Обратите внимание на переменную paramCount. Её значение — размер массива, переданного в качестве аргумента, уменьшенный на 1. Дальше это значение используется в качестве индекса. Однако проверка перед использованием не гарантирует, что индекс не отрицательный. В случае обращения по индексу с отрицательным значением будет выброшено исключение.
Возможно, разработчики уверены в том, что closurePlusParamTypes всегда содержит хотя бы один элемент. Может быть, в данный момент это действительно так. Но нет гарантий того, что в будущем в данный метод не будет передана пустая коллекция.
Issue 6
public static bool TryEmit(....)
{
....
if ((parent & ParentFlags.InlinedLambdaInvoke) != 0)
{
var index = closure.GetLabelOrInvokeIndex(gt.Target); // <=
var invokeIndex = closure.Labels
.Items[index] // <=
.InlinedLambdaInvokeIndex;
....
}
....
}Предупреждение PVS-Studio: V3106 Possible negative index value. The value of 'index' index could reach -1. ExpressionCompiler.cs 1977
Анализатор указывает на потенциальное обращение по отрицательному индексу. Переменная index содержит значение, использующееся в качестве индекса. Оно будет получено в результате выполнения метода GetLabelOrInvokeIndex. Рассмотрим определение этого метода:
public short GetLabelOrInvokeIndex(object labelTarget)
{
var count = Labels.Count;
var items = Labels.Items;
for (short i = 0; i < count; ++i)
if (items[i].Target == labelTarget)
return i;
return -1;
}Здесь мы видим, что один из вариантов возвращаемого значения — -1. Нетрудно догадаться, что такой сценарий не совсем желателен, так как будет выброшено исключение.
Стоит отметить, что при работе с возвращаемым значением GetLabelOrInvokeIndex в ряде других мест присутствует проверка на то, что оно не равно -1. Одна их них:
public short AddInlinedLambdaInvoke(InvocationExpression e)
{
var index = GetLabelOrInvokeIndex(e);
if (index == -1) // <=
{
ref var label = ref Labels.PushSlot();
label.Target = e;
index = (short)(Labels.Count - 1);
}
return index;
}Issue 7
[Serializable]
public class BusHostInfo : HostInfo
{
public BusHostInfo()
{
}
public BusHostInfo(bool initialize)
{
FrameworkVersion = Environment.Version.ToString();
OperatingSystemVersion = Environment.OSVersion.ToString();
var entryAssembly = System.Reflection.Assembly.GetEntryAssembly()
?? System.Reflection.Assembly.GetCallingAssembly();
MachineName = Environment.MachineName;
MassTransitVersion = typeof(HostInfo).GetTypeInfo()
.Assembly
.GetName()
.Version
?.ToString();
try
{
using var currentProcess = Process.GetCurrentProcess();
ProcessId = currentProcess.Id;
ProcessName = currentProcess.ProcessName;
if ("dotnet".Equals(ProcessName, StringComparison.OrdinalIgnoreCase))
ProcessName = GetUsefulProcessName(ProcessName);
}
catch (PlatformNotSupportedException)
{
ProcessId = 0;
ProcessName = GetUsefulProcessName("UWP");
}
var assemblyName = entryAssembly.GetName();
Assembly = assemblyName.Name;
AssemblyVersion = assemblyName.Version?.ToString() ?? "Unknown";
}
....
}Предупреждение PVS-Studio: V3117 Constructor parameter 'initialize' is not used. BusHostInfo.cs 17
Параметр одной из перегрузок конструктора класса BusHostInfo не был использован. Стоит отметить, что у данного класса есть публичный конструктор без параметров, который вообще ничего не делает. В итоге получается следующая ситуация: если использовать конструктор с параметром, то вне зависимости от его значения инициализация производится. Если же воспользоваться конструктором, который не принимает параметров, то инициализация отсутствует. Такое поведение выглядит весьма странно.
Возможно, раньше параметр влиял на логику работы, однако перестал после рефакторинга. Разработчик мог намерено не удалять неиспользуемый параметр, чтобы избежать ошибок у пользователей. Ошибки могли возникнуть из-за того, что класс BusHostInfo публичный, как и рассматриваемый конструктор. Это значит, что он доступен пользователям напрямую. Если поменять сигнатуру метода (убрать параметр), то в местах вызова данного конструктора возникнут ошибки из-за отсутствия нужной перегрузки. Если данное предположение верно, то хотелось бы видеть комментарий, поясняющий этот момент.
Можно сказать, что код проекта оказался чистым. Во многом это заслуга разработчиков, однако стоит учесть, что они уже использовали другой статический анализатор. Это стало понятно, исходя из комментариев для подавления его срабатываний. Даже при этом условии удалось найти ряд подозрительных мест. Возможно, анализатором пользовались ранее, а сейчас перестали.
В любом случае, надеюсь, что эта статья поможет разработчикам найти и исправить ошибки, чтобы повысить качество и чистоту кода проекта :).
Если у вас появилось желание проверить исходный код интересующего проекта, то вы можете бесплатно попробовать PVS-Studio. Удачного использования!