Мы используем куки, чтобы пользоваться сайтом
было удобно.
За 2023 год разработчиками PVS-Studio было написано немало статей о проверке Open Source C#-проектов. По традиции мы делимся с вами 10-ю самыми интересными ошибками, найденными за этот год. Приятного чтения!
Для попадания в топ нужно соответствовать нескольким критериям:
Как можно заметить, критериев не так много. Однако отобрать 10 лучших срабатываний оказалось не совсем тривиальной задачей. Да, хотелось бы рассмотреть больше ошибок, но мы решили не изменять традициям и составили топ-10.
Предыдущие подборки C# багов можно найти здесь:
Не буду томить, приступим к просмотру новых срабатываний!
P.S. Не стоит слишком серьёзно относиться к распределению позиций в топе. Порядок во многом формировался исходя из субъективного мнения автора.
Наш топ открывает срабатывание анализатора из статьи о проверке MudBlazor:
public static bool operator ==(ResizeOptions l, ResizeOptions r)
=> l.Equals(r);
public static bool operator !=(ResizeOptions l, ResizeOptions r)
=> !l.Equals(r);Предупреждения PVS-Studio:
В реализациях перегрузки операторов '==' и '!=' левый операнд не проверяется на null. Если значение параметра l – null, то при вызове метода Equals будет выброшено исключение типа NullReferenceException. Параметр метода Equals, кстати, так же разыменовывается без проверки:
public bool Equals(ResizeOptions other)
{
if (ReportRate != other.ReportRate || ....) // <=
{
return false;
}
....
}Такое поведение может стать весьма неприятным сюрпризом для пользователя, если класс публичный. Именно таким и является ResizeOptions.
Чтобы убедиться в наличии проблемы, попробуем сравнить объект типа ResizeOptions с null.
Вполне ожидаемо получаем исключение типа NullReferenceException. Если правый операнд — null, поведение будет аналогичным.
Кстати о необходимости учитывать такие случаи говорится в документации Microsoft.
На 9-ом месте расположилось срабатывание из статьи о проверке проекта Microsoft PowerToys:
public static List<PluginPair> AllPlugins
{
get
{
....
try
{
// Return a comparable produce version.
var fileVersion = FileVersionInfo.GetVersionInfo(x.ExecuteFilePath);
return ((uint)fileVersion.ProductMajorPart << 48)
| ((uint)fileVersion.ProductMinorPart << 32)
| ((uint)fileVersion.ProductBuildPart << 16)
| ((uint)fileVersion.ProductPrivatePart);
}
catch (System.IO.FileNotFoundException)
{
return 0U;
}
....
}
}Предупреждения PVS-Studio:
Не обращайте внимания на тип свойства и на тип фактически возвращаемого значения. Код пришлось сильно сократить, поэтому показанный отрывок относится к лямбда выражению. Анализатор выдал на него два предупреждения. Тип uint имеет размер 32 бита. Получается, что первый сдвиг на 48 бит эквивалентен сдвигу на 16. Второй сдвиг на 32 бита эквивалентен сдвигу на 0 бит.
Сложно сказать, что здесь хотели сделать. Но, возможно, вместо uint стоит использовать ulong.
На 8-ом месте ещё одно срабатывание из ранее упомянутой статьи о MudBlazor:
internal void DateValueChanged(DateTime? value)
{
_valueDate = value;
if (value != null)
{
var date = value.Value.Date;
// get the time component and add it to the date.
if (_valueTime != null)
{
date.Add(_valueTime.Value); // <=
}
_filterDefinition.Value = date;
}
else
_filterDefinition.Value = value;
_dataGrid.GroupItems();
}Предупреждение PVS-Studio: V3010 The return value of function 'Add' is required to be utilized. Filter.cs 140
При вызове метода Add у переменной типа DateTime не было использовано возвращаемое значение. Скорее всего, разработчик хотел добавить значение _valueTime.Value к date. Однако он не учёл, что метод Add для объекта типа DateTime возвращает результат добавления, а не изменяет исходный объект. В итоге имеем бесполезный вызов, который по задумке наверняка должен был на что-то влиять.
Следующее срабатывание было взято из статьи о проверке Ryujinx:
private void YesButton_Clicked(object sender, EventArgs args)
{
....
Window.Functions = _mainWindow.Window.Functions =
WMFunction.All & WMFunction.Close;
....
}Предупреждение PVS-Studio: V3182 The result of 'WMFunction.All & WMFunction.Close' expression is '0'. It is possible that the '|' operator should be used instead. UpdateDialog.cs 69
Как видно из предупреждения, анализатор ругается на оператор '&'. Чтобы понять причину, давайте взглянем на перечисление WMFunction:
[Flags]
public enum WMFunction
{
All = 0x1,
Resize = 0x2,
Move = 0x4,
Minimize = 0x8,
Maximize = 0x10,
Close = 0x20
}Мы имеем дело с битовыми флагами, но в данном случае реализация объединения не будет работать: результатом операции побитового И (&) для значений WMFunction.All и WMFunction.Close будет ноль.
Скорее всего, для корректной работы метода следует заменить '&' на '|'.
Движемся дальше. Сейчас перед нами предстанет срабатывание из статьи о проверке AWS SDK для .NET:
private static string GetXamarinInformation()
{
var xamarinDevice = Type.GetType("Xamarin.Forms.Device, Xamarin.Forms.Core");
if (xamarinDevice == null)
{
return null;
}
var runtime = xamarinDevice.GetProperty("RuntimePlatform")
?.GetValue(null)
?.ToString() ?? "";
var idiom = xamarinDevice.GetProperty("Idiom")
?.GetValue(null)
?.ToString() ?? "";
var platform = runtime + idiom;
if (string.IsNullOrEmpty(platform))
{
platform = UnknownPlatform;
}
return string.Format(CultureInfo.InvariantCulture, "Xamarin_{0}", "Xamarin");
}Предупреждение PVS-Studio: V3137 The 'platform' variable is assigned but is not used by the end of the function. InternalSDKUtils.netstandard.cs 70
Последняя строка метода выглядит очень странно. С помощью String.Format в шаблон "Xamarin_{0}" подставляют строковый литерал "Xamarin". При этом значение переменной platform, которое может хранить необходимую информацию, игнорируется. Выглядит странно.
Скорее всего, выражение return должно выглядеть так:
return string.Format(CultureInfo.InvariantCulture, "Xamarin_{0}", platform);Кстати, рядом есть похожий метод с получением информации о Unity. Он написан по схожему шаблону, но возвращаемое значение уже формируется нормально:
private static string GetUnityInformation()
{
var unityApplication
= Type.GetType("UnityEngine.Application, UnityEngine.CoreModule");
if (unityApplication == null)
{
return null;
}
var platform = unityApplication.GetProperty("platform")
?.GetValue(null)
?.ToString() ?? UnknownPlatform;
return string.Format(CultureInfo.InvariantCulture, "Unity_{0}", platform);
}Вторую половину топа открывает срабатывание из статьи о проверке BTCPay Server:
private IActionResult Validate(StoreBaseData request)
{
....
if (request.PaymentTolerance < 0 && request.PaymentTolerance > 100)
ModelState.AddModelError(nameof(request.PaymentTolerance),
"PaymentTolerance can only be between 0 and 100 percent");
....
}Предупреждение PVS-Studio: V3022 Expression 'request.PaymentTolerance < 0 && request.PaymentTolerance > 100' is always false. Probably the '||' operator should be used here. BTCPayServer\Controllers\GreenField\GreenfieldStoresController.cs 241
Каждый хоть раз сталкивался с отменой операции из-за неверного ввода в финансовых приложениях. Иногда они не очень удачно уведомляют пользователя о неверных данных, но иногда в коде просто есть глупая ошибка.
Так, в этом фрагменте кода перепутали операторы '&&' и '||'. Код предназначался для проверки значения на вхождение в диапазон от 0 до 100, но в итоге предупреждение пользователю не будет выдано.
Как результат — неверно введённое значение пройдёт дальше по коду и может привести к ошибке в логике приложения.
На 4-ом месте расположилось срабатывание, которое практически ничем не уступает финалистам. Его я взял из статьи о проверке MudBlazor.
internal async Task<bool> StartResizeColumn(....)
{
....
// In case resize mode is column, we have to find any column right
// of the current one that can also be resized and is not hidden.
var nextResizableColumn = _columns.Skip(_....) + 1)
.FirstOrDefault(c =>
c.Resizable ?? true && !c.Hidden); // <=
....
}Предупреждение PVS-Studio: V3177 The 'true' literal belongs to the '&&' operator with a higher priority. It is possible the literal was intended to belong to '??' operator instead. DataGridColumnResizeService.cs 52
Рассмотрим выражение c.Resizable ?? true && !c.Hidden. Стоит сразу сказать, что приоритет у '??' ниже, чем у '&&'. Исходя из этого, если c.Resizable — null, то оператор null-coalescing вернёт результат операции true && !c.Hidden. Операция "&&" с литералом true бессмысленна. Скорее всего, правильный вариант выглядит так:
(c.Resizable ?? true) && !c.HiddenРазница этих записей проявит себя, если значения c.Resizable и c.Hidden будут равны true.
Есть ещё один момент, указывающий, что была допущена ошибка. Строчкой выше написан комментарий, что столбец должен иметь возможность изменения размера (информация об этом содержится в c.Resizable) и не быть скрытым (это можно узнать из c.Hidden). Если c.Resizable — true, то логика работы будет нарушена, так как значение c.Hidden никак не повлияет на результат.
И наконец мы добрались до тройки лидеров. Её открывает срабатывание, которое я взял из статьи про Обзор Top-3 Open Source игр на C#. Проект, в котором была обнаружена ошибка — Barotrauma. Описанное срабатывание хорошо демонстрирует мощь статического анализа, так как найти такую ошибку глазами совсем нелёгкая задача.
Рассмотрим фрагмент кода с ошибкой:
public void RecreateSprites()
{
....
for (int i = 0; i < DecorativeSprites.Count; i++)
{
var decorativeSprite = DecorativeSprites[i];
decorativeSprite.Remove();
var source = decorativeSprite.Sprite.SourceElement;
DecorativeSprites[i] = new DecorativeSprite(source, ....);
}
}Предупреждение PVS-Studio: V3080. Possible null dereference. Consider inspecting 'decorativeSprite.Sprite'. Limb.cs 462.
В этом случае ошибка заключается в неправильном порядке выполнения операций, из-за чего в приведённом коде неизбежно будет выброшено исключение типа NullReferenceException. Чтобы это понять, достаточно посмотреть на реализацию метода decorativeSprite.Remove:
partial class DecorativeSprite : ISerializableEntity
{
....
public Sprite Sprite { get; private set; }
....
public void Remove()
{
Sprite?.Remove();
Sprite = null;
....
}
}В указанном методе свойству Sprite присваивается значение null.
Если ещё раз взглянуть на метод, в котором вызывается Remove, становится понятно, что при обращении к decorativeSprite.Sprite.SourceElement будет выброшено исключение. Такое поведение обусловлено вызовом Remove перед обращением.
Вероятно, здесь нужно поменять местами вызов метода Remove и инициализацию переменной source.
С небольшим отставанием от победителя на втором месте расположилось срабатывание из статьи "PVS-Studio научился анализировать Blazor компоненты". Кстати, статья не зря имеет такое название :). PVS-Studio действительно научился анализировать Blazor компоненты. Если у вас есть Blazor проект, то предлагаю опробовать на нём анализатор.
Вернёмся к предупреждению. Анализатор выдал его на код из проекта MudBlazor.
Рассмотрим подозрительный фрагмент кода:
@code
{
....
public void Evaluate()
{
....
var exp = new Expression(CalcExpression);
var result = exp.Eval();
if (result == double.NaN)
{
Current = "ERROR";
return;
}
Current = Math.Round( result,8).ToString(CultureInfo.InvariantCulture);
CalcExpression = Current;
}
....
}Предупреждение PVS-Studio: V3076 Comparison of 'result' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead.
Анализатор сообщает, что сравнение с double.NaN бессмысленно. Но почему? Согласно MSDN сравнение двух NaN значений через оператор '==' всегда возвращает false. Для корректного сравнения необходимо использовать метод double.IsNaN.
Вот такое простое срабатывание, но, думаю, оно достойно второй позиции в топе, так как для многих особенность сравнения с NaN может быть не совсем очевидна.
Вот он — венец сегодняшнего топа. Почему именно это срабатывание на первом месте? Как мне кажется, проблема, которую оно показывает, является самой сложной для выявления относительно других представителей топа. Это срабатывание было взято из статьи о проверке проекта Microsoft PowerToys.
Предлагаю вам найти ошибку самостоятельно:
private static int CalculateClosestSpaceIndex(List<int> spaceIndices,
int firstMatchIndex)
{
if (spaceIndices.Count == 0)
{
return -1;
}
else
{
int? ind = spaceIndices.OrderBy(item => (firstMatchIndex - item))
.Where(item => firstMatchIndex > item)
.FirstOrDefault();
int closestSpaceIndex = ind ?? -1;
return closestSpaceIndex;
}
}Удалось определить, в чём же здесь проблема? Буду признателен, если вы поделитесь своим вариантом в комментариях. Только без подглядываний в описание ошибки :).
Предупреждение PVS-Studio: V3022 Expression 'ind' is always not null. The operator '??' is excessive. StringMatcher.cs 230
Анализатор посчитал, что оператор '??' не нужен, так как ind всегда не равен null. Так ли это на самом деле? Ind имеет nullable тип, а значит в него можно записать null. Значение, записанное в Ind, было получено с использованием метода FirstOrDefault, который может вернуть null. Казалось бы, дело закрыто, анализатор ошибся. Однако не всё так просто. Копнём чуть глубже.
Если быть точнее, то FirstOrDefault возвращает не null, а default(TSource). default(int?) — это null. Вот только TSource в данном случае int, так как TSource берётся по типу элементов перечисляемой последовательности. В данном случае LINQ применяется к параметру spaceIndices с типом List<int>. Получается, что в ind будет записан default(int), то есть 0. А вот и ошибка в логике. Метод поиска в случае ненахождения вернёт 0, а не -1, как должен.
За 2023 мы проверили немало C# проектов, но и нельзя сказать, что много. Однако это не помешало составить этот топ. Даже наоборот, есть ряд интересных срабатываний, которые не были добавлены сюда, так как не хотелось изменять традициям топа 10-ти.
Предлагаю почитать несколько достойных статей, ошибки из которых по той или иной причине не попали в этот топ:
Вы также можете попробовать составить собственный топ срабатываний или же просто проверить интересующий проект :). Для этого предлагаю вам попробовать PVS-Studio.
0
0
0
28
0
37
0
131
0
Попробуйте PVS-Studio бесплатно
Мы ответим вам на
Если вы так и не получили ответ, пожалуйста, проверьте, отфильтровано ли письмо в одну из следующих стандартных папок:
0